Have Expand, ExpandFromCache, and ExpandHelper take additional DescriptorCache
parameters. These are then passed into PubkeyProvider::GetPubKey which
also takes them as arguments.
Reading and writing to the cache is pushed down into GetPubKey. The old cache where
pubkeys are serialized to a vector is completely removed and instead xpubs are being
cached in DescriptorCache.
5bad7921d0 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c9061 [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/13557#pullrequestreview-135905054 I recommended not including bip32 deriviation by default in PSBTs:
> _Bit of a privacy issue_: let's say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn't sign it. Wallet A then calls `walletprocesspsbt` which signs it and _spontaneously adds the master_fingerprint and bip32 path_. Same issue with `walletcreatefundedpsbt`.
>
> Adding `bip32_derivs` should probably be opt-in.
In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.
More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).
ACKs for top commit:
instagibbs:
utACK 5bad7921d0
jonatack:
ACK 5bad7921d0 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
meshcollider:
utACK 5bad7921d0
Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
e193a84fb2 Refactor message hashing into a utility function (Jeffrey Czyz)
f8f0d9893d Deduplicate the message signing code (Vasil Dimov)
2ce3447eb1 Deduplicate the message verifying code (Vasil Dimov)
Pull request description:
The message signing and verifying logic was replicated in a few places
in the code. Consolidate in a newly introduced `MessageSign()` and
`MessageVerify()` and add unit tests for them.
ACKs for top commit:
Sjors:
re-ACK e193a84fb2
achow101:
ACK e193a84fb2
instagibbs:
utACK e193a84fb2
meshcollider:
utACK e193a84fb2
Tree-SHA512: b0e02a7d4623a98c8f8c77627af1725e6df07700de4630c2f75da6beacdf55414c38ba147bc6d2a757491ab07c827dddf93e8632fe600478760e255714ddab88
12a2f37718 util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t nTime) by checking gmtime_s/gmtime_r return value (practicalswift)
Pull request description:
Avoid potential uninitialized read in `FormatISO8601DateTime(int64_t)` by checking `gmtime_s`/`gmtime_r` return value.
Before this patch `FormatISO8601DateTime(67768036191676800)` resulted in:
```
==5930== Conditional jump or move depends on uninitialised value(s)
==5930== at 0x4F44C0A: std::ostreambuf_iterator<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<long>(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==5930== by 0x4F511A4: std::ostream& std::ostream::_M_insert<long>(long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==5930== by 0x4037C3: void tinyformat::formatValue<int>(std::ostream&, char const*, char const*, int, int const&) (tinyformat.h:358)
==5930== by 0x403725: void tinyformat::detail::FormatArg::formatImpl<int>(std::ostream&, char const*, char const*, int, void const*) (tinyformat.h:543)
==5930== by 0x402E02: tinyformat::detail::FormatArg::format(std::ostream&, char const*, char const*, int) const (tinyformat.h:528)
==5930== by 0x401B16: tinyformat::detail::formatImpl(std::ostream&, char const*, tinyformat::detail::FormatArg const*, int) (tinyformat.h:907)
==5930== by 0x4017AE: tinyformat::vformat(std::ostream&, char const*, tinyformat::FormatList const&) (tinyformat.h:1054)
==5930== by 0x401765: void tinyformat::format<int, int, int, int, int, int>(std::ostream&, char const*, int const&, int const&, int const&, int const&, int const&, int const&) (tinyformat.h:1064)
==5930== by 0x401656: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<int, int, int, int, int, int>(char const*, int const&, int const&, int const&, int const&, int const&, int const&) (tinyformat.h:1073)
==5930== by 0x4014CC: FormatISO8601DateTime[abi:cxx11](long) (…)
```
The same goes for other very large positive and negative arguments.
Fix by simply checking the `gmtime_s`/`gmtime_r` return value :)
ACKs for top commit:
MarcoFalke:
ACK 12a2f37718
theStack:
re-ACK 12a2f37718
elichai:
re ACK 12a2f37718
Tree-SHA512: 066142670d9bf0944d41fa3f3c702b1a460b5471b93e76a619b1e818ff9bb9c09fe14c4c37e9536a04c99533f7f21d1b08ac141e1b829ff87ee54c80d0e61d48
bca8665d08 scripted-diff: Wallet: Rename incorrectly named *UsedDestination (Luke Dashjr)
Pull request description:
These functions are used to mark/check if a key of our own has been used to spend (and only for avoid-reuse wallets), which has nothing to do with the destination/address itself.
Give them more accurate names to avoid confusion.
-BEGIN VERIFY SCRIPT-
sed -i -e 's/UsedDestination/SpentKey/g' $(git grep -l 'UsedDestination' ./src)
-END VERIFY SCRIPT-
ACKs for top commit:
practicalswift:
ACK bca8665d08 -- patch looks correct and rationale makes sense
instagibbs:
ACK bca8665d08, much more meaningful name, thanks
kallewoof:
ACK bca8665d08
Tree-SHA512: ff13d9061ffa748e92eb41ba962c3ec262a43e4b6abd62408b38c6f650395d6ae5851554257d1900fb02767a88d08380d592a27210192ee9abb72d0945976686
faca8eff39 test: Remove incorrect assumptions in validation_flush_tests (MarcoFalke)
fa31eebfe9 test: Tabs to spaces in all tests (MarcoFalke)
Pull request description:
The tests assume standard library internals that may not hold on all supported archs or when the code is instrumented for sanitizer or debug use cases
Fixes#18111
ACKs for top commit:
jamesob:
ACK faca8eff39 pending passing tests
fjahr:
ACK faca8eff39
Tree-SHA512: 60a5ae824bdffb0762f82f67957b31b185385900be5e676fcb12c23d53f5eea734601680c2e3f0bdb8052ce90e7ca1911b1342affb67e43d91a506b111406f41
fac52dafa0 test: Set catch_system_errors=no on boost unit tests (MarcoFalke)
Pull request description:
Closes#16700
Can be tested by adding an `assert(0)` and then running either `make check` or `./src/test/test_bitcoin -t bla_tests --catch_system_errors=no/yes`
ACKs for top commit:
practicalswift:
ACK fac52dafa0
Empact:
Tested ACK fac52dafa0
Tree-SHA512: ec00636951b2c1137aaf43610739d78d16f823f7da76a726d47f93b8b089766fb66b21504b3c5413bcf8b6b5c3db0ad74027d677db24a44487d6d79a6bdee2e0
These functions are used to mark/check if a key of our own has been used to spend (and only for avoid-reuse wallets), which has nothing to do with the destination/address itself.
Give them more accurate names to avoid confusion.
-BEGIN VERIFY SCRIPT-
sed -i -e 's/UsedDestination/SpentKey/g' $(git grep -l 'UsedDestination' ./src)
-END VERIFY SCRIPT-
7e80f646b2 Get the OutputType for a descriptor (Andrew Chow)
Pull request description:
Adds a `GetOutputType()` method to get the OutputType of a descriptor. Some descriptors don't have a determinate OutputType, so we actually use an `Optional<OutputType>`. For descriptors with indeterminate OutputType, we return `nullopt`.
`addr()` and `raw()` use OutputTypes as determined by the CTxDestination they have. For simplicity, `ScriptHash` destinations are `LEGACY` even though they could be `P2SH_SEGWIT`.
`combo()`, `pk()`, and `multi()` are `nullopt` as they either don't have an OutputType or they have multiple. `DescriptorImpl` defaults to `nullopt`.
`pkh()` is `LEGACY` as expected
`wpkh()` and `wsh()` are `BECH32` as expected.
`sh()` checks whether the sub-descriptor is `BECH32`. If so, it is `P2SH_SEGWIT`. Otherwise it is `LEGACY`.
The descriptor tests are updated to check the OutputType too.
ACKs for top commit:
fjahr:
ACK 7e80f646b2
meshcollider:
utACK 7e80f646b2
instagibbs:
cursory ACK 7e80f646b2
Sjors:
Code review ACK 7e80f646b2
jonatack:
ACK 7e80f64 code review/build/tests
Tree-SHA512: c5a813447b62e982435e1c948066f8d6c148c9ebffb0a5eb5a9028b173b01d5ead2f076a5ca3f7f37698538baa346f82a977ee48f583d89cb4e5ebd9111b2341
7f1475c711 rpc: update validateaddress RPCExamples to bech32 (Sebastian Falbesoner)
Pull request description:
Another small step to get rid of legacy addresses in the RPC help texts and by that encourage the use of bech32 addresses by default. The (invalid) address is the same as in the `getaddressinfo` RPC (see 2ee0cb3330, kudos to jonatack!), I don't think it adds any value to have a different example address per RPC.
ACKs for top commit:
fanquake:
ACK 7f1475c711
MarcoFalke:
ACK 7f1475c711
Tree-SHA512: 2350f61fa942a9053f9f5c860ea446965dc7209c71c81bdb98a859d03ca23b225ad72c9c506e4a55c8d8988823d9cfbe808c1a452a1eeadb70ab186b146dd4ca
a304a3632f Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky)
eb7d8a5b07 [test] check for addmultisigaddress regression (Sjors Provoost)
005f8a92cc wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky)
Pull request description:
Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable.
The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files.
This change adds a lot of comments and allows reverting commit 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible
ACKs for top commit:
Sjors:
re-ACK a304a3632f (rebase, slight text changes and my test)
achow101:
re-ACK a304a3632f
meshcollider:
utACK a304a3632f
Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e
8bca30ea17 [rpc] expose ability to mock scheduler via the rpc (Amiti Uttarwar)
7c8b6e5b52 [lib] add scheduler to node context (Amiti Uttarwar)
930d837542 [test] add chainparams property to indicate chain allows time mocking (Amiti Uttarwar)
1cd43e83c6 [test] unit test for new MockForward scheduler method (Amiti Uttarwar)
a6f63598ad [util] allow scheduler to be mocked (Amiti Uttarwar)
Pull request description:
This PR is to support functional tests by allowing the scheduler to be mocked via the RPC.
It adds a `MockForward` method to the scheduler class that iterates through the task queue and reschedules them to be `delta_seconds` sooner.
This is currently used to support functional testing of the "unbroadcast" set tracking in #18038. If this patch is accepted, it would also be useful to simplify the code in #16698.
ACKs for top commit:
MarcoFalke:
ACK 8bca30ea17, only change is some style fixups 🕓
Tree-SHA512: 2a97fe8ade2b7fd1fb5cdfa1dcafb3227a377d7a847e3845a228bc119eb77824b4aefa43d922a06d583939b22725e223f308cf092961048079d36f6b1d9a639b
4e9efac678 test: Check wallet name in -walletnotify script (João Barbosa)
9a5b5ee81f wallet: Replace %w by wallet name in -walletnotify script (João Barbosa)
Pull request description:
Fixes#13237.
ACKs for top commit:
laanwj:
ACK 4e9efac678
Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203
fad027fb0c scripted-diff: Add missing spaces in RPCResult, Fix type names (MarcoFalke)
Pull request description:
This makes the rendered diff smaller when the RPCResult is machine generated later on (Previous attempts: #14601 and #14459)
ACKs for top commit:
Sjors:
ACK fad027fb0c
Tree-SHA512: 48afd571b1cd349ca0b29bb444c1c7cda657e07dd96c610d479f931ccd938186aec98e533d0552b5b10afc9a3d7b911359260a49448e8e1106e3647b2c71f3ba
And add unit test for it.
The purpose of using a preamble or "magic" text as part of signing and
verifying a message was not given when the code was repeated in a few
locations. Make a test showing how it is used to prevent inadvertently
signing a transaction.
The logic of signing a message was duplicated in 3 places:
src/qt/signverifymessagedialog.cpp
SignVerifyMessageDialog::on_signMessageButton_SM_clicked()
src/rpc/misc.cpp
signmessagewithprivkey()
src/wallet/rpcwallet.cpp
signmessage()
Move the logic into
src/util/message.cpp
MessageSign()
and call it from all the 3 places.
The logic of verifying a message was duplicated in 2 places:
src/qt/signverifymessagedialog.cpp
SignVerifyMessageDialog::on_verifyMessageButton_VM_clicked()
src/rpc/misc.cpp
verifymessage()
with the only difference being the result handling. Move the logic into
a dedicated
src/util/message.cpp
MessageVerify()
which returns a set of result codes, call it from the 2 places and just
handle the results differently in the callers.
25bc17fceb refactor: rpc: Remove vector copy from listtransactions (João Barbosa)
Pull request description:
Current approach
- copy accumulated `ret` vector to `arrTmp`
- drop unnecessary elements from `arrTmp`
- reverse `arrTmp`
- clear `ret`
- copy `arrTmp` to the `ret`
New approach
- create a vector from the accumulated `ret` with just the necessary elements already reversed
- copy it to the result
This PR doesn't change behavior.
ACKs for top commit:
ryanofsky:
Code review ACK 25bc17fceb. Just comment and commit message tweaks since last review
Tree-SHA512: 87906561e3accdbdb0f4a8194cbcd76ea53ae53d0ce135b90bc54a5f77e300b14ef08505e7daf1fe52426f135442a743da5a027416a769bd454922357cebe7c0
also contains the following changes:
- rpc: factor out example bech32 address for RPCExamples
- doc: update developer notes wrt RPCExamples addresses
(mention the EXAMPLE_ADDRESS constant as an example for an invalid bech32
address suitable for RPCExamples help documentation)
c9fe61291e gui: Throttle GUI update pace when -reindex (Hennadii Stepanov)
Pull request description:
This is grabbed from #17565.
All **laanwj**'s and **ryanofsky**'s suggestions are implemented.
With this PR, the GUI does not freeze when a user runs:
```
$ ./src/qt/bitcoin-qt -reindex
```
ACKs for top commit:
jonasschnelli:
utACK c9fe61291e
Tree-SHA512: c7be316cb73d3d286bdf8429a960f71777d13a73d059869a64e23ad276499252b561a3a5b9613c4c1ad58cc0de26283c1ec72be745c401f604eaa05f70bf7d64
bf36a3ccc2 gui: Fix race in WalletModel::pollBalanceChanged (Russell Yanofsky)
Pull request description:
Poll function was wrongly setting cached height to the current chain height instead of the chain height at the time of polling.
This bug could cause balances to appear out of date, and was first introduced a0704a8996 (diff-2e3836af182cfb375329c3463ffd91f8L117). Before that commit, there wasn't a problem because cs_main was held during the poll update.
Currently, the problem should be rare. But if 8937d99ce81a27ae5e1012a28323c0e26d89c50b from #17954 were merged, the problem would get worse, because the wrong cachedNumBlocks value would be set if the wallet was polled in the interval between a block being connected and it processing the BlockConnected notification.
MarcoFalke also points out that a0704a8996 could lead to GUI hangs as well, because previously the pollBalanceChanged method, which runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call, but after could make blocking LOCK(cs_main) calls, potentially locking up the GUI.
Thanks to John Newbery for finding this bug this while reviewing https://github.com/bitcoin/bitcoin/pull/17954.
ACKs for top commit:
Empact:
utACK bf36a3ccc2
jonasschnelli:
utACK bf36a3c
Tree-SHA512: 1f4f229fa70a6d1fcf7be3806dca3252e86bc1755168fb421258389eb95aae67f863cb1216e6dc086b596c33560d1136215a4c87b5ff890abc8baaa3333b47f4
5f26855f10 test: Remove ubsan alignment suppressions (Wladimir J. van der Laan)
9d933ef919 prevector: avoid misaligned member accesses (Anthony Towns)
Pull request description:
Ensure prevector data is appropriately aligned. Earlier discussion in #17530.
**Edit laanwj**: In contrast to #17530, it does this without increase in size of any of the coin cache data structures (x86_64, clang)
| Struct | (size,align) before | (size,align) after |
| ------------- | ------------- | ------- |
| Coin | 48, 8 | 48, 8 |
| CCoinsCacheEntry | 56, 8 | 56, 8 |
| CScript | 32, 1 | 32, 8 |
ACKs for top commit:
laanwj:
ACK 5f26855f10
practicalswift:
ACK 5f26855f10
jonatack:
ACK 5f26855f10
Tree-SHA512: 98d112d6856f683d5b212410b73f3071d2994f1efb046a2418a35890aa1cf1aa7c96a960fc2e963fa15241e861093c1ea41951cf5b4b5431f88345eb1dd0a98a
Make LegacyScriptPubKeyMan::CanProvide method able to recognize p2sh scripts
when the redeem script is present in the mapScripts map without the p2sh script
also having to be added to the mapScripts map. This restores behavior prior to
https://github.com/bitcoin/bitcoin/pull/17261, which I think broke backwards
compatibility with old wallet files by no longer treating addresses created by
`addmultisigaddress` calls before #17261 as solvable.
The reason why tests didn't fail with the CanProvide implementation in #17261
is because of a workaround added in 4a7e43e846
"Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem
for new `addmultisigaddress` RPC calls without fixing it for multisig addresses
already created in old wallet files.
This change adds a lot of comments and allows reverting commit
4a7e43e846 "Store p2sh scripts in
AddAndGetDestinationForScript", so the AddAndGetDestinationForScript() function,
CanProvide() method, and mapScripts map should all be more comprehensible
Poll function was wrongly setting cached height to the current chain height
instead of the chain height at the time of polling.
This bug could cause balances to appear out of date, and was first introduced
a0704a8996 (r378452145)
Before that commit, there wasn't a problem because cs_main was held during the
poll update.
Currently, the problem should be rare. But if
8937d99ce81a27ae5e1012a28323c0e26d89c50b from #17954 were merged, the problem
would get worse, because the wrong cachedNumBlocks value would be set if the
wallet was polled in the interval between a block being connected and it
processing the BlockConnected notification.
MarcoFalke <falke.marco@gmail.com> also points out that a0704a8996 could lead
to GUI hangs as well, because previously the pollBalanceChanged method, which
runs on the GUI thread, would only make a nonblocking TRY_LOCK(cs_main) call,
but after could make blocking LOCK(cs_main) calls, potentially locking up the
GUI.
Thanks to John Newbery <john@johnnewbery.com> for finding this bug this while
reviewing https://github.com/bitcoin/bitcoin/pull/17954.
4537ba5f21 test: add unit test for non-standard txs with too large tx size (Sebastian Falbesoner)
Pull request description:
Approaches another missing unit test of issue #17394: Checks that the function `IsStandardTx()` returns rejection reason `"tx-size"` if the transaction weight is larger than `MAX_STANDARD_TX_WEIGHT` (=400000 vbytes).
ACKs for top commit:
Empact:
Code Review ACK 4537ba5f21
instagibbs:
ACK 4537ba5f21
Tree-SHA512: ab32e3e47e0b337253aef3da9b7c97d01f4130d00d5860588dfed02114eec3ba49473acc6419448affd63e883fd827bf308716965606eaddee242c4c5a4eb799
3c94b0039d Convert undo.h to new serialization framework (Pieter Wuille)
3cd8ab9d11 Make std::vector and prevector reuse the VectorFormatter logic (Pieter Wuille)
abf8624356 Add custom vector-element formatter (Pieter Wuille)
37d800bea0 Add a constant for the maximum vector allocation (5 Mbyte) (Pieter Wuille)
Pull request description:
The next step of changes from #10785.
This one adds:
* A meta-formatter for vectors, which serializes the vector elements using another formatter
* Switch the undo.h code to the new framework, using the above (where undo entries are serialized as a vector, each of which uses a modified serializer for the UTXOs).
ACKs for top commit:
laanwj:
code review ACK 3c94b0039d
jonatack:
Qualified ACK 3c94b0039d
ryanofsky:
Code review ACK 3c94b0039d. Changes since last review: renaming formatter classes, adding suggested static_assert, and removing temporary in VectorFormatter
Tree-SHA512: 44eebf51a303f6adbbc1ca2b9d043e8ae7fd37e06778e026590892f8d09f8253067862a68ba8ca5d733fd2f8e7c84edd255370f5a4b6560259427a65f94632df
ac57859e53 qt: Fix deprecated QCharRef usage (Hennadii Stepanov)
Pull request description:
From Qt docs:
- [`QKeyEvent::text()`](https://doc.qt.io/qt-5/qkeyevent.html#text):
> Return values when modifier keys such as Shift, Control, Alt, and Meta are pressed differ among platforms and could return an empty string.
- [`QString::operator[]()`](https://doc.qt.io/qt-5/qstring.html#operator-5b-5d):
> **Note:** Before Qt 5.14 it was possible to use this operator to access a character at an out-of-bounds position in the string, and then assign to such a position, causing the string to be automatically resized. Furthermore, assigning a value to the returned `QCharRef` would cause a detach of the string, even if the string has been copied in the meanwhile (and the `QCharRef` kept alive while the copy was taken). These behaviors are deprecated, and will be changed in a future version of Qt.
Since Qt 5.14 this causes a `QCharRef` warning if any modifier key is pressed while the splashscreen is still displayed.
Fix#18080.
Note: Ctrl+Q will also close the spashscreen now.
ACKs for top commit:
jonasschnelli:
utACK ac57859e53
Tree-SHA512: a7e5559410bd05c406007ab0243f458b82d434b0543276ed331254c8d7a6b1aaa54d0b406f799b830859294975004380160f8af04ba403d3bf185d51e6784f54
2af3e16ca9 Qt: pass clientmodel changes from walletframe to walletviews (Jonas Schnelli)
Pull request description:
Fixes#18090
We currently don't pass `clientmodel` changes from the `walletframe` to the `walletviews` leading to possible invalid access during shutdown because all walletviews miss the nullifying of the clientmodel.
TODO: needs investigation if this is should be backported.
ACKs for top commit:
laanwj:
Good catch, code review ACK 2af3e16ca9
Tree-SHA512: f8c0a114f01deac07fb311112d144f3bfc1c1882dd19e8742b372dd597d7a5d59cd0af99fc50494de2334cad98d6701675317474e40fe8820d04c058aeca1b75
