[pull] master from bitcoin:master #1452
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add a unit test that exercises the `IsInitialBlockDownload()` decision matrix by varying the cached latch, `BlockManager::LoadingBlocks()`, and tip work/recency inputs. This documents the current latching behavior and provides a baseline for later refactors.
Rename and invert the internal IBD latch so the cached value directly matches `IsInitialBlockDownload()` (true while in IBD, then latched to false). This is a behavior-preserving refactor to avoid double negatives.
Factor the chain tip work/recency check out of `ChainstateManager::IsInitialBlockDownload()` into a reusable `CChain::IsTipRecent()` helper, and annotate it as requiring `cs_main` since it's reading mutable state. Also introduce a local `chainman_ref` in the kernel import-blocks wrapper to reduce repetition and keep follow-up diffs small. `IsInitialBlockDownload` returns were also unified to make the followup move clean. Co-authored-by: Patrick Strateman <patrick.strateman@gmail.com> Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
`ChainstateManager::IsInitialBlockDownload()` is queried on hot paths and previously acquired `cs_main` internally, contributing to lock contention. Cache the IBD status in `m_cached_is_ibd`, and introduce `ChainstateManager::UpdateIBDStatus()` to latch it once block loading has finished and the current chain tip has enough work and is recent. Call the updater after tip updates and after `ImportBlocks()` completes. Since `IsInitialBlockDownload()` no longer updates the cache, drop `mutable` from `m_cached_is_ibd` and only update it from `UpdateIBDStatus()` under `cs_main`. Update the new unit test to showcase the new `UpdateIBDStatus()`. Co-authored-by: Patrick Strateman <patrick.strateman@gmail.com> Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
On FreeBSD, the default ephemeral port range (10000-65535) overlaps with the test framework's static port range (11000-26000), causing intermittent "address already in use" failures when tests use dynamic port allocation (port=0). Add a helper function that sets the IP_PORTRANGE/IPV6_PORTRANGE socket option to IP_PORTRANGE_HIGH before binding, which requests ports from the high range (49152-65535) instead. This range does not overlap with the test framework's static ports. Constants from FreeBSD's netinet/in.h and netinet6/in6.h: - IP_PORTRANGE = 19 (for IPv4 sockets) - IPV6_PORTRANGE = 14 (for IPv6 sockets) - IP_PORTRANGE_HIGH = 1 Fixes: #34331 Co-Authored-By: Vasil Dimov <vd@FreeBSD.org> Co-Authored-By: MarcoFalke <*~=\`'#}+{/-|&$^_@721217.xyz>
The previous commit added set_ephemeral_port_range() to avoid port conflicts on FreeBSD by requesting ports from the high ephemeral range (49152-65535) instead of the default range which overlaps with the test framework's static port range. That fix was applied to the SOCKS5 server but not to P2P listeners created via NetworkThread.create_listen_server(). This commit extends the fix to cover P2P listeners as well. When port=0 is requested (dynamic allocation), we now: 1. Manually create a socket with the appropriate address family 2. Call set_ephemeral_port_range() to configure the port range 3. Bind and listen on the socket 4. Pass the pre-configured socket to asyncio's create_server() This ensures that dynamically allocated ports for P2P listeners also come from the high range on FreeBSD, avoiding conflicts with the test framework's static port assignments. Co-Authored-By: Vasil Dimov <vd@FreeBSD.org>
fad0422 refactor: Remove remaining std::bind, check via clang-tidy (MarcoFalke) Pull request description: `std::bind` has many issues: * It is verbose in a meaningless way * Overriden args are silently accepted and dropped at runtime without a compile error. Same for accidental duplicates. One could use `std::bind_front` similar to commit fa26755. Though, I think the remaining cases are better off with lambdas. So do that here, and enable the `modernize-avoid-bind` clang-tidy rule to avoid `std::bind` bugs in the future. ACKs for top commit: fjahr: Code review ACK fad0422 purpleKarrot: Code review ACK fad0422 Tree-SHA512: 38b17e26eda3ae47d84a8c34298309dc1eeb4ed434fda58b5803ef031c4c2edfb17222f5208f37af727bf340e32b37c7f81784f461d2b65fbc6227f3cd53eea4
…lockDownload()` 557b41a validation: make `IsInitialBlockDownload()` lock-free (Lőrinc) b9c0ab3 chain: add `CChain::IsTipRecent` helper (Lőrinc) 8d531c6 validation: invert `m_cached_finished_ibd` to `m_cached_is_ibd` (Lőrinc) 8be54e3 test: cover IBD exit conditions (Lőrinc) Pull request description: This PR is a follow-up to the stale #32885. ### Problem `ChainstateManager::IsInitialBlockDownload()` currently acquires `cs_main` internally, even though most existing call sites already hold the lock. This becomes relevant for proposals like #34054, which would call `IsInitialBlockDownload()` from the scheduler thread without holding `cs_main`, potentially introducing lock contention. ### Fix Make `ChainstateManager::IsInitialBlockDownload()` lock-free by caching its result in a single atomic `m_cached_is_ibd` (true while in IBD, latched to false on exit). Move the IBD exit checks out of `IsInitialBlockDownload()` (reader-side) into a new `ChainstateManager::UpdateIBDStatus()` (writer-side, called under cs_main). Call UpdateIBDStatus() at strategic points where IBD exit conditions may change, after active chain tip updates in `ConnectTip()`, `DisconnectTip()`, and `LoadChainTip()`, and after `ImportBlocks()` returns. With this, `IsInitialBlockDownload()` becomes a lock-free atomic read, avoiding internal `cs_main` acquisition on hot paths. ### Testing and Benchmarks This isn't strictly an optimization (though some usecases might benefit from it), so rather as a sanity check I ran a reindex-chainstate and an `AssumeUTXO` load (without background validation). <details> <summary>assumeutxo load | 910000 blocks | dbcache 4500 | i9-ssd | x86_64 | Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz | 16 cores | 62Gi RAM | xfs | SSD</summary> ``` COMMITS="595504a43209bead162da54a204df7d140a25f0e 63e822b"; \ CC=gcc; CXX=g++; \ BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/ShallowBitcoinData"; LOG_DIR="$BASE_DIR/logs"; UTXO_SNAPSHOT_PATH="$BASE_DIR/utxo-910000.dat"; \ (echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done; echo "") && \ for DBCACHE in 4500; do \ (echo "assumeutxo load | 910000 blocks | dbcache ${DBCACHE} | $(hostname) | $(uname -m) | $(lscpu | grep 'Model name' | head -1 | cut -d: -f2 | xargs) | $(nproc) cores | $(free -h | awk '/^Mem:/{print $2}') RAM | $(df -T $BASE_DIR | awk 'NR==2{print $2}') | $(lsblk -no ROTA $(df --output=source $BASE_DIR | tail -1) | grep -q 0 && echo SSD || echo HDD)";) &&\ hyperfine \ --sort command \ --runs 3 \ --export-json "$BASE_DIR/assumeutxo-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$DBCACHE-$CC-$(date +%s).json" \ --parameter-list COMMIT ${COMMITS// /,} \ --prepare "killall -9 bitcoind 2>/dev/null; rm -rf $DATA_DIR/blocks $DATA_DIR/chainstate $DATA_DIR/chainstate_snapshot $DATA_DIR/debug.log; git clean -fxd; git reset --hard {COMMIT} && \ cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=RelWithDebInfo && ninja -C build bitcoind bitcoin-cli -j2 && \ ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=1 -printtoconsole=0; sleep 20 && \ ./build/bin/bitcoind -datadir=$DATA_DIR -daemon -blocksonly -connect=0 -dbcache=$DBCACHE -printtoconsole=0; sleep 20" \ --conclude "build/bin/bitcoin-cli -datadir=$DATA_DIR stop || true; killall bitcoind || true; sleep 10; \ echo '{COMMIT} | dbcache=$DBCACHE | chainstate: $(find $DATA_DIR/chainstate_snapshot -type f 2>/dev/null | wc -l) files, $(du -sb $DATA_DIR/chainstate_snapshot 2>/dev/null | cut -f1) bytes' >> $DATA_DIR/debug.log; \ cp $DATA_DIR/debug.log $LOG_DIR/debug-assumeutxo-{COMMIT}-dbcache-$DBCACHE-$(date +%s).log" \ "COMPILER=$CC DBCACHE=$DBCACHE ./build/bin/bitcoin-cli -datadir=$DATA_DIR -rpcclienttimeout=0 loadtxoutset $UTXO_SNAPSHOT_PATH"; \ done 595504a Merge #34236: Add sedited to trusted-keys 63e822b validation: make `IsInitialBlockDownload()` lock-free assumeutxo load | 910000 blocks | dbcache 4500 | i9-ssd | x86_64 | Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz | 16 cores | 62Gi RAM | xfs | SSD Benchmark 1: COMPILER=gcc DBCACHE=4500 ./build/bin/bitcoin-cli -datadir=/mnt/my_storage/ShallowBitcoinData -rpcclienttimeout=0 loadtxoutset /mnt/my_storage/utxo-910000.dat (COMMIT = 595504a) Time (mean ± σ): 418.452 s ± 0.461 s [User: 0.001 s, System: 0.001 s] Range (min … max): 418.070 s … 418.964 s 3 runs Benchmark 2: COMPILER=gcc DBCACHE=4500 ./build/bin/bitcoin-cli -datadir=/mnt/my_storage/ShallowBitcoinData -rpcclienttimeout=0 loadtxoutset /mnt/my_storage/utxo-910000.dat (COMMIT = 63e822b) Time (mean ± σ): 415.994 s ± 0.294 s [User: 0.001 s, System: 0.001 s] Range (min … max): 415.788 s … 416.330 s 3 runs Relative speed comparison 1.01 ± 0.00 COMPILER=gcc DBCACHE=4500 ./build/bin/bitcoin-cli -datadir=/mnt/my_storage/ShallowBitcoinData -rpcclienttimeout=0 loadtxoutset /mnt/my_storage/utxo-910000.dat (COMMIT = 595504a) 1.00 COMPILER=gcc DBCACHE=4500 ./build/bin/bitcoin-cli -datadir=/mnt/my_storage/ShallowBitcoinData -rpcclienttimeout=0 loadtxoutset /mnt/my_storage/utxo-910000.dat (COMMIT = 63e822b) ``` </details> <details> <summary>2026-01-12 | reindex-chainstate | 931139 blocks | dbcache 4500 | i9-ssd | x86_64 | Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz | 16 cores | 62Gi RAM | SSD</summary> ``` for DBCACHE in 4500; do \ COMMITS="595504a43209bead162da54a204df7d140a25f0e 63e822b"; \ STOP=931139; 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 "" && echo "$(date -I) | reindex-chainstate | ${STOP} blocks | dbcache ${DBCACHE} | $(hostname) | $(uname -m) | $(lscpu | grep 'Model name' | head -1 | cut -d: -f2 | xargs) | $(nproc) cores | $(free -h | awk '/^Mem:/{print $2}') RAM | SSD"; 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 -9 bitcoind 2>/dev/null; rm -f $DATA_DIR/debug.log; git clean -fxd; git reset --hard {COMMIT} && \ cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DENABLE_IPC=OFF && ninja -C build bitcoind -j1 && \ ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 20; rm -f $DATA_DIR/debug.log" \ --conclude "killall bitcoind || true; sleep 5; grep -q 'height=0' $DATA_DIR/debug.log && grep -q 'Disabling script verification at block #1' $DATA_DIR/debug.log && grep -q 'height=$STOP' $DATA_DIR/debug.log; \ 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"; done 595504a Merge #34236: Add sedited to trusted-keys 63e822b validation: make `IsInitialBlockDownload()` lock-free 2026-01-12 | reindex-chainstate | 931139 blocks | dbcache 4500 | i9-ssd | x86_64 | Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz | 16 cores | 62Gi RAM | SSD Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=931139 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 595504a) Time (abs ≡): 17187.310 s [User: 33104.415 s, System: 937.548 s] Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=931139 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 63e822b) Time (abs ≡): 17240.300 s [User: 33164.803 s, System: 976.485 s] Relative speed comparison 1.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=931139 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 595504a) 1.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=931139 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 63e822b) ``` </details> ACKs for top commit: sedited: ACK 557b41a sipa: utACK 557b41a mzumsande: Code Review ACK 557b41a Tree-SHA512: 174015b9785846fc3375bd9d6e4ef91de47ffb659a94d645c49d333a33a32986d5c3cb2eb5a0a7245f96580ed6fea4ba5b7f93cac7e42e3b225f0a01538a2e5c
… allocation 2845f10 test: extend FreeBSD ephemeral port range fix to P2P listeners (node) 34bed0e test: use IP_PORTRANGE_HIGH on FreeBSD for dynamic port allocation (woltx) Pull request description: Reopening #34336. I’ve now tested it on FreeBSD and confirmed it works. On FreeBSD, the default ephemeral port range (10000-65535) overlaps with the test framework's static port range (11000-26000), possibly causing intermittent "address already in use" failures when tests use dynamic port allocation (`port=0`). This PR adds a helper that sets `IP_PORTRANGE_HIGH` via `setsockopt()` before binding, requesting ports from 49152-65535 instead, which avoids the overlap, as suggested in #34331 (comment) by @maflcko . From FreeBSD's [sys/netinet/in.h](https://cgit.freebsd.org/src/tree/sys/netinet/in.h): ```c #define IP_PORTRANGE 19 #define IP_PORTRANGE_HIGH 1 #define IPPORT_EPHEMERALFIRST 10000 /* default range start */ #define IPPORT_HIFIRSTAUTO 49152 /* high range start */ ``` See also: FreeBSD https://man.freebsd.org/cgi/man.cgi?query=ip&sektion=4 man page. Fixes #34331 ACKs for top commit: vasild: ACK 2845f10 hebasto: ACK 2845f10, I have reviewed the code and it looks OK. Tree-SHA512: ce501ce3e8a4023e07bad572df2b85d6829becf133813e4529aebba83e4eba59fa8b48e9d2197ebbb226adaf3054fad720775a787244d6b38c0078ee086102f4
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )