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
This is safe because MilliSleep is never executed in a boost::thread,
the only type of thread that is interruptible.
* The RPC server uses std::thread
* The wallet is either executed in an RPC thread or the main thread
* bitcoin-cli, benchmarks and tests are only one thread (the main thread)
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/MilliSleep\((\S+)\);/UninterruptibleSleep(std::chrono::milliseconds{\1});/g' $(git grep -l MilliSleep)
-END VERIFY SCRIPT-
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
Before, macros defined in `<event2/http.h>` have been used
for some HTTP status codes.
`<event2/http.h>` is included implicitly and the usage
of its status code macros is inconsistent with the majority
HTTP response implementations in this file.
Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is
consistently used for all HTTP response implementations.
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
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
