6d19815cd44031ff2b45fc9532f579cd81c62749 rest: replace `rf_names[0].rf` by `RESTResponseFormat::UNDEF` for code clarity (Eval EXEC)
Pull request description:
I'm reviewing the bitcoin's rest.cpp source code.
In the function: `ParseDataFormat`, `rf_names[0].rf` is actualy `RESTResponseFormat::UNDEF`:
e3f416dbf7/src/rest.cpp (L48-L57)
so it would be more clarity and code readability to use `return RESTResponseFormat::UNDEF;` to replace `return rf_names[0].rf;`
ACKs for top commit:
maflcko:
lgtm ACK 6d19815cd44031ff2b45fc9532f579cd81c62749
brunoerg:
code review ACK 6d19815cd44031ff2b45fc9532f579cd81c62749
Tree-SHA512: 420454f1cc09db44c1d76423d8623a0b8865d41d6c34015844ff83d78a9373e3e26f3f62818d1502b33eb063caf904750e858b74ddecd76750577ae82b64b0c1
4e69aa5701a2dad3805ea26718e6a406adb8b748 doc: fix `BlockConnected` incorrect comment (ismaelsadeeq)
Pull request description:
This is a simple PR that fixes the `BlockConnected` validation interface notification comment, which incorrectly states that a vector of transactions removed from the mempool is as a parameter of the method.
Originally, this was the case when the method was first introduced in https://github.com/bitcoin/bitcoin/pull/9725
However, the method has since changed, and this is no longer accurate. Keeping the outdated comment is now misleading.
This PR removes the information about the method parameters from the docstring, aligning it with the style of other notifications methods. As noticed in this PR, comments listing parameters can become stale and go uncorrected.
Therefore, this PR simply removes the inaccurate comment without listing the current returned values.
ACKs for top commit:
l0rinc:
ACK 4e69aa5701a2dad3805ea26718e6a406adb8b748
maflcko:
lgtm ACK 4e69aa5701a2dad3805ea26718e6a406adb8b748
Tree-SHA512: 3737313f7a9da55c67c78ce01bab5005946f4e1fccbb471560ff3af8c8275cb5cf876f6c53400c93f0ba1fdf134f28766ed573cbe62903127a3129ca8ce88db6
fcfd3db563e89fd79820a4cdfa102d624d801de1 remove RPCTimerInterface and RPCRunLater (Matthew Zipkin)
8a1765795fd3bff79d790102ca7cefa8fd9b204c use WalletContext scheduler for walletpassphrase callback (Matthew Zipkin)
Pull request description:
This removes the dependency on libevent for events scheduled by RPC commands, like re-locking a wallet some time after decryption with walletpassphrase. Since walletpassphrase is currently the only RPC that does this, `RPCRunLater`, `RPCTimerInterface` and all related methods are left unused, and deleted in the second commit. Any future RPC that needs to execute a callback in the future can follow the pattern in this PR and just use a scheduler from node or wallet context.
This is an alternative approach to #32796, described in https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3014309449
ACKs for top commit:
fjahr:
Code Review ACK fcfd3db563e89fd79820a4cdfa102d624d801de1
achow101:
ACK fcfd3db563e89fd79820a4cdfa102d624d801de1
furszy:
ACK fcfd3db563e89fd79820a4cdfa102d624d801de1
Tree-SHA512: 04f5e9c3f73f598c3d41d6e35bb59c64c7b93b03ad9fce3c40901733147ce7764f41f475fef1527d44af18f722759996a31ca83b48cb52153795d5022fecfd14
fa946520d229ae45b30519bccc9eaa2c47b4a093 refactor: Use structured binding for-loop (MarcoFalke)
eeeec1579ec5a3aa7b10ff62f87d197ae311a666 rpc: Use type-safe exception to pass RPC help (MarcoFalke)
Pull request description:
The current "catch-all" `catch (const std::exception& e)` in `CRPCTable::help` is problematic, because it could catch exceptions unrelated to passing the help string up.
Fix this by using a dedicated exception type.
ACKs for top commit:
l0rinc:
tested ACK fa946520d229ae45b30519bccc9eaa2c47b4a093 (edited)
achow101:
ACK fa946520d229ae45b30519bccc9eaa2c47b4a093
rkrux:
re-ACK fa946520d229ae45b30519bccc9eaa2c47b4a093
Tree-SHA512: 23dac6e0fe925561bfbf421e6a7441d546eed8c1492ac41ca4ed7dfcd12f4d2ef39c35f105a0291aac511365d98f08fbdc9a4f0bf627172873b8f23c2be45e76
b1a8ac07e91dd1d305fcbc16ea931d60e46c0055 doc: Release note for removed watchonly parameters and results (Ava Chow)
15710869e19e707ef03492c55030dcefa16269d8 wallet: Remove ISMINE_WATCH_ONLY (Ava Chow)
4439bf4b41a6997d4d965f00a8c40efa9cf6895b wallet, spend: Remove fWatchOnly from CCoinControl (Ava Chow)
1337c72198a7d32935431d64e9e58c12f9003abc wallet, rpc: Remove watchonly from RPCs (Ava Chow)
e81d95d435744e48615973dc22acce1a291bd20d wallet: Remove watchonly balances (Ava Chow)
d20dc9c6aae089ab926fd135febd69a8f0744a18 wallet: Wallets without private keys cannot grind R (Ava Chow)
9991f49c38c084967ca66791d838c99b42f000eb test: Watchonly wallets should estimate larger size (Ava Chow)
Pull request description:
Descriptor wallets do not use the watchonly behavior as it is not possible to mix watchonly and non-watchonly in a descriptor wallet. With legacy wallets now removed, all of the watchonly handling and reporting code is no longer needed. This PR removes watchonly options and results from the RPCs and the handling of watchonly things from the wallet's internals.
With all of the watchonly things removed, ISMINE_WATCH_ONLY is removed as well.
Split from #32523
Depends on #32594 for tests that are easier to read
ACKs for top commit:
Eunovo:
ACK b1a8ac07e9
maflcko:
re-ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055 🌈
rkrux:
ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055
furszy:
light code review ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055
Tree-SHA512: bc87f37a13294f7208991be8f93899b49e5bdf87c70e0f66d9c4cb09c03be6c202320406f27e9a35aa2f57319d19a3f0c07d5e5ddbc97c7edab165b1656d6612
1632fc104be8f171f59a023800b2f9f20d0a3cff txgraph: Track multiple potential would-be clusters in Trim (improvement) (Pieter Wuille)
4608df37e02abde09da5d6668dcd4dc5a6e7c569 txgraph: add Trim benchmark (benchmark) (Pieter Wuille)
9c436ff01cffe51e34d0f29c445db11bb807c6c3 txgraph: add fuzz test scenario that avoids cycles inside Trim() (tests) (Pieter Wuille)
938e86f8fecd65ca90b97e6cf896f8c59fb590ba txgraph: add unit test for TxGraph::Trim (tests) (glozow)
a04e205ab03efa105f7886449c2c9316f67a85c1 txgraph: Add ability to trim oversized clusters (feature) (Pieter Wuille)
eabcd0eb6fca5790ea19e7c1613ae06df8d21918 txgraph: remove unnecessary m_group_oversized (simplification) (Greg Sanders)
19b14e61eae7f0f0bfc8d17b06cc0e3ebd1f994d txgraph: Permit transactions that exceed cluster size limit (feature) (Pieter Wuille)
c4287b9b71c6d5222bcd0d2af3185de93ce76078 txgraph: Add ability to configure maximum cluster size/weight (feature) (Pieter Wuille)
Pull request description:
Part of cluster mempool (#30289).
During reorganisations, it is possible that dependencies get added which would result in clusters that violate policy limits (cluster count, cluster weight), when linking the new from-block transactions to the old from-mempool transactions. Unlike RBF scenarios, we cannot simply reject the changes when they are due to received blocks. To accommodate this, add a `TxGraph::Trim()`, which removes some subset of transactions (including descendants) in order to make all resulting clusters satisfy the limits.
Conceptually, the way this is done is by defining a rudimentary linearization for the entire would-be too-large cluster, iterating it from beginning to end, and reasoning about the counts and weights of the clusters that would be reached using transactions up to that point. If a transaction is encountered whose addition would violate the limit, it is removed, together with all its descendants.
This rudimentary linearization is like a merge sort of the chunks of the clusters being combined, but respecting topology. More specifically, it is continuously picking the highest-chunk-feerate remaining transaction among those which have no unmet dependencies left. For efficiency, this rudimentary linearization is computed lazily, by putting all viable transactions in a heap, sorted by chunk feerate, and adding new transactions to it as they become viable.
The `Trim()` function is rather unusual compared to the `TxGraph` functionality added in previous PRs, in that `Trim()` makes it own decisions about what the resulting graph contents will be, without good specification of how it makes that decision - it is just a best-effort attempt (which is improved in the last commit). All other `TxGraph` mutators are simply to inform the graph about changes the calling mempool code decided on; this one lets the decision be made by txgraph.
As part of this, the "oversized" property is expanded to also encompass a configurable cluster weight limit (in addition to cluster count limit).
ACKs for top commit:
instagibbs:
reACK 1632fc104be8f171f59a023800b2f9f20d0a3cff
glozow:
reACK 1632fc104be via range-diff
ismaelsadeeq:
reACK 1632fc104be8f171f59a023800b2f9f20d0a3cff 🛰️
Tree-SHA512: ccacb54be8ad622bd2717905fc9b7e42aea4b07f824de1924da9237027a97a9a2f1b862bc6a791cbd2e1a01897ad2c7c73c398a2d5ccbce90bfbeac0bcebc9ce
de4eef52d123b781b833841a9765d1788010ac6b threading: use correct mutex name in reverse_lock fatal error messages (Cory Fields)
Pull request description:
"Now that REVERSE_LOCK requires the name of the actual mutex, it can be used for better error messages." - theuni
This is a follow-up to this comment https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2981287545
I just cherry-picked the commit 85c2848eb575f4abaa81fdd4e8f3b2048693dd98
ACKs for top commit:
theuni:
Re-ACK de4eef52d123b781b833841a9765d1788010ac6b
TheCharlatan:
ACK de4eef52d123b781b833841a9765d1788010ac6b
Tree-SHA512: 1109381e1f0589093f7c737cb1ebd1c43324a9e1ea34b5f05a9171d06ab44cca0c5ead43c581f6e37ded1f0463ab8a280f3319c288d39a4625109b5c08a7cb68
c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702 flatfile: check whether the file has been closed successfully (Vasil Dimov)
4bb5dd78ea4b578922a3316b37b486f96cb0beec util: check that a file has been closed before ~AutoFile() is called (Vasil Dimov)
8bb34f07df9ad45faf25c32c99a4dd70759b25be Explicitly close all AutoFiles that have been written (Vasil Dimov)
a69c4098b273b6db5d2212ba91cfc713c1634c5d rpc: take ownership of the file by WriteUTXOSnapshot() (Hodlinator)
Pull request description:
`fclose(3)` may fail to flush the previously written data to disk, thus a failing `fclose(3)` is as serious as a failing `fwrite(3)`.
Previously the code ignored `fclose(3)` failures. This PR improves that by changing all users of `AutoFile` that use it to write data to explicitly close the file and handle a possible error.
---
Other alternatives are:
1. `fflush(3)` after each write to the file (and throw if it fails from the `AutoFile::write()` method) and hope that `fclose(3)` will then always succeed. Assert that it succeeds from the destructor 🙄. Will hurt performance.
2. Throw nevertheless from the destructor. Exception within the exception in C++ I think results in terminating the program without a useful message.
3. (this is implemented in the latest incarnation of this PR) Redesign `AutoFile` so that its destructor cannot fail. Adjust _all_ its users 😭. For example, if the file has been written to, then require the callers to explicitly call the `AutoFile::fclose()` method before the object goes out of scope. In the destructor, as a sanity check, assume/assert that this is indeed the case. Defeats the purpose of a RAII wrapper for `FILE*` which automatically closes the file when it goes out of scope and there are a lot of users of `AutoFile`.
4. Pass a new callback function to the `AutoFile` constructor which will be called from the destructor to handle `fclose()` errors, as described in https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2243842400. My thinking is that if that callback is going to only log a message, then we can log the message directly from the destructor without needing a callback. If the callback is going to do more complicated error handling then it is easier to do that at the call site by directly calling `AutoFile::fclose()` instead of getting the `AutoFile` object out of scope (so that its destructor is called) and inspecting for side effects done by the callback (e.g. set a variable to indicate a failed `fclose()`).
ACKs for top commit:
l0rinc:
ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
achow101:
ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
hodlinator:
re-ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
Tree-SHA512: 3994ca57e5b2b649fc84f24dad144173b7500fc0e914e06291d5c32fbbf8d2b1f8eae0040abd7a5f16095ddf4e11fe1636c6092f49058cda34f3eb2ee536d7ba
67dc7523f3e103c8359b546d38f28c1feb2b9b34 cmake, test: Disable tests instead of ignoring them (Hennadii Stepanov)
bb9157db5d3920d971e365e416de4e23866c715a cmake, refactor: Switch to `Python3::Interpreter` imported target (Hennadii Stepanov)
Pull request description:
This PR:
1. Switches to a modern CMake approach by using the `Python3::Interpreter` imported target, which is more robust than using variables.
2. Disables the `util_rpcauth_test` test explicitly instead of silently ignoring it.
A build and test log for the case when Python is unavailable is provided below:
```
$ cmake -B build
$ cmake --build build -j 16
$ ctest --test-dir build -j $(nproc) -R "^util"
Internal ctest changing into directory: /bitcoin/build
Test project /bitcoin/build
Start 115: util_tests
Start 117: util_trace_tests
Start 114: util_string_tests
Start 116: util_threadnames_tests
Start 1: util_rpcauth_test
1/5 Test #1: util_rpcauth_test ................***Not Run (Disabled) 0.00 sec
2/5 Test #114: util_string_tests ................ Passed 0.11 sec
3/5 Test #117: util_trace_tests ................. Passed 0.11 sec
4/5 Test #116: util_threadnames_tests ........... Passed 0.11 sec
5/5 Test #115: util_tests ....................... Passed 0.13 sec
100% tests passed, 0 tests failed out of 4
Total Test time (real) = 0.13 sec
The following tests did not run:
1 - util_rpcauth_test (Disabled)
```
ACKs for top commit:
purpleKarrot:
ACK 67dc7523f3e103c8359b546d38f28c1feb2b9b34
janb84:
tACK 67dc7523f3e103c8359b546d38f28c1feb2b9b34
Tree-SHA512: 5fc7ebe31ac03f4b8a53ecfcfc1cace0f647a1d2c989651988edae96bdfbbe2dee171714e57cb028e65ead1bb40806a82d9821746451dbf005538601fd33ea88
d6aaffcb11adcf47480fcc5081af9dcb732decf3 test: check P2SH sigop count for coinbase tx (brunoerg)
Pull request description:
We currently do not test that `GetP2SHSigOpCount` returns 0 for coinbase transactions (see line L129 at https://corecheck.dev/mutation/src/consensus/tx_verify.cpp). This PR addresses it.
ACKs for top commit:
darosior:
That said, i guess unit-tested dead consensus code is better than not-unit-tested dead consensus code. utACK d6aaffcb11adcf47480fcc5081af9dcb732decf3
theStack:
ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3
w0xlt:
ACK d6aaffcb11
ishaanam:
ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3
pablomartin4btc:
ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3
Tree-SHA512: a7d7306f064bb2ec7e93e92625848ae38e150ebb67bde37cd15be1038816b154e867ad21ecd2685d8de5341b67e3b768d30b7654e27b541f33e8f9d63e52261d
b78990734621b8fe46c68a6e7edaf1fbd2f7d351 wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet (furszy)
e86d71b749c08bde6002b9aa2baee824975a518a wallet: refactor, dedup wallet re-loading code (furszy)
1de423e0a08bbc63eed36c8772e9ef8b48e80fb8 wallet: introduce method to return all db created files (furszy)
d04f6a97ba9a55aa9455e1a805feeed4d630f59a refactor: remove sqlite dir path back-and-forth conversion (furszy)
Pull request description:
Currently, the migration process creates a brand-new descriptor wallet with no
connection to the user's legacy wallet when the legacy wallet lacks key material
and contains only watch-only scripts. This behavior is not aligned with user
expectations. If the legacy wallet contains only watch-only scripts, the migration
process should only generate a watch-only wallet instead.
TODO List:
* Explain that `migratewallet` renames the watch-only after migration, and
also that the wallet will not have keys enabled.
ACKs for top commit:
achow101:
ACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351
pablomartin4btc:
tACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351
rkrux:
LGTM ACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351
Tree-SHA512: 1d583ac4b206fb477e9727daf4b5ad9c3e18b12d40e1ab4a61e8565da44c3d0327c892b51cf47b4894405d122e414cefb6b6366c357e02a74a7ca96e06762d83
In the existing Trim function, as soon as the set of accepted transactions
would exceed the max cluster size or count limit, the acceptance loop is
stopped, removing all later transactions. However, it is possible that by
excluding some of those transactions the would-be cluster splits apart into
multiple would-clusters. And those clusters may well permit far more
transactions before their limits are reached.
Take this into account by using a union-find structure inside TrimTxData to
keep track of the count/size of all would-be clusters that would be formed
at any point, and only reject transactions which would cause these resulting
partitions to exceed their limits.
This is not an optimization in terms of CPU usage or memory; it just
improves the quality of the transactions removed by Trim().
Trim internally builds an approximate dependency graph of the merged cluster,
replacing all existing dependencies within existing clusters with a simple
linear chain of dependencies. This helps keep the complexity of the merging
operation down, but may result in cycles to appear in the general case, even
though in real scenarios (where Trim is called for stitching re-added mempool
transactions after a reorg back to the existing mempool transactions) such
cycles are not possible.
Add a test that specifically targets Trim() but in scenarios where it is
guaranteed not to have any cycles. It is a special case, is much more a
whitebox test than a blackbox test, and relies on randomness rather than
fuzz input. The upside is that somewhat stronger properties can be tested.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
During reorganisations, it is possible that dependencies get add which
result in clusters that violate limits (count, size), when linking the
new from-block transactions to the old from-mempool transactions.
Unlike RBF scenarios, we cannot simply reject these policy violations
when they are due to received blocks. To accomodate this, add a Trim()
function to TxGraph, which removes transactions (including descendants)
in order to make all resulting clusters satisfy the limits.
In the initial version of the function added here, the following approach
is used:
- Lazily compute a naive linearization for the to-be-merged cluster (using
an O(n log n) algorithm, optimized for far larger groups of transactions
than the normal linearization code).
- Initialize a set of accepted transactions to {}
- Iterate over the transactions in this cluster one by one:
- If adding the transaction to the set makes it exceed the max cluster size
or count limit, stop.
- Add the transaction to the set.
- Remove all transactions from the cluster that were not included in the set
(note that this necessarily includes all descendants too, because they
appear later in the naive linearization).
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
This removes the restriction added in the previous commit that individual
transactions do not exceed the max cluster size limit.
With this change, the responsibility for enforcing cluster size limits can
be localized purely in TxGraph, without callers (and in particular, tests)
needing to duplicate the enforcement for individual transactions.
This is integrated with the oversized property: the graph is oversized when
any connected component within it contains more than the cluster count limit
many transactions, or when their combined size/weight exceeds the cluster size
limit.
It becomes disallowed to call AddTransaction with a size larger than this limit,
though this limit will be lifted in the next commit.
In addition, SetTransactionFeeRate becomes SetTransactionFee, so that we do not
need to deal with the case that a call to this function might affect the
oversizedness.
a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf miniscript: Make `operator""_mst` `consteval` (Pieter Wuille)
14052162b19ac22f465f7db7880a6ab5d588a98c Revert "miniscript: make operator_mst consteval" (Hennadii Stepanov)
Pull request description:
Same as https://github.com/bitcoin/bitcoin/pull/28657, but without the refactoring required to work around [fixed](https://github.com/bitcoin/bitcoin/pull/28657#discussion_r2095743353) MSVC bugs.
The second commit has been taken from https://github.com/bitcoin/bitcoin/pull/29167.
ACKs for top commit:
sipa:
ACK a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf
hodlinator:
re-ACK a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf
Tree-SHA512: 8b531f9d6c450a8a5218865da05ffb5093d09ce2c0bee9874c0160795c4b1713928730d894ea3cd0b12b133346971ae3a00ed2fe8d9fd8a50b67a74ef81fde98
6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2 refactor: CTransaction equality should consider witness data (Cory Fields)
cbf9b2dab1d8800d63d65904ccfd64e1e439e510 mempool: codify existing assumption about duplicate txids during removal (Cory Fields)
e9331cd6ab2c756c56e8b27a2de2a6d4884c0c06 wallet: IsEquivalentTo should strip witness data in addition to scriptsigs (Cory Fields)
Pull request description:
I stumbled upon the `CTransaction` comparison operators while refactoring some nearby code. I found it surprising and not at all obvious that two transactions would test equal even if their witness data differed. It seems like an unnecessary potential footgun. Fix that by comparing against wtxid rather than txid.
Outside of tests, there were only 3 users of these functions in the code-base:
- Its use in the mempool has been replaced with an explicit txid comparison, as that's a tighter constraint and matches the old behavior. glozow suggested also upgrading this to an `Assume()`.
- Its use in the wallet was accidentally doing the correct thing by ignoring witness data. I've changed that to an explicit witness removal so that `IsEquivalentTo` continues to work as-intended.
- Its use in `getrawtransaction` is indifferent to the change.
ACKs for top commit:
maflcko:
review ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2 🦋
achow101:
ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2
glozow:
ACK 6efbd1e1dcdfbe9eae2d5c22abab3ee616a75ff2
Tree-SHA512: 89be424889f49e7e26dd2bdab7fbc8b2def59bf002ae8b94989b349ce97245f007d6c96e409a626cbf0de9df83ae2485b4815b40a70f7aa5b6c720eb34a6c017
8cc9845b8ddf4f93a02c622e7df8d1095dc1a640 wallet, rpc: Use `OUTPUT_TYPES` to describe the output types instead of hardcoding them (w0xlt)
Pull request description:
Follow-up to https://github.com/bitcoin/bitcoin/pull/32429, built on top of it.
This PR addresses the https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2076251627 that the RPC documentation does not use `OUTPUT_TYPES`, but rather hardcodes them, as is already the case for the `getnewaddress` command.
So here the output types are changed from `std::string` to `std::string_view` so that the values are known at compile time or during the early stages of program startup, before main() execution.
It also updates `wallet/rpc/addresses.cpp` to write the RPC docs according to `OUTPUT_TYPES` instead of using hardcoded version.
It also updates the documentation in outputtypes.h, adding Doxygen comments,
ACKs for top commit:
maflcko:
lgtm ACK 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640
achow101:
ACK 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640
Tree-SHA512: e86d813d6d158dd2f6c62519a7ecaa878f2e4f686b5bae82028a106bd6671a13b10fb366f9bb7b94974777217e1852f38e8aa05bba00cd27f94f4412167a3562
215e5999e2070a38c68e343c5c3f1dc37d567f58 wallet: Remove unused CachedTxGet{Available,Immature}Credit (Ava Chow)
49675de035e7c668d6857a32d929b7e1e85e83e3 wallet: Have GetDebit use the wallet's TXO set (Ava Chow)
17d453cb3a6fdbb0ebc8538c228c89712c989499 wallet: Recompute wallet TXOs after descriptor migration (Ava Chow)
764016eb2259676d834cf6829f5b0e04b135d407 wallet: Retrieve TXO directly in FetchSelectedInputs (Ava Chow)
c1801b78f1c11e596378a44458a484b6a9de71d8 wallet: Use wallet's TXO set in AvailableCoins (Ava Chow)
dde7cbe105ba6daaa636466d0fa3a83d15609417 wallet: Change balance calculation to use m_txos (Ava Chow)
96e7a89c5e0bf5e1a4497d8296e8841edd7ebbf1 wallet: Recalculate the wallet's txos after any imports (Ava Chow)
ae888c38d080c9c41925faeb53044e5bd0b7f9ee wallet: Exit IsTrustedTx early if wtx is already in trusted_parents (Ava Chow)
ae0876ec4273a8b978e2c602435a9ed25f48c976 wallet: Keep track of transaction outputs owned by the wallet (Ava Chow)
0f269bc48c3905c0782c9d175ad487b27ebaf54b walletdb: Load Txs last (Ava Chow)
5cc32ee2a7addb38ae4a4c97d306d0c5d9cc2d5e test: Test for balance update due to untracked output becoming spendable (Ava Chow)
8222341d4f9c2b7c572b27dea16036d6ea372067 wallet: MarkDirty after AddWalletDescriptor (Ava Chow)
e02f2d331ce6d9c5b6b8f62a79dd40695d2283a2 bench: Have AvailableCoins benchmark include a lot of unrelated utxos (Ava Chow)
Pull request description:
Currently, the wallet is not actually aware about its own transaction outputs. Instead, it will iterate all of the transactions stored in `mapWallet`, and then all of the outputs of those transactions, in order to figure out what belongs to it for the purposes of coin selection and balance calculation. For balance calculation, there is caching that results in it only iterating all of the transactions, but not all of the outputs. However when the cache is dirty, everything is iterated. This is especially problematic for wallets that have a lot of transactions, or transactions that have a lot of unrelated outputs (as may occur with coinjoins or batched payments).
This PR helps to resolve this issue by making the wallet track all of the outputs that belong to it in a new member `m_txos`. Note that this includes outputs that may have already been spent. Both balance calculation (`GetBalance`) and coin selection (`AvailableCoins`) are updated to iterate `m_txos`. This is generally faster since it ignores all of the unrelated outputs, and it is not slower as in the worst case of wallets containing only single output transactions, it's exactly the same number of outputs iterated.
`m_txos` is memory only, and it is populated during wallet loading. When each transaction is loaded, all of its outputs are checked to see if it is `IsMine`, and if so, an entry added to `m_txos`. When new transactions are received, the same procedure is done.
Since imports can change the `IsMine` status of a transaction (although they can only be "promoted" from watchonly to spendable), all of the import RPCs will be a bit slower as they re-iterate all transactions and all outputs to update `m_txos`.
Each output in `m_txos` is stored in a new `WalletTXO` class. This class contains references to the parent `CWalletTx` and the `CTxOut` itself. It also caches the `IsMine` value of the txout. This should be safe as `IsMine` should not change unless there are imports. This allows us to have additional performance improvements in places that use these `WalletTXO`s as they can use the cached `IsMine` rather than repeatedly calling `IsMine` which can be expensive.
The existing `WalletBalance` benchmark demonstrates the performance improvement that this PR makes. The existing `WalletAvailableCoins` benchmark doesn't as all of the outputs used in that benchmark belong to the test wallet. I've updated that benchmark to have a bunch of unrelated outputs in each transaction so that the difference is demonstrated.
This is part of a larger project to have the wallet actually track and store a set of its UTXOs.
Built on #24914 as it requires loading the txs last in order for `m_txos` to be built correctly.
***
## Benchmarks:
Master:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 34,590,013.00 | 28.91 | 0.0% | 812,669,269.00 | 148,360,642.50 | 5.478 | 18,356,853.00 | 0.2% | 0.76 | `WalletAvailableCoins`
| 3,193.46 | 313,139.91 | 0.4% | 96,868.06 | 13,731.82 | 7.054 | 26,238.01 | 0.1% | 0.01 | `WalletBalanceClean`
| 26,871.18 | 37,214.59 | 3.3% | 768,179.50 | 115,544.39 | 6.648 | 154,171.09 | 0.1% | 0.01 | `WalletBalanceDirty`
| 3,177.30 | 314,732.47 | 0.2% | 96,868.06 | 13,646.20 | 7.099 | 26,238.01 | 0.1% | 0.01 | `WalletBalanceMine`
| 10.73 | 93,186,952.53 | 0.1% | 157.00 | 46.14 | 3.403 | 36.00 | 0.0% | 0.01 | `WalletBalanceWatch`
| 590,497,920.00 | 1.69 | 0.1% |12,761,692,005.00 |2,536,899,595.00 | 5.030 | 129,124,399.00 | 0.7% | 6.50 | `WalletCreateEncrypted`
| 182,929,529.00 | 5.47 | 0.0% |4,199,271,397.00 | 785,477,302.00 | 5.346 | 75,363,377.00 | 1.1% | 2.01 | `WalletCreatePlain`
| 699,337.20 | 1,429.93 | 0.7% | 18,054,294.00 | 3,005,072.20 | 6.008 | 387,756.60 | 0.3% | 0.04 | `WalletCreateTxUseOnlyPresetInputs`
| 32,068,583.80 | 31.18 | 0.5% | 562,026,110.00 | 137,457,635.60 | 4.089 | 90,667,459.40 | 0.3% | 1.78 | `WalletCreateTxUsePresetInputsAndCoinSelection`
| 36.62 | 27,306,578.40 | 0.5% | 951.00 | 157.05 | 6.056 | 133.00 | 0.0% | 0.01 | `WalletIsMineDescriptors`
| 35.00 | 28,569,989.42 | 0.7% | 937.00 | 150.33 | 6.233 | 129.00 | 0.0% | 0.01 | `WalletIsMineMigratedDescriptors`
| 203,284,889.00 | 4.92 | 0.0% |4,622,691,895.00 | 872,875,275.00 | 5.296 | 90,345,002.00 | 1.2% | 1.02 | `WalletLoadingDescriptors`
| 1,165,766,084.00 | 0.86 | 0.0% |24,139,316,211.00 |5,005,218,705.00 | 4.823 |2,664,455,775.00 | 0.1% | 1.17 | `WalletMigration`
PR:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 33,975,750.50 | 29.43 | 0.1% | 794,719,150.50 | 145,763,550.00 | 5.452 | 16,036,630.50 | 0.2% | 0.75 | `WalletAvailableCoins`
| 2,442.01 | 409,498.46 | 0.2% | 60,782.04 | 10,500.60 | 5.788 | 9,492.01 | 0.3% | 0.01 | `WalletBalanceClean`
| 2,763.12 | 361,909.21 | 0.2% | 61,493.05 | 11,859.48 | 5.185 | 9,625.01 | 0.2% | 0.01 | `WalletBalanceDirty`
| 2,347.98 | 425,898.72 | 0.3% | 60,782.04 | 10,082.73 | 6.028 | 9,492.01 | 0.2% | 0.01 | `WalletBalanceMine`
| 11.67 | 85,654,630.36 | 0.2% | 176.00 | 50.18 | 3.508 | 40.00 | 0.0% | 0.01 | `WalletBalanceWatch`
| 590,119,519.00 | 1.69 | 0.1% |12,754,398,258.00 |2,534,998,522.00 | 5.031 | 129,078,027.00 | 0.7% | 6.50 | `WalletCreateEncrypted`
| 183,124,790.00 | 5.46 | 0.1% |4,199,212,926.00 | 786,323,886.00 | 5.340 | 75,354,437.00 | 1.1% | 2.02 | `WalletCreatePlain`
| 669,643.00 | 1,493.33 | 0.1% | 17,213,904.20 | 2,877,336.40 | 5.983 | 394,292.80 | 0.3% | 0.04 | `WalletCreateTxUseOnlyPresetInputs`
| 26,205,987.80 | 38.16 | 0.8% | 365,551,340.80 | 112,376,905.20 | 3.253 | 65,684,276.20 | 0.4% | 1.44 | `WalletCreateTxUsePresetInputsAndCoinSelection`
| 34.75 | 28,778,846.38 | 0.1% | 937.00 | 149.41 | 6.271 | 129.00 | 0.0% | 0.01 | `WalletIsMineDescriptors`
| 29.91 | 33,428,072.85 | 0.2% | 920.00 | 128.63 | 7.152 | 126.00 | 0.0% | 0.01 | `WalletIsMineMigratedDescriptors`
| 202,437,985.00 | 4.94 | 0.1% |4,626,686,256.00 | 869,439,274.00 | 5.321 | 90,961,305.00 | 1.1% | 1.02 | `WalletLoadingDescriptors`
| 1,158,394,152.00 | 0.86 | 0.0% |24,143,589,972.00 |4,971,946,380.00 | 4.856 |2,665,355,654.00 | 0.1% | 1.16 | `WalletMigration`
ACKs for top commit:
davidgumberg:
untested reACK 215e599
murchandamus:
reACK 215e5999e20
ishaanam:
reACK 215e5999e2070a38c68e343c5c3f1dc37d567f58
w0xlt:
reACK 215e5999e2
Tree-SHA512: d6b929de56f67930678db654e46f15fb71008390189c701a026b2d76af8f14a7c9769e49835ce7e2b6515d2934a77aad8de0b1a82231a2e1de5337de25db9629
6967e8e8abbc35ac98e8e3745a8bbed56e77526f add more bad p2p ports (Jameson Lopp)
Pull request description:
Add a few more ports used by extremely well adopted services that require authentication and really ought not be used by bitcoin nodes for p2p traffic.
ACKs for top commit:
Sjors:
utACK 6967e8e8abbc35ac98e8e3745a8bbed56e77526f
l0rinc:
ACK 6967e8e8abbc35ac98e8e3745a8bbed56e77526f
glozow:
ACK 6967e8e8abbc35ac98e8e3745a8bbed56e77526f
Tree-SHA512: bbe86aef2be9727338712ded8f90227f5d12f633ab5d324c8907c01173945d1c4d9899e05565f78688842bbf5ebb010d22173969e4168ea08d4e33f01fe9569d
28299ce77636d7563ec545d043cf1b61bd2f01c1 p2p: remove vestigial READ_STATUS_CHECKBLOCK_FAILED (Greg Sanders)
bac9ee4830664c86c1cb3d38a5b19c722aae2f54 p2p: Add witness mutation check inside FillBlock (Greg Sanders)
Pull request description:
Since #29412, we have not allowed mutated blocks to continue being processed immediately the block is received, but this is only done for the legacy BLOCK message.
Extend these checks as belt-and-suspenders to not allow similar mutation strategies to affect relay by honest peers by applying the check inside `PartiallyDownloadedBlock::FillBlock`, immediately before returning `READ_STATUS_OK`.
ACKs for top commit:
Crypt-iQ:
ACK 28299ce77636d7563ec545d043cf1b61bd2f01c1
achow101:
ACK 28299ce77636d7563ec545d043cf1b61bd2f01c1
stratospher:
ACK 28299ce7.
dergoegge:
Code review ACK 28299ce77636d7563ec545d043cf1b61bd2f01c1
Tree-SHA512: 883d7c12ca096234b425e6fe12e46b0611607600916e6ac8d1c8112224aa76924b7b074754910163ac2ec15379075d618a9ece3642649ac7629cddb0d4e432ea
c48846ec4169f749d28da05de849c43a488c3a70 doc: add release notes for #32540 (Roman Zeyde)
d4e212e8a69ea118acb6caa1a7efe64a77bdfdd2 rest: fetch spent transaction outputs by blockhash (Roman Zeyde)
Pull request description:
Today, it is possible to fetch a block's spent prevouts in order to build an external index by using the `/rest/block/BLOCKHASH.json` endpoint. However, its performance is low due to JSON serialization overhead.
We can significantly optimize it by adding a new [REST API](https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md) endpoint, using a binary response format (returning a collection of spent txout lists, one per each block transaction):
```
$ BLOCKHASH=00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054
$ ab -k -c 1 -n 100 http://localhost:8332/rest/block/$BLOCKHASH.json
Document Length: 13278152 bytes
Requests per second: 3.53 [#/sec] (mean)
Time per request: 283.569 [ms] (mean)
$ ab -k -c 1 -n 10000 http://localhost:8332/rest/spenttxouts/$BLOCKHASH.bin
Document Length: 195591 bytes
Requests per second: 254.47 [#/sec] (mean)
Time per request: 3.930 [ms] (mean)
```
Currently, this PR is being used and tested by Bindex[^1].
This PR would allow to improve the performance of external indexers such as electrs[^2], ElectrumX[^3], Fulcrum[^4] and Blockbook[^5].
[^1]: https://github.com/romanz/bindex-rs
[^2]: https://github.com/romanz/electrs (also [blockstream.info](https://github.com/Blockstream/electrs) and [mempool.space](https://github.com/mempool/electrs) forks)
[^3]: https://github.com/spesmilo/electrumx
[^4]: https://github.com/cculianu/Fulcrum
[^5]: https://github.com/trezor/blockbook
ACKs for top commit:
maflcko:
re-ACK c48846ec4169f749d28da05de849c43a488c3a70 📶
TheCharlatan:
Re-ACK c48846ec4169f749d28da05de849c43a488c3a70
achow101:
ACK c48846ec4169f749d28da05de849c43a488c3a70
Tree-SHA512: cf423541be90d6615289760494ae849b7239b69427036db6cc528ac81df10900f514471d81a460125522c5ffa31e9747ddfca187a1f93151e4ae77fe773c6b7b
95969bc58ae0cd928e536d7cb8541de93e8c7205 test: added fuzz coverage to consensus/merkle.cpp (kevkevinpal)
Pull request description:
### Summary
This adds a new fuzz target "merkle" which adds fuzz coverage to `consensus/merkle.cpp`
I can also add this to an existing fuzz target if that is preferable
Before:
After:
ACKs for top commit:
marcofleon:
ReACK 95969bc58ae0cd928e536d7cb8541de93e8c7205
Prabhat1308:
ACK [`95969bc`](95969bc58a)
maflcko:
lgtm ACK 95969bc58ae0cd928e536d7cb8541de93e8c7205
achow101:
ACK 95969bc58ae0cd928e536d7cb8541de93e8c7205
Tree-SHA512: e1fe8b69444733516bfa6cf2adaa199fde4c7c5582b7b908408f9313ed0f2e8cb803d27d707a1716d49606d5eaef8c1e722990bbc3cffc30fa91fe73d2233e9d
9341b5333ad54ccdb7c16802ff06c51b956948e7 blockstorage: make block read hash checks explicit (Lőrinc)
2371b9f4ee0b108ebbb8afedc47d73ce0f97d272 test/bench: verify hash in `ComputeFilter` reads (Lőrinc)
5d235d50d6dd0cc23175a1484e8ebb6cdc6e2183 net: assert block hash in `ProcessGetBlockData` and `ProcessMessage` (Lőrinc)
Pull request description:
A follow-up to https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2094072165, after which validating the hash of a read block from disk doesn't incur the cost of calculating its hash anymore.
### Summary
This PR adds explicit checks that the read block header's hash matches the one we were expecting.
### Context
After the previous PR, validating a block's hash during read operations became essentially free. This PR leverages that by requiring callers to provide a block's expected hash (or `std::nullopt`), preventing silent failures caused by corrupted or mismatched data. Most `ReadBlock` usages were updated with expected hashes and now fail on mismatch.
### Changes
* added hash assertions in `ProcessGetBlockData` and `ProcessMessage` to validate that the block read from disk matches the expected hash;
* updated tests and benchmark to pass the correct block hash to `ReadBlock()`, ensuring the hash validation is tested - or none if we already expect PoW failure;
* removed the default value for `expected_hash`, requiring an explicit hash for all block reads.
### Why is the hash still optional (but no longer has a default value)
* for header-error tests, where the goal is to trigger failures early in the parsing process;
* for out-of-order orphan blocks, where the child hash isn't available before the initial disk read.
ACKs for top commit:
maflcko:
review ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7 🕙
achow101:
ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
hodlinator:
ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
janb84:
re ACK 9341b5333ad54ccdb7c16802ff06c51b956948e7
Tree-SHA512: cf1d4fff4c15e3f8898ec284929cb83d7e747125d4ee759e77d369f1716728e843ef98030be32c8d608956a96ae2fbefa0e801200c333b9eefd6c086ec032e1f
9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6 node: cap -dbcache to 1GiB on 32-bit architectures (Antoine Poinsot)
2c43b6adebbfabb3c8dd82fe821ce0a5d6173b3b init: cap -maxmempool to 500 MB on 32-bit systems (Antoine Poinsot)
Pull request description:
32-bit architecture is limited to 4GiB of RAM, so it doesn't make sense to set a too high value. A too high value could cause an OOM unbeknownst to the user a while after startup as mempool / dbcache fills.
ACKs for top commit:
achow101:
ACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6
instagibbs:
utACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6
dergoegge:
Code review ACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6
glozow:
utACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6
Tree-SHA512: cc7541b2c0040fc21a43916caec464dfb443af808f4e85deffa1187448ffff6edb0d69f9ebdb43915d145b8b4694d8465afe548f88da53ccebc9ce4b7c34b735
173394d9511ed091e73eb12cb28f819026c09576 depends: Build `qt` package for FreeBSD hosts (Hennadii Stepanov)
Pull request description:
This PR continues the work started in https://github.com/bitcoin/bitcoin/pull/23948.
Here is an excerpt from the log:
```
$ ./build/bin/bitcoin-qt -printtoconsole
2025-06-12T01:06:56Z Bitcoin Core version v29.99.0-15de25ba2a28 (release build)
2025-06-12T01:06:56Z Qt 6.7.3 (static), plugin=xcb
2025-06-12T01:06:56Z Static plugins:
2025-06-12T01:06:56Z QMinimalIntegrationPlugin, version 395008
2025-06-12T01:06:56Z QXcbIntegrationPlugin, version 395008
2025-06-12T01:06:56Z Style: fusion / QFusionStyle
2025-06-12T01:06:56Z System: FreeBSD 14.3-RELEASE, x86_64-little_endian-lp64
```
And here are the screenshots:
ACKs for top commit:
vasild:
ACK 173394d9511ed091e73eb12cb28f819026c09576
Tree-SHA512: 42a0bd11e4ef1a23efcfe6c4ab179dc667a076e65060891ce8358b3fe78de4e3ea33f975387d4236cc2ac620e2935b0a29c278065a47f038c66658106bf36755
When a legacy wallet has been migrated to contain descriptors, but
before the transactions have been updated to match, we need to recompute
the wallet TXOs so that the transaction update will work correctly.
