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.
6efbd1e1dc refactor: CTransaction equality should consider witness data (Cory Fields)
cbf9b2dab1 mempool: codify existing assumption about duplicate txids during removal (Cory Fields)
e9331cd6ab 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 6efbd1e1dc🦋
achow101:
ACK 6efbd1e1dc
glozow:
ACK 6efbd1e1dc
Tree-SHA512: 89be424889f49e7e26dd2bdab7fbc8b2def59bf002ae8b94989b349ce97245f007d6c96e409a626cbf0de9df83ae2485b4815b40a70f7aa5b6c720eb34a6c017
8cc9845b8d 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 8cc9845b8d
achow101:
ACK 8cc9845b8d
Tree-SHA512: e86d813d6d158dd2f6c62519a7ecaa878f2e4f686b5bae82028a106bd6671a13b10fb366f9bb7b94974777217e1852f38e8aa05bba00cd27f94f4412167a3562
215e5999e2 wallet: Remove unused CachedTxGet{Available,Immature}Credit (Ava Chow)
49675de035 wallet: Have GetDebit use the wallet's TXO set (Ava Chow)
17d453cb3a wallet: Recompute wallet TXOs after descriptor migration (Ava Chow)
764016eb22 wallet: Retrieve TXO directly in FetchSelectedInputs (Ava Chow)
c1801b78f1 wallet: Use wallet's TXO set in AvailableCoins (Ava Chow)
dde7cbe105 wallet: Change balance calculation to use m_txos (Ava Chow)
96e7a89c5e wallet: Recalculate the wallet's txos after any imports (Ava Chow)
ae888c38d0 wallet: Exit IsTrustedTx early if wtx is already in trusted_parents (Ava Chow)
ae0876ec42 wallet: Keep track of transaction outputs owned by the wallet (Ava Chow)
0f269bc48c walletdb: Load Txs last (Ava Chow)
5cc32ee2a7 test: Test for balance update due to untracked output becoming spendable (Ava Chow)
8222341d4f wallet: MarkDirty after AddWalletDescriptor (Ava Chow)
e02f2d331c 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 215e5999e2
ishaanam:
reACK 215e5999e2
w0xlt:
reACK 215e5999e2
Tree-SHA512: d6b929de56f67930678db654e46f15fb71008390189c701a026b2d76af8f14a7c9769e49835ce7e2b6515d2934a77aad8de0b1a82231a2e1de5337de25db9629
6967e8e8ab 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 6967e8e8ab
l0rinc:
ACK 6967e8e8ab
glozow:
ACK 6967e8e8ab
Tree-SHA512: bbe86aef2be9727338712ded8f90227f5d12f633ab5d324c8907c01173945d1c4d9899e05565f78688842bbf5ebb010d22173969e4168ea08d4e33f01fe9569d
28299ce776 p2p: remove vestigial READ_STATUS_CHECKBLOCK_FAILED (Greg Sanders)
bac9ee4830 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 28299ce776
achow101:
ACK 28299ce776
stratospher:
ACK 28299ce7.
dergoegge:
Code review ACK 28299ce776
Tree-SHA512: 883d7c12ca096234b425e6fe12e46b0611607600916e6ac8d1c8112224aa76924b7b074754910163ac2ec15379075d618a9ece3642649ac7629cddb0d4e432ea
9341b5333a blockstorage: make block read hash checks explicit (Lőrinc)
2371b9f4ee test/bench: verify hash in `ComputeFilter` reads (Lőrinc)
5d235d50d6 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 9341b5333a🕙
achow101:
ACK 9341b5333a
hodlinator:
ACK 9341b5333a
janb84:
re ACK 9341b5333a
Tree-SHA512: cf1d4fff4c15e3f8898ec284929cb83d7e747125d4ee759e77d369f1716728e843ef98030be32c8d608956a96ae2fbefa0e801200c333b9eefd6c086ec032e1f
9f8e7b0b3b node: cap -dbcache to 1GiB on 32-bit architectures (Antoine Poinsot)
2c43b6adeb 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 9f8e7b0b3b
instagibbs:
utACK 9f8e7b0b3b
dergoegge:
Code review ACK 9f8e7b0b3b
glozow:
utACK 9f8e7b0b3b
Tree-SHA512: cc7541b2c0040fc21a43916caec464dfb443af808f4e85deffa1187448ffff6edb0d69f9ebdb43915d145b8b4694d8465afe548f88da53ccebc9ce4b7c34b735
173394d951 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 173394d951
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.
Instead of searching mapWallet for the preselected inputs, search
m_txos.
wallet_fundrawtransaction.py spends external inputs and needs the change
output to also belong to the test wallet for the oversized tx test.
Instead of iterating every transaction and every output stored in wallet
when trying to figure out what outputs can be spent, iterate the TXO set
which should be a lot smaller.
Since we track the outputs owned by the wallet with m_txos, we can now
calculate the balance of the wallet by iterating m_txos and summing up
the amounts of the unspent txos.
As ISMINE_USED is not an actual isminetype that we attach to outputs and
was just passed into `CachedTxGetAvailableCredit` for convenience, we
pull the same determining logic from that function into `GetBalances` in
order to preserve existing behavior.
After adding a wallet descriptor (typically by import), mark all balance
caches dirty. This allows transactions that the wallet already knows
about that have outputs that are now ISMINE_SPENDABLE after the import
to actually be shown in balance calculations. Legacy wallet imports
would do this, but importdescriptors did not.
One of the main issues with AvailableCoins is its performance when txs
have unrelated outputs, so update the benchmark to check the performance
of that.
c3fe85e2d6 wallet, rpc, test: Remove deprecated getunconfirmedbalance (Ava Chow)
0ec255139b wallet, rpc: Remove deprecated balances from getwalletinfo (Ava Chow)
Pull request description:
`getwalletinfo` result fields `balance`, `immature_balance`, and `unconfirmed_balance`, and the `getunconfirmedbalance` RPC have all been deprecated since 0.19.0. It's been long enough that they should either be removed or undeprecated. The functionality provided by these RPCs is provided by `getbalances`.
ACKs for top commit:
davidgumberg:
ACK c3fe85e2d6
rkrux:
ACK c3fe85e2d6
BrandonOdiwuor:
ACK c3fe85e2d6 removing the deprecated `balance, unconfirmed_balance, immature_balance` fields from `getwalletinfo` and `getunconfirmedbalance` RPCs, as this infomation can be found on the `getbalances` RPC
w0xlt:
reACK c3fe85e2d6
Tree-SHA512: c7c4acfd9cabc7517ba813b95281a6c6a717a417312afd9346298669b4f7bd37724ad977148ce42db7fd47fc3d1f5a8482d8ff2e7b9cb74756b171a5b8b91ef2
47237cd193 wallet, rpc: Output wallet flags in getwalletinfo (Ava Chow)
bc2a26b296 wallet: Add GetWalletFlags (Ava Chow)
69f588a99a wallet: Set upgraded descriptor cache flag for newly created wallets (Ava Chow)
Pull request description:
Newly created wallets will always have an upgraded descriptor cache, so set those.
Also, to verify this behavior, add a new `flags` field to `getwalletinfo` and check that in the functional tests.
Split from #32489
ACKs for top commit:
Sjors:
ACK 47237cd193
w0xlt:
ACK 47237cd193
rkrux:
ACK 47237cd193
Tree-SHA512: 97c7f85b858efe5ced9b8aafb6cd7c1a547de6f8013b82bfc75bc567cf73c9db5e168e3980355756541305520022fd776b8d4d240d3fb34ed86c27d2acaf4863
9eb2c82e7c walletdb: Remove unused upgraded_txs (Ava Chow)
c668033709 wallet: Remove unused fTimeReceivedIsTxTime (Ava Chow)
Pull request description:
`CWalletTx::fTimeReceivedIsTxTime` is no longer used and can be removed. This additionally allows the removal of the `upgraded_txs` loop in `LoadWallet`.
ACKs for top commit:
maflcko:
lgtm ACK 9eb2c82e7c
Eunovo:
ACK 9eb2c82e7c
davidgumberg:
ACK 9eb2c82e7c
PeterWrighten:
ACK 9eb2c82e7c
rkrux:
ACK 9eb2c82e7c
w0xlt:
ACK 9eb2c82e7c
Tree-SHA512: 05cf3a50f0d8ab6ef423ad1113c5ce6f45bfdc90e2c0dcf61c2dceced2465502e574b4b5b0091fcbb4bdd2182f8d69224f1e5516c7c505de07102b84a5f40e9c
272cd09b79 log: Use warning level while scanning wallet dir (MarcoFalke)
1777644367 qa, wallet: Verify warning when failing to scan (Hodlinator)
893e51ffeb wallet: Correct dir iteration error handling (Hodlinator)
Pull request description:
Make wallet DB properly detect and report failure to scan wallet directory. Seems to have been broken since moving from Boost to `std::filesystem`.
Found while reviewing: https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753
ACKs for top commit:
achow101:
ACK 272cd09b79
maflcko:
re-ACK 272cd09b79 🍽
rkrux:
tACK 272cd09b79
Tree-SHA512: 969afde2e37f885ed0c823dc36d2dbeaa0378639849c6a26f8ac67b4f1997eea95bbcae6d58aef5b716807210f37eb166c0cda7ba1d6caffd34249970833af3a
cd1ae1b4df fuzz: wallet: remove FundTx from FuzzedWallet (brunoerg)
Pull request description:
`FundTx` was used by the `wallet_notifications` target which we recently removed. So it's now unused and can be removed.
ACKs for top commit:
maflcko:
lgtm ACK cd1ae1b4df
kevkevinpal:
ACK [cd1ae1b](cd1ae1b4df)
dergoegge:
utACK cd1ae1b4df
Tree-SHA512: 909cc4c8a0ac2a5f8844993ccf0e725021932888da3591925799145daf9196eadfcd0ebbc74a44f4a245074ded4cb8c3c099513f109ce2681dceff36b5f74bcc
a18e572328 test: more template verification tests (Sjors Provoost)
10c908808f test: move gbt proposal mode tests to new file (Sjors Provoost)
94959b8dee Add checkBlock to Mining interface (Sjors Provoost)
6077157531 ipc: drop BlockValidationState special handling (Sjors Provoost)
74690f4ed8 validation: refactor TestBlockValidity (Sjors Provoost)
Pull request description:
This PR adds the IPC equivalent of the `getblocktemplate` RPC in `proposal` mode.
In order to do so it has `TestBlockValidity` return error reasons as a string instead of `BlockValidationState`. This avoids complexity in IPC code for handling the latter struct.
The new Mining interface method is used in `miner_tests`.
It's not used by the `getblocktemplate` and `generateblock` RPC calls, see https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2096473337
The `inconclusive-not-best-prevblk` check is moved from RPC
code to `TestBlockValidity`.
Test coverage is increased by `mining_template_verification.py`.
Superseedes #31564
## Background
### Verifying block templates (no PoW)
Stratum v2 allows miners to generate their own block template. Pools may wish (or need) to verify these templates. This typically involves comparing mempools, asking miners to providing missing transactions and then reconstructing the proposed block.[^0] This is not sufficient to ensure a proposed block is actually valid. In some schemes miners could take advantage of incomplete validation[^1].
The Stratum Reference Implementation (SRI), currently the only Stratum v2 implementation, collects all missing mempool transactions, but does not yet fully verify the block.[^2]. It could use the `getblocktemplate` RPC in `proposal` mode, but using IPC is more performant, as it avoids serialising up to 4 MB of transaction data as JSON.
(although SRI could use this PR, the Template Provider role doesn't need it, so this is _not_ part of #31098)
[^0]: https://github.com/stratum-mining/sv2-spec/blob/main/06-Job-Declaration-Protocol.md
[^1]: https://delvingbitcoin.org/t/pplns-with-job-declaration/1099/45?u=sjors
[^2]: https://github.com/stratum-mining/stratum/blob/v1.1.0/roles/jd-server/src/lib/job_declarator/message_handler.rs#L196
ACKs for top commit:
davidgumberg:
reACK a18e572328
achow101:
ACK a18e572328
TheCharlatan:
ACK a18e572328
ryanofsky:
Code review ACK a18e572328 just adding another NONFATAL_UNREACHABLE since last review
Tree-SHA512: 1a6c29f45a1666114f10f55aed155980b90104db27761c78aada4727ce3129e6ae7a522d90a56314bd767bd7944dfa46e85fb9f714370fc83e6a585be7b044f1
a201a99f8c thread-safety: fix annotations with REVERSE_LOCK (Cory Fields)
aeea5f0ec1 thread-safety: add missing lock annotation (Cory Fields)
832c57a534 thread-safety: modernize thread safety macros (Cory Fields)
Pull request description:
This is one of several PRs to cleanup/modernize our threading primitives.
While replacing the old critical section locks in the mining code with a `REVERSE_LOCK`, I noticed that our thread-safety annotations weren't hooked up to it. This PR gets `REVERSE_LOCK` working properly.
Firstly it modernizes the attributes as-recommended by the [clang docs](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) (ctrl+f for `USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES`). There's a subtle difference between the old `unlock_function` and new `release_capability`, where our `reverse_lock` only works with the latter. I believe this is an upstream bug. I've [reported and attempted a fix here](https://github.com/llvm/llvm-project/pull/139343), but either way it makes sense to me to modernize.
The second adds a missing annotation pointed out by a fixed `REVERSE_LOCK`. Because clang's thread-safety annotations aren't passed through a reference to `UniqueLock` as one may assume (see [here](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-alias-analysis) for more details), `cs_main` has to be listed explicitly as a requirement.
The last commit actually fixes the `reverse_lock` by making it a `SCOPED_LOCK` and using the pattern [found in a clang test](https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/warn-thread-safety-analysis.cpp#L3126). Though the docs don't describe how to accomplish it, the functionality was added [in this commit](6a68efc959). Due to aliasing issues (see link above), in order to work correctly, the original mutex has to be passed along with the lock, so all existing `REVERSE_LOCK`s have been updated. To ensure that the mutexes actually match, a runtime assertion is added.
ACKs for top commit:
fjahr:
re-ACK a201a99f8c
davidgumberg:
reACK a201a99f8c
theuni:
Ok, done. Those last pushes can be ignored. ACKs on a201a99 are still fresh.
ryanofsky:
Code review ACK a201a99f8c. Just dropping 0065b9673db5da2994b0b07c1d50ebfb19af39d0 and fixing incorrect `reverse_lock::lockname` initialization since last review.
TheCharlatan:
Re-ACK a201a99f8c
Tree-SHA512: 2755fae0c41021976a1a633014a86d927f104ccbc8014c01c06dae89af363f92e5bc5d4276ad6d759302ac4679fe02a543758124d48318074db1c370989af7a7
9dfc61d95f test: detect no external signer connected (Sjors Provoost)
0a4ee93529 wallet: use PSBTError::EXTERNAL_SIGNER_NOT_FOUND (Sjors Provoost)
8ba2f9b7c8 refactor: use util::Result for GetExternalSigner() (Sjors Provoost)
Pull request description:
When attempting to sign a transaction involving an external signer, if the device isn't connected we throw an `std::runtime_error`. This prevents the (mainly GUI) code that's actually supposed to handle this case from running.
This PR returns a `PSBTError::EXTERNAL_SIGNER_NOT_FOUND` instead of throwing.
The first commit is a refactor to have `GetExternalSigner()` return a `util::Result<ExternalSigner>` so the caller can decide how to handle the error. There are two other places where call `GetExternalSigner()` which this PR doesn't change (which I think is fine there).
Before:
After (the translation already exist):
Fixes#32426
Additionally use `LogWarning` instead of `std::cerr` for both a missing signer and failure to sign.
ACKs for top commit:
achow101:
ACK 9dfc61d95f
brunoerg:
code review ACK 9dfc61d95f
Tree-SHA512: 22515f4f0b4f50cb0ef532b729e247f11a68be9c90e384942d4277087b2e76806a1cdaa57fb51d5883dacf0a428e5279674aab37cce8c0d3d7de0f96346b8233
Without proper annotations, clang thinks that mutexes are still held for the
duration of a reverse_lock. This could lead to subtle bugs as
EXCLUSIVE_LOCKS_REQUIRED(foo) passes when it shouldn't.
As mentioned in the docs [0], clang's thread-safety analyzer is unable to deal
with aliases of mutexes, so it is not possible to use the lock's copy of the
mutex for that purpose. Instead, the original mutex needs to be passed back to
the reverse_lock for the sake of thread-safety analysis, but it is not actually
used otherwise.
[0]: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
6ecb9fc65f chore: use `std::vector<std::byte>` for `BlockManager::ReadRawBlock()` (Roman Zeyde)
Pull request description:
Following [this comment](https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2135820932), this PR changes `BlockManager::ReadRawBlock()` to accept a `std::vector<std::byte>` instead of `std::vector<uint8_t>`, in order to avoid casts during its invocations.
It also adds a new `SpanReader` constructor to allow reading from a span of `std::byte`s (in addition to span of `uint8_t`).
ACKs for top commit:
l0rinc:
ACK 6ecb9fc65f
maflcko:
re-ACK 6ecb9fc65f
TheCharlatan:
Re-ACK 6ecb9fc65f
Tree-SHA512: b0976c34b8da4fa1e6d805a89de2883f48ba431a71069e8c1ae450f48e425cc41aff1a5d479a7d40312a972aaf1f92e9478a985a14a1357c6b3e564e988d03e5
