07a926474b node: change a tx-relay on/off flag to enum (Vasil Dimov)
Pull request description:
Previously the `bool relay` argument to `BroadcastTransaction()` designated:
```
relay=true: add to the mempool and broadcast to all peers
relay=false: add to the mempool
```
Change this to an `enum`, so it is more readable and easier to extend with a 3rd option. Consider these example call sites:
```cpp
Paint(true);
// Or
Paint(/*is_red=*/true);
```
vs
```cpp
Paint(RED);
```
The idea for putting `TxBroadcastMethod` into `node/types.h` by Ryan.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting it in its own PR to reduce the size of #29415 and because it does not logically depend on the other commits from there.
ACKs for top commit:
optout21:
ACK 07a926474b
kevkevinpal:
ACK [07a9264](https://github.com/bitcoin/bitcoin/pull/33567/commits/07a926474b5a6fa1d3d4656362a0117611f6da2f)
laanwj:
Concept and code review ACK 07a926474b. Agree with the general reasoning and the change in #29415 is a valid motivation to change this interface.
glozow:
utACK 07a926474b
Tree-SHA512: ec8f6fa56a6d2422a0fbd5941ff2792685e8d8e7b9dd50bba9f3e21ed9b4a4a26c89b0d7e4895d48f30b7a635f2eddd894af26b5266410952cbdaf5c40b42966
1a1f46c228 refactor/doc: Add blockman param to `GetTransaction` doc comment and reorder out param (Musa Haruna)
Pull request description:
Follow-up to [#27125](https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190350876)
This PR addresses a minor documentation and style nit mentioned during review:
- Adds the missing `@param[in] blockman` line to the `GetTransaction()` doc comment.
- Moves the output parameter `hashBlock` to the end of both the function
declaration and definition, as suggested in the comment.
ACKs for top commit:
l0rinc:
ACK 1a1f46c228
maflcko:
re-lgtm-ut-cr-rfm-ACK 1a1f46c228
kevkevinpal:
reACK [1a1f46c](https://github.com/bitcoin/bitcoin/pull/33683/commits/1a1f46c2285994908df9c11991c1f363c9733087)
Tree-SHA512: 5807a1ae6c383e691e948648dcb1e029620eaff3dcdff73d88c6dc268a7af5559a30c491b72f038b3f7e812e1845f4f063b49bd3671edfac1bb3a170c84be4f5
b6f8c48946 coins: increase default `dbbatchsize` to 32 MiB (Lőrinc)
8bbb7b8bf8 refactor: Extract default batch size into kernel (Lőrinc)
Pull request description:
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)
### Summary
When the in-memory UTXO set is flushed to LevelDB (after IBD or AssumeUTXO load), it does so in batches to manage memory usage during the flush.
A hidden `-dbbatchsize` config option exists to modify this value. This PR only changes the default from `16` MiB to `32` MiB.
Using a larger default reduces the overhead of many small writes and improves I/O efficiency (especially on HDDs). It may also help LevelDB optimize writes more effectively (e.g., via internal ordering).
The change is meant to speed up a critical part of IBD: dumping the accumulated work to disk.
### Context
The UTXO set has grown significantly since [2017](https://github.com/bitcoin/bitcoin/pull/10148/files#diff-d102b6032635ce90158c1e6e614f03b50e4449aa46ce23370da5387a658342fdR26-R27), when the original fixed 16 MiB batch size was chosen.
With the current multi-gigabyte UTXO set and the common practice of using larger `-dbcache` values, the fixed 16 MiB batch size leads to several inefficiencies:
* Flushing the entire UTXO set often requires thousands of separate 16 MiB write operations.
* Particularly on HDDs, the cumulative disk seek time and per-operation overhead from numerous small writes significantly slow down the flushing process.
* Each `WriteBatch` call incurs internal LevelDB overhead (e.g., MemTable handling, compaction triggering logic). More frequent, smaller batches amplify this cumulative overhead.
Flush times of 20-30 minutes are not uncommon, even on capable hardware.
### Considerations
As [noted by sipa](https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587500105), flushing involves a temporary memory usage increase as the batch is prepared. A larger batch size naturally leads to a larger peak during this phase. Crashing due to OOM during a flush is highly undesirable, but now that [#30611](https://github.com/bitcoin/bitcoin/pull/30611) is merged, the most we'd lose is the first hour of IBD.
Increasing the LevelDB write batch size from 16 to 32 MiB raised the measured peaks by ~70 MiB in my tests during UTXO dump. The option remains hidden, and users can always override it.
The increased peak memory usage (detailed below) is primarily attributed to LevelDB's `leveldb::Arena` (backing MemTables) and the temporary storage of serialized batch data (e.g., `std::string` in `CDBBatch::WriteImpl`).
Performance gains are most pronounced on systems with slower I/O (HDDs), but some SSDs also show measurable improvements.
### Measurements:
AssumeUTXO proxy, multiple runs with error bars (flushing time is faster that the measured loading + flushing):
* Raspberry Pi, dbcache=500: ~30% faster with 32 MiB vs 16 MiB, peak +~75 MiB and still < 1 GiB.
* i7 + HDD: results vary by dbcache, but 32 MiB usually beats 16 MiB and tracks close to 64 MiB without the larger peak.
* i9 + fast NVMe: roughly flat across 16/32/64 MiB. The goal here is to avoid regressions, which holds.
### Reproducer:
```bash
# Set up a clean demo environment
rm -rfd demo && mkdir -p demo
# Build Bitcoin Core
cmake -B build -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc)
# Start bitcoind with minimal settings without mempool and internet connection
build/bin/bitcoind -datadir=demo -stopatheight=1
build/bin/bitcoind -datadir=demo -blocksonly=1 -connect=0 -dbcache=3000 -daemon
# Load the AssumeUTXO snapshot, making sure the path is correct
# Expected output includes `"coins_loaded": 184821030`
build/bin/bitcoin-cli -datadir=demo -rpcclienttimeout=0 loadtxoutset ~/utxo-880000.dat
# Stop the daemon and verify snapshot flushes in the logs
build/bin/bitcoin-cli -datadir=demo stop
grep "FlushSnapshotToDisk: completed" demo/debug.log
```
---
This PR originally proposed 64 MiB, then a dynamic size, but both were dropped: 64 MiB increased peaks more than desired on low-RAM systems, and the dynamic variant underperformed across mixed hardware. 32 MiB is a simpler default that captures most of the gains with a modest peak increase.
For more details see: https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-3234329502
---
While the PR isn't about IBD in general, rather about a critical section of it, I have measured a reindex-chainstate until 900k blocks, showing a 1% overall speedup:
<details>
<summary>Details</summary>
```python
COMMITS="e6bfd95d5012fa1d91f83bf4122cb292afd6277f af653f321b135a59e38794b537737ed2f4a0040b"; \
STOP=900000; DBCACHE=10000; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done; echo "") && \
hyperfine \
--sort command \
--runs 1 \
--export-json "$BASE_DIR/rdx-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
--parameter-list COMMIT ${COMMITS// /,} \
--prepare "killall bitcoind 2>/dev/null; rm -f $DATA_DIR/debug.log; git checkout {COMMIT}; git clean -fxd; git reset --hard && \
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release && ninja -C build bitcoind && \
./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 10" \
--cleanup "cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
"COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0"
e6bfd95d50 Merge bitcoin-core/gui#881: Move `FreespaceChecker` class into its own module
af653f321b coins: derive `batch_write_bytes` from `-dbcache` when unspecified
Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = e6bfd95d50)
Time (abs ≡): 25016.346 s [User: 30333.911 s, System: 826.463 s]
Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = af653f321b135a59e38794b537737ed2f4a0040b)
Time (abs ≡): 24801.283 s [User: 30328.665 s, System: 834.110 s]
Relative speed comparison
1.01 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = e6bfd95d50)
1.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = af653f321b135a59e38794b537737ed2f4a0040b)
```
</details>
ACKs for top commit:
laanwj:
Concept and code review ACK b6f8c48946
TheCharlatan:
ACK b6f8c48946
andrewtoth:
ACK b6f8c48946
Tree-SHA512: a72008feca866e658f0cb4ebabbeee740f9fb13680e517b9d95eaa136e627a9dd5ee328456a2bf040401f4a1977ffa7446ad13f66b286b3419ff0c35095a3521
57f7c68821 test: add functional test for `TestShell` (matching doc example) (Sebastian Falbesoner)
53874f7934 doc: test: update TestShell example instructions/options (Sebastian Falbesoner)
Pull request description:
This PR adds a functional framework test for the `TestShell` class. The primary motivation for this is to avoid that the example instructions for the interactive Python shell in `test-shell.md` get outdated or broken without noticing, a problem we had already several times in the past (see #26520, #27906, #31415). Having a copy is still not perfect, as docs and functional test have to be kept in sync, but I don't expect this to be a problem in practice, assuming the hint in the functional test comment is hopefully noticed if changes are made.
Alternatively, the example instructions in `test-shell.md` could be removed with a hint to the functional test (I tend to prefer to keep the docs as-is though, showing the full REPL interaction).
The first commit contain two small removal fix-ups in `test-shell.md` regarding the result of the `createwallet` RPC call and the mentioning of the `noshutdown` option that was removed earlier (see #31620). Could be backported to v30.
ACKs for top commit:
brunoerg:
ACK 57f7c68821
stratospher:
ACK 57f7c68.
pinheadmz:
ACK 57f7c68821
Tree-SHA512: 25c35af46b742bbefce7838708437529bbf613fa3d1f0fba590cacef0acdde82b3a78c7a01459c73eaac26ce3f1041e54092d098f0fc94a8c76ac0b4f4970338
1a7fb5eeee fees: return current block height in estimateSmartFee (ismaelsadeeq)
ab49480d9b fees: rename fees_args to block_policy_estimator_args (ismaelsadeeq)
06db08a435 fees: refactor: rename fees to block_policy_estimator (ismaelsadeeq)
6dfdd7e034 fees: refactor: rename policy_fee_tests.cpp to feerounder_tests.cpp (ismaelsadeeq)
Pull request description:
This PR is a simple refactoring that does four things:
1. Renames `test/policy_fee_tests.cpp` to `test/feerounder_tests.cpp`.
2. Renames `policy/fees.{h,cpp}` to `policy/fees/block_policy_estimator.{h,cpp}`.
3. Renames `policy/fees_args.cpp` to `policy/fees/block_policy_estimator_args.cpp`.
4. Modifies `estimateSmartFee` to return the block height at which the estimate was made by adding a `best_height` unsigned int value to the `FeeCalculation` struct.
**Motivation**
In preparation for adding a new fee estimator, the `fees` directory is created so we can organize code into `block_policy_estimator` and `mempool` because
a) It would be clunky to add more code directly under `fees`.
b) Having `policy/fees.{h,cpp}` and `policy/mempool.{h,cpp}` would also be undesirable.
Therefore, it makes sense to structure the it as `policy/fees/block_policy_estimator`, `policy/fees/mempool`, etc.
Hence test file were also updated accordingly.
The current block height is also returned because later in #30157 we log the height at which each estimate is made (at the debug log category of fee estimation :) ). This feature is particularly useful for empirical data analysis.
ACKs for top commit:
maflcko:
re-ACK 1a7fb5eeee🐤
polespinasa:
re ACK 1a7fb5eeee
willcl-ark:
ACK 1a7fb5eeee
janb84:
re ACK 1a7fb5eeee
Tree-SHA512: fef7ace2a9f262ec0361fb7a46df5108afc46b5c4b059caadf2fd114740aefbb2592389d11646c13d0e28bf0ef2cfcfbab3e659c4d4288b8ebe64725fd1963c0
944e5ff848 doc: mention key removal in rpc interface modification (rkrux)
Pull request description:
A discussion in a previous PR 32618 prompted me to add this note: https://github.com/bitcoin/bitcoin/pull/32618#discussion_r2181951390
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
maflcko:
lgtm ACK 944e5ff848
stickies-v:
ACK 944e5ff848
glozow:
lgtm ACK 944e5ff848
Tree-SHA512: f64c086c99e7c73a3ae7d60b2e8e06c8e7a3a49305a66d5c5a96db9b4ebbd01928ab5ccbcbdac26f400d16662f84469c448625e1f55ec2a9a920eff8a05fc379
0465574c12 test: Fixes send_blocks_and_test docs (Sergi Delgado Segura)
09c95f21e7 test: Adds block tiebreak over restarts tests (Sergi Delgado Segura)
18524b072e Make nSequenceId init value constants (Sergi Delgado Segura)
8b91883a23 Set the same best tip on restart if two candidates have the same work (Sergi Delgado Segura)
5370bed21e test: add functional test for complex reorgs (Pieter Wuille)
ab145cb3b4 Updates CBlockIndexWorkComparator outdated comment (Sergi Delgado Segura)
Pull request description:
This PR grabs some interesting bits from https://github.com/bitcoin/bitcoin/pull/29284 and fixes some edge cases in how block tiebreaks are dealt with.
## Regarding #29284
The main functionality from the PR was dropped given it was not an issue anymore, however, reviewers pointed out some comments were outdated https://github.com/bitcoin/bitcoin/pull/29284#discussion_r1522023578 (which to my understanding may have led to thinking that there was still an issue) it also added test coverage for the aforementioned case which was already passing on master and is useful to keep.
## New functionality
While reviewing the superseded PR, it was noticed that blocks that are loaded from disk may face a similar issue (check https://github.com/bitcoin/bitcoin/pull/29284#issuecomment-1994317785 for more context).
The issue comes from how tiebreaks for equal work blocks are handled: if two blocks have the same amount of work, the one that is activatable first wins, that is, the one for which we have all its data (and all of its ancestors'). The variable that keeps track of this, within `CBlockIndex` is `nSequenceId`, which is not persisted over restarts. This means that when a node is restarted, all blocks loaded from disk are defaulted the same `nSequenceId`: 0.
Now, when trying to decide what chain is best on loading blocks from disk, the previous tiebreaker rule is not decisive anymore, so the `CBlockIndexWorkComparator` has to default to its last rule: whatever block is loaded first (has a smaller memory address).
This means that if multiple same work tip candidates were available before restarting the node, it could be the case that the selected chain tip after restarting does not match the one before.
Therefore, the way `nSequenceId` is initialized is changed to:
- 0 for blocks that belong to the previously known best chain
- 1 to all other blocks loaded from disk
ACKs for top commit:
sipa:
utACK 0465574c12
TheCharlatan:
ACK 0465574c12
furszy:
Tested ACK 0465574c12.
Tree-SHA512: 161da814da03ce10c34d27d79a315460a9c98d019b85ee35bc5daa991ed3b6a2e69a829e421fc70d093a83cf7a2e403763041e594df39ed1991445e54c16532a
cc5dda1de3 headerssync: Make HeadersSyncState more flexible and move constants (Hodlinator)
8fd1c2893e test(headerssync): Test returning of pow_validated_headers behavior (Hodlinator)
7b00643ef5 test(headerssync): headers_sync_chainwork test improvements (Hodlinator)
04eeb9578c doc(test): Improve comments (Hodlinator)
fe896f8faa refactor(test): Store HeadersSyncState on the stack (Hodlinator)
f03686892a refactor(test): Break up headers_sync_state (Hodlinator)
e984618d0b refactor(headerssync): Process spans of headers (Hodlinator)
a4ac9915a9 refactor(headerssync): Extract test constants ahead of breakup into functions (Hodlinator)
Pull request description:
### Background
As part of the release process we often run *contrib/devtools/headerssync-params.py* and increase the values of the constants `HEADER_COMMITMENT_PERIOD` and `REDOWNLOAD_BUFFER_SIZE` in *src/headerssync.cpp* as per *doc/release-process.md* (example: 11a2d3a63e). This helps fine tune the memory consumption per `HeadersSyncState`-instance in the face of malicious peers.
(The `REDOWNLOAD_BUFFER_SIZE`/`HEADER_COMMITMENT_PERIOD` ratio determines how many Headers Sync commitment bits must match between PRESYNC & REDOWNLOAD phases before we start permanently storing headers from a peer. For more details see comments in *src/headerssync.h* and *contrib/devtools/headerssync-params.py*).
### Problem: Not feeding back headers until completing sync
During v30 release process #33274 made `REDOWNLOAD_BUFFER_SIZE` exceed the `target_blocks` constant used to control the length of chains generated for testing Headers Sync (`15000`, *headers_sync_chainwork_tests.cpp*).
The `HeadersSyncState::m_redownloaded_headers`-buffer now does not reach the `REDOWNLOAD_BUFFER_SIZE`-threshold during those unit tests. As a consequence `HeadersSyncState::PopHeadersReadyForAcceptance()` will not start feeding back headers until the PoW threshold has been met. While this will not cause the unit test to start failing on master, it means we have gone from testing behavior that resembles mainnet (way more than `REDOWNLOAD_BUFFER_SIZE` headers to reach the PoW limit), to behavior that is not possible/expected there.
### Solution
Avoid testing this unrealistic condition of completing Headers Sync before reaching `REDOWNLOAD_BUFFER_SIZE` by making tests able to define their own values through the new `HeadersSyncParams` instead of having them hard-coded for all chains & tests.
### Commits
* First 6 commits refactor and improve the unit tests in order to clarify latter changes.
* We then add checks for the behavior around the `REDOWNLOAD_BUFFER_SIZE` threshold.
* The main change: we extract the section from *headerssync.cpp* containing the constants to *kernel/chainparams.cpp*, making `HeadersSyncState` no longer hard-coded to mainnet.
### Notes
This PR used to be called "headerssync: Preempt unrealistic unit test behavior".
ACKs for top commit:
l0rinc:
reACK cc5dda1de3
marcofleon:
code review ACK cc5dda1de3
danielabrozzoni:
reACK cc5dda1de3
Tree-SHA512: ccc824dcbbb8ad5ae98c3bf5808b38467aac0230739898a758c9b939eecd74f982df088fa0ba81cc1c1732f19a607b135a6e9577bb9fcf7f8570567ce92f66e6
d0e1bbad01 test: repeat block malleability test with relayable block over P2P (Musa Haruna)
Pull request description:
This PR adds a functional test to repeat the existing malleability check for oversized coinbase witness nonce size using a block that is small enough to be relayed over the P2P network.
This addresses the TODO in test_block_malleability by ensuring behavior is consistent between submitblock RPC and P2P relay.
ACKs for top commit:
maflcko:
lgtm ACK d0e1bbad01
janb84:
re ACK d0e1bbad01
glozow:
utACK d0e1bbad01
Tree-SHA512: 05aec4fade5af8043f40274a8d2f3cf3f540acd038138975bdefbbbc81e105792d6d2588256a2ee5ddb1e05b37fe2d0b3d287160d2dbe86e1aac7cfa9cc02116
8f7673257a miner: fix empty mempool case for waitNext() (Sjors Provoost)
Pull request description:
Block template fees are calculated by looping over `new_tmpl->vTxFees` and return (early) once the `fee_threshold` is exceeded.
This left an edge case when the mempool is empty, which this commit fixes and adds a test for.
Also update `test/functional/interface_ipc.py` to reflect the new behavior,
Fixes https://github.com/Sjors/sv2-tp/issues/9
ACKs for top commit:
optout21:
ACK 8f7673257a
cedwies:
tACK 8f76732
sipa:
utACK 8f7673257a
zaidmstrr:
Concept ACK [8f76732](https://github.com/bitcoin/bitcoin/pull/33566/commits/8f7673257a1a86717c1d83770dc857fc254df107)
Tree-SHA512: ef200fe95e96f810e425283bc37f945c4bf5efa16f4b74820b8a07968f30c5146bca213a372124be84b48beead5dfd35f2b5d10d188d0a465f847ebab61de10a
9b43428c96 TxGraph: change m_excluded_clusters (Greg Sanders)
Pull request description:
Change BlockBuilderImpl's m_excluded_clusters to unordered set since ordering is not used.
Change the set to a set of sequence numbers for a modest stability increase under fuzz testing.
ACKs for top commit:
sipa:
ACK 9b43428c96
marcofleon:
tACK 9b43428c96
glozow:
ACK 9b43428c96
Tree-SHA512: 140a492af93f3eff756847a8168aab2624bb7df407f177dde6f3b07e9db2d0ced6b125e2b126f4957ccd054272056bedf74f9f0e64a80d90c16fd94e0fa86a44
24d861da78 coins: only adjust `cachedCoinsUsage` on `EmplaceCoinInternalDANGER` insert (Lőrinc)
d7c9d6c291 coins: fix `cachedCoinsUsage` accounting to prevent underflow (Lőrinc)
39cf8bb3d0 refactor: remove redundant usage tracking from `CoinsViewCacheCursor` (Lőrinc)
67cff8bec9 refactor: assert newly-created parent cache entry has zero memory usage (Lőrinc)
Pull request description:
### Summary
This PR fixes `cachedCoinsUsage` accounting bugs in `CCoinsViewCache` that caused UBSan `unsigned-integer-overflow` violations during testing. The issues stemmed from incorrect decrement timing in `AddCoin()`, unconditional reset in `Flush()` on failure, and incorrect increment in `EmplaceCoinInternalDANGER()` when insertion fails.
### Problems Fixed
**1. `AddCoin()` underflow on exception**
- Previously decremented `cachedCoinsUsage` *before* the `possible_overwrite` validation
- If validation threw, the map entry remained unchanged but counter was decremented
- This corrupted accounting and later caused underflow
- **Impact**: Test-only in current codebase, but unsound accounting that could affect future changes
**2. `Flush()` accounting drift on failure**
- Unconditionally reset `cachedCoinsUsage` to 0, even when `BatchWrite()` failed
- Left the map populated while the counter read zero
- **Impact**: Test-only (production `BatchWrite()` returns `true`), but broke accounting consistency
**3. Cursor redundant usage tracking**
- `CoinsViewCacheCursor::NextAndMaybeErase()` subtracted usage when erasing spent entries
- However, `SpendCoin()` already decremented and cleared the `scriptPubKey`, leaving `DynamicMemoryUsage()` at 0
- **Impact**: Redundant code that obscured actual accounting behavior
**4. `EmplaceCoinInternalDANGER()` double-counting**
- Incremented `cachedCoinsUsage` even when `try_emplace` did not insert (duplicate key)
- Inflated the counter on duplicate attempts
- **Impact**: Mostly test-reachable (AssumeUTXO doesn't overwrite in production), but incorrect accounting
### Testing
To reproduce the historical UBSan failures on the referenced baseline and to verify the fix, run:
```
MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh
```
The change was tested with the related unit and fuzz test, and asserted before/after each `cachedCoinsUsage` change (in production code and fuzz) that the calculations are still correct by recalculating them from scratch.
<details>
<summary>Details</summary>
```C++
bool CCoinsViewCache::CacheUsageValid() const
{
size_t actual{0};
for (auto& entry : cacheCoins | std::views::values) actual += entry.coin.DynamicMemoryUsage();
return actual == cachedCoinsUsage;
}
```
or
```patch
diff --git a/src/coins.cpp b/src/coins.cpp
--- a/src/coins.cpp(revision fd3b1a7f4bb2ac527f23d4eb4cfa40a3215906e5)
+++ b/src/coins.cpp(revision 872a05633bfdbd06ad82190d7fe34b42d13ebfe9)
@@ -96,6 +96,7 @@
fresh = !it->second.IsDirty();
}
if (!inserted) {
+ Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
}
it->second.coin = std::move(coin);
@@ -133,6 +134,7 @@
bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) {
CCoinsMap::iterator it = FetchCoin(outpoint);
if (it == cacheCoins.end()) return false;
+ Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
TRACEPOINT(utxocache, spent,
outpoint.hash.data(),
@@ -226,10 +228,12 @@
if (itUs->second.IsFresh() && it->second.coin.IsSpent()) {
// The grandparent cache does not have an entry, and the coin
// has been spent. We can just delete it from the parent cache.
+ Assert(cachedCoinsUsage >= itUs->second.coin.DynamicMemoryUsage());
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
cacheCoins.erase(itUs);
} else {
// A normal modification.
+ Assert(cachedCoinsUsage >= itUs->second.coin.DynamicMemoryUsage());
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
if (cursor.WillErase(*it)) {
// Since this entry will be erased,
@@ -279,6 +283,7 @@
{
CCoinsMap::iterator it = cacheCoins.find(hash);
if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) {
+ Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
TRACEPOINT(utxocache, uncache,
hash.hash.data(),
```
</details>
ACKs for top commit:
optout21:
reACK 24d861da78
andrewtoth:
ACK 24d861da78
sipa:
ACK 24d861da78
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/32313/commits/24d861da7894add47747eff69dd3fc71fbcdd7d0
Tree-SHA512: ff1b756b46220f278ab6c850626a0f376bed64389ef7f66a95c994e1c7cceec1d1843d2b24e8deabe10e2bdade2a274d9654ac60eb2b9bf471a71db8a2ff496c
ac599c4a9c test: Test MuSig2 in the wallet (Ava Chow)
68ef954c4c wallet: Keep secnonces in DescriptorScriptPubKeyMan (Ava Chow)
4a273edda0 sign: Create MuSig2 signatures for known MuSig2 aggregate keys (Ava Chow)
258db93889 sign: Add CreateMuSig2AggregateSig (Ava Chow)
bf69442b3f sign: Add CreateMuSig2PartialSig (Ava Chow)
512b17fc56 sign: Add CreateMuSig2Nonce (Ava Chow)
82ea67c607 musig: Add MuSig2AggregatePubkeys variant that validates the aggregate (Ava Chow)
d99a081679 psbt: MuSig2 data in Fill/FromSignatureData (Ava Chow)
4d8b4f5336 signingprovider: Add musig2 secnonces (Ava Chow)
c06a1dc86f Add MuSig2SecNonce class for secure allocation of musig nonces (Ava Chow)
9baff05e49 sign: Include taproot output key's KeyOriginInfo in sigdata (Ava Chow)
4b24bfeab9 pubkey: Return tweaks from BIP32 derivation (Ava Chow)
f14876213a musig: Move synthetic xpub construction to its own function (Ava Chow)
fb8720f1e0 sign: Refactor Schnorr sighash computation out of CreateSchnorrSig (Ava Chow)
a4cfddda64 tests: Clarify why musig derivation adds a pubkey and xpub (Ava Chow)
39a63bf2e7 descriptors: Add a doxygen comment for has_hardened output_parameter (Ava Chow)
2320184d0e descriptors: Fix meaning of any_key_parsed (Ava Chow)
Pull request description:
This PR implements MuSig2 signing so that the wallet can receive and spend from imported `musig(0` descriptors.
The libsecp musig module is enabled so that it can be used for all of the MuSig2 cryptography.
Secnonces are handled in a separate class which holds the libsecp secnonce object in a `secure_unique_ptr`. Since secnonces must not be used, this class has no serialization and will only live in memory. A restart of the software will require a restart of the MuSig2 signing process.
ACKs for top commit:
fjahr:
tACK ac599c4a9c
rkrux:
lgtm tACK ac599c4a9c
theStack:
Code-review ACK ac599c4a9c🗝️
Tree-SHA512: 626b9adc42ed2403e2f4405321eb9ce009a829c07d968e95ab288fe4940b195b0af35ca279a4a7fa51af76e55382bad6f63a23bca14a84140559b3c667e7041e
0626b90f50 multiprocess: align our logging with libmultiprocess's (Cory Fields)
9d068225ee multiprocess: update multiprocess EventLoop construction to use options (Cory Fields)
Pull request description:
This fixes https://github.com/bitcoin-core/libmultiprocess/issues/215 on Core's side. ~It depends on https://github.com/bitcoin-core/libmultiprocess/pull/220 being merged upstream, and a PR to update our subtree. I've included a subtree merge from my repo here for now, but will rebase on top of the merge from upstream once it's in.~ Edit: Rebased on top of #33518.
For context: before https://github.com/bitcoin-core/libmultiprocess/pull/220, libmultiprocess serializesd every log message parameter, even if that message was ultimately going to be discarded. The upstream PR accomplished 2 main things:
- Creates logging categories, similar to Core's
- Using macros, avoids serializing parameters for disabled log levels.
That allowed the expensive serialization to be skipped, but the default log level is `Trace`. This PR updates our usage of libmultiprocess options to honor our log categories and levels.
Because of the substantial unnecessary overhead (see the [flamegraphs](https://github.com/bitcoin-core/libmultiprocess/issues/215). Logging accounts for 50% of my application's cpu time, and nearly 10% of bitcoin-node's, both of which go to ~0% once fixed), it'd be a shame to ship the first multiprocess binaries without this fixed. So I propose that we also backport this (and the required libmultiprocess subtree merge) ~to v30. Sorry about the timing~ :(
Edit: Didn't make it for v30, but it would still make sense to backport for a v30.1.
ACKs for top commit:
Sjors:
ACK 0626b90f50
TheCharlatan:
ACK 0626b90f50
sipa:
utACK 0626b90f50
Tree-SHA512: 70b63b62d1f6de547f4d4775538d7bcaf32f57d8a72c5b26762b57755810c8be6942d9dfebab43cf1c1d8d025a555f72a48e9ebf3d84f8d40d6592ca801cda5d
023cd5a546 txgraph: add SingletonClusterImpl (mem optimization) (Pieter Wuille)
e346250732 txgraph: give Clusters a range of intended tx counts (preparation) (Pieter Wuille)
e93b0f09cc txgraph: abstract out creation of empty Clusters (refactor) (Pieter Wuille)
6baf12621f txgraph: comment fixes (doc fix) (Pieter Wuille)
726b995739 txgraph: make Cluster an abstract class (refactor) (Pieter Wuille)
2602d89edd txgraph: avoid accessing other Cluster internals (refactor) (Pieter Wuille)
04c808ac4c txgraph: expose memory usage estimate function (feature) (Pieter Wuille)
7680bb8fd4 txgraph: keep track of Cluster memory usage (preparation) (Pieter Wuille)
4ba562e5f4 txgraph: keep data structures compact (mem optimization) (Pieter Wuille)
bb5cb222ae depgraph: add memory usage control (feature) (Pieter Wuille)
b1637a90de txgraph: avoid holes in DepGraph positions (mem optimization) (Pieter Wuille)
2b1d302508 txgraph: move some sanity checks from Cluster to TxGraphImpl (refactor) (Pieter Wuille)
d40302fbaf txgraph: Make level of Cluster implicit (optimization) (Pieter Wuille)
Pull request description:
Part of #30289.
This adds a few optimizations to reduce `TxGraph`'s memory usage, and makes sure that dynamic memory it uses doesn't linger after shrinking clusters. Finally, it exposes a function `GetMainMemoryUsage()` to compute `TxGraph`'s approximate memory usage.
It makes the `Cluster` type abstract, with two instances (`SingletonClusterImpl` for 1-transaction clusters, and `GenericClusterImpl` for others).
On my 64-bit system, I obtain the following numbers:
* `SingletonClusterImpl`: 48 bytes, plus 16 bytes malloc overhead in its `unique_ptr`, plus 8-byte pointer in `m_clusters`
* `GenericClusterImpl`: 104 bytes, plus 16 bytes malloc overhead in its `unique_ptr`, plus 8-byte pointer in `m_clusters`, plus 72 bytes malloc overhead inside its vectors and `DepGraph`, plus 40 bytes per transaction in those.
* `TxGraphImpl::Entry`: 72 bytes per transaction
* `TxGraphImpl::ChunkData`: 8 bytes, plus 56 bytes in `std::set` overhead + malloc overhead, all per chunk.
* `TxGraph::Ref`: 16 bytes per transaction
This overall amounts to 200 bytes per cluster, plus 64 bytes per chunk, plus 128 bytes per transaction, but only 224 bytes overall per singleton cluster.
ACKs for top commit:
l0rinc:
code review reACK 023cd5a546
instagibbs:
reACK 023cd5a546
ismaelsadeeq:
reACK 023cd5a546🚢
glozow:
reACK 023cd5a546
Tree-SHA512: c957b27f47318be7c25d71453df2ae9d4e7bf21dab13b6e5e975cca122a221a99b15c584872491225785d276a9165f090675ee0f4460a2775bd3271933e3b246
e4335a3192 Revert "depends: Update URL for `qrencode` package source tarball" (Ava Chow)
a89a822e6e Revert "depends: Use hash instead of file name for package download stamp" (Ava Chow)
Pull request description:
The new URL breaks CI on the current release branches, see https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3380802351.
The old URL also no longer exists so the tarball is fetched from the depends sources cache that we host, but the original tarball has already been overwritten on there. We will need to manually reinstate the original tarball.
ACKs for top commit:
m3dwards:
utACK e4335a3192
maflcko:
review ACK e4335a3192💤
glozow:
ACK e4335a3192
Tree-SHA512: a5028342d77b4768daaec8688acd364795d683aed2bea0407c7827d44f814a97d50cc3b30c2de2a8296a2b212115fe1e76c57685a74e93387fc57afdabb93bd2
1aaaaa078b fuzz: Drop unused workaround after Apple-Clang bump (MarcoFalke)
fadad7a494 Drop support for EOL macOS 13 (MarcoFalke)
Pull request description:
Now that macOS 13 is EOL (https://en.wikipedia.org/wiki/MacOS_Ventura), it seems odd to still support it.
(macOS Ventura 13.7.8 received its final security update on 20 Aug 2025: https://support.apple.com/en-us/100100)
This patch will only be released in version 31.x, another 6 months out from now.
So:
* Update the depends build and release note template to drop EOL macOS 13.
* As a result, update the earliest Xcode to version 16 in CI.
* Also, bump the macOS CI runner to version 15, to avoid issues when version 14 will be at its EOL in about 1 year.
This also allows to drop a small workaround in the fuzz tests and unlocks libcpp hardening (https://github.com/bitcoin/bitcoin/pull/33462)
ACKs for top commit:
stickies-v:
re-ACK 1aaaaa078b
l0rinc:
code review ACK 1aaaaa078b
hodlinator:
re-ACK 1aaaaa078b
hebasto:
ACK 1aaaaa078b.
Tree-SHA512: 6d247a8432ef8ea8c6ff2a221472b278f8344346b172980299507f9898bb9e8e16480c128b1f4ca692bcbcc393da2b2fd6895ac5f118bc09e0f30f910529d20c
c76de2eea1 net: support overriding the proxy selection in ConnectNode() (Vasil Dimov)
Pull request description:
Normally `ConnectNode()` would choose whether to use a proxy and which one. Make it possible to override this from the callers and same for `OpenNetworkConnection()` - pass down the proxy to `ConnectNode()`.
Document both functions.
This is useful if we want to open connections to IPv4 or IPv6 peers through the Tor SOCKS5 proxy.
Also have `OpenNetworkConnection()` return whether the connection succeeded or not. This can be used when the caller needs to keep track of how many (successful) connections were opened.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting it in its own PR to reduce the size of #29415 and because it does not depend on the other commits from there.
ACKs for top commit:
stratospher:
ACK c76de2e.
optout21:
ACK c76de2eea1
mzumsande:
Code Review ACK c76de2eea1
andrewtoth:
ACK c76de2eea1
Tree-SHA512: 1d266e4280cdb1d0599971fa8b5da58b1b7451635be46abb15c0b823a1e18cf6e7bcba4a365ad198e6fd1afee4097d81a54253fa680c8b386ca6b9d68d795ff0
1ff9e92948 key: use static context for libsecp256k1 calls where applicable (Sebastian Falbesoner)
Pull request description:
The dynamically created [signing context](https://github.com/bitcoin/bitcoin/blob/2d6a0c464912c325faf35d4ad28b1990e828b414/src/key.cpp#L19) for libsecp256k1 calls is only needed for functions that involve generator point multiplication with a secret key, i.e. different variants of public key creation and signing. The API docs hint to those by stating "[(not secp256k1_context_static)](https://github.com/bitcoin-core/secp256k1/blob/b4756543028065b3ae6f30e9e6d7f1ecf2bb08c6/include/secp256k1.h#L645)" for the context parameter. In our case that applies to the following calls:
- `secp256k1_ec_pubkey_create`
- `secp256k1_keypair_create`
- `secp256k1_ellswift_create`
- `secp256k1_ecdsa_sign`
- `secp256k1_ecdsa_sign_recoverable`
- `secp256k1_schnorrsig_sign32`
- `ec_seckey_export_der` (not a direct secp256k1 function, but calls `secp256k1_ec_pubkey_create` inside)
For all the other secp256k1 calls we can simply use the static context. This is done for consistency to other calls that already use `secp256k1_context_static`, and also to reduce dependencies on the global signing context variable. Looked closer at this in the course of reviewing #29675, where some functions used the signing context that didn't need to, avoiding a move to another module (see https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2333831377).
ACKs for top commit:
Eunovo:
ACK https://github.com/bitcoin/bitcoin/pull/33399/commits/1ff9e929489e625a603e8755b8efe849feda1f16
furszy:
ACK 1ff9e92948
rkrux:
crACK 1ff9e92948
Tree-SHA512: f091efa56c358057828f3455d4ca9ce40ec0d35f3e38ab147fe3928bb5dbf7ffbc27dbf97b71937828ab95ea4e9be5f96d89a2d29e2aa18df4542aae1b33e258
df67bb6fd8 test: Remove convert_to_json_for_cli (Ava Chow)
44a493e150 cli: Allow arguments to be both strings and json (Ava Chow)
Pull request description:
There are some RPCs where the argument can be either JSON that needs to be parsed, or a string that we can pass straight through. However, `bitcoin-cli` would always parse those arguments as JSON which makes for some cumbersome argument passing when using those RPCs. Notably, `hash_or_height` in `getblockstats` and `gettxoutsetinfo` do this, and results in a more cumbersome command of `bitcoin-cli getblockstats '"<hash>"'`. Otherwise, using a normal invocation of `bitcoin-cli getblockstats <hash>` results in `error: Error parsing JSON`. This PR marks those particular options as also being a string so that when `bitcoin-cli` fails to parse the argument as JSON, it will assume that the argument is a string and pass it straight through.
ACKs for top commit:
ryanofsky:
Code review ACK df67bb6fd8, just rebased since last review. I do still think it would be good to improve the test (https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2369570345)
rkrux:
Light code review, lgtm ACK df67bb6fd8
mzumsande:
Code Review ACK df67bb6fd8
Tree-SHA512: 6c488570fbb24d0cf10508416c56accfc7af5163b7a7187d22d78c812424a9e3ecc95906d3e295fbf6af54bf80903aa448fd879dd6a9944ba8b4d1a33eb29ef2
88b0647f02 wallet: Always write last hardened cache flag in migrated wallets (Ava Chow)
8a08eef645 tests: Check that the last hardened cache upgrade occurs (Ava Chow)
Pull request description:
#32597 set the descriptor cache upgraded flag for newly created wallets, but migrated wallets still did not have the flag set when they are migrated. For consistency, and to avoid an unnecessary upgrade, we should be setting this flag for migrated wallets.
The flag would end up being set anyways at the end of migration when the wallet is reloaded as it would perform the automatic upgrade at that time. However, this is unnecessary and we should just set it from the get go.
This PR also adds a couple tests to verify that the flag is being set, and that the upgrade is being performed.
ACKs for top commit:
cedwies:
re-ACK 88b0647
rkrux:
lgtm ACK 88b0647f02
pablomartin4btc:
ACK 88b0647f02
Tree-SHA512: 7d0850db0ae38eedd1e6a3bfaa548c6c612182291059fb1a47279a4c4984ee7914ecd02d8c7e427ef67bf9f5e67cbc57a7ae4412fad539e1bf3e05c512a60d69
2427939935 test: forbid copying of DebugLogHelper (Daniel Pfeifer)
d6aa266d43 test: don't throw from the destructor of DebugLogHelper (Vasil Dimov)
Pull request description:
Throwing an exception from the destructor of a class is a bad practice because the destructor will be called when an object of that type is alive on the stack and another exception is thrown, which will result in "exception during the exception". This would terminate the program without any messages.
Instead print the message to the standard error output and call `std::abort()`.
---
This change is part of https://github.com/bitcoin/bitcoin/pull/26812. It is an improvement on its own, so creating a separate PR for it following the discussion at https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2345091587. Getting it in will reduce the size of #26812.
ACKs for top commit:
Crypt-iQ:
crACK 2427939
l0rinc:
Code review reACK 2427939935
optout21:
crACK 2427939935
furszy:
utACK 2427939935
Tree-SHA512: 918c1e40d2db4ded6213cd78a18490ad10a9f43c0533df64bdf09f0b216715415030e444712981e4407c32ebf552fbb0e3cce718e048df10c2b8937caf015564
0a26731c4c test: Add submitblock test in interface_ipc (TheCharlatan)
Pull request description:
Expands the ipc mining test a bit with submitting a solved block and checking its validity.
ACKs for top commit:
Sjors:
ACK 0a26731c4c
marcofleon:
code review ACK 0a26731c4c
zaidmstrr:
Tested ACK [0a26731](https://github.com/bitcoin/bitcoin/pull/33380/commits/0a26731c4cc182e887ce959cdd301227cdc752d7)
Tree-SHA512: 35c87d88496eec469bddedf2ae82c494626abb47ae15d5a45d6ab0400199c86501199c3e569e83836549830042be76b197b470e1100a317bdfef2578a9d5a92f
d45f3717d2 txgraph: use enum Level instead of bool main_only (Pieter Wuille)
Pull request description:
Part of #30289. Inspired by https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2331387778.
Since there has been more than one case in the development of #28676 of calling a `TxGraph` function without correctly setting the `bool main_only` argument that many of its interface functions have, make these mandatory and explicit, using an `enum class Level`:
```c++
enum class Level {
TOP, //!< Refers to staging if it exists, main otherwise.
MAIN //!< Always refers to the main graph, whether staging is present or not.
};
```
ACKs for top commit:
instagibbs:
ACK d45f3717d2
vasild:
ACK d45f3717d2
glozow:
code review ACK d45f3717d2
Tree-SHA512: d1c4b37e8ab3ec91b414df8970cb47aa080803f68da5881c8e1cbdc6939dea7851e0f715192cf3edd44b7f328cd6b678474d41f9cd9da8cb68f6c5fd78cb71b1
a341e11ac9 ci: test IPC on additional hosts (Sjors Provoost)
6aee573bfc ci: enable IPC tests in CI (Pieter Wuille)
8d2ee88fa2 tests: add functional tests for IPC interface (Pieter Wuille)
3cc9a06c8d test: Add TestNode ipcbind option (Ryan Ofsky)
3cceb60a71 test: Provide path to `bitcoin` binary (Ryan Ofsky)
8c7f005629 test: add is_ipc_compiled() and skip_if_no_ipc() functions (Ryan Ofsky)
Pull request description:
This adds support to the functional test framework to run the multiprocess `bitcoin-node` binary, and then tests it in a new `interface_ipc.py` functional test through the `pycapnp` module.
ACKs for top commit:
Sjors:
ACK a341e11ac9
ryanofsky:
Code review ACK a341e11ac9. Changes since last review: rebasing, switching to miniwallet and expanding wallet test, improving pycapnp install steps in instructions and CI.
TheCharlatan:
ACK a341e11ac9
Tree-SHA512: 98330283cf0d66d5537eec1219345b8aec5740dbc2171bd8b70680d7a282e0962fcdf0588a75518110761e8bc95af57d6097c93937cac710805b0df10837093c
5c8bf7b39e doc: add release notes for version 3 transactions (ishaanam)
4ef8065a5e test: add truc wallet tests (ishaanam)
5d932e14db test: extract `bulk_vout` from `bulk_tx` so it can be used by wallet tests (ishaanam)
2cb473d9f2 rpc: Support version 3 transaction creation (Bue-von-hon)
4c20343b4d rpc: Add transaction min standard version parameter (Bue-von-hon)
c5a2d08011 wallet: don't return utxos from multiple truc txs in AvailableCoins (ishaanam)
da8748ad62 wallet: limit v3 tx weight in coin selection (ishaanam)
85c5410615 wallet: mark unconfirmed v3 siblings as mempool conflicts (ishaanam)
0804fc3cb1 wallet: throw error at conflicting tx versions in pre-selected inputs (ishaanam)
cc155226fe wallet: set m_version in coin control to default value (ishaanam)
2e9617664e wallet: don't include unconfirmed v3 txs with children in available coins (ishaanam)
ec2676becd wallet: unconfirmed ancestors and descendants are always truc (ishaanam)
Pull request description:
This PR Implements the following:
- If creating a v3 transaction, `AvailableCoins` doesn't return unconfirmed v2 utxos (and vice versa)
- `AvailableCoins` doesn't return an unconfirmed v3 utxo if its transaction already has a child
- If a v3 transaction is kicked out of the mempool by a sibling, mark the sibling as a mempool conflict
- Throw an error if pre-selected inputs are of the wrong transaction version
- Allow setting version to 3 manually in `createrawtransaction` (uses commits from #31936)
- Limits a v3 transaction weight in coin selection
Closes#31348
To-Do:
- [x] Test a v3 sibling conflict kicking out one of our transactions from the mempool
- [x] Implement separate size limit for TRUC children
- [x] Test that we can't fund a v2 transaction when everything is v3 unconfirmed
- [x] Test a v3 sibling conflict being removed from the mempool
- [x] Test limiting v3 transaction weight in coin selection
- [x] Simplify tests
- [x] Add documentation
- [x] Test that user-input max weight is not overwritten by truc max weight
- [x] Test v3 in RPCs other than `createrawtransaction`
ACKs for top commit:
glozow:
reACK 5c8bf7b39e
achow101:
ACK 5c8bf7b39e
rkrux:
ACK 5c8bf7b39e
Tree-SHA512: da8aea51c113e193dd0b442eff765bd6b8dc0e5066272d3e52190a223c903f48788795f32c554f268af0d2607b5b8c3985c648879cb176c65540837c05d0abb5
de0675f9de refactor: Move `transaction_identifier.h` to primitives (marcofleon)
6f068f65de Remove implicit uint256 conversion and comparison (marcofleon)
9c24cda72e refactor: Convert remaining instances from uint256 to Txid (marcofleon)
d2ecd6815d policy, refactor: Convert uint256 to Txid (marcofleon)
f6c0d1d231 mempool, refactor: Convert uint256 to Txid (marcofleon)
aeb0f78330 refactor: Convert `mini_miner` from uint256 to Txid (marcofleon)
326f244724 refactor: Convert RPCs and `merkleblock` from uint256 to Txid (marcofleon)
49b3d3a92a Clean up `FindTxForGetData` (marcofleon)
Pull request description:
This is the final leg of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
All of these changes are straightforward `uint256` --> `Txid` along with any necessary explicit conversions. Also, `transaction_identifier.h` is moved to primitives in the last commit, as `Txid` and `Wtxid` become fundamental types after this PR.
ACKs for top commit:
stickies-v:
re-ACK de0675f9de, no changes since a20724d926d5844168c6a13fa8293df8c8927efe except address review nits.
janb84:
re ACK de0675f9de
dergoegge:
re-ACK de0675f9de
theStack:
Code-review ACK de0675f9de
Tree-SHA512: 2413160fca7ab146a8d79d18ce3afcf7384cacc73c513d41928904aa453b4dd7a350064cee71e9c5d015da5904c7c81ac17603e50a47441ebc5b0c653235dd08
db3228042b util: detect and warn when using exFAT on macOS (willcl-ark)
Pull request description:
exFAT is known to cause intermittent corruption on MacOS.
Therefore we should warn when using this fs format for either the blocks or data directories.
See #28552 for more context.
ACKs for top commit:
l0rinc:
ACK db3228042b
marcofleon:
reACK db3228042b
ismaelsadeeq:
reACK db3228042b
Tree-SHA512: e4453a8e24b35c135e4eb0b4e47fe0c80f8b54700f458909c403aa37a0d2979ee165347bcd76e48e4d1ae5d3bae13f50e6afe714e33226a52f907b95df9d3b46
Let's say an attacker wants to use/exhaust the network's bandwidth, and
has the choice between renting resources from a commercial provider and
getting the network to "spam" itself it by sending unconfirmed
transactions. We'd like the latter to be more expensive than the former.
The bandwidth for relaying a transaction across the network is roughly
its serialized size (plus relay overhead) x number of nodes. A 1000vB
transaction is 1000-4000B serialized. With 100k nodes, that's 0.1-0.4GB
If the going rate for commercial services is 10c/GB, that's like 1-4c per kvB
of transaction data, so a 1000vB transaction should pay at least $0.04.
At a price of 120k USD/BTC, 100sat is about $0.12. This price allows us
to tolerate a large decrease in the conversion rate or increase in the
number of nodes.
Back when we implemented coin age priority as a miner policy, miners
mempools might admit transactions paying very low fees, but then want to
set a higher fee for block inclusion. However, since coin age priority
was removed in v0.15, the block assembly policy is solely based on fees,
so we do not need to apply minimum feerate rules in multiple places. In
fact, the block assembly policy ignoring transactions that are added to
the mempool is likely undesirable as we waste resources accepting and
storing this transaction.
Instead, rely on mempool policy to enforce a minimum entry feerate to
the mempool (minrelaytxfee). Set the minimum block feerate to the
minimum non-zero amount (1sat/kvB) so it collects everything it finds in
mempool into the block.
ca64b71ed5 test: fix scripts in `blockfilter_basic_test` (UdjinM6)
Pull request description:
`std::vector` fill ctor is like this:
```
// Constructs a vector with `count` copies of elements with value `value`.
explicit vector( size_type count, const T& value = T(), const Allocator& alloc = Allocator() ); // (until C++11)
vector( size_type count, const T& value, const Allocator& alloc = Allocator() ); // (since C++11)(constexpr since C++20)
```
https://en.cppreference.com/w/cpp/container/vector/vector.html
i.e. `std::vector<unsigned char>(0, 65)` means a vector with `0` copies of `65` which feels wrong. I believe `count` and `value` were swapped in `blockfilter_basic_test` scripts.
ACKs for top commit:
furszy:
ACK ca64b71ed5
pablomartin4btc:
ACK ca64b71ed5
janb84:
ACK ca64b71ed5
Tree-SHA512: 2cfc7f09788b0a1afdffc9cd6663204c7f1775dabdbe1046cdcd42936c479658c348cb46e0d8835645e6c508e8b40a598cbe6534084b6780a6b60378bcbd0f96
27aefac425 validation: detect witness stripping without re-running Script checks (Antoine Poinsot)
2907b58834 policy: introduce a helper to detect whether a transaction spends Segwit outputs (Antoine Poinsot)
eb073209db qa: test witness stripping in p2p_segwit (Antoine Poinsot)
Pull request description:
Since it was introduced in 4eb515574e (#18044), the detection of a stripped witness relies on running the Script checks 3 times. In the worst case, this consists in running Script validation for every single input 3 times.
Detection of a stripped witness is necessary because in this case wtxid==txid, and the transaction's wtxid must not be added to the reject filter or it could allow a malicious peer to interfere with txid-based orphan resolution as used in 1p1c package relay.
However it is not necessary to run Script validation to detect a stripped witness (much less so doing it 3 times in a row). There are 3 types of witness program: defined program types (Taproot, P2WPKH and P2WSH), undefined types, and the Pay-to-anchor carve-out.
For defined program types, Script validation with an empty witness will always fail (by consensus). For undefined program types, Script validation is always going to fail regardless of the witness (by standardness). For P2A, an empty witness is never going to lead to a failure.
Therefore it holds that we can always detect a stripped witness without re-running Script validation. However this might lead to more "false positives" (cases where we return witness stripping for an otherwise invalid transaction) than the existing implementation. For instance a transaction with one P2PKH input with an invalid signature and one P2WPKH input with its witness stripped. The existing implementation would treat it as consensus invalid while the implementation in this PR would always consider it witness stripped.
h/t AJ: this essentially implements a variant of https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3135258539.
ACKs for top commit:
sipa:
re-ACK 27aefac425
Crypt-iQ:
re-ACK 27aefac425
glozow:
reACK 27aefac425
Tree-SHA512: 70cf76b655b52bc8fa2759133315a3f11140844b6b80d9de3c95f592050978cc01a87bd2446e3a9c25cc872efea7659d6da3337b1a709511771fece206e9f149
e017ef3c7e init: make `-blockmaxweight` startup option debug-only (ismaelsadeeq)
Pull request description:
This PR updates `-blockmaxweight` startup option to be debug-only so that it will be hidden from help text.
The option is currently unlikely to be used on mainnet, after the addition of the new `blockreservedweight` option. however it can be useful for test and signet network see https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2925674473
ACKs for top commit:
Sjors:
tACK e017ef3c7e
fjahr:
ACK e017ef3c7e
polespinasa:
tACK e017ef3c7e
Tree-SHA512: 6c18781826b2f96b13b70b7f1624481f5971746a613079d0d9528366f274ba657a02611f134d7a64f35ecb7e5faf2e3cd025458b04574ac68f804372f6eb715f
a3cf623364 test: Test max_selection_weight edge cases (Murch)
57fe8acc8a test: Check max_weight_exceeded error (Murch)
Pull request description:
I tested all of the reported surviving mutants that @brunoerg reported in https://gist.github.com/brunoerg/834063398d5002f738506d741513e310.
I found that all Mutants except for 12, 14, 17, 37, and 39 were now being caught by one of the existing tests. This fixes Mutants 14, 37, and 39.
Mutant 17 is not fixed, because I consider it acceptable that running BnB for 100,001 instead of 100,000 comparisons doesn’t cause an issue, and Mutant 12 is not yet fixed, because at `fee` = `long_term_fee`, the waste of inputs is 0 and only excess matters, and I haven’t evaluated yet, whether it needs to be fixed.
ACKs for top commit:
achow101:
ACK a3cf623364
jlest01:
ACK https://github.com/bitcoin/bitcoin/pull/33060/commits/a3cf623364e84819bc16fd407b80d8dba46bbcb5
brunoerg:
code review ACK a3cf623364
Tree-SHA512: db67c52127ed98f809f64a903c6b3a012e56cf665a0cd851457af7c85c37ec3af8bb72035d7ad370dd883f99cf3014464e3576559899e37c1d6ee01230511754
cc33e45789 test: improve assertion for SRD max weight test (yancy)
Pull request description:
Replace generic assertion with a result specific assertion showing the correctness of the solution found. If the max weight parameter is exceeded, the least valuable `UTXOs` are removed from the result. Therefore, only the most valued _encountered_ `UTXO's` are selected. While the smallest set would include all the most valued `UTXO's`, in the case of the test there is one high value `UTXO` that is never found before the target value is reached.
Correct the test comment to be more specific about why the assertion is a good result.
ACKs for top commit:
murchandamus:
ACK cc33e45789
furszy:
ACK cc33e45789
Tree-SHA512: bad224063ba830c27fba1b7b80e411ac7cd6c3edcb60bade4e6e3010f3b5d360a921de742c7c20efea8fa839d7939f338270658f66bbcebedebe5c5c8a3e8f9b
444dcb2f99 fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging` (Sebastian Falbesoner)
Pull request description:
In the `txgraph` fuzz test, the `CommitStaging` step updates the `SimTxGraph` levels simply by erasing the front (=main) one in the `sims` vector, i.e. the staging level instance takes the place of the main level instance:
https://github.com/bitcoin/bitcoin/blob/83a2216f5278c1123ba740f1c8a055652e033ddd/src/test/fuzz/txgraph.cpp#L668-L672
This also includes the `real_is_optimal` flag (reflecting whether the corresponding real graph is known to be optimally linearized), without taking into account that this flag should only be set if _both_ levels before the commiting are optimal.
E.g. in case of #33097, at this point the main level is not optimally linearized, while the staging level is, and due to the incorrect propagation of the latter the simulation incorrectly assumes that the main level is optimal after, leading to the assertion fail in the additional checks that are ran in this case[1]. Fix this by setting the flag in the resulting main level explicitly. This is done in a generic way, in case there will ever be more than two levels (not sure what is planned in this direction), a simpler alternative would be e.g. `main_optimal = sim[0].real_is_optimal && sim[1].real_is_optimal`.
Fixes#33097.
[1] see https://github.com/theStack/bitcoin/commit/0aedf09ccc0ad4fff5956bdc012778534612c017 for the printf-debug-session-clutter, if that is useful/interesting for anyone (most of the output turned out to be irrelevant to the actual cause of #33097, but it was an entertaining way to discover the interface and get a first glimpse of `TxGraph` internals as a cluster-mempool newbie).
ACKs for top commit:
sipa:
ACK 444dcb2f99
glozow:
ACK 444dcb2f99
Tree-SHA512: c20580e14628fcdc34dabb646a097e02e95b26c5740fcd5ce50f3472e4ee08f20b9a146c9ff16c85e19e57b05af1560e41a9220289c60c15083ad897dc62a0f0
c6e2c31c55 rpc: unhide waitfor{block,newblock,blockheight} (Sjors Provoost)
0786b7509a rpc: add optional blockhash to waitfornewblock (Sjors Provoost)
Pull request description:
The `waitfornewblock` is inherently racy as the tip may have changed since the last RPC call, and can even change during initial processing of this call.
Add an optional `blockhash` argument so the caller can specify their current tip. Return immediately if our tip is different.
I've made it fail if `LookupBlockIndex` fails. This should never happen if the user got the block hash from our RPC in the first place.
Finally, the `waitfor{block,newblock,blockheight}` RPC methods are no longer hidden in `help`:
- the changes in #30409 ensured these methods _could_ work in the GUI
- #31785 removed the guards that prevented GUI users from using them
- this PR makes `waitfornewblock` reliable
So there's no more reason to hide them.
ACKs for top commit:
TheCharlatan:
Re-ACK c6e2c31c55
ryanofsky:
Code review ACK c6e2c31c55. Just rebased and tweaked documentation since last review.
glozow:
utACK c6e2c31c55
Tree-SHA512: 84a0c94cb9a2e4449e7a395cf3dce1650626bd852e30e0e238a1aafae19d57bf440bfac226fd4da44eaa8d1b2fa4a8c1177b6c716235ab862a72ff5bf8fc67ac
aac0b6dd79 test: test sendall and send do anti-fee-sniping (ishaanam)
20802c7b65 wallet, rpc: add anti-fee-sniping to `send` and `sendall` (ishaanam)
Pull request description:
Currently, `send` and `sendall` don't do anti-fee-sniping because they don't use `CreateTransaction`. This PR adds anti-fee-sniping to these RPCs by calling `DiscourageFeeSniping` from `FinishTransaction` when the user does not specify a locktime.
ACKs for top commit:
achow101:
ACK aac0b6dd79
murchandamus:
ACK aac0b6dd79
glozow:
ACK aac0b6dd79
Tree-SHA512: d4f1b43b5bda489bdba46b0af60e50bff0de604a35670e6ea6e1de2b539f16b3f68805492f51d6d2078d421b63432ca22a561a5721d1a37686f2e48284e1e646
62ed1f92ef txgraph: check that DoWork finds optimal if given high budget (tests) (Pieter Wuille)
f3c2fc867f txgraph: add work limit to DoWork(), try optimal (feature) (Pieter Wuille)
e96b00d99e txgraph: make number of acceptable iterations configurable (feature) (Pieter Wuille)
cfe9958852 txgraph: track amount of work done in linearization (preparation) (Pieter Wuille)
6ba316eaa0 txgraph: 1-or-2-tx split-off clusters are optimal (optimization) (Pieter Wuille)
fad0eb091e txgraph: reset quality when merging clusters (bugfix) (Pieter Wuille)
Pull request description:
Part of #30289. Builds on top of #31553.
So far, the `TxGraph::DoWork()` function took no parameters, and just made all clusters reach the "acceptable" internal quality level by performing a minimum number of improvement iterations on it, but:
* Did not attempt to go beyond that.
* Was broken, as the QualityLevel of optimal clusters that merge together was not being reset.
Fix this by adding an argument to `DoWork()` to control how much work it is allowed to do right now, which will first be used to get all clusters to the acceptable level, and if more budget remains, use it to try to get some or all clusters optimal. The function will now return `true` if all clusters are known to be optimal (and thus no further work remains). This is verified in the tests, by remembering whether the graph is optimal, and if it is at the end of the simulation run, verify that the overall linearization cannot be improved further.
ACKs for top commit:
instagibbs:
ACK 62ed1f92ef
ismaelsadeeq:
Code review ACK 62ed1f92ef
glozow:
ACK 62ed1f92ef
Tree-SHA512: 5f57d4052e369f3444e72e724f04c02004e0f66e365faa59c9f145323e606508380fc97bb038b68783a62ae9c10757f1b628b3b00b2ce9a46161fca2d4336d73
1c10b7351e RPC: Return permitbaremultisig and maxdatacarriersize in getmempoolinfo (Kristaps Kaupe)
Pull request description:
Other node relay settings like `fullrbf` and `minrelaytxfee` are already returned, makes sense to add these two too.
ACKs for top commit:
ajtowns:
ACK 1c10b7351e
maflcko:
lgtm ACK 1c10b7351e
theStack:
ACK 1c10b7351e
Tree-SHA512: 1750d7d12de511f0ac34922ea9c58c4b9b55c3aaf22109abfd7dbe01ad1eb7b48fb4a6b074a0baf0e55ee2270fcc969b6830e499ff33adbcd0b9c761fb25e563
6135e0553e wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC (Ava Chow)
Pull request description:
If the locked coin needs to be persisted to the wallet database, insteead of having the RPC figure out when to create a WalletBatch and having LockCoin's behavior depend on it, have LockCoin take whether to persist as a parameter so it makes the batch.
Since unlocking a persisted locked coin requires a database write as well, we need to track whether the locked coin was persisted to the wallet database so that it can erase the locked coin when necessary.
Keeping track of whether a locked coin was persisted is also useful information for future PRs.
Split from #32489
ACKs for top commit:
rkrux:
ACK 6135e05
Sjors:
ACK 6135e0553e
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/32593/commits/6135e0553e6e58fcf506700991fa178f2c50a266
Tree-SHA512: 0e2367fc4d50c62ec41443374b64c4c5ecf679998677df47fb8776cfb44704713bc45547e32e96cd30d1dbed766f5d333efb6f10eb0e71271606638e07e61a01
This rule was originally introduced along with a very early proposal for
package relay as a way to verify that the "correct"
child-with-unconfirmed-parents package was provided for a transaction,
where correctness was defined as all of the transactions unconfirmed
parents. However, we are not planning to introduce a protocol where
peers would be asked to send these packages.
This rule has downsides: if a transaction has multiple parents but only
1 that requires package CPFP to be accepted, the current rule prevents
us from accepting that package. Even if the other parents are already in
mempool, the p2p logic will only submit the 1p1c package, which fails
this check. See the test in p2p_1p1c_network.py
We just need enough transactions to push us above the orphanage limits
and trigger trimming. Reducing the number of transactions cuts the
runtime of this test significantly.
249889bee6 orphanage: avoid vtx iteration when no orphans (furszy)
41ad2be434 mempool: Avoid expensive loop in `removeForBlock` during IBD (Lőrinc)
Pull request description:
During Initial Block Download, the mempool is usually empty, but `CTxMemPool::removeForBlock` is still called for every connected block where we:
* iterate over every transaction in the block even though none will be found in the empty `mapTx`, always leaving `txs_removed_for_block` empty...
* which is pre-allocated regardless with `40 bytes * vtx.size()`, even though it will always remain empty.
Similarly to https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2140691354, this change introduces a minor performance & memory optimization by only executing the loop if any of the affected mempool maps have any contents. The second commit is cherry-picked from there since it's related to this change as well.
ACKs for top commit:
optout21:
ACK 249889bee6
glozow:
ACK 249889bee6
ismaelsadeeq:
reACK 249889bee6
Tree-SHA512: 80d06ff1515164529cdc3ad21db3041bb5b2a1d4b72ba9e6884cdf40c5f1477fee7479944b8bca32a6f0bf27c4e5501fccd085f6041a2dbb101438629cfb9e4b
This test was introduced in #28251 to ensure that the mempool is not
trimmed in the middle of a package evaluation and the m_view cache
is updated when evictions and replacements happen so coins are no longer
visible in subsequent package transactions. These two things have
coverage in other tests as well, and are pretty unlikely to happen.
This test is also brittle: it requires evaluation of the parents in a
particular order, and creates a transaction that itself is not
enough to trigger eviction but will be pushed out immediately by the
package spending from it. While the current magic number 2000 works, we
do not have a way to query remaining space in the mempool if mempool
data structures change, and it can differ across platforms.
For the default number of peers (125), allows each to relay a default
descendant package (up to 25-1=24 can be missing inputs) of small (9
inputs or fewer) transactions out of order.
This limit also gives acceptable bounds for worst case LimitOrphans iterations.
Functional tests aren't changed to check for larger cap because it would
make the runtime too long.
Also deletes the now-unused DEFAULT_MAX_ORPHAN_TRANSACTIONS.
This is largely a reimplementation using boost::multi_index_container.
All the same public methods are available. It has an index by outpoint,
per-peer tracking, peer worksets, etc.
A few differences:
- Limits have changed: instead of a global limit of 100 unique orphans,
we have a maximum number of announcements (which can include duplicate
orphans) and a global memory limit which scales with the number of
peers.
- The maximum announcements limit is 100 to match the original limit,
but this is actually a stricter limit because the announcement count
is not de-duplicated.
- Eviction strategy: when global limits are reached, a per-peer limit
comes into play. While limits are exceeded, we choose the peer whose
“DoS score” (max usage / limit ratio for announcements and memory
limits) is highest and evict announcements by entry time, sorting
non-reconsiderable ones before reconsiderable ones. Since announcements
are unique by (wtxid, peer), as long as 1 announcement remains for a
transaction, it remains in the orphanage.
- This eviction strategy means no peer can influence the eviction of
another peer’s orphans.
- Also, since global limits are a multiple of per-peer limits, as long
as a peer does not exceed its limits, its orphans are protected from
eviction.
- Orphans no longer expire, since older announcements are generally
removed before newer ones.
- GetChildrenFromSamePeer returns the transactions from newest to
oldest.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Move towards a model where TxOrphanage is initialized with limits that
it remembers throughout its lifetime.
Remove the param. Limiting by number of unique orphans will be removed
in a later commit.
Now that -maxorphantx is gone, this does not change the node behavior.
The parameter is only used in tests.
merge-script