MarcoFalke reported the case of positional arguments silently overwriting the
named "args" parameter in bitcoin-cli
https://github.com/bitcoin/bitcoin/pull/19762#discussion_r1035761471 and this
behavior is confusing and was not intended when support for "args" parameters
was added to bitcoin-cli in #19762.
Instead of letting one "args" value overwrite the other in the client, just
pass the values to the server verbatim, and let the error be handled server
side.
Specifying same named parameter multiple times is still allowed by bitcoin-cli.
The client implementation overwrites earlier option values with later ones
before sending to server. This is tested by interface_bitcoin_cli.py
Rationale for allowing client parameters to be specified multiple times in
bitcoin-cli is that this behavior has been supported for a long time, and that
when using the command line interactively, it can be convenient to override
earlier option values with new values without having to go back and remove the
old value.
But for the RPC server, there isn't really a good use-case for earlier values
to be discarded if multiple values are specified. JSON keys are generally
supposed to be unique and if they aren't it's probably an indication of some
problem generating the RPC request.
Current behavior isn't ideal and will be changed in upcoming commits, but it's
useful to have test coverage regardless.
MarcoFalke reported the case of bitcoin-cli positional arguments overwriting
the named "args" parameter in
https://github.com/bitcoin/bitcoin/pull/19762#discussion_r1035761471
The CoinsResult class will now count the raw total amount and the effective
total amount internally (inside the 'CoinsResult::Add' and 'CoinsResult::Erase'
methods).
So there is no discrepancy between what we add/erase and the total values.
(which is what was happening on the coinselector_test because the 'CoinsResult'
object is manually created there, and we were not keeping the total amount
in sync with the outputs being added/removed).
Aside from the cleanup, this solves a bug in the following-up commit. Because, in these
tests, we are manually adding/erasing outputs from the CoinsResult object but never
updating the internal total amount field.
This exercises the bug inside CoinsResult::Erase that
ends up on (1) a wallet crash or (2) a created and
broadcasted tx that contains a reduced recipient's amount.
This is covered by making the wallet selects the preset
inputs twice during the coin selection process.
Making the wallet think that the selection process result covers
the entire tx target when it does not. It's actually creating
a tx that sends more coins than what inputs are covering for.
Which, combined with the SFFO option, makes the wallet
incorrectly reduce the recipient's amount by the difference
between the original target and the wrongly counted inputs.
Which means, a created and relayed tx sending less coins to
the destination than what the user inputted.
8f2dac5409 [test] Add p2p_tx_privacy.py (dergoegge)
ce63fca13e [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge)
845e3a34c4 [net processing] Ensure transaction announcements are only queued for fully connected peers (dergoegge)
Pull request description:
`TxRelay::m_next_inv_send_time` is initialized to 0, which means that any txids in `TxRelay::m_tx_inventory_to_send` will be announced on the first call to `PeerManagerImpl::SendMessages` for a fully connected peer (i.e. it completed the version handshake).
Prior to #21160, `TxRelay::m_tx_inventory_to_send` was guaranteed to be empty on the first `SendMessages` call, as transaction announcements were only queued for fully connected peers. #21160 replaced a `CConnman::ForEachNode` call with a loop over `PeerManagerImpl::m_peer_map`, in which the txid for a transaction to be relayed is added to `TxRelay::m_tx_inventory_to_send` for all peers. Even for those peers that have not completed the version handshake. Prior to the PR this was not the case as `ForEachNode` has a "fully connected check" before calling a function for each node.
ACKs for top commit:
MarcoFalke:
ACK 8f2dac5409🔝
jnewbery:
utACK 8f2dac5409
Tree-SHA512: e9eaccf7e00633ee0806fff1068b0e413a69a5e389d96c9659f68079915a6381ad5040c61f716cfcde77931d1b563b1049da97a232a95c6cd8355bd3d13404b9
5e65a216d1 wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow)
88afc73ae0 tests: Test for migrating encrypted wallets (Andrew Chow)
86ef7b3c7b wallet: Avoid null pointer deref when cleaning up migratewallet (Andrew Chow)
Pull request description:
When `migratewallet` fails, we do an automatic cleanup in order to reset everything so that the user does not experience any interruptions. However, this apparently has a segfault in it, caused by the the pointers to the watchonly and solvables wallets being nullptr. If those wallets are not created (either not needed, or failed early on), we will accidentally attempt to dereference these nullptrs, which causes a segfault.
This failure can be easily reached by trying to migrate an encrypted wallet. Currently, we can't migrate encrypted wallets because of how we unload wallets before migrating, and therefore forget the encryption key if the wallet was unlocked. So any encrypted wallets will fail, entering the cleanup, and because watchonly and solvables wallets don't exist yet, the segfault is reached.
This PR fixes this by not putting those nullptrs in a place that we will end up dereferencing them later. It also adds a test that uses the encrypted wallet issue.
ACKs for top commit:
S3RK:
reACK 5e65a216d1
stickies-v:
ACK [5e65a21](5e65a216d1)
furszy:
diff ACK 5e65a21
Tree-SHA512: f75643797220d4232ad3ab8cb4b46d0f3667f00486e910ca748c9b6d174d446968f1ec4dd7f907da1be9566088849da7edcd8cd8f12de671c3241b513deb8e80
1b77db2653 test: add `ismine` test for descriptor scriptpubkeyman (w0xlt)
Pull request description:
Currently `src/wallet/test/ismine_tests.cpp` has tests for the legacy ScriptPubKeyMan only.
This PR adds tests for the descriptor ScriptPubKeyMan.
ACKs for top commit:
ishaanam:
ACK 1b77db2653
achow101:
ACK 1b77db2653
furszy:
ACK 1b77db26 with a non-blocking comment.
Tree-SHA512: 977b5d1e71f9468331aeb4ebaf3708dd651f9f3018d4544a395b87ca6d7fb8bfa6d20acc1a4f6e096e240e81d30fb7a6e8add190e52536e7a3cb5a80f392883f
This commit documents our assumption about
TxRelay::m_tx_inventory_to_send being empty prior to version handshake
completion.
The added Assume acts as testing oracle for our fuzzing tests to
potentially detect if the assumption is violated.
46339d29b1 test, refactor: Reorder sendtxrcncl tests for better readability (Gleb Naumenko)
14263c13f1 p2p, refactor: Extend logs for unexpected sendtxrcncl (Gleb Naumenko)
87493e112e p2p, test, refactor: Minor code improvements (Gleb Naumenko)
00c5dec818 p2p: Clarify sendtxrcncl policies (Gleb Naumenko)
ac6ee5ba21 test: Expand unit and functional tests for txreconciliation (Gleb Naumenko)
bc84e24a4f p2p, refactor: Switch to enum class for ReconciliationRegisterResult (Gleb Naumenko)
a60f729e29 p2p: Drop roles from sendtxrcncl (Gleb Naumenko)
6772cbf69c tests: stabilize sendtxrcncl test (Gleb Naumenko)
Pull request description:
Non-trivial changes include:
- Getting rid of roles in `sendtxrcncl` message (summarized in the [BIP PR](https://github.com/bitcoin/bips/pull/1376));
- Disconnect the peer if it send `sendtxrcncl` although we are in `blocksonly` and notified the peer with `fRelay=0`;
- Don't send `sendtxrcncl` to feeler connections.
ACKs for top commit:
vasild:
ACK 46339d29b1
ariard:
ACK 46339d2
mzumsande:
Code Review ACK 46339d29b1
Tree-SHA512: b5cc6934b4670c12b7dbb3189e739ef747ee542ec56678bf4e4355bfb481b746d32363c173635685b71969b3fe4bd52b1c8ebd3ea3b35c82044bba69220f6417
If migratewallet fails, we do a cleanup which removes the watchonly and
solvables wallets if they were created. However, if they were not, their
pointers are nullptr and we don't check for that, which causes a
segfault during the cleanup. So check that they aren't nullptr before
cleaning them up.
13d9760829 test: load wallet, coverage for crypted keys (furszy)
373c99633e refactor: move DuplicateMockDatabase to wallet/test/util.h (furszy)
ee7a984f85 refactor: unify test/util/wallet.h with wallet/test/util.h (furszy)
cc5a5e8121 wallet: bugfix, invalid crypted key "checksum_valid" set (furszy)
Pull request description:
At wallet load time, the crypted key "checksum_valid" variable is always set to false. Which, on every wallet decryption call, forces the process to re-write all the ckeys to db when it's not needed.
Note:
The first commit fixes the issue, the two commits in the middle are cleanups so `DuplicateMockDatabase`
can be used without duplicating code. And, the last one is pure test coverage for the crypted keys loading
process.
Includes test coverage for the following scenarios:
1) "All ckeys checksums valid" test:
Loads an encrypted wallet with all the crypted keys with a valid checksum and
verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.
(we force a complete ckeys re-write if we find any missing crypted key checksum
during the wallet loading process)
2) "Missing checksum in one ckey" test:
Verifies that loading up a wallet with, at least one, 'ckey' with no checksum
triggers a complete re-write of the crypted keys.
3) "Invalid ckey checksum error" test:
Verifies that loading up a ckey with an invalid checksum stops the wallet loading
process with a corruption error.
4) "Invalid ckey pubkey error" test:
Verifies that loading up a ckey with an invalid pubkey stops the wallet loading
process with a corruption error.
ACKs for top commit:
achow101:
ACK 13d9760829
aureleoules:
ACK 13d9760829
Tree-SHA512: 9ea630ee4a355282fbeee61ca04737294382577bb4b2631f50e732568fdab8f72491930807fbda58206446c4f26200cdc34d8afa14dbe1241aec713887d06a0b
d8b12a75db rpc: Allow named and positional arguments to be used together (Ryan Ofsky)
Pull request description:
It's nice to be able to use named options and positional arguments together.
Most shell tools accept both, and python functions combine options and arguments allowing them to be passed with even more flexibility. This change adds support for python's approach so as a motivating example:
```sh
bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=1
```
Can be shortened to:
```sh
bitcoin-cli -named createwallet mywallet load_on_startup=1
```
JSON-RPC standard doesn't have a convention for passing named and positional parameters together, so this implementation makes one up and interprets any unused `"args"` named parameter as a positional parameter array.
This change is backwards compatible. It doesn't change the interpretation of any previously valid calls, just treats some previously invalid calls as valid.
Another use case even if you only occasionally use named arguments is that you can define an alias:
```
alias bcli='bitcoin-cli -named'
```
And now use both named named and unnamed arguments from the same alias without having to manually add `-named` option for named arguments or see annoying error "No '=' in named argument... this needs to be present for every argument (even if it is empty)`" for unnamed arguments
ACKs for top commit:
achow101:
ACK d8b12a75db
stickies-v:
re-ACK d8b12a75d
aureleoules:
re-ACK d8b12a75db
Tree-SHA512: 0cff8b50f584bcbbd376624adccf40536566ed8d1bcd6c88ad565dbc208f19d5e7a48c994efd6329d42b560149340d330397278f08a2912af5f3418d8c8837a9
These belong in libbitcoin_common.a, not libbitcoin_util.a, because they aren't
general-purpose utilities, they just contain common code that is used by both
the node and the wallet. Another reason to reason to not include these in
libbitcoin_util.a is to prevent them from being used by the kernel library.
There is no situation in which CNodeStateStats could be
missing for a legitimate reason - this can only happen if
there is a race condition between peer disconnection and
the getpeerinfo call, in which case the disconnected peer
doesn't need to be included in the response.
7082ce3e88 scripted-diff: rename and de-globalise g_cs_orphans (Anthony Towns)
733d85f79c Move all g_cs_orphans locking to txorphanage (Anthony Towns)
a936f41a5d txorphanage: make m_peer_work_set private (Anthony Towns)
3614819864 txorphange: move orphan workset to txorphanage (Anthony Towns)
6f8e442ba6 net_processing: Localise orphan_work_set handling to ProcessOrphanTx (Anthony Towns)
0027174b39 net_processing: move ProcessOrphanTx docs to declaration (Anthony Towns)
9910ed755c net_processing: Pass a Peer& to ProcessOrphanTx (Anthony Towns)
89e2e0da0b net_processing: move extra transactions to msgproc mutex (Anthony Towns)
ff8d44d196 Remove unnecessary includes of txorphange.h (Anthony Towns)
Pull request description:
Moves extra transactions to be under the `m_msgproc_mutex` lock rather than `g_cs_orphans` and refactors orphan handling so that the lock can be internal to the `TxOrphange` class.
ACKs for top commit:
dergoegge:
Code review ACK 7082ce3e88
glozow:
ACK 7082ce3e88 via code review and some [basic testing](https://github.com/glozow/bitcoin/blob/review-26295/src/test/orphanage_tests.cpp#L150). I think putting txorphanage in charge of handling peer work sets is the right direction.
Tree-SHA512: 1ec454c3a69ebd45ff652770d6a55c6b183db71aba4d12639ed70f525f0035e069a81d06e9b65b66e87929c607080a1c5e5dcd2ca91eaa2cf202dc6c02aa6818
This fully closes bitcoin#12179. Currently, in the GUI, when a user
abandons a transaction, a call is made to remove it from the list,
and another signal fires (eventually) that adds it back to the GUI
with a trash can icon.
There are no conditions where the abandoned transaction should be
directly removed from the GUI. If the underlying model changes, the
deletion will be reflected anyway.
fa3b2cf277 fuzz: Move-only net utils (MarcoFalke)
Pull request description:
This should speed up fuzz builds when `src/test/fuzz/util.h` is modified. Also, it makes sense on its own.
ACKs for top commit:
dergoegge:
ACK fa3b2cf277
Tree-SHA512: 03d6abeb728ac8eb3f28167e8ac43d8d6e7e1b1738ec14f58a36e17502081fdde2d56f2d47a9e11b991754667e83b2eb22d154e394c0c1c4ffa0945db86b7e21
This code was a bit hard to understand, so make it less dense and
add more explanations. Doesn't change behavior.
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
f362920c2c doc: clarify that NetPermissionFlags::Implicit is only about whitelists (Vasil Dimov)
Pull request description:
`NetPermissionFlags::Implicit` applies just to connections from `-whitebind` or `-whitelist`, clarify that in its comment.
ACKs for top commit:
Zero-1729:
crACK f362920c2c
aureleoules:
ACK f362920c2c
hernanmarino:
re ACK f362920c2c
Tree-SHA512: 03f6f8be221c6819bdd0b5b56b69b4e3a6dd25e5ca5a247eeb1261113144b9b74cf064a0b7815317782a0a18365dd3dab97963bd238e9b231dbe7e1cf0395683
b89530483d util: move threadinterrupt into util (fanquake)
Pull request description:
Alongside thread and threadnames. It's part of libbitcoin_util.
ACKs for top commit:
ryanofsky:
Code review ACK b89530483d. No changes since last review other than rebase
theuni:
ACK b89530483d.
Tree-SHA512: 0421f4d1881ec295272446804b27d16bf63e6b62b272f8bb52bfecde9ae6605e8109ed16294690d3e3ce4b15cc5e7c4046f99442df73adb10bdf069d3fb165aa
fa2d01470a test: Use type-safe NodeSeconds for TestMemPoolEntryHelper (MacroFake)
Pull request description:
test-only refactor to drop the deprecated `GetTime` in favour of the type-safe alternative
ACKs for top commit:
aureleoules:
ACK fa2d01470a - verified that there is no behavior change
Tree-SHA512: 5b64dae19c7bba9e8d90377c85891bc86f60ffbe67ea28d5ed3bd38f6dc30d3fbfba00bf49a16792922bddf83a52c632b6e5e5d8ffe1619fd9bf63effc60d59a
Adds test coverage for the wallet's crypted key loading from db process.
The following scenarios are covered:
1) "All ckeys checksums valid" test:
Loads an encrypted wallet with all the crypted keys with a valid checksum and
verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.
(we force a complete ckeys re-write if we find any missing crypted key checksum
during the wallet loading process)
2) "Missing checksum in one ckey" test:
Verifies that loading up a wallet with, at least one, 'ckey' with no checksum
triggers a complete re-write of the crypted keys.
3) "Invalid ckey checksum error" test:
Verifies that loading up a ckey with an invalid checksum stops the wallet loading
process with a corruption error.
4) "Invalid ckey pubkey error" test:
Verifies that loading up a ckey with an invalid pubkey stops the wallet loading
process with a corruption error.
files share the same purpose, and we shouldn't have wallet code
inside the test directory.
This later is needed to use wallet util functions in the bench
and test binaries without be forced to duplicate them.
0eeb9b0442 [fuzz] Move ConsumeNetAddr to fuzz/util/net.h (dergoegge)
291c8697d4 [fuzz] Make ConsumeNetAddr produce valid onion addresses (dergoegge)
c9ba3f836e [netaddress] Make OnionToString public (dergoegge)
Pull request description:
The chance that the fuzzer is able to guess a valid onion address is probably slim, as they are Base32 encoded and include a checksum. Right now, any target using `ConsumeNetAddr` would have a hard time uncovering bugs that require valid onion addresses as input.
This PR makes `ConsumeNetAddr` produce valid onion addresses by using the 32 bytes given by the fuzzer as the pubkey for the onion address and forming a valid address according to the torv3 spec.
ACKs for top commit:
vasild:
ACK 0eeb9b0442
brunoerg:
ACK 0eeb9b0442
Tree-SHA512: 7c687a4d12f9659559be8f0c3cd4265167d1261d419cfd3d503fd7c7f207cc0db745220f02fb1737e4a5700ea7429311cfc0b42e6c15968ce6a85f8813c7e1d8
cc597bd56d src/bitcoin-cli.cpp: -getinfo help - grammar correction (@RandyMcMillan)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: a5321968d0d377e1481170b4220a1319bf9040ec198b27c011609a5b7a81e9193500b750980c7de423b8b99655ed0f7772a9621e0b230aa6cc5d7b48167ed4f9
c8dc0e3eaa refactor: Inline `CTxMemPoolEntry` class's functions (Hennadii Stepanov)
75bbe594e5 refactor: Move `CTxMemPoolEntry` class to its own module (Hennadii Stepanov)
Pull request description:
This PR:
- gets rid of the `policy/fees` -> `txmempool` -> `policy/fees` circular dependency
- is an alternative to #13949, which nukes only one circular dependency
ACKs for top commit:
ryanofsky:
Code review ACK c8dc0e3eaa. Just include and whitespace changes since last review, and there's a moveonly commit now so it's very easy to review
theStack:
Code-review ACK c8dc0e3eaa
glozow:
utACK c8dc0e3eaa, agree these changes are an improvement.
Tree-SHA512: 36ece824e6ed3ab1a1e198b30a906c8ac12de24545f840eb046958a17315ac9260c7de26e11e2fbab7208adc3d74918db7a7e389444130f8810548ca2e81af41
6630a1e844 Add warning on first startup if free disk space is less than necessary (Ben Woosley)
Pull request description:
This reworks/revives https://github.com/bitcoin/bitcoin/pull/15848 to add a check for low disk space on first startup and issue a warning if disk space is below the expected space required to accommodate the blocks.
This PR was fashioned by a team of developers at the [bitcoin++](https://www.btcplusplus.dev/) conference workshop: "[Let's contribute to Bitcoin Core](https://sched.co/12P6Z)"
Fixes#15813
ACKs for top commit:
achow101:
ACK 6630a1e844
willcl-ark:
tACK 6630a1e844 rebased on master. Warning shows on first start but not on restart after some blocks have been downloaded.
aureleoules:
ACK 6630a1e844
pablomartin4btc:
re-ACK 6630a1e844
hernanmarino:
ReACK 6630a1e844
Tree-SHA512: 0f18acabdf2b514e96e2eea8f304960b952226b83dc91334cf7d1f6355ea2f257aaec0ee38d43ac36435385ecd918333d20657c35a8a7407e7cf2680ccb643bb
At wallet load time, we set the crypted key "checksum_valid" variable always to false.
Which, on every wallet decryption call, forces the process to re-write the entire ckeys to db when
it's not needed.
ac410e6fc0 log: improve some validation log messages to include hashPrevBlock (Skuli Dulfari)
Pull request description:
When there is an issue with a previous block the current log messages do not indicate hashPrevBlock. Adding it makes debugging easier.
ACKs for top commit:
stickies-v:
ACK ac410e6fc0
aureleoules:
reACK ac410e6fc0
theStack:
ACK ac410e6fc0
Tree-SHA512: d91481321f4474bb4fdf6ad55d1c897437b631b0a12308815c4ac5b053c8a76726e2d93f2aa0701e8cfd48fba7fad19ef5ffca3c67d3aa973dc593df806f1757
8a5014cd8a Fixes bitcoin#26490 by preventing notifications (John Moffett)
Pull request description:
This is a PR to address https://github.com/bitcoin/bitcoin/issues/26490
The menu bar currently subscribes to window focus change notifications to enable or disable certain menu options in response to the window status.
Notifications are automatically unsubscribed (disconnected in Qt parlance) if the sender is deleted -- in this case, the sender is the QTApplication object (`qApp`). However, MacOS 13 sends a window focus change notification *after* the main window has been destroyed but *before* `qApp` has been fully destroyed.
Since the menu bar is deleted in the main window's destructor, it no longer exists when it receives these notifications (in two different places via lambda expressions). The solution is to pass the main window (`this`) as context when subscribing to the notifications. In this [overloaded version](https://doc.qt.io/qt-5/qobject.html#connect-1) of `connect`, Qt automatically unsubscribes to notifications if the sender OR context (here the main window object) is destroyed. Since the spurious notifications are sent after the main window object is destroyed, this change prevents them from being sent.
Tested on Mac OS 13 and 12 only.
ACKs for top commit:
hebasto:
ACK 8a5014cd8a
Tree-SHA512: 3dff0a252fe0e93dd68cf5503135ecf6a72bcf385ba38407d6021ab77cca323f8bbe58aeca90ec124aa2a22ab9d35b706946179ac3b5d171c96a7010de51a090
2222ec71fd util: Move error message formatting of NonFatalCheckError to cpp (MacroFake)
Pull request description:
This allows to strip down the header file.
ACKs for top commit:
hebasto:
re-ACK 2222ec71fd, only rebased and suggested changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/25112#pullrequestreview-1182361605).
aureleoules:
ACK 2222ec71fd
Tree-SHA512: 313b3c891bb000cf606df1793b068f93df99915a254fbd67a45f003d440cce7355cdcc6b196f35757cc02d3697970d30e9de0d675f2aa8eb74107c13d663927a
fa84df1f03 scripted-diff: wallet: rename AvailableCoinsParams members to snake_case (furszy)
61c2265629 wallet: group AvailableCoins filtering parameters in a single struct (furszy)
f0f6a3577b RPC: listunspent, add "include immature coinbase" flag (furszy)
Pull request description:
Simple PR; adds a "include_immature_coinbase" flag to `listunspent` to include the immature coinbase UTXOs on the response. Requested by #25728.
ACKs for top commit:
danielabrozzoni:
reACK fa84df1f03
achow101:
ACK fa84df1f03
aureleoules:
reACK fa84df1f03
kouloumos:
reACK fa84df1f03
theStack:
Code-review ACK fa84df1f03
Tree-SHA512: 0f3544cb8cfd0378a5c74594480f78e9e919c6cfb73a83e0f3112f8a0132a9147cf846f999eab522cea9ef5bd3ffd60690ea2ca367dde457b0554d7f38aec792
db929893ef Faster -reindex by initially deserializing only headers (Larry Ruane)
c72de9990a util: add CBufferedFile::SkipTo() to move ahead in the stream (Larry Ruane)
48a68908ba Add LoadExternalBlockFile() benchmark (Larry Ruane)
Pull request description:
### Background
During the first part of reindexing, `LoadExternalBlockFile()` sequentially reads raw blocks from the `blocks/blk00nnn.dat` files (rather than receiving them from peers, as with initial block download) and eventually adds all of them to the block index. When an individual block is initially read, it can't be immediately added unless all its ancestors have been added, which is rare (only about 8% of the time), because the blocks are not sorted by height. When the block can't be immediately added to the block index, its disk location is saved in a map so it can be added later. When its parent is later added to the block index, `LoadExternalBlockFile()` reads and deserializes the block from disk a second time and adds it to the block index. Most blocks (92%) get deserialized twice.
### This PR
During the initial read, it's rarely useful to deserialize the entire block; only the header is needed to determine if the block can be added to the block index immediately. This change to `LoadExternalBlockFile()` initially deserializes only a block's header, then deserializes the entire block only if it can be added immediately. This reduces reindex time on mainnet by 7 hours on a Raspberry Pi, which translates to around a 25% reduction in the first part of reindexing (adding blocks to the index), and about a 6% reduction in overall reindex time.
Summary: The performance gain is the result of deserializing each block only once, except its header which is deserialized twice, but the header is only 80 bytes.
ACKs for top commit:
andrewtoth:
ACK db929893ef
achow101:
ACK db929893ef
aureleoules:
ACK db929893ef - minor changes and new benchmark since last review
theStack:
re-ACK db929893ef
stickies-v:
re-ACK db929893e
Tree-SHA512: 5a5377192c11edb5b662e18f511c9beb8f250bc88aeadf2f404c92c3232a7617bade50477ebf16c0602b9bd3b68306d3ee7615de58acfd8cae664d28bb7b0136
MacOS 13 sends a window focus change notification after the main
window has been destroyed but before the QTApplication has been
destroyed. This results in the menu bar receiving a notification
despite it no longer existing. The solution is to pass the main
window as context when subscribing to the notifications. Qt
automatically unsubscribes to notifications if the sender OR
context is destroyed.
Since faf44876db, the maxtipage comparison
in IsInitialBlockDownload() has been broken, since the NodeClock::now()
time_point is in the system's native denomination (micrcoseconds).
Without this patch, specifying the maximum allowable -maxtipage
(9223372036854775807) results in a SIGABRT crash.
Co-authored-by: MacroFake <falke.marco@gmail.com>
2dede9f675 Adjust RPCTypeCheckObj error string (Leonardo Araujo)
Pull request description:
Unifies the JSON type error strings as mentioned in #26214. Also refer to #25737.
ACKs for top commit:
furszy:
ACK 2dede9f6
Tree-SHA512: c918889e347ba32cb6d0e33c0de5956c2077dd40c996151e16741b0c4983ff098c60258206ded76ad7bbec4876c780c6abb494a97e4f1e05717d28a59b9167a6
fa09525751 univalue: string_view test (MacroFake)
1111c7e3f1 univalue: Avoid std::string copies (MacroFake)
Pull request description:
This shouldn't matter too much, unless a really large string is pushed into a json struct, but I think it also clarifies the code.
ACKs for top commit:
martinus:
Code review ACK fa09525751
aureleoules:
reACK fa09525751
ryanofsky:
Code review ACK fa09525751
Tree-SHA512: 74c441912bd0b00cdb9ea7890121f71ae5d62a7594e7d29aa402c9e3f033710c5d3afb27a37c552e6513804b249aa37e375ce013a3db853a25d1fd7b6e6cd3a8
This change allows to simplify CI tests, and makes it easier to
integrate the `bench_bitcoin` binary into CMake custom targets or
commands, as `COMMAND` does not support output redirection
In the wallet key-value-loading routine, most legacy type entries
require a LegacyScriptPubKeyMan instance after successful
deserialization. On a descriptor wallet, creating that (via method
`GetOrCreateLegacyScriptPubKeyMan`) fails and then leads to a
null-pointer dereference crash. Fix this by throwing an error if
if the wallet flags indicate that we have a descriptor wallet and there
is a legacy entry found.
It's nice to be able to use named options and positional arguments together.
Most shell tools accept both, and python functions combine options and
arguments allowing them to be passed with even more flexibility. This change
adds support for python's approach so as a motivating example:
bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=1
Can be shortened to:
bitcoin-cli -named createwallet mywallet load_on_startup=1
JSON-RPC standard doesn't have a convention for passing named and positional
parameters together, so this implementation makes one up and interprets any
unused "args" named parameter as a positional parameter array.
25ef049d60 log: mempool: log removal reason in validation interface (James O'Beirne)
Pull request description:
Currently the exact reason a transaction is removed from the mempool isn't logged. It is sometimes detectable from context, but adding the `reason` to the validation interface logs (where it is already passed) seems like an easy way to disambiguate.
For example in the case of mempool expiry, the logs look like this:
```
[validationinterface.cpp:220] [TransactionRemovedFromMempool] [validation] Enqueuing TransactionRemovedFromMempool: txid=<txid> wtxid=<wtxid>
[txmempool.cpp:1050] [RemoveUnbroadcastTx] [mempool] Removed <txid> from set of unbroadcast txns before confirmation that txn was sent out
[validationinterface.cpp:220] [operator()] [validation] TransactionRemovedFromMempool: txid=<txid> wtxid=<wtxid>
[validation.cpp:267] [LimitMempoolSize] [mempool] Expired 1 transactions from the memory pool
```
There is no context-free way to know $txid was evicted on the basis of expiry. This change will make that case (and probably others) clear.
ACKs for top commit:
0xB10C:
ACK 25ef049d60
Tree-SHA512: 9890f9fa16f66c8a9296798d8c28993e1b81da17cf592946f2abc22041f0b30b0911ab86a0c48d4aa46b9a8b3f7f5de67778649ac48c97740b0a09aa6816e0af
c3b1fe59db rpc: doc: add missing option "bech32m" for `change_type` parameters (Sebastian Falbesoner)
Pull request description:
Affects the help of the `fundrawtransaction`, `send` and `walletcreatefundedpsbt` RPCs.
This was found by manually inspecting the results of `$ git grep p2sh-segwit.*bech32`.
ACKs for top commit:
achow101:
ACK c3b1fe59db
Tree-SHA512: a3f1f8fde5905c80e1b95bd042ca0bc73d08c1c0e79c52ab0d6d12d7afdd4aa288afb41e12279fcea328a396f3d0a5564018170c0a11c5aa26dc6d44d2a62b1c
0de30ed509 tests: Test Taproot PSBT signing with keys in other descriptor (Andrew Chow)
6efcdf6b7f tests: Use new wallets for each test in wallet_taproot.py (Andrew Chow)
8781a1b6bb psbt: Include output pubkey in additional pubkeys to sign (Andrew Chow)
323890d0d7 sign: Fill in taproot pubkey info for all script path sigs (Andrew Chow)
Pull request description:
A user reported on [stackexchange](https://bitcoin.stackexchange.com/q/115742/48884) that they were unable to sign for a `multi_a` script using a wallet that only had the corresponding keys (i.e. it did not have the `multi_a()` descriptor). This PR fixes this issue.
Additionally, `wallet_taproot.py` is modified to test for this scenario by having another wallet in `do_test_psbt` which contains descriptors that only have the keys involved in the descriptor being tested. `wallet_taproot.py` was also modified to create new wallets for each test case rather than sharing wallets throughout as the sharing could result in the signing wallet having the keys in a different descriptor and accidentally result in failing to detect a test failure.
The changes to the test also revealed a similar issue with `rawtr()` descriptors, which has also been fixed by checking if a descriptor can produce a `SigningProvider` for the Taproot output pubkey.
ACKs for top commit:
instagibbs:
crACK 0de30ed509
darosior:
ACK 0de30ed509
Tree-SHA512: 12e131dd8afd93da7b1288c9054de2415a228d4477b97102da3ee4e82ce9de20b186260c3085a4b7b067bd8b74400751dcadf153f113db83abc59e7466e69f14
Currently the exact reason a transaction is removed from the mempool isn't
logged. It is sometimes detectable from context, but adding the `reason` to
the validation interface logs (where it is already passed) seems like an easy
way to disambiguate.
For example, in the case of mempool expiry, the logs look like this:
```
[validationinterface.cpp:220] [TransactionRemovedFromMempool] [validation] Enqueuing TransactionRemovedFromMempool: txid=<txid> wtxid=<wtxid>
[txmempool.cpp:1050] [RemoveUnbroadcastTx] [mempool] Removed <txid> from set of unbroadcast txns before confirmation that txn was sent out
[validationinterface.cpp:220] [operator()] [validation] TransactionRemovedFromMempool: txid=<txid> wtxid=<wtxid>
[validation.cpp:267] [LimitMempoolSize] [mempool] Expired 1 transactions from the memory pool
```
There is no context-free way to know $txid was evicted on the basis of expiry.
This change will make that case (and probably others) clear.
fa3ea81c3e refactor: Add LIFETIMEBOUND / -Wdangling-gsl to Assert() (MacroFake)
Pull request description:
Currently compiles clean, but I think it may still be useful.
Can be tested by adding an `&`:
```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 5766fff92d..300c1ec60f 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -125,7 +125,7 @@ BOOST_AUTO_TEST_CASE(util_check)
// Check -Wdangling-gsl does not trigger when copying the int. (It would
// trigger on "const int&")
- const int nine{*Assert(std::optional<int>{9})};
+ const int& nine{*Assert(std::optional<int>{9})};
BOOST_CHECK_EQUAL(9, nine);
}
```
Output:
```
test/util_tests.cpp:128:29: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling-gsl]
const int& nine{*Assert(std::optional<int>{9})};
^~~~~~~~~~~~~~~~~~~~~
./util/check.h:75:50: note: expanded from macro 'Assert'
#define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
^~~
1 warning generated.
ACKs for top commit:
jonatack:
ACK fa3ea81c3e
theuni:
ACK fa3ea81c3e
Tree-SHA512: 17dea4d75f2ee2bf6e1b6a6f6d8f439711c777df0390574e8d8edb6ac9ee807a135341e4439050bd6a15ecc4097a1ba9a7ab15d27541ebf70a4e081fa6871877
fa24239a1c net: Avoid SetTxRelay for feeler connections (MacroFake)
Pull request description:
Seems odd to reserve memory for the struct (the heaviest member being `m_tx_inventory_known_filter`) when it is never used.
This also avoids sending out `msg_sendtxrcncl` before disconnecting. This shouldn't matter, as other messages, such as `msg_wtxidrelay`, `msg_sendaddrv2`, `msg_verack` or `msg_getaddr` are still sent. Though, it allows to test the changes here as a side-effect.
ACKs for top commit:
naumenkogs:
ACK fa24239a1c
vasild:
ACK fa24239a1c
jonatack:
ACK fa24239a1c
mzumsande:
ACK fa24239a1c
Tree-SHA512: d7604c7eb4df8f2de811e600bdd312440ee03e508d3a0f09ae79f7f2d3eeec663bfd47a2d079fa50b756d61e35dfa998de068a7b9afaf35378fa0e62a538263d
e049fd76f0 Bugfix: Check for readlink buffer overflow and handle gracefully (Luke Dashjr)
Pull request description:
If readlink returns the size of the buffer, an overflow may have (safely) occurred.
Pass a buffer size of MAX_PATH+1 (the size of the actual buffer) to detect this scenario.
ACKs for top commit:
hebasto:
ACK e049fd76f0.
Tree-SHA512: 188bace79cbe556efe7782e46b870c02729b07b104a9316b0f7d50013504972e85baf507403d2d6060bb2bf3e13f40d735bddd18255d97a60810208c3de87691
* Add optional fee response in BTC to getrawtransaction
* Add optional prevout(s) response to getrawtransaction showing utxos being spent
* Add getrawtransaction_verbosity functional test to validate fields
In addition to the pubkeys in hd_keypaths and tap_bip32_keypaths, also
see if the descriptor can produce a SigningProvider for the output
pubkey.
Also slightly refactors this area to reduce code duplication.
Taproot pubkey info was not being added for multi_a signing. The filling
of this info is moved into the common function CreateTaprootScriptSig so
that any signing of taproot scripts will include the pubkey info.
fa29ef00ad refactor: Silence GCC Wmissing-field-initializers in ChainstateManagerOpts (MacroFake)
Pull request description:
The `std::optional` fields in the struct that fall back to chain param defaults if not provided should be initialized to `std::nullopt`. This already happens with the current code.
However, for consistency with `check_block_index` and to silence a GCC warning, add the "missing" `{}`.
ACKs for top commit:
achow101:
ACK fa29ef00ad
hebasto:
ACK fa29ef00ad, tested on Ubuntu 22.04 + GCC 11.3.
jonatack:
ACK fa29ef00ad
Tree-SHA512: bdec9c56df5d601a5616e107fed48737b13b0a7242b6526092fb682b5016544a4bc08666b60304c668d44c6f7ac69d3788093d921382c1d6c577c1f9fe31fc50
3fcb545ab2 bench: benchmark transaction creation process (furszy)
a8a75346d7 wallet: SelectCoins, return early if target is covered by preset-inputs (furszy)
f41712a734 wallet: simplify preset inputs selection target check (furszy)
5baedc3351 wallet: remove fetch pre-selected-inputs responsibility from SelectCoins (furszy)
295852f619 wallet: encapsulate pre-selected-inputs lookup into its own function (furszy)
37e7887cb4 wallet: skip manually selected coins from 'AvailableCoins' result (furszy)
94c0766b0c wallet: skip available coins fetch if "other inputs" are disallowed (furszy)
Pull request description:
#### # Context (Current Flow on Master)
In the transaction creation process, in order to select which coins the new transaction will spend,
we first obtain all the available coins known by the wallet, which means walking-through the
wallet txes map, gathering the ones that fulfill certain spendability requirements in a vector.
This coins vector is then provided to the Coin Selection process, which first checks if the user
has manually selected any input (which could be internal, aka known by the wallet, or external),
and if it does, it fetches them by searching each of them inside the wallet and/or inside the
Coin Control external tx data.
Then, after finding the pre-selected-inputs and gathering them in a vector, the Coin Selection
process walks-through the entire available coins vector once more just to erase coins that are
in both vectors. So the Coin Selection process doesn’t pick them twice (duplicate inputs inside
the same transaction).
#### # Process Workflow Changes
Now, a new method, `FetchCoins` will be responsible for:
1) Lookup the user pre-selected-inputs (which can be internal or external).
2) And, fetch the available coins in the wallet (excluding the already fetched ones).
Which will occur prior to the Coin Selection process. Which allows us to never include the
pre-selected-inputs inside the available coins vector in the first place, as well as doing other
nice improvements (written below).
So, Coin Selection can perform its main responsibility without mixing it with having to fetch
internal/external coins nor any slow and unneeded duplicate coins verification.
#### # Summarizing the Improvements:
1) If any pre-selected-input lookup fail, the process will return the error right away.
(before, the wallet was fetching all the wallet available coins, walking through the
entire txes map, and then failing for an invalid pre-selected-input inside SelectCoins)
2) The pre-selected-inputs lookup failure causes are properly described on the return error.
(before, we were returning an "Insufficient Funds" error for everything, even if the failure
was due a not solvable external input)
3) **Faster Coin Selection**: no longer need to "remove the pre-set inputs from the available coins
vector so that Coin Selection doesn't pick them" (which meant to loop-over the entire
available coins vector at Coin Selection time, erasing duplicate coins that were pre-selected).
Now, the available coins vector, which is built after the pre-selected-inputs fetching,
doesn’t include the already selected inputs in the first place.
4) **Faster transaction creation** for transactions that only use manually selected inputs.
We now will return early, as soon as we finish fetching the pre-selected-inputs and
not perform the resources expensive calculation of walking-through the entire wallet
txes map to obtain the available coins (coins that we will not use).
---------------------------
Added a new bench (f6d0bb2) measuring the transaction creation process, for a wallet with ~250k UTXO, only using the pre-selected-inputs inside coin control. Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically.
#### Result on this PR (tip f6d0bb2d):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,048,675.00 | 953.58 | 0.3% | 0.06 | `WalletCreateTransaction`
vs
#### Result on master (tip 4a4289e2):
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 96,373,458.20 | 10.38 | 0.2% | 5.30 | `WalletCreateTransaction`
The benchmark took to run in master: **96.37 milliseconds**, while in this PR: **1 millisecond** 🚀 .
ACKs for top commit:
S3RK:
Code Review ACK 3fcb545ab2
achow101:
ACK 3fcb545ab2
aureleoules:
reACK 3fcb545ab2
Tree-SHA512: 42f833e92f40c348007ca565a4c98039e6f1ff25d8322bc2b27115824744779baf0b0a38452e4e2cdcba45076473f1028079bbd0f670020481ec5d3db42e4731
eb679a7896 rpc: make `address` field optional (w0xlt)
Pull request description:
Close https://github.com/bitcoin/bitcoin/issues/26338.
This PR makes optional the `address` field in the response of `listtransactions` and `listsinceblock` RPC.
And adds two tests that fail on master, but not on this branch.
ACKs for top commit:
achow101:
ACK eb679a7896
aureleoules:
ACK eb679a7896
Tree-SHA512: b267439626e2ec3134ae790c849949a4c40ef0cebd20092e8187be3db0a61941b2da10bbbba92ca880b8369f46c1aaa806d057eaa5159325f65cbec7cb33c52f
b8b59ff9fe gui: update the screen after loading wallet (w0xlt)
Pull request description:
Currently, the user loads a wallet and the screen does not switch to the selected wallet after loading (File -> Open Wallet -> wallet name).
This PR changes that by making the `OpenWalletActivity::opened` signal connection a `Qt::QueuedConnection` type.
ACKs for top commit:
jarolrod:
ACK b8b59ff9fe
hebasto:
ACK b8b59ff9fe, tested on Ubuntu 22.04.
Tree-SHA512: 43cd755638b643f481014a7933a0af25df2d109e859cb5f878bc04e562950d550716fa38465140060e28526b2441688580cbcbe4ec6819566b4f95162ca5e527
This ensures that during shutdown, including failed initialization, the
`SplashScreen::m_connected_wallet_handlers` is deleted before the wallet
context is.
0cc23fc603 Fix typo in comment SHA256->SHA512 (Elichai Turkel)
Pull request description:
The comment says it's the SHA-256 state, while it's actually the SHA-512 state
ACKs for top commit:
andrewtoth:
ACK 0cc23fc603
aureleoules:
ACK 0cc23fc603
Tree-SHA512: 4e390ceefb847d3bbe4f5caab390a4fdd14892fe443f58c32b08b3444fccd611cff22938c3dfa611dfd2497736f779fae4165497b4208e48aa8fc9d2236f943b
Goal 1:
Benchmark the transaction creation process for pre-selected-inputs only.
Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically.
Goal 2:
Benchmark the transaction creation process for pre-selected-inputs and coin selection.
-----------------------
Benchmark Setup:
1) Generates a 5k blockchain, loading the wallet with 5k transactions with two outputs each.
2) Fetch 4 random UTXO from the wallet's available coins and pre-select them as inputs inside CoinControl.
Benchmark (Goal 1):
Call `CreateTransaction` providing the coin control, who has set `m_allow_other_inputs=false` and
the manually selected coins.
Benchmark (Goal 2):
Call `CreateTransaction` providing the coin control, who has set `m_allow_other_inputs=true` and
the manually selected coins.
we are already computing the preset inputs total amount inside `PreSelectedInputs::Insert`,
which internally decides whether to use the effective value or the raw output value based on
the 'subtract_fee_outputs' flag.
so if there is an error in any of the pre-set coins, we can fail right away
without computing the wallet available coins set (calling `AvailableCoins`)
which is a slow operation as it goes through the entire wallet's txes map.
----------------------
And to make the Coin Selection flow cleared, have decoupled SelectCoins in two functions:
1) AutomaticCoinSelection.
2) SelectCoins.
1) AutomaticCoinSelection:
Receives a set of coins and selects the best subset of them to
cover the target amount.
2) SelectCoins
In charge of select all the user manually selected coins first ("pre-set inputs"), and
if coin_control 'm_allow_other_inputs=true', call 'AutomaticCoinSelection' to select a
subset of coins owned by the wallet to cover for the target - preset_inputs.total_amount
remaining value.
First step towards decoupling the pre-selected-inputs fetching functionality
from `SelectCoins`. Which, will let us not waste resources calculating the
available coins if one of the pre-set inputs has an error.
(right now, if one of the pre-set inputs is invalid, we first walk through
the entire wallet txes map just to end up failing right after it finish)
No need to walk through the entire wallet's txes map just to get
coins that we could have gotten by just doing a simple map.find(out.hash).
(Which is what we are doing inside `SelectCoins` anyway)
no need to waste resources calculating the wallet available coins if
they are not going to be used.
The 'm_allow_other_inputs=true` default value change is to correct
an ugly misleading behavior:
The tx creation process was having a workaround patch to automatically
fall back to select coins from the wallet if `m_allow_other_inputs=false`
(previous default value) and no manual inputs were selected.
This could be seen in master in flows like `sendtoaddress`, `sendmany`
and even the GUI, where the `m_allow_other_inputs` value isn't customized
and the wallet still selects and adds coins to the tx internally.
5826bf546e test: Add test for getblockfrompeer on syncing pruned nodes (Fabian Jahr)
7fa851fba8 rpc: Pruned nodes can not fetch unsynced blocks (Fabian Jahr)
Pull request description:
This PR prevents `getblockfrompeer` from getting used on blocks that the node has not synced past yet if the node is in running in prune mode.
### Problem
While a node is still catching up to the tip that it is aware of via the headers, the user can currently use to fetch blocks close to or at the tip. These blocks are stored in the block/rev file that otherwise contains blocks the node is receiving as part of the syncing process.
This creates a problem for pruned nodes: The files containing a fetched block are not pruned during syncing because they contain a block close to the tip. This means the entire file (~130MB) will not be pruned until the tip has moved on far enough from the fetched block. In extreme cases with heavy pruning (like 550) and multiple blocks being fetched this could mean that the disc usage far exceeds what the user expects, potentially running out of space.
### Approach
There would be certainly other approaches that could fix the problem while still allowing the current behavior, but all of the ideas I came up with seemed like overkill for a niche problem on a new RPC where it's still unclear how and how much it will be used.
### Testing
So far I did not see a simple enough way to test this I am still looking into it and if it's complex will potentially add it in a follow-up. What would be needed is a way to have a node fetch headers but not sync the blocks yet, that seems like a pattern that could be generally useful.
To manually reproduce the problematic behavior:
1. Start a node with current `master` with `-prune=550` and an empty/new datadir, Testnet and Mainnet should both work.
2. While the node is syncing run `getblockfrompeer` on the current tip and a few other recent blocks.
3. Go to your datadir and observe the blocks folder: There should be a few full `blk*.dat` and `rev*.dat` files that are not being pruned. When you "pinned" a few of these files the blocks folder should be significantly above the target size of 550MB.
ACKs for top commit:
Sjors:
utACK 5826bf546e
achow101:
ACK 5826bf546e
aureleoules:
tACK 5826bf546e
Tree-SHA512: aa3f477ec755a9df2331c047cb10b3cd08292522bf6ad7a36a7ea36d7eba4894b84de8bd23003c9baea5ac0c53b77142c3c2819ae7528cece9d10a0d06c850d8
0582932260 test: add test for fast rescan using block filters (top-up detection) (Sebastian Falbesoner)
ca48a4694f rpc: doc: mention rescan speedup using `blockfilterindex=1` in affected wallet RPCs (Sebastian Falbesoner)
3449880b49 wallet: fast rescan: show log message for every non-skipped block (Sebastian Falbesoner)
935c6c4b23 wallet: take use of `FastWalletRescanFilter` (Sebastian Falbesoner)
70b3513904 wallet: add `FastWalletRescanFilter` class for speeding up rescans (Sebastian Falbesoner)
c051026586 wallet: add method for retrieving the end range for a ScriptPubKeyMan (Sebastian Falbesoner)
845279132b wallet: support fetching scriptPubKeys with minimum descriptor range index (Sebastian Falbesoner)
088e38d3bb add chain interface methods for using BIP 157 block filters (Sebastian Falbesoner)
Pull request description:
## Description
This PR is another take of using BIP 157 block filters (enabled by `-blockfilterindex=1`) for faster wallet rescans and is a modern revival of #15845. For reviewers new to this topic I can highly recommend to read the corresponding PR review club (https://bitcoincore.reviews/15845).
The basic idea is to skip blocks for deeper inspection (i.e. looking at every single tx for matches) if our block filter doesn't match any of the block's spent or created UTXOs are relevant for our wallet. Note that there can be false-positives (see https://bitcoincore.reviews/15845#l-199 for a PR review club discussion about false-positive rates), but no false-negatives, i.e. it is safe to skip blocks if the filter doesn't match; if the filter *does* match even though there are no wallet-relevant txs in the block, no harm is done, only a little more time is spent extra.
In contrast to #15845, this solution only supports descriptor wallets, which are way more widespread now than back in the time >3 years ago. With that approach, we don't have to ever derive the relevant scriptPubKeys ourselves from keys before populating the filter, and can instead shift the full responsibility to that to the `DescriptorScriptPubKeyMan` which already takes care of that automatically. Compared to legacy wallets, the `IsMine` logic for descriptor wallets is as trivial as checking if a scriptPubKey is included in the ScriptPubKeyMan's set of scriptPubKeys (`m_map_script_pub_keys`): e191fac4f3/src/wallet/scriptpubkeyman.cpp (L1703-L1710)
One of the unaddressed issues of #15845 was that [the filter was only created once outside the loop](https://github.com/bitcoin/bitcoin/pull/15845#discussion_r343265997) and as such didn't take into account possible top-ups that have happened. This is solved here by keeping a state of ranged `DescriptorScriptPubKeyMan`'s descriptor end ranges and check at each iteration whether that range has increased since last time. If yes, we update the filter with all scriptPubKeys that have been added since the last filter update with a range index equal or higher than the last end range. Note that finding new scriptPubKeys could be made more efficient than linearly iterating through the whole `m_script_pub_keys` map (e.g. by introducing a bidirectional map), but this would mean introducing additional complexity and state and it's probably not worth it at this time, considering that the performance gain is already significant.
Output scripts from non-ranged `DescriptorScriptPubKeyMan`s (i.e. ones with a fixed set of output scripts that is never extended) are added only once when the filter is created first.
## Benchmark results
Obviously, the speed-up indirectly correlates with the wallet tx frequency in the scanned range: the more blocks contain wallet-related transactions, the less blocks can be skipped due to block filter detection.
In a [simple benchmark](https://github.com/theStack/bitcoin/blob/fast_rescan_functional_test_benchmark/test/functional/pr25957_benchmark.py), a regtest chain with 1008 blocks (corresponding to 1 week) is mined with 20000 scriptPubKeys contained (25 txs * 800 outputs) each. The blocks each have a weight of ~2500000 WUs and hence are about 62.5% full. A global constant `WALLET_TX_BLOCK_FREQUENCY` defines how often wallet-related txs are included in a block. The created descriptor wallet (default setting of `keypool=1000`, we have 8*1000 = 8000 scriptPubKeys at the start) is backuped via the `backupwallet` RPC before the mining starts and imported via `restorewallet` RPC after. The measured time for taking this import process (which involves a rescan) once with block filters (`-blockfilterindex=1`) and once without block filters (`-blockfilterindex=0`) yield the relevant result numbers for the benchmark.
The following table lists the results, sorted from worst-case (all blocks contain wallte-relevant txs, 0% can be skipped) to best-case (no blocks contain walltet-relevant txs, 100% can be skipped) where the frequencies have been picked arbitrarily:
wallet-related tx frequency; 1 tx per... | ratio of irrelevant blocks | w/o filters | with filters | speed gain
--------------------------------------------|-----------------------------|-------------|--------------|-------------
~ 10 minutes (every block) | 0% | 56.806s | 63.554s | ~0.9x
~ 20 minutes (every 2nd block) | 50% (1/2) | 58.896s | 36.076s | ~1.6x
~ 30 minutes (every 3rd block) | 66.67% (2/3) | 56.781s | 25.430s | ~2.2x
~ 1 hour (every 6th block) | 83.33% (5/6) | 58.193s | 15.786s | ~3.7x
~ 6 hours (every 36th block) | 97.22% (35/36) | 57.500s | 6.935s | ~8.3x
~ 1 day (every 144th block) | 99.31% (143/144) | 68.881s | 6.107s | ~11.3x
(no txs) | 100% | 58.529s | 5.630s | ~10.4x
Since even the (rather unrealistic) worst-case scenario of having wallet-related txs in _every_ block of the rescan range obviously doesn't take significantly longer, I'd argue it's reasonable to always take advantage of block filters if they are available and there's no need to provide an option for the user.
Feedback about the general approach (but also about details like naming, where I struggled a lot) would be greatly appreciated. Thanks fly out to furszy for discussing this subject and patiently answering basic question about descriptor wallets!
ACKs for top commit:
achow101:
ACK 0582932260
Sjors:
re-utACK 0582932260
aureleoules:
ACK 0582932260 - minor changes, documentation and updated test since last review
w0xlt:
re-ACK 0582932260
Tree-SHA512: 3289ba6e4572726e915d19f3e8b251d12a4cec8c96d041589956c484b5575e3708b14f6e1e121b05fe98aff1c8724de4564a5a9123f876967d33343cbef242e1
`m_headers_sync` is already reset in IsContinuationOfLowWorkHeadersSync
if there is a failure, so there is no need to also reset in
TryLowWorkHeaderSync.
aaaa7bd0ba iwyu: Add missing includes (MacroFake)
fa9ebec096 Remove g_parallel_script_checks (MacroFake)
fa7c834b9f Move ::fCheckBlockIndex into ChainstateManager (MacroFake)
fa43188d86 Move ::fCheckpointsEnabled into ChainstateManager (MacroFake)
cccca83099 Move ::nMinimumChainWork into ChainstateManager (MacroFake)
fa29d0b57c Move ::hashAssumeValid into ChainstateManager (MacroFake)
faf44876db Move ::nMaxTipAge into ChainstateManager (MacroFake)
Pull request description:
It seems preferable to assign globals to a class (in this case `ChainstateManager`), than to leave them dangling. This should clarify scope for code-readers, as well as clarifying unit test behaviour.
ACKs for top commit:
dergoegge:
Code review ACK aaaa7bd0ba
ryanofsky:
Code review ACK aaaa7bd0ba. No changes since last review, other than rebase
aureleoules:
reACK aaaa7bd0ba
Tree-SHA512: 83ec3ba0fb4f1dad95810d4bd4e578454e0718dc1bdd3a794cc4e48aa819b6f5dad4ac4edab3719bdfd5f89cbe23c2740a50fd56c1ff81c99e521c5f6d4e898d
9153ff3e27 rpc: add non-regression test about deriveaddresses crash when index is 2147483647 (muxator)
addf9d6502 rpc: fix crash in deriveaddresses when derivation index is 2147483647 (muxator)
Pull request description:
This PR is a proposal for fixing #26274 (better described there).
The problem is due to a signed int wrapping when the `index` parameter of the `deriveaddresses` RPC call has the value `2^31-1`.
```C++
for (int i = range_begin; i <= range_end; ++i) {
```
* the first commit adds a "temporary" test case (`test/functional/rpc_deriveaddresses_crash.py`) that shows the crash, and can be used to generate a core dump;
* the second commit fixes the problem giving an explicit size to the `i` variable in a for loop, from `int` to `int64_t`. The same commit also removes the ephemeral test case and adds a passing test to `test/functional/rpc_deriveaddresses.py`, in order to prevent future regressions.
This is my first submission to this project and I do not know its conventions. Please advise if something needs to be changed.
ACKs for top commit:
achow101:
ACK 9153ff3e27
Tree-SHA512: 0477b57b15dc2c682cf539d6002f100d44a8c7e668041aa3340c39dcdbd40e083c75dec6896b6c076b044a01c2e5254272ae6696d8a1467539391926f270940a
796b020c37 wallet: add taproot support to external signer (Sjors Provoost)
Pull request description:
Builds on #22558 (merged on 2022-06-28).
[HWI 2.1.0](https://github.com/bitcoin-core/HWI/releases/tag/2.1.0) or newer is required to import and use taproot descriptors. Older versions will work, but won't import a taproot descriptor.
Tested with HWI 2.1.1:
* Trezor T (firmware v2.5.1) on Signet: signs, change detection works
* Ledger Nano S (firmware 2.1.0, Bitcoin app 2.0.6): signs, change detection works
Only the most basic `tr(key)` descriptor is supported, script path spending is completely untested (if it works at all).
ACKs for top commit:
jb55:
utACK 796b020c37
achow101:
ACK 796b020c37
Tree-SHA512: 6dcb7eeb45421a3bbf2bdabeacd29979867db69077d7bf192bb77faa4bfefe446487b8df07bc40f9457009a88e598bdc09f769e6106fed2833ace7ef205a157a
This extra method will be needed for updating the filter set for
faster wallet rescans; after an internal top-up has happened, we only
want to add the newly created scriptPubKeys.
This is useful for speeding up wallet rescans and is based on an
earlier version from PR #15845 ("wallet: Fast rescan with BIP157 block
filters"), which was never merged.
Co-authored-by: MacroFake <falke.marco@gmail.com>
This makes the stalling detection mechanism (previously a fixed
timeout of 2s) adaptive:
If we disconnect a peer for stalling, double the timeout for the
next peer - and let it slowly relax back to its default
value each time the tip advances. (Idea by Pieter Wuille)
This makes situations more unlikely in which we'd keep on
disconnecting many of our peers for stalling, even though our
own bandwidth is insufficient to download a block in 2 seconds.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
When a block is initially read from a blk*.dat file during reindexing,
it can be added to the block index only if all of its ancestor blocks
have been added, which is rare. If the block's ancestors have not been
added, the block must be re-read from disk later when it can be added.
This commit: During the initial block read, deserialize only its header,
rather than the entire block, since this is sufficient to determine
if its parent (and thus all its ancestors) has been added. This is a
performance improvement.
SkipTo() reads data from the file into the CBufferedFile object
(memory), but, unlike this object's read() method, SkipTo() doesn't
transfer data into a caller's memory buffer. This is useful because
after skipping forward in the stream in this way, the user can, if
needed, rewind the stream (SetPos()) and access the object's memory
buffer including ranges that were skipped over (without needing to
read from the disk file).
7ad15d1100 [net processing] Handle IsContinuationOfLowWorkHeadersSync return value correctly when new headers sync is started (dergoegge)
Pull request description:
This PR fixes a bug in the headers sync logic that enables submitting headers to a nodes block index that don't lead to a chain that surpasses our DoS limit.
The issue is that we ignore the return value on [the first `IsContinuationOfLowWorkHeadersSync` call after a new headers sync is started](fabc031048/src/net_processing.cpp (L2553-L2568)), which leads to us passing headers to [`ProcessNewBlockHeaders`](fabc031048/src/net_processing.cpp (L2856)) when that initial `IsContinuationOfLowWorkHeadersSync` call returns `false`. One easy way (maybe the only?) to trigger this is by sending 2000 headers where the last header has a different `nBits` value than the prior headers (which fails the pre-sync logic [here](fabc031048/src/headerssync.cpp (L189))). Those 2000 headers will be passed to `ProcessNewBlockHeaders`.
I haven't included a test here so far because we can't test this without changing the default value for `CRegTestParams::consensus.fPowAllowMinDifficultyBlocks` or doing some more involved refactoring.
ACKs for top commit:
sipa:
ACK 7ad15d1100
glozow:
ACK 7ad15d1100
Tree-SHA512: 9aabb8bf3700401e79863d0accda0befd2a83c4d469a53f97d827e51139e2f826aee08cdfbc8866b311b153f61fdac9b7aa515fcfa2a21c5e2812c2bf3c03664
It is never a nullptr, otherwise an assertion would fire in
UpdatePeerStateForReceivedHeaders.
Passing a reference makes the code easier to read and less brittle.
a079103c94 gui: update peers window "Transaction Relay" label and tooltip (Jon Atack)
Pull request description:
to current v24.0 p2p behavior. Similar updates have been made to RPC getpeerinfo and CLI -netinfo.
Top commit has no ACKs.
Tree-SHA512: 400a794f655f799eefcb77c479fef6bcd3f81aede2af54a4a9bcb7c0c783e2e3f18bc5fd2484a79e8c30af279747a05fc0ebb69dbc47375d4c55b16ceba97b99
c2a21c0670 gui: use fallback value for Version and User Agent during peer connection (Jon Atack)
Pull request description:
During connection setup for a peer, getpeerinfo returns `"version": 0, "subver": ""` and the GUI Peers window displays 0 and an empty field, respectively.
Give these fields the same behavior as the other fields in the GUI Peers window: display the fallback value in `src/qt/forms/debugwindow.ui` (i.e. `N/A`) until a valid result is available after the peer connection completes.
An alternative would be to display nothing for both, as is the case currently for User Agent.
ACKs for top commit:
jarolrod:
ACK c2a21c0670
furszy:
code ACK c2a21c06
Tree-SHA512: 4f0060fa9abde120a2bb48c9dcc87894d9bb70c33e6ab43b22400a4bcd0ceff0fa098adf7f385b0a7a4cf5d7053463b36fe1232e19a8d5025eecd8db9833f73b
fa51cc9651 refactor: Use type-safe time point for CWallet::m_next_resend (MacroFake)
Pull request description:
`GetTime` is not type-safe, thus deprecated, see 75cbbfa279/src/util/time.h (L62-L70)
ACKs for top commit:
shaavan:
Code Review ACK fa51cc9651
aureleoules:
ACK fa51cc9651
Tree-SHA512: 030de10070518580763ea75079442e2f934c54d3083be3ebe35e7f1bc6db2096745bb46d95aa1e6efe29ced30a048acfe5cd999178e6787b7647dfbec5ecb444
- Fix getblockstats for block height 0 which previously returned an error.
- Introduce alternative utxo_*_actual statistics which exclude unspendables: Genesis block, BIP30, unspendable outputs
- Update test data
- Explicitly test Genesis block results
Making the checks to identify BIP30 available outside of validation.cpp is needed for reporting and tracking statistics on specific blocks and the UTXO set correctly.
dddd1acf58 net: Set relay in version msg to peers with relay permission (MacroFake)
Pull request description:
Seems odd to set the `relay` permission in -blocksonly mode and also ask the peer not to relay transactions.
ACKs for top commit:
dergoegge:
ACK dddd1acf58
naumenkogs:
ACK dddd1acf58
mzumsande:
ACK dddd1acf58
Tree-SHA512: 7bb0e964993ea4982747ae2801fe963ff88586e2ded03015b60ab83172b5b61f2d50e9cde9d7711b7ab207f8639467ecafc4d011ea151ec6c82c722f510f4df7
deba6fe315 test: update feature_config_args.py (josibake)
2e3826cbcd util: warn if reindex is used in conf (josibake)
5e744f4238 util: disallow setting conf in bitcoin.conf (josibake)
Pull request description:
In help from `bitcoind -h` it specifes that `conf` can only be used from the commandline. However, if `conf` is set in a `bitcoin.conf` file, there is no error and from reading the logs it seems as if the `conf=<other file>` is being used, despite it being ignored. To recreate, you can setup a `bitcoin.conf` file in the default directory, add `conf=<some other file>.conf` and in the separate config file set whichever config value you want and verify that it is being ignored. alternatively, if you set `includeconf=<some other file>.conf` , your config in `<some other file>` will be picked up.
This PR fixes this by having the node error when reading the config file if `conf=` is set.
Additionally, it was mentioned in a recent [PR review club](https://bitcoincore.reviews/24858) that if `reindex=1` is set in the config file, the node will reindex on every startup, which is undesirable:
```irc
17:14 <larryruane> michaelfolkson: Reindex is requested by the user (node operator) as a configuration option (command line or in the config file, tho you probably would never put it in the file, or else it would reindex on every startup!)
```
This PR also has a commit to warn if `reindex=1` is set in the config file.
ACKs for top commit:
hebasto:
ACK deba6fe315, tested on Ubuntu 22.04.
aureleoules:
tACK deba6fe315
ryanofsky:
Code review ACK deba6fe315.
Tree-SHA512: 619fd0aa14e98af1166d6beb92651f5ba3f10d38b8ee132957f094f19c3a37313d9f4d7be2e4019f3fc9a2ca5fa42d03eb539ad820e27efec7ee58a26eb520b1
315fd4dbab test: Test for out of bounds vout in sendall (Andrew Chow)
b132c85650 wallet: Check utxo prevout index out of bounds in sendall (Andrew Chow)
708b72b715 test: Test that sendall works with watchonly spending specific utxos (Andrew Chow)
6bcd7e2a3b wallet: Correctly check ismine for sendall (Andrew Chow)
Pull request description:
The `sendall` RPC would previously fail when used with a watchonly wallet and specified inputs. This failure was caused by checking isminetype equality with ISMINE_ALL rather than a bitwise AND as IsMine can never return ISMINE_ALL.
Also added a test.
ACKs for top commit:
w0xlt:
ACK 315fd4dbab
furszy:
ACK 315fd4db
Tree-SHA512: fb55cf6524e789964770b803f401027319f0351433ea084ffa7c5e6f1797567a608c956b7f7c5bd542aa172c4b7b38b07d0976f5ec587569efead27266e8664c
3e9d0bea8d build: only run high priority benchmarks in 'make check' (furszy)
466b54bd4a bench: surround main() execution with try/catch (furszy)
3da7cd2a76 bench: explicitly make all current benchmarks "high" priority (furszy)
05b8c76232 bench: add "priority level" to the benchmark framework (furszy)
f1593780b8 bench: place benchmark implementation inside benchmark namespace (furszy)
Pull request description:
This is from today's meeting, a simple "priority level" for the benchmark framework.
Will allow us to run certain benchmarks while skip non-prioritized ones in `make check`.
By default, `bench_bitcoin` will run all the benchmarks. `make check`will only run the high priority ones,
and have marked all the existent benchmarks as "high priority" to retain the current behavior.
Could test it by modifying any benchmark priority to something different from "high", and
run `bench_bitcoin -priority-level=high` and/or `bench_bitcoin -priority-level=medium,low`
(the first command will skip the modified bench while the second one will include it).
Note: the second commit could be avoided by having a default arg value for the priority
level but.. an explicit set in every `BENCHMARK` macro call makes it less error-prone.
ACKs for top commit:
kouloumos:
re-ACK 3e9d0bea8d
achow101:
ACK 3e9d0bea8d
theStack:
re-ACK 3e9d0bea8d
stickies-v:
re-ACK 3e9d0bea8d
Tree-SHA512: ece59bf424c5fc1db335f84caa507476fb8ad8c6151880f1f8289562e17023aae5b5e7de03e8cbba6337bf09215f9be331e9ef51c791c43bce43f7446813b054
e133264c5b Add test for PSBT input verification (Greg Sanders)
d25699280a Verify PSBT inputs rather than check for fields being empty (Greg Sanders)
Pull request description:
In a few keys spots, PSBT finality is checked by looking for non-empty witness data.
This complicates a couple things:
1) Empty data can be valid in certain cases
2) User may be passed bogus final data by a counterparty during PSBT work happening, and end up with incorrect signatures that they may not be able to check in other contexts if the UTXO doesn't exist yet in chain/mempool, timelocks, etc.
On the whole I think these heavier checks are worth it in case someone is actually assuming the signatures are correct if our API is saying so.
ACKs for top commit:
achow101:
ACK e133264c5b
Tree-SHA512: 9de4fbb0be1257b081781f5df908fd55666e3acd5c4e36beb3b3f2f5a6aed69ff77068c44cde6127e159e773293fd9ced4c0bb47e693969f337e74dc8af030da
5d3f98d278 refactor: Replace m_params with chainman.GetParams() (Aurèle Oulès)
Pull request description:
Fixes a TODO introduced in #24595.
Removes `m_params` from `CChainState` class and replaces it with `m_chainman.GetParams()`.
ACKs for top commit:
MarcoFalke:
review ACK 5d3f98d278🌎
Tree-SHA512: de0fe31450d281cc7307c0d820495e86c93c7998e77a148db2c703da66cff1059e6560c041f1864913c42075aa24d259c2623d45e929ca0a8056ed330a9f9978
1c48dae76f test: Use C++11 member initializers for TestMemPoolEntryHelper (MacroFake)
fad7f2239c test: Remove unused txmempool include from tests (MacroFake)
Pull request description:
Seems odd to include this heavy header in all tests despite it only being used in a few tests.
Can be reviewed with `--color-moved=dimmed-zebra --ignore-all-space`
ACKs for top commit:
aureleoules:
reACK 1c48dae76f
hebasto:
ACK 1c48dae76f, I have reviewed the code and it looks OK, I agree it can be merged.
w0xlt:
ACK 1c48dae76f
Tree-SHA512: 31f2808d04ec33bfc2409832b8e59e6c870eaa98fbcf879e1c786492c7d07134711b30f8290bdb34e1b8f7b8f2f11dae8e10c64e7eb31f584b2f5c58fcc7743b
b147322a7a Use `PACKAGE_NAME` in messages rather than hardcoding "Bitcoin Core" (Hennadii Stepanov)
Pull request description:
Usually, we do not hardcode "Bitcoin Core" in the user-faced messages.
See:
- bitcoin/bitcoin#18646
- bitcoin/bitcoin#19282
Also grammar has been improved -- singular instead of plural.
ACKs for top commit:
jarolrod:
ACK b147322a7a
Tree-SHA512: b135c18703dfdd7b63d4cb27d1ac48f6a9dbf69382142ae381f33bf561cbf57477a11d1c73263aa834f705206d7dd5716df2523d38ed0d4cfec8babc38bb017a
This changes the flag for the bitcoin-chainstate executable. Previously
it was false, now it is the chain's default value (still false for the
main chain).
This changes the minimum chain work for the bitcoin-chainstate
executable. Previously it was uint256{}, now it is the chain's default
minimum chain work.
af781bf4b2 doc: fix typo in doc/libraries.md (fanquake)
9e9ae6101f doc: remove library commentary from src/Makefile.am (fanquake)
Pull request description:
Deduplicate the makefile comments, in favour of doc/libraries.md. I think a single, more comprehensive source of truth is preferable. Diagrams are also useful. Came up in https://github.com/bitcoin/bitcoin/pull/26292#issuecomment-1275094478.
ACKs for top commit:
ryanofsky:
Code review ACK af781bf4b2, nice cleanups
hebasto:
ACK af781bf4b2, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: df61ed1394102221701ae2dfa42886dfabe9d9fd7f601b794e2195f93d8f7c2a1cd1c000a77d0a969b42328e8ebc0387755c57291837b283fdf376dbd98fdda1
We optimistically pre-register a peer for txreconciliations
upon sending txreconciliation support announcement.
But if, at VERACK, we realize that the peer never sent
WTXIDRELAY message, we should unregister the peer
from txreconciliations, because txreconciliations rely on wtxids.
Once we received a reconciliation announcement support
message from a peer and it doesn't violate our protocol,
we store the negotiated parameters which will be used
for future reconciliations.
If we're connecting to the peer which might support
transaction reconciliation, we announce we want to reconcile
with them.
We store the reconciliation salt so that when the peer
responds with their salt, we are able to compute the
full reconciliation salt.
This behavior is enabled with a CLI flag.
e899d4ca6f init: limit bip30 exceptions to coinbase txs (Chris Geihsler)
511eb7fdea Ignore problematic blocks in DisconnectBlock (Chris Geihsler)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/22596
When using checklevel=4, block verification fails because of duplicate coinbase transactions involving blocks 91812 and 91722. There was already a check in place within `ConnectBlock` to ignore the problematic blocks, but `DisconnectBlock` did not contain a similar check to ignore these blocks when called from `VerifyDB`.
By ignoring these two blocks in `DisconnectBlock`, the block verification process succeeds at checklevel=4.
(Note to reviewers: this is my first contribution to Bitcoin Core, so any feedback is most welcome. Thanks in advance for reviewing!)
## Steps to reproduce:
Use the following bitcoin.conf file and start bitcoind. I only used block data through block ~100000 so that the verification process was much faster.
```
assumevalid=0
checkblocks=0
checklevel=4
```
Without this change, you will see the following error when the blocks are verified:
```
2022-04-14T02:56:44Z init message: Verifying blocks…
2022-04-14T02:56:44Z Verifying last 101881 blocks at level 4
2022-04-14T02:56:44Z [0%]...[10%]...[20%]...[30%]...[40%]...ERROR: VerifyDB(): *** coin database inconsistencies found (last 10160 blocks, 142571 good transactions before that)
2022-04-14T02:57:01Z : Corrupted block database detected.
Please restart with -reindex or -reindex-chainstate to recover.
: Corrupted block database detected.
Please restart with -reindex or -reindex-chainstate to recover.
```
With this change, you will see this instead:
```
2022-04-14T02:32:29Z init message: Verifying blocks…
2022-04-14T02:32:29Z Verifying last 101746 blocks at level 4
2022-04-14T02:32:29Z [0%]...[10%]...[20%]...[30%]...[40%]...[50%]...[60%]...[70%]...[80%]...[90%]...[DONE].
2022-04-14T02:32:48Z No coin database inconsistencies in last 101746 blocks (226126 transactions)
```
ACKs for top commit:
laanwj:
Code review ACK e899d4ca6f
achow101:
ACK e899d4ca6f
jamesob:
(Biased) ACK e899d4ca6f ([`jamesob/ackr/24851.2.seejee.init_ignore_bip_30_verif`](https://github.com/jamesob/bitcoin/tree/ackr/24851.2.seejee.init_ignore_bip_30_verif))
Tree-SHA512: d2f6d25e9619aee32c1a73fe846b1b587698eaa5a4994fa6424f1038f45654f9fd52b74a69843cc84d90168d74827130ccf8e9201502f5d52281acdb20429291
a8250e30f1 doc: add release note about `/rest/deploymentinfo` (brunoerg)
5c96020024 doc: add `/deploymentinfo` in REST-interface (brunoerg)
3e44bee08e test: add coverage for `/rest/deploymentinfo` (brunoerg)
91497031cb rest: add `/deploymentinfo` (brunoerg)
Pull request description:
#23508 added a new RPC named `getdeploymentinfo`, it moved the softfork section from `getblockchaininfo` into this new one. In the REST interface, we have an endpoint named`/rest/chaininfo.json` (which refers to `getblockchaininfo`), so, this PR adds a new REST endpoint named `/deploymentinfo` which refers to `getdeploymentinfo`.
You can use it by passing a block hash, e.g: '/rest/deploymentinfo/<BLOCKHASH>.json' or you can use it without passing a block hash to get the 'deploymentinfo' for the last block.
ACKs for top commit:
jonatack:
re-ACK a8250e30f1 rebase-only since my last review at c65f82bb
achow101:
ACK a8250e30f1
stickies-v:
re-ACK a8250e30f1
Tree-SHA512: 0735183b6828d51a72ed0e2be5a09b314ac4693f548982c6e9adaa0ef07a55aa428d3b2d1b1de70b83169811a663a8624b686166e5797f624dcc00178b9796e6
8173f160e0 style: rename variables to match coding style (Vasil Dimov)
8b4ad203d0 fees: make FeeFilterRounder::feeset const (Vasil Dimov)
e7a5bf6be7 fees: make the class FeeFilterRounder thread-safe (Vasil Dimov)
Pull request description:
Make the class `FeeFilterRounder` thread-safe so that its methods can be called concurrently by different threads on the same object. Currently it has just one method (`round()`).
The second commit is optional, but it improves readability, showing that the `feeset` member will never be changed, thus does not need protection from concurrent access.
ACKs for top commit:
jonatack:
re-ACK 8173f160e0
laanwj:
Code review ACK 8173f160e0
promag:
Code review ACK 8173f160e0
Tree-SHA512: 94b809997c485c0d114fa702d0406b980be8eaaebcfefa56808ed670aa943959c2f16cfd0ef72b4752fe2a409a23af1b4b7f2f236e51212957759569e3bbbefd
bfb9b94ebe wallet: remove duplicate descriptor type check in GetNewDestination (furszy)
76b982a4a5 wallet: remove unused `nAccountingEntryNumber` field (furszy)
599ff5adfc wallet: avoid double TopUp() calls on descriptor wallets (furszy)
Pull request description:
Found it while was digging over a `getnewaddress` timeout on the functional test suite.
### Context:
We are calling `TopUp()` twice in the following flows for descriptor wallets:
A) `CWallet::GetNewDestination`:
1) Calls spk_man->TopUp()
2) Calls spk_man->GetNewDestination() --> which, after the basic script checks, calls TopUp() again.
B) `CWallet::GetReservedDestination`:
1) Calls spk_man->TopUp()
2) Calls spk_man->GetReservedDestination() --> which calls to GetNewDestination (which calls to TopUp again).
### Changes:
Move `TopUp()` responsibility from the wallet class to each scriptpubkeyman.
So each spkm can decide to call it or not after perform the basic checks
for the new destination request.
Aside from that, remove the unused `nAccountingEntryNumber` wallet field. And a duplicated descriptor type check in `GetNewDestination`
ACKs for top commit:
aureleoules:
re-ACK bfb9b94ebe.
achow101:
ACK bfb9b94ebe
theStack:
Code-review ACK bfb9b94ebe
Tree-SHA512: 3ab73f37729e50d6c6a4434f676855bc1fb404619d63c03e5b06ce61c292c09c59d64cb1aa3bd9277b06f26988956991d62c90f9d835884f41ed500b43a12058
a3789c700b Improve getpeerinfo pingtime, minping, and pingwait help docs (Jon Atack)
df660ddb1c Update getpeerinfo/-netinfo/TxRelay#m_relay_txs relaytxes docs (for v24 backport) (Jon Atack)
1f448542e7 Always return getpeerinfo "minfeefilter" field (for v24 backport) (Jon Atack)
9cd6682545 Make getpeerinfo field order consistent with its help (for v24 backport) (Jon Atack)
Pull request description:
Various updates and fixups, mostly targeting v24. Please refer to the commit messages for details.
ACKs for top commit:
achow101:
ACK a3789c700b
brunoerg:
ACK a3789c700b
vasild:
ACK a3789c700b
Tree-SHA512: b8586a9b83c1b18786b5ac1fc1dba91573c13225fc2cfc8d078f4220967c95056354f6be13327f33b4fcf3e9d5310fa4e1bdc93102cbd6574f956698993a54bf
626b7c8493 fuzz: add scanblocks as safe for fuzzing (James O'Beirne)
94fe5453c7 test: rpc: add scanblocks functional test (Jonas Schnelli)
6ef2566b68 rpc: add scanblocks - scan for relevant blocks with descriptors (Jonas Schnelli)
a4258f6e81 rpc: move-only: consolidate blockchain scan args (James O'Beirne)
Pull request description:
Revives #20664. All feedback from the previous PR has either been responded to inline or incorporated here.
---
Major changes from Jonas' PR:
- consolidated arguments for scantxoutset/scanblocks
- substantial cleanup of the functional test
Here's the range-diff (`git range-diff master jonasschnelli/2020/12/filterblocks_rpc jamesob/2021-11-scanblocks`): https://gist.github.com/jamesob/aa4a975344209f0316444b8de2ec1d18
### Original PR description
> The `scanblocks` RPC call allows one to get relevant blockhashes from a set of descriptors by scanning all blockfilters in a given range.
>
> **Example:**
>
> `scanblocks start '["addr(<bitcoin_address>)"]' 661000` (returns relevant blockhashes for `<bitcoin_address>` from blockrange 661000->tip)
>
> ## Why is this useful?
> **Fast wallet rescans**: get the relevant blocks and only rescan those via `rescanblockchain getblockheader(<hash>)[height] getblockheader(<hash>)[height])`. A future PR may add an option to allow to provide an array of blockhashes to `rescanblockchain`.
>
> **prune wallet rescans**: (_needs additional changes_): together with a call to fetch blocks from the p2p network if they have been pruned, it would allow to rescan wallets back to the genesis block in pruned mode (relevant #15946).
>
> **SPV mode** (_needs additional changes_): it would be possible to build the blockfilterindex from the p2p network (rather then deriving them from the blocks) and thus allow some sort of hybrid-SPV mode with moderate bandwidth consumption (related #9483)
ACKs for top commit:
furszy:
diff re-ACK 626b7c8
Tree-SHA512: f84e4dcb851b122b39e9700c58fbc31e899cdcf9b587df9505eaf1f45578cc4253e89ce2a45d1ff21bd213e31ddeedbbcad2c80810f46755b30acc17b07e2873
bf95976061 doc: add note about snapshot chainstate init (James O'Beirne)
e4d7995286 test: add testcases for snapshot initialization (James O'Beirne)
cced4e7336 test: move-only-ish: factor out LoadVerifyActivateChainstate() (James O'Beirne)
51fc9241c0 test: allow on-disk coins and block tree dbs in tests (James O'Beirne)
3c361391b8 test: add reset_chainstate parameter for snapshot unittests (James O'Beirne)
00b357c215 validation: add ResetChainstates() (James O'Beirne)
3a29dfbfb2 move-only: test: make snapshot chainstate setup reusable (James O'Beirne)
8153bd9247 blockmanager: avoid undefined behavior during FlushBlockFile (James O'Beirne)
ad67ff377c validation: remove snapshot datadirs upon validation failure (James O'Beirne)
34d1590331 add utilities for deleting on-disk leveldb data (James O'Beirne)
252abd1e8b init: add utxo snapshot detection (James O'Beirne)
f9f1735f13 validation: rename snapshot chainstate dir (James O'Beirne)
d14bebf100 db: add StoragePath to CDBWrapper/CCoinsViewDB (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: https://github.com/bitcoin/bitcoin/pull/15606)
---
Half of the replacement for #24232. The original PR grew larger than expected throughout the review process.
This change adds the ability to initialize a snapshot-based chainstate during init if one is detected on disk. This is of course unused as of now (aside from in unittests) given that we haven't yet enabled actually loading snapshots.
Don't be scared! There are some big move-only commits in here.
Accompanying changes include:
- moving the snapshot coinsdb directory from being called `chainstate_[base blockhash]` to `chainstate_snapshot`, since we only support one snapshot in use at a time. This simplifies some logic, but it necessitates writing that base blockhash out to a file within the coinsdb dir. See [discussion here](https://github.com/bitcoin/bitcoin/pull/24232#discussion_r832762880).
- adding a simple fix in `FlushBlockFile()` that avoids a crash when attemping to flush to disk before `LoadBlockIndexDB()` is called, which happens when calling `MaybeRebalanceCaches()` during multiple chainstate init.
- improving the unittest to allow testing with on-disk chainstates - necessary to test a simulated restart and re-initialization.
ACKs for top commit:
naumenkogs:
utACK bf95976061
ariard:
Code Review ACK bf9597606
ryanofsky:
Code review ACK bf95976061. Changes since last review: rebasing, switching from CAutoFile to AutoFile, adding comments, switching from BOOST_CHECK to Assert in test util, using chainman.GetMutex() in tests, destroying one ChainstateManager before creating a new one in tests
fjahr:
utACK bf95976061
aureleoules:
ACK bf95976061
Tree-SHA512: 15ae75caf19f8d12a12d2647c52897904d27b265a7af6b4ae7b858592eeadb8f9da6c2394b6baebec90adc28742c053e3eb506119577dae7c1e722ebb3b7bcc0
9e386afb67 tests: Test that PSBT_OUT_TAP_TREE is included correctly (Andrew Chow)
30ff25cf37 psbt: Only include m_tap_tree if it has scripts (Andrew Chow)
0577d423ad psbt: Change m_tap_tree to store just the tuples (Andrew Chow)
22c051ca70 tests: Test that PSBT_OUT_TAP_TREE is combined correctly (Andrew Chow)
7df6e1bb77 psbt: Fix merging of m_tap_tree (Andrew Chow)
0652dc53b2 [BugFix]: Do not allow deserializing PSBT with empty PSBT_OUT_TAP_TREE (Jeremy Rubin)
Pull request description:
PSBT_OUT_TAP_TREE should not be included for outputs that do not have such a tree. This should be disallowed during parsing, as well as prior to serialization when the field is populated during updating.
Also added some test cases.
Alternative to #25856
ACKs for top commit:
instagibbs:
ACK 9e386afb67
darosior:
ACK 9e386afb67
Tree-SHA512: ce5c02a69752d176dbd967c1e8d30129b1905c8f186aeeef034576c1de82059271a1ee846bd040f5be4e66bb77ba711dcf14ac1e597c5707d7e7e2293f6cfefb
b01682a812 refactor: revert m_next_resend to not be std::atomic (stickies-v)
9245f45670 wallet: only update m_next_resend when actually resending (stickies-v)
7fbde8af5c refactor: carve out tx resend timer logic into ShouldResend (stickies-v)
01f3534632 refactor: remove unused locks for ResubmitWalletTransactions (stickies-v)
c6e8e11fb0 wallet: fix capitalization in docstring (stickies-v)
Pull request description:
This PR addresses the outstanding comments/issues from #25768:
- capitalization [typo](https://github.com/bitcoin/bitcoin/pull/25768#discussion_r958572522) in docstring
- remove [unused locks](01f3534632) that we previously needed for `ReacceptWalletTransactions()`
- before #25768, only `ResendWalletTransactions()` would reset `m_next_resend` (formerly called `nNextResend`). By unifying it with `ReacceptWalletTransactions()` into `ResubmitWalletTransactions()`, the number of callsites that would reset the `m_next_resend` timer increased
- since `m_next_resend` is only used in case of `relay=true` (formerly `ResendWalletTransactions()`), this is unintuitive
- it leads to [unexpected behaviour](https://github.com/bitcoin/bitcoin/pull/25768#issuecomment-1252619427) such as transactions potentially never being rebroadcasted.
- it makes the ResubmitWalletTransactions()` logic [more complicated than strictly necessary](https://github.com/bitcoin/bitcoin/pull/25768#discussion_r962828563)
- since #25768, we relied on an earlier call of `ResubmitWalletTransactions(relay=false, force=true)` to initialize `m_next_resend()`, I think we can more elegantly do that by just providing `m_next_resend` with a default value
- just to highlight: this commit introduces behaviour change
Note: the `if (!fBroadcastTransactions)` in `CWallet:ShouldResend()` is duplicated on purpose, since it potentially avoids the slightly more expensive `if (!chain().isReadyToBroadcast())` check afterwards. I don't have a strong view on it, so happy to remove that additional check to reduce the diff, too.
ACKs for top commit:
aureleoules:
ACK b01682a812
achow101:
ACK b01682a812
Tree-SHA512: ac5f1d8858f8dd736dd1480f385984d660c1916b62a42562317020e8f9fd6a30bd8f23d973d47e4c9480d744c5ba39fdbefd69568a5eb0589a8422d7e5971c1c
861cb3fadc test: move SyncWithValidationInterfaceQueue() before Stop() in txindex_tests (Vasil Dimov)
6526dc3b78 test: silence TSAN false positive in coinstatsindex_initial_sync (Vasil Dimov)
Pull request description:
Silence false positives from TSAN about unsynchronized calls to `BaseIndex::~BaseIndex()` and `BaseIndex::SetBestBlockIndex()`. They are synchronized, but beyond the comprehension of TSAN - by `SyncWithValidationInterfaceQueue()`, called from `BaseIndex::BlockUntilSyncedToCurrentChain()`.
Fixes https://github.com/bitcoin/bitcoin/issues/25365
ACKs for top commit:
MarcoFalke:
review ACK 861cb3fadc
ryanofsky:
Code review ACK 861cb3fadc. Just comment change since last review.
Tree-SHA512: 8c30fdf2fd11d54e9adfa68a67185ab820bd7bd9f7f3ad6456e7e6d219fa9cf6d34b41e98e723eae86cb0c1baef7f3fc57b1b011a13dc3fe3d78334b9b5596de
b527b54950 net: convert standalone SetSocketNonBlocking() to Sock::SetNonBlocking() (Vasil Dimov)
29f66f7682 moveonly: move SetSocketNonBlocking() from netbase to util/sock (Vasil Dimov)
b4bac55679 net: convert standalone IsSelectableSocket() to Sock::IsSelectable() (Vasil Dimov)
5db7d2ca0a moveonly: move IsSelectableSocket() from compat.h to sock.{h,cpp} (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
* convert standalone `IsSelectableSocket()` to `Sock::IsSelectable()`
* convert standalone `SetSocketNonBlocking()` to `Sock::SetNonBlocking()`
This further encapsulates syscalls inside the `Sock` class and makes the callers mockable.
ACKs for top commit:
jonatack:
ACK b527b54950 review/debug build/unit tests at each commit, cross-referenced the changes with `man select` and `man errno`, ran a signet node on the last commit with ip4/ip6//tor/i2p/cjdns and network connections were nominal
dergoegge:
Code review ACK b527b54950
Tree-SHA512: af783ce558c7a89e173f7ab323fb3517103d765c19b5d14de29f64706b4e1fea3653492e8ea73ae972699986aaddf2ae72c7cfaa7dad7614254283083b7d2632
bcb0cacac2 reindex, log, test: fixes#21379 (mruddy)
Pull request description:
Fixes#21379.
The blocks/blk?????.dat files are mutated and become increasingly malformed, or corrupt, as a result of running the re-indexing process.
The mutations occur after the re-indexing process has finished, as new blocks are appended, but are a result of a re-indexing process miscalculation that lingers in the block manager's `m_blockfile_info` `nSize` data until node restart.
These additions to the blk files are non-fatal, but also not desirable.
That is, this is a form of data corruption that the reading code is lenient enough to process (it skips the extra bytes), but it adds some scary looking log messages as it encounters them.
The summary of the problem is that the re-index process double counts the size of the serialization header (magic message start bytes [4 bytes] + length [4 bytes] = 8 bytes) while calculating the blk data file size (both values already account for the serialization header's size, hence why it is over accounted).
This bug manifests itself in a few different ways, after re-indexing, when a new block from a peer is processed:
1. If the new block will not fit into the last blk file processed while re-indexing, while remaining under the 128MiB limit, then the blk file is flushed to disk and truncated to a size that is 8 greater than it should be. The truncation adds zero bytes (see `FlatFileSeq::Flush` and `TruncateFile`).
1. If the last blk file processed while re-indexing has logical space for the new block under the 128 MiB limit:
1. If the blk file was not already large enough to hold the new block, then the zeros are, in effect, added by `fseek` when the file is opened for writing. Eight zero bytes are added to the end of the last blk file just before the new block is written. This happens because the write offset is 8 too great due to the miscalculation. The result is 8 zero bytes between the end of the last block and the beginning of the next block's magic + length + block.
1. If the blk file was already large enough to hold the new block, then the current existing file contents remain in the 8 byte gap between the end of the last block and the beginning of the next block's magic + length + block. Commonly, when this occcurs, it is due to the blk file containing blocks that are not connected to the block tree during reindex and are thus left behind by the reindex process and later overwritten when new blocks are added. The orphaned blocks can be valid blocks, but due to the nature of concurrent block download, the parent may not have been retrieved and written by the time the node was previously shutdown.
ACKs for top commit:
LarryRuane:
tested code-review ACK bcb0cacac2
ryanofsky:
Code review ACK bcb0cacac2. This is a disturbing bug with an easy fix which seems well-worth merging.
mzumsande:
ACK bcb0cacac2 (reviewed code and did some testing, I agree that it fixes the bug).
w0xlt:
tACK bcb0cacac2
Tree-SHA512: acc97927ea712916506772550451136b0f1e5404e92df24cc05e405bb09eb6fe7c3011af3dd34a7723c3db17fda657ae85fa314387e43833791e9169c0febe51
fa08663344 rpc: Return coinbase flag in scantxoutset (MacroFake)
Pull request description:
I guess it can't hurt to return this for someone that wants to know it
ACKs for top commit:
aureleoules:
ACK fa08663344
shaavan:
ACK fa08663344
Tree-SHA512: 04c554b3ed9877bab93ffcf0c1a4430cd41b30c5f4f3bf462a518fc8b3d68832dd85a29e81bd805eaa16e987856933d7a888a8c126f670bb2844bbd5ca1bf902
04526787b5 Validate `port` options (amadeuszpawlik)
f8387c4234 Validate port value in `SplitHostPort` (amadeuszpawlik)
Pull request description:
Validate `port`-options, so that invalid values are rejected early in the startup.
Ports are `uint16_t`s, which effectively limits a port's value to <=65535. As discussed in https://github.com/bitcoin/bitcoin/pull/24116 and https://github.com/bitcoin/bitcoin/pull/24344, port "0" is considered invalid too.
Proposed in https://github.com/bitcoin/bitcoin/issues/21893#issuecomment-835784223
The `SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut)` now returns a bool that indicates whether the port value was set and within the allowed range. This is an improvement that can be used not only for port validation of options at startup, but also in rpc calls, etc,
ACKs for top commit:
luke-jr:
utACK 04526787b5
ryanofsky:
Code review ACK 04526787b5. Just suggested changes since last review: reverting some SplitHostPort changes, adding release notes, avoiding 'GetArgs[0]` problem.
Tree-SHA512: f1ac80bf98520b287a6413ceadb41bc3a93c491955de9b9319ee1298ac0ab982751905762a287e748997ead6198a8bb7a3bc8817ac9e3d2468e11ab4a0f8496d
75c3f9f880 sync: rename AnnotatedMixin::UniqueLock to AnnotatedMixin::unique_lock (Vasil Dimov)
8d9ee8efe8 sync: remove DebugLock alias template (Vasil Dimov)
4b2e16763f sync: avoid confusing name overlap (Mutex) (Vasil Dimov)
9d7ae4b66c sync: remove unused template parameter from ::UniqueLock (Vasil Dimov)
11c190e3f1 sync: simplify MaybeCheckNotHeld() definitions by using a template (Vasil Dimov)
Pull request description:
Summary:
* Reduce 4 of the `MaybeCheckNotHeld()` definitions to 2 by using a template.
* Remove unused template parameter from `::UniqueLock`.
* Use `MutexType` instead of `Mutex` for a template parameter name to avoid overlap/confusion with the `Mutex` class.
* Rename `AnnotatedMixin::UniqueLock` to `AnnotatedMixin::unique_lock` to avoid overlap/confusion with the global `UniqueLock` and for consistency with `UniqueLock::reverse_lock`.
The first commit `sync: simplify MaybeCheckNotHeld() definitions by using a template` is also part of https://github.com/bitcoin/bitcoin/pull/25390
ACKs for top commit:
aureleoules:
ACK 75c3f9f880 - LGTM
ryanofsky:
Code review ACK 75c3f9f880. Nice cleanups! Just suggested changes since last review: keeping UniqueLock name and fixing a missed rename in a code comment
Tree-SHA512: ec261f6a444bdfe4f06e844b57b3606fdd9b2f842647cae15266d9729970d87585c808d482fbba0b31c33a4aa03527c36e282c92b28d9052711f75a7048c96f1
fabf1cdb20 Use steady clock for bench logging (MacroFake)
faed342a23 scripted-diff: Rename time symbols (MacroFake)
Pull request description:
Instead of using `0.001` and similar constants to "convert" an int64_t to milliseconds, use the type-safe `Ticks<>` helper. Also, use steady clock instead of system clock, since the durations are used for benchmarking.
ACKs for top commit:
fanquake:
ACK fabf1cdb20 - validation bench output still looks sane.
Tree-SHA512: e6525b5fdad6045ca500c56014897d7428ad288aaf375933d3b5939feddf257f6910d562eb66ebcde9186bef9a604ee8d763a318253838318d59df2a285be7c2
43b8777dc3 refactor: move run_command from util to common (Cory Fields)
192325a77d kernel: move RunCommandParseJSON to its own file (Cory Fields)
Pull request description:
Because libbitcoinkernel does not include this new object, this has the side-effect of eliminating its unnecessary `boost::process` dependency.
This leaves libbitcoinkernel with 3 remaining boost dependencies:
- `boost::date_time` for `util/time.cpp`, which I'll separate out next. Exactly like this PR.
- `boost::signals2` for which I have a POC re-implementation here: https://github.com/theuni/bitcoin/commits/replace-boost-signals
- `boost::multi_index` which I'm not sure about yet.
ACKs for top commit:
ryanofsky:
Code review ACK 43b8777dc3. Could consider squashing the two commits, so the code just moves once instead of twice.
fanquake:
ACK 43b8777dc3
Tree-SHA512: f2a46cac34aaadfb8a1442316152ad354f6990021b82c78d80cae9fd43cd026209ffd62132eaa99d5d0f8cf34e996b6737d318a9d9a3f1d2ff8d17d697abf26d
1c36bafc5f wallet: have prune error take precedence over assumedvalid (James O'Beirne)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/pull/23997#discussion_r891412739.
From Russ Yanofsky:
> Agree with all of Marco's points here and think this should be updated
>
> If havePrune and hasAssumedValidChain are both true, better to show havePrune error message. Assumed-valid error message is vague and not very actionable. Would suggest "Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}"
ACKs for top commit:
MarcoFalke:
ACK 1c36bafc5f
aureleoules:
ACK 1c36bafc5f
Tree-SHA512: bfb0024bb962525cbbd392ade3c0331a8b0525e7f2f2ab52b2dbb9b6dd6311070d85ecb762a7689db84a30991971865698ab6fec187206e6a92133790c5a91dc
faa15527d7 test: Use dedicated mempool in TestBasicMining (MacroFake)
fafab384a0 test: Use dedicated mempool in TestPackageSelection (MacroFake)
fa4055d79c test: Use dedicated mempool in TestPrioritisedMining (MacroFake)
fa29218285 test: Pass mempool reference to AssemblerForTest (MacroFake)
Pull request description:
This cleans up the miner tests:
* Removes duplicate/redundant and thus confusing chainparams object.
* Uses a fresh mempool for each subtest instead of using the "global" one from the testing setup. This makes it easier to follow the tests in smaller scopes. Also it makes sure the mempool is truly cleared by reconstructing it. Finally, this removes calls to `clear`, see https://github.com/bitcoin/bitcoin/pull/19909
ACKs for top commit:
glozow:
utACK faa15527d7
Tree-SHA512: ced1260f6ab70fba74b0fac7ff4fc7adfddcd2f3bee785249d2a4a9055ac253eff9090edbda7a17e72a71a81b56ff708d5ff64e1f57ebc7b7747d6c88fec51e3
adb1714426 Fix comment typos in scriptpubkeyman.cpp, wallet.cpp, wallet.h (Dimitris Tsapakidis)
Pull request description:
Fixes a number of comment typos found in the code.
Top commit has no ACKs.
Tree-SHA512: c2c996b66d33ecf0ee734b76303a0f2444e184d2f3ff6931768712ca51011ad51e54336c33a2ff55133766d20ae6adcbb14ddc754dde58b1fe9167d68f54fec5
Use `UniqueLock` directly. Type deduction works just fine from the first
argument to the constructor of `UniqueLock`, so there is no need to
repeat
```cpp
UniqueLock<typename std::remove_reference<typename std::remove_pointer<decltype(cs)>::type>::type>
```
five times in the `LOCK` macros. Just `UniqueLock` suffices.
Use `MutexType` instead of `Mutex` for the template parameter of
`UniqueLock` because there is already a class named `Mutex` and the
naming overlap is confusing. `MutexType` is used elsewhere in `sync.h`.
8891949bdc index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Ryan Ofsky)
Pull request description:
Since commit f08c9fb0c6 from PR https://github.com/bitcoin/bitcoin/pull/21726, index `BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has also been a race condition in the `coinstatsindex_initial_sync` unit test.
It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the last connected block to be fully processed, than to be able to return before prune locks are set, so this switches the order of `m_best_block_index = block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more reliable.
Also since commit f08c9fb0c6, there has been a race condition in the `coinstatsindex_initial_sync` test. Before that commit, the atomic index best block pointer `m_best_block_index` was updated as the last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain` could safely be used in tests to wait for the last `BlockConnected` notification to be finished before stopping and destroying the index. But after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer sufficient, and there is a race between the test shutdown code which destroys the index object and the new code introduced in that commit calling `AllowPrune()` and `GetName()` on the index object. Reproducibility instructions for this are in https://github.com/bitcoin/bitcoin/issues/25365#issuecomment-1259744133
This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, https://github.com/bitcoin/bitcoin/pull/26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors https://github.com/bitcoin/bitcoin/issues/25365.
There is no known race condition outside of test code currently, because the bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not `BlockUntilSyncedToCurrentChain` to safely shut down.
Co-authored-by: vasild
Co-authored-by: MarcoFalke
ACKs for top commit:
mzumsande:
re-ACK 8891949bdc
Tree-SHA512: 52e29e3772a0c92873c54e5ffb31dd66a909b68a2031b7585713cd1d976811289c98bd9bb41679a8689062f03be4f97bb8368696e789caa4607c2fd8b1fe289b
fabbbe32ee Remove unused CDataStream::rdbuf method (MacroFake)
Pull request description:
It is unused and seems unlikely to be ever used.
ACKs for top commit:
theStack:
Code-review ACK fabbbe32ee
aureleoules:
ACK fabbbe32ee
Tree-SHA512: 5804642658f96a0fb51482ebf3a062bb0f997c1e0527455afa4aceeeb6c1ad139a98b14a7c8a0909daba733a83bdc24fcadad45060ead4be6eb3dc3e66c129e2
33b12e5df6 docs: improve docs where MemPoolLimits is used (stickies-v)
6945853c0b test: use NoLimits() in MempoolIndexingTest (stickies-v)
3a86f24a4c refactor: mempool: use CTxMempool::Limits (stickies-v)
b85af25f87 refactor: mempool: add MemPoolLimits::NoLimits() (stickies-v)
Pull request description:
Mempool currently considers 4 limits regarding ancestor and descendant count and size, which get passed around between functions quite a bit. This PR uses `CTxMemPool::Limits` introduced in https://github.com/bitcoin/bitcoin/pull/25290 to simplify those signatures and callsites.
The purpose of this PR is to improve readability and maintenance, without behaviour change.
As noted in the first commit "refactor: mempool: change MemPoolLimits members to uint", we currently have an underflow issue where a user could pass a negative `-limitancestorsize`, which is eventually cast to an unsigned integer. This behaviour already exists. Because it's orthogonal and to minimize scope, I think this should be fixed in a separate PR.
ACKs for top commit:
hebasto:
ACK 33b12e5df6, I have reviewed the code and it looks OK, I agree it can be merged.
glozow:
reACK 33b12e5df6
Tree-SHA512: 591c6dcee1894f1c3ca28b34a680eeadcf0d40cda92451b4a422c03087b27d682b5e30ba4367abd75a99b5ccb115b7884b0026958d3c7dddab030549db5a4056
01bf4af4f2 docs: fix m_children to be a member of CTxMemPoolEntry (stickies-v)
Pull request description:
Small documentation fix to reflect that `m_children` [is a member](73b61717a9/src/txmempool.h (L99)) of `CTxMemPoolEntry`, not `CTxMemPool`
ACKs for top commit:
hebasto:
ACK 01bf4af4f2, wrong wording was introduced in bitcoin/bitcoin#19478.
glozow:
ACK 01bf4af4f2
Tree-SHA512: b66c43b92fda44682b1f67c43073ca9e133a6dc03cd28253e571e67170531138c20b22ffdb08f312fb2d47a1f869b876611646b54325c8b614d12049befad578
From Russ Yanofsky:
"Agree with all of Marco's points here and think this should be updated
If havePrune and hasAssumedValidChain are both true, better to show
havePrune error message. Assumed-valid error message is vague and not
very actionable. Would suggest "Error loading wallet. Wallet requires
blocks to be downloaded, and software does not currently support loading
wallets while blocks are being downloaded out of order though assumeutxo
snapshots. Wallet should be able to load successfully after node sync
reaches height {block_height}"
Co-authored-by: MacroFake <MarcoFalke@gmail.com>
Co-authored-by: Russ Yanofsky <russ@yanofsky.org>
Previously vExtraTxnForCompact and vExtraTxnForCompactIt were protected
by g_cs_orphans; protect them by g_msgproc_mutex instead, as they
are only used during message processing.
Help from `bitcoind -h` states that conf can only be used from the commandline.
However, if conf is set in a bitcoin.conf file, it is ignored but there is no error.
Show an error to user if conf is set in a .conf file and prompt them to use
`includeconf` if they wish to specify additional config files.
Adds `IsConfSupported` function to allow for easily adding conf options
to disallow or throw warnings for.
Instead of having an entire TaprootBuilder which may or may not be
complete, and could potentially have future changes that interact oddly
with taproot tree tuples, have m_tap_tree be just the tuples.
When needed in other a TaprootBuilder for actual use, the tuples will be
added to a a TaprootBuilder that, in the future, can take in whatever
other data is needed as well.
Merging should be checking that the current PSBTOutput doesn't have a
taptree and the other one's is copied over. The original merging had
this inverted and would remove m_tap_tree if the other did not have it.
0f40d65321 refactor: remove duplicate code from BlockAssembler (James O'Beirne)
Pull request description:
Found while reminding myself how transactions are chosen for blocks. Take it or leave it!
ACKs for top commit:
glozow:
ACK 0f40d65321
theStack:
Concept and code-review ACK 0f40d65321
Tree-SHA512: 8a2694e670ce3fe897ab8f64f64c8df5f8487fc1264527a3abbcba0e5b921fb693416497ccd62508295bc33f202c65556b91b6af463acb91aab43138d2492c14
2147483647 is the maximum positive value of a signed int32, and - currently -
the maximum value that the deriveaddresses bitcoin RPC call accepts as
derivation index due to its input validation routines.
Before this change, when the derivation index (and thus range_end) reached
std::numeric_limits<int_32_t>::max(), the "i" variable in the for cycle (which
is declared as int, and as such 32 bits in size on most platforms) would be
incremented at the end of the first iteration and then warp back to
-2147483648. This caused SIGABRT in bitcoind and a core dump.
This change assigns "i" an explicit size of 64 bits on every platform,
sidestepping the problem.
Fixes#26274.
Check `port` options for invalid values (ports are parsed as uint16, so
in practice values >65535 are invalid; port 0 is undefined and therefore
considered invalid too). This allows for an early rejection of faulty
values and an supplying an informative message to the user.
Splits tests in `feature_proxy.py` to cover both invalid `hostname`
and `port` values.
Adds a release-note as previously valid `-port` and `-rpcport` values
can now result in errors.
Forward the validation of the port from `ParseUInt16(...)`.
Consider port 0 as invalid.
Add suitable test for the `SplitHostPort` function.
Add doxygen description to the `SplitHostPort` function.
Since commit f08c9fb0c6 from PR
https://github.com/bitcoin/bitcoin/pull/21726, index
`BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has
also been a race condition in the `coinstatsindex_initial_sync` unit test.
It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the
last connected block to be fully processed, than to be able to return before
prune locks are set, so this switches the order of `m_best_block_index =
block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more
reliable.
Also since commit f08c9fb0c6, there has been a
race condition in the `coinstatsindex_initial_sync` test. Before that commit,
the atomic index best block pointer `m_best_block_index` was updated as the
last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain`
could safely be used in tests to wait for the last `BlockConnected`
notification to be finished before stopping and destroying the index. But
after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer
sufficient, and there is a race between the test shutdown code which destroys
the index object and the new code introduced in that commit calling
`AllowPrune()` and `GetName()` on the index object. Reproducibility
instructions for this are in
https://github.com/bitcoin/bitcoin/issues/25365#issuecomment-1259744133
This commit fixes the `coinstatsindex_initial_sync` race condition, even though
it will require an additional change to silence TSAN false positives,
https://github.com/bitcoin/bitcoin/pull/26188, after it is fixed. So this
partially addresses but does not resolve the bug reporting TSAN errors
https://github.com/bitcoin/bitcoin/issues/25365.
There is no known race condition outside of test code currently, because the
bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not
`BlockUntilSyncedToCurrentChain` to safely shut down.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Co-authored-by: MacroFake <falke.marco@gmail.com>
During connection setup for a peer, getpeerinfo returns "version": 0, "subver": ""
and the GUI Peers window displays 0 and an empty field, respectively.
Give these fields the same behavior as the other fields in the GUI Peers window:
display the fallback value in src/qt/forms/debugwindow.ui (i.e. "N/A") until a
valid result is available after the peer connection completes.
The (100, 1000000, 1000, 1000000) limits are arbitrarily high and
don't restrict anything, they are just meant to calculate ancestors
properly. Using NoLimits() makes this intent more clear and simplifies
the code.
Simplifies function signatures by removing repetition of all the
ancestor/descendant limits, and increases readability by being
more verbose by naming the limits, while still reducing the LoC.
There are quite a few places in the codebase that require us to
construct a CTxMemPool without limits on ancestors and descendants.
This helper function allows us to get rid of all that duplication.
8a6b6dfcd8 fuzz: pass max fee into ConsumeTxMemPoolEntry (fanquake)
eb15569280 fuzz: add util/mempool/h.cpp (fanquake)
Pull request description:
Moving the heavy (Boost) mempool code out of fuzz/util.h. Means that (for ex) a crypto_common fuzz unit doesn't need to care about seeing endless Boost headers. This results in a ~10% speedup (for me) when compiling the fuzz tests. Your results may vary.
ACKs for top commit:
MarcoFalke:
review ACK 8a6b6dfcd8🍮
Tree-SHA512: 27dc9d9581ac0b1b319cc0dc08fe5f8fbf9269386a5cb23f6fd5d8231bf015ed942ab4414d8001220541be0013756354578ddab1fec607c6fba04daf421bc870
Quoting ryanofsky: "util can be the library for things included in the kernel
which the kernel can depend on, and common can be the library for other code
that needs to be shared internally, but should not be part of the kernel or
shared externally."
7d14577d0f refactor: move DEFAULT_BLOCKFILTERINDEX from val to blockfilterindex (fanquake)
c87d569189 refactor: move DEFAULT_COINSTATSINDEX from validation to coinstatsindex (fanquake)
2bfc1e6aaa refactor: move DEFAULT_TXINDEX from validation to txindex (fanquake)
Pull request description:
Move `*index` default constants out of `validation.h`.
ACKs for top commit:
stickies-v:
re-ACK 7d14577d0f
aureleoules:
ACK 7d14577d0f
Tree-SHA512: 3021db1a63ceb714dee4b91f755d1fb9a6633adb6f1081e34e4179900e7543e3a7b06fe47507d580a3a2caf52f7ede784cb36716d521c76b0404bdc798f0186a
4bee62e9b8 kernel: remove util/bytevectorhash.cpp (fanquake)
Pull request description:
This is no-longer used.
ACKs for top commit:
hebasto:
ACK 4bee62e9b8, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 4d61f87b640ef3c759008631433b3e6d2bd2ac54bbe0b287f32ea1569760048f17a66cfe846b94ec458a7db5d064be6da59299b9280572a3dc649df60760c63f
d0d9cf7aea test: Check external coin effective value is used in CoinSelection (Aurèle Oulès)
76b79c1a17 wallet: Use correct effective value when checking target (Aurèle Oulès)
Pull request description:
Fixes#26185. The following assert failed because it was not checked in the parent function.
2bd9aa5a44/src/wallet/coinselection.cpp (L391)
ACKs for top commit:
glozow:
reACK d0d9cf7aea
furszy:
ACK d0d9cf7a
Tree-SHA512: e126daba1115e9d143f2a582c6953e7ea55e96853b6e819c7744fd7a23668f7d9854681d43ef55d8774655bc54e7e87c1c9fccd746d9e30fbf3caa82ef808ae9
30cc1c6609 refactor: Drop `owns_lock()` call (Hennadii Stepanov)
bff4e068b6 refactor: Do not discard `try_lock()` return value (Hennadii Stepanov)
Pull request description:
Microsoft's C++ Standard Library uses the `[[nodiscard]]` attribute for `try_lock()`.
See: https://github.com/microsoft/STL/blob/main/stl/inc/mutex
This change allows to drop the current suppression for the warning C4838 and helps to prevent the upcoming warning C4858.
See: 539c26c923Fixesbitcoin/bitcoin#26017.
Split from bitcoin/bitcoin#25819.
ACKs for top commit:
vasild:
ACK 30cc1c6609
Tree-SHA512: ce17404e1c78af4f763129753caf8e5a0e1c91ba398778fe912f9fcc56a847e8112460d1a1a35bf905a593b7d8e0b16c6b099ad74976b67dca5f4f3eda6ff621
9cbfe40d8a net: remove useless call to IsReachable() from CConnman::Bind() (Vasil Dimov)
Pull request description:
`CConnman::Bind()` is called without `BF_EXPLICIT` only when passed
either `0.0.0.0` or `::`. For those addresses `IsReachable()` is always
true (regardless of the `-onlynet=` setting!), meaning that the `if`
condition never evaluates to true.
`IsReachable()` is always true for the "any" IPv4 and IPv6 addresses
because `CNetAddr::GetNetwork()` returns `NET_UNROUTABLE` instead of
`NET_IPV4` or `NET_IPV6` and the network `NET_UNROUTABLE` is always
considered reachable.
It follows that `BF_EXPLICIT` is unnecessary, remove it too.
ACKs for top commit:
naumenkogs:
ACK 9cbfe40d8a
aureleoules:
ACK 9cbfe40d8a
mzumsande:
ACK 9cbfe40d8a
Tree-SHA512: 4e53ee8a73ddd133fd4ff25635135b65e5c19d1fc56fe5c30337406560664616c0adff414dca47602948919f34c81073aae6bfc2871509f3912663d86750928e
079cf88c0d refactor: move Boost datetime usage to wallet (fanquake)
Pull request description:
This means we don't need Boost Datetime in a `--disable-wallet` build, and it isn't included in the kernel (via time.h/cpp). Split from a larger boost removal branch/effort.
ACKs for top commit:
hebasto:
re-ACK 079cf88c0d
aureleoules:
re-ACK 079cf88c0d - rebased and two additional unit tests since my last review.
jarolrod:
crACK 079cf88c0d
Tree-SHA512: c84f47158a4f21902f211c059d8c4bd55ffe95a256835deee723653be08cca49eeddfc33a2316b0cd31805e81cf77eaa39c6c9dcff4cda11a26ba4c1c143974e
9d14f27bdd log: log RPC port on startup (James O'Beirne)
Pull request description:
I just spent a few hours trying to figure out why "18444" wasn't getting me to regtest's RPC server. I'm not the sharpest tool in the shed, but I was maybe understandably confused because "Bound to 127.0.0.1:18445" appears in the logs, which I assumed was the P2P port.
This change logs the RPC listening address by default on startup, which seems like a basic piece of information that shouldn't be buried under `-debug`.
ACKs for top commit:
dergoegge:
ACK 9d14f27bdd
jarolrod:
ACK 9d14f27bdd
aureleoules:
ACK 9d14f27bdd
Tree-SHA512: 5c86f018c0b8d6264abf878c921afe53033b23ab4cf289276bb1ed28fdf591c9d8871a4baa4098c363cb2aa9a637d2e4e18e56b14dfc7d767ee40757d7ff2e7c
Since m_next_resend is now only called from MaybeResendWalletTxs()
we don't have any potential race conditions anymore, so the usage
of std::atomic can be reverted.
We only want to relay our resubmitted transactions once every 12-36h.
By separating the timer update logic out of ResubmitWalletTransactions
and into MaybeResendWalletTxs we avoid non-relay calls (previously in
the separate ReacceptWalletTransactions function) from resetting that
timer.
Moves the logic of whether or not transactions should actually be
resent out of the function that's resending them. This reduces
responsibilities of ResubmitWalletTransactions and allows
carving out the updating of m_next_resend in a future commit.
fa2c72dda0 rpc: Set RPCArg options with designated initializers (MacroFake)
Pull request description:
For optional constructor arguments, use a new struct. This comes with two benefits:
* Earlier unused optional arguments can be omitted
* Designated initializers can be used
ACKs for top commit:
stickies-v:
re-ACK fa2c72dda0
Tree-SHA512: 2a0619548187cc7437fee2466ac4780746490622f202659f53641be01bc2a1fea4416d1a77f3e963bf7c4cce62899b61fab0b9683440cf82f68be44f63826658
Previously, we would prepare to self-announce to a new peer while
parsing a VERSION message from that peer. This is redundant, because we
do something very similar in MaybeSendAddr(), which is called from
SendMessages() after the version handshake is finished.
There are a couple of differences:
1) MaybeSendAddr() self-advertises to all peers we do address relay with,
not just outbound ones.
2) GetLocalAddrForPeer() called from MaybeSendAddr() makes a
probabilistic decision to either advertise
what they think we are or what we think we are, while
PushAddress(self) on VERSION deterministically only does
the former if the address from the latter is unroutable.
3) During VERSION processing, we haven't received a potential sendaddrv2 message
from our peer yet, so self-advertisements with addresses from addrV2-only networks
would always be dropped in PushAddress().
Since it's confusing to have two slightly different mechanisms for self-advertising,
and the one in MaybeSendAddr() is better, remove the one in VERSION.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Microsoft's C++ Standard Library uses the `[[nodiscard]]` attribute for
`try_lock()`.
See: https://github.com/microsoft/STL/blob/main/stl/inc/mutex
This change allows to drop the current suppression for the warning C4838
and helps to prevent the upcoming warning C4858.
See: 539c26c923
bdcafb9133 p2p: ProcessHeadersMessage(): fix received_new_header (Larry Ruane)
Pull request description:
Follow-up to #25717. The commit "Utilize anti-DoS headers download strategy" changed how this bool variable is computed, so that its value is now the opposite of what it should be.
Prior to #25717:
```
bool received_new_header{WITH_LOCK(::cs_main, return m_chainman.m_blockman.LookupBlockIndex(headers.back().GetHash()) == nullptr)};
```
After #25717 (simplified):
```
{
LOCK(cs_main);
last_received_header = m_chainman.m_blockman.LookupBlockIndex(headers.back().GetHash());
}
bool received_new_header{last_received_header != nullptr};
```
ACKs for top commit:
dergoegge:
ACK bdcafb9133
glozow:
ACK bdcafb9133, I believe this is correct and don't see anything to suggest the switch was intentional.
stickies-v:
ACK bdcafb9133
Tree-SHA512: 35c12762f1429585a0b1c15053e310e83efb28c3d8cbf4092fad9fe81c893f6d766df1f2b20624882acb9654d0539a0c871f587d7090dc2a198115adf59db3ec
810c3dc7ef doc, rpc: mention that `listdescriptors` result is sorted by string representation (Sebastian Falbesoner)
d99af861d0 test: check that `listdescriptors` descriptor strings are sorted (Sebastian Falbesoner)
Pull request description:
This small PR adds a test for the change introduced in PR #25931 ("rpc: sort listdescriptors result", commit 50996241f2). The correctness of the test can easily be verified by commenting out the `std::sort` call in the `listdescriptors` RPC implementation:
```diff
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 09c74ea2da..3ed1a69b26 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -1829,9 +1829,11 @@ RPCHelpMan listdescriptors()
});
}
+ /*
std::sort(wallet_descriptors.begin(), wallet_descriptors.end(), [](const auto& a, const auto& b) {
return a.descriptor < b.descriptor;
});
+ */
UniValue descriptors(UniValue::VARR);
for (const WalletDescInfo& info : wallet_descriptors) {
```
leading to a fail of the functional test `wallet_listdescriptors.py`.
ACKs for top commit:
jarolrod:
ACK 810c3dc7ef
aureleoules:
ACK 810c3dc7ef
Tree-SHA512: 31770e3149b8a0251ecfa8662a2270c149f778eb910985f48a91d6a5d288b7b1c2244f9f1b798ebe3f1aa9f0b935cb4d6f12d5d28f78bcde3c4a61af76d11d0a
553ff452c0 build: remove stdlib.h from header checks (fanquake)
a63d4cb26a refactor: use <cstdlib> over stdlib.h (fanquake)
Pull request description:
We already use a mix of `<cstlib>` and `stdlib.h` unconditionally throughout
the codebase.
Us checking this header also duplicates work already done by autotools.
Currently stdlib.h is checked for 3 times during a ./configure run, after
this change, at least it's only twice.
Similar to #26150.
ACKs for top commit:
kristapsk:
ACK 553ff452c0
TheCharlatan:
ACK 553ff452c0
Tree-SHA512: 0a43d39d3df180a1614dbd3a1ee1531b0969ffe4a0c09dfe9d2f3f0ec16196b5fd7523309f6722936a8c8b20908508724e1903e939dd81c3b4538d85d0f42953
a60d9eb9e6 Bugfix: Wallet: Lock cs_wallet for SignMessage (Luke Dashjr)
Pull request description:
cs_desc_main is typically locked within scope of a cs_wallet lock, but:
CWallet::IsLocked locks cs_wallet
...called from DescriptorScriptPubKeyMan::GetKeys
...called from DescriptorScriptPubKeyMan::GetSigningProvider which locks cs_desc_main first, but has no access to cs_wallet ...called from DescriptorScriptPubKeyMan::SignMessage ...called from CWallet::SignMessage which can access and lock cs_wallet
Resolve the out of order locks by grabbing cs_wallet in CWallet::SignMessage first
-------------
Note this is currently only an issue for the GUI (which lacks sufficient testing apparently), but can be reproduced by #26082 (CI fails as a result)
ACKs for top commit:
achow101:
ACK a60d9eb9e6
w0xlt:
ACK a60d9eb9e6
Tree-SHA512: 60f6959b0ceaf4d9339ba1a47154734034b637c41b1f9e26748a2dbbc3a2a95fc3696019103c55ae70c91d910ba8f3d7f4e27d263030eb60b689f290c4d82ea9
Follow-up to #25717. The commit "Utilize anti-DoS headers download
strategy" changed how this bool variable is computed, so that its value
is now the opposite of what it should be.
to the current p2p behavior. We only initialize the Peer::TxRelay m_relay_txs
data structure if it isn't an outbound block-relay-only connection and fRelay=true
(the peer wishes to receive tx announcements) or we're offering NODE_BLOOM to this peer.
fa4ba04c15 fuzz: Remove no-op call to get() (MacroFake)
fa642286b8 fuzz: Avoid timeout in bitdeque fuzz target (MacroFake)
Pull request description:
I'd guess that any bug should be discoverable within `10` ops. However, `900` seems also better than no limit at all, which causes timeouts such as https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=50892
ACKs for top commit:
sipa:
ACK fa4ba04c15
Tree-SHA512: f6bd25e78d5f04c6f88e9300c2fa3d0993a0911cb0fd1b414077adc0edde1a06ad72af5e2f50f0ab1324f91999ae57d879686c545b2e6c19ae7f637a8804bd48
55aad5f3a9 build: remove stdio.h from header checks (fanquake)
b95633121b refactor: use <cstdio> over stdio.h (fanquake)
Pull request description:
We already use a mix of `<cstdio>` and `stdio.h` unconditionally throughout
the codebase.
Us checking this header also duplicates work already done by autotools.
Currently `stdio.h` is checked for 3 times during a ./configure run, after
this change, at least it's only twice.
ACKs for top commit:
TheCharlatan:
ACK 55aad5f3a9
kristapsk:
ACK 55aad5f3a9
Tree-SHA512: a83cc724528ab92aacfa53048b12fcccec3962637ca7fad30f6c610365edeb0e951f74e37832ad7d3f79ca9b8d7203cb10165c89d0e4b63eeda7a970dab82dfb
648f6950cd Correct sanity-checking script_size calculation (Pieter Wuille)
Pull request description:
Fix a bug in the script_size sanity-check in the miniscript string parser, found by oss-fuzz in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=51636, and introduced in e8cc2e4afc (#25540).
This bug would cause an assertion failure when feeding a miniscript with a `thresh(k,...)` fragment, with k >= 128, to an RPC.
ACKs for top commit:
darosior:
utACK 648f6950cd
achow101:
ACK 648f6950cd
Tree-SHA512: d86a0721758cd1e42ef02050b542f0935efdc19447a1ca76a3ade96352a6ee8261eef3d4a5cbdec77bf0ad14dfed42e9eb6bd4246b816a9f6f06d786900da9e7
58b7df3caa wallet: AvailableCoins, simplify output script type acquisition (furszy)
Pull request description:
There is an unnecessary `ExtractDestination()` call and subsequent result parse into an `CScriptID`.
The `Solver()` call, which we are already doing below anyway, retrieves the script type and, in the P2SH case, the program id.
ACKs for top commit:
achow101:
ACK 58b7df3caa
aureleoules:
re-ACK 58b7df3caa
rajarshimaitra:
ACK 58b7df3caa
w0xlt:
ACK 58b7df3caa
Tree-SHA512: 51080766877c34cb2232ee3a1cb6b6a62b829c9297c67b99577742b94854a737a74d248015a4603ca9b6cd0a3c9e1d6d78673ff3cc9fc65dd82deea72dc537fd
ff7c81f63a build: remove duplicate / unneeded libs from bench_bitcoin (fanquake)
Pull request description:
EVENT_*_LIBS are already in LDADD.
Move wallet libs into the wallet conditional, similar to zmq.
ACKs for top commit:
theuni:
ACK ff7c81f63a
Tree-SHA512: 6bd92f03478d56cd38645e38c0e6c4614cdf9c745124069d0d1d80483d76f5c656e1749061455ba04c619684513a063dda3f8f4bd09fe7b66911714d83592f25
e68d380797 rpc: remove unneeded RPCTypeCheckArgument checks (furszy)
55566630c6 rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR (furszy)
Pull request description:
Same rationale as #26039, tackling another angle of the problem.
#### Context
We have the same univalue type error checking code spread/duplicated few times:
`RPCTypeCheckObj`, `RPCTypeCheckArgument`, `UniValue::checkType`.
In the first two functions, we are properly returning an `RPC_TYPE_ERROR` while in `UniValue::checkType`
we are throwing an `std::runtime_error` which is caught by the RPC server request handler, who invalidly
treats it as `RPC_MISC_ERROR` (which is a generic error return code that provides no information to the user).
#### Proposed Changes
Throw a custom exception from `Univalue::checkType` (instead of a plain
`std::runtime_error`) and catch it on the RPC server request handler.
So we properly return `RPC_TYPE_ERROR` (-3) on every arg type error and
not the general `RPC_MISC_ERROR` (-1).
This will allow us to remove all the `RPCTypeCheckArgument` calls. As them are redundant since #25629.
Top commit has no ACKs.
Tree-SHA512: 4e4c41851fd4e2b01a2d8b94e71513f9831f810768ebd89684caca4901e87d3677980003949bcce441f9ca607a1b38a5894839b6c492f5947b8bab8cd9423ba6
68209a7b5c rpc: make addpeeraddress work with cjdns addresses (Martin Zumsande)
a8a9ed67cc init: Abort if i2p/cjdns are chosen via -onlynet but unreachable (Martin Zumsande)
Pull request description:
If the networks i2p / cjdns are chosen via `-onlynet` but the user forgot to provide `-i2psam` / `-cjdnsreachable`, no outbound connections will be made - it would be nice to inform the user about that.
The solution proposed here mimics existing behavior for `-onlynet=onion` and non-specified `-onion`/`-proxy` where we already abort with an InitError - if reviewers would prefer to just print a warning, please say so.
The second commit adds CJDNS support to the debug-only `addpeeraddress` RPC allowing to add CJDNS addresses to addrman for testing and debug purposes. (if `-cjdnsreachable=1`)
This is the result of an [IRC discussion](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2022-09-01#848066;) with vasild.
ACKs for top commit:
vasild:
ACK 68209a7b5c
dergoegge:
ACK 68209a7b5c
Tree-SHA512: 6db9787f01820190f14f90a0b39e4206603421eb7521f792879094d8bbf4d4d0bfd70665eadcc40994ac7941a15ab5a8d65c4779fba5634c0e6fa66eb0972b8d
fad61573ed Fix nNextResend data race in ResubmitWalletTransactions (MacroFake)
Pull request description:
Now that `ResubmitWalletTransactions` is called from more than one thread, it is no longer thread-safe.
Introduced in 5291933fed.
ACKs for top commit:
achow101:
ACK fad61573ed
jonatack:
ACK fad61573ed
stickies-v:
However, I think the current data race UB fix in fad61573e is the most critical to get into v24, so: ACK fad61573e - but open to further improvements.
Tree-SHA512: 54da2ed1c5f44e33588ac1d21ce26908fcf0bfe785c28ba8f6a479389b5ab7a0b32b016d4c482a2ccb405e0686efb61ffe23e427f5e589dc7d2b3c7469978977
d575a675cc net_processing: add thread safety annotation for m_highest_fast_announce (Anthony Towns)
0ae7987f68 net_processing: add thread safety annotations for PeerManagerImpl members accessed only via the msgproc thread (Anthony Towns)
a66a7ccb82 net_processing: add thread safety annotations for Peer members accessed only via the msgproc thread (Anthony Towns)
bf12abe454 net: drop cs_sendProcessing (Anthony Towns)
1e78f566d5 net: add NetEventsInterface::g_msgproc_mutex (Anthony Towns)
Pull request description:
There are many cases where we assume message processing is single-threaded in order for how we access node-related memory to be safe. Add an explicit mutex that we can use to document this, which allows the compiler to catch any cases where we try to access that memory from other threads and break that assumption.
ACKs for top commit:
MarcoFalke:
review ACK d575a675cc 📽
dergoegge:
Code review ACK d575a675cc
w0xlt:
ACK d575a675cc
vasild:
ACK d575a675cc modulo the missing runtime checks
Tree-SHA512: b886d1aa4adf318ae64e32ccaf3d508dbb79d6eed3f1fa9d8b2ed96f3c72a3d38cd0f12e05826c9832a2a1302988adfd2b43ea9691aa844f37d8f5c37ff20e05
b6a65568df Fix issues identified by codespell 2.2.1 and update ignored words (Jon Atack)
8f2010de6e Bump codespell version to 2.2.1 (Jon Atack)
Pull request description:
as well as one in `test/lint/lint-locale-dependence.py` not seen by the spelling linter.
Can be tested locally by running `test/lint/lint-spelling.py` on this branch versus on master and by checking the CI linter result.
ACKs for top commit:
satsie:
ACK b6a65568df
Tree-SHA512: ab4ba029a9a5de5926fa5d336bd3b21245acf0649c6aa69a48c223bd22327e13beb32e970f66f54db58cd318731b643e1c7ace9a89776ed2a069cddc02363b71
fa2b8ae0a2 util: improve bitcoin-wallet exit codes (MacroFake)
Pull request description:
Refactors `bitcoin-wallet` so that it doesn't return a non-zero exit code by default, and makes the option handling more inline with the other binaries. i.e outputting `Error: too few parameters` if you don't pass any options.
Fixing this means we can check the process output in `gen-manpages.py`; which addresses the remaining [review comment](https://github.com/bitcoin/bitcoin/pull/24263#discussion_r806126705) from #24263.
Top commit has no ACKs.
Tree-SHA512: 80bd8098faefb4401ca1e4d49937ef6c960cf60ce0e7fb9dc38904fbc2fd92e319ec04570381da84943b7477845bf6be00e977f4c0451b247a6698662ce8f1bf
I just spent a few hours trying to figure out why "18444" wasn't getting
me to regtest's RPC server. I'm not the sharpest tool in the shed, but I
was maybe understandably confused because "Bound to
127.0.0.1:18445" appears in the logs, which I assumed was the P2P port.
This change logs the RPC listening address, which seems like a basic
piece of information that shouldn't be buried in debug logs.
cs_desc_main is typically locked within scope of a cs_wallet lock, but:
CWallet::IsLocked locks cs_wallet
...called from DescriptorScriptPubKeyMan::GetKeys
...called from DescriptorScriptPubKeyMan::GetSigningProvider which locks cs_desc_main first, but has no access to cs_wallet
...called from DescriptorScriptPubKeyMan::SignMessage
...called from CWallet::SignMessage which can access and lock cs_wallet
Resolve the out of order locks by grabbing cs_wallet in CWallet::SignMessage first
faf5bb87da doc: Move -permitbaremultisig to the relay help category (MacroFake)
Pull request description:
This transaction relay setting doesn't have anything to do with establishing p2p connections.
ACKs for top commit:
glozow:
concept ACK faf5bb87da
luke-jr:
utACK faf5bb87da, but (nit) prefer to keep it on a single line
Tree-SHA512: 436fcff5191c346fe16b3208411886e3239fb5819322673a45cf0c0f9a3070563d312da8bb5d5f6060b36d305e59e5b58928526d5042cb3dac29ce7740d17c1c
c3e536555a Bugfix: Wallet: Return util::Error rather than non-error nullptr when CreateWallet/LoadWallet/RestoreWallet fail (Luke Dashjr)
335ff98c8a Bugfix: Wallet: Wrap RestoreWallet content in a try block to ensure exceptions become returned errors and incomplete wallet directory is removed (Luke Dashjr)
Pull request description:
Bug 1: `copy_file` can throw exceptions, but `RestoreWallet` is expected to return a nullptr with a populated `errors` parameter. This is fixed by wrapping `copy_file` and `LoadWallet` (for good measure) in a `try` block, and converting any exceptions to the intended return style.
Bug 2: `util::Result` turns what would have been a `false` unique_ptr into a `true` nullptr result, which leads to nullptr dereferences in at least the 3 cases of wallet creation/loading/restoring. This is fixed by keeping the pointer as a plain `std::unique_ptr` until actually returning it (ie, after the nullptr check).
Fixes https://github.com/bitcoin-core/gui/issues/661
ACKs for top commit:
achow101:
ACK c3e536555a
Tree-SHA512: 4291b3dbbb147acea2e63a704324c9371bc16ecb4237f8753729b0b0a6e55c9758ad61bfe8bd432fd7b0bae95d8b63a9831e61ac8b8d5c0197b550a2e0f4a105
