Deduplicate code looping over chainstate objects and calling
ActivateBestChain() and avoid need for code outside ChainstateManager to use
the GetAll() method.
Use to replace m_active_chainstate, m_ibd_chainstate, and m_snapshot_chainstate
members. This has several benefits:
- Ensures ChainstateManager treats chainstates instances equally, making
distinctions based on their attributes, not having special cases and making
assumptions based on their identities.
- Normalizes ChainstateManager representation so states that should be
impossible to reach and validation code has no handling for (like
m_snapshot_chainstate being set and m_ibd_chainstate being unset, or both
being set but m_active_chainstate pointing to the m_ibd_chainstate) can no
longer be represented.
- Makes ChainstateManager more extensible so new chainstates can be added for
different purposes, like indexing or generating and validating assumeutxo
snapshots without interrupting regular node operations. With the
m_chainstates member, new chainstates can be added and handled without needing
to make changes all over validation code or to copy/paste/modify the existing
code that's been already been written to handle m_ibd_chainstate and
m_snapshot_chainstate.
- Avoids terms that are confusing and misleading:
- The term "active chainstate" term is confusing because multiple chainstates
will be active and in use at the same time. Before a snapshot is validated,
wallet code will use the snapshot chainstate, while indexes will use the IBD
chainstate, and netorking code will use both chainstates, downloading
snapshot blocks at higher priority, but also IBD blocks simultaneously.
- The term "snapshot chainstate" is ambiguous because it could refer either
to the chainstate originally loaded from a snapshot, or to the chainstate
being used to validate a snapshot that was loaded, or to a chainstate being
used to produce a snapshot, but it is arbitrary used to refer the first
thing. The terms "most-work chainstate" or "assumed-valid chainstate" should
be less ambiguous ways to refer to chainstates loaded from snapshots.
- The term "IBD chainstate" is not just ambiguous but actively confusing
because technically IBD ends and the node is considered synced when the
snapshot chainstate finishes syncing, so in practice the IBD chainstate
will mostly by synced after IBD is complete. The term "fully-validated" is
a better way of describing the characteristics and purpose of this
chainstate.
SnapshotBlockhash() is only called two places outside of tests, and is used
redundantly in some tests, checking the same field as other checks. Simplify by
dropping the method and using the m_from_snapshot_blockhash field directly.
IsSnapshotValidated() is only called one place outside of tests, and is use
redundantly in some tests, asserting that a snapshot is not validated when a
snapshot chainstate does not even exist. Simplify by dropping the method and
checking Chainstate m_assumeutxo field directly.
IsSnapshotActive() method is only called one place outside of tests and
asserts, and is confusing because it returns true even after the snapshot is
fully validated.
The documentation which said this "implies that a background validation
chainstate is also in use" is also incorrect, because after the snapshot is
validated, the background chainstate gets disabled and IsUsable() would return
false.
Change ChainstateRole parameter passed to wallets and indexes. Wallets and
indexes need to know whether chainstate is historical and whether it is fully
validated. They should not be aware of the assumeutxo snapshot validation
process.
CurrentChainstate() is basically the same as ActiveChainstate() except it
requires cs_main to be locked when it is called, instead of locking cs_main
internally.
The name "current" should also be less confusing than "active" because multiple
chainstates can be active, and CurrentChainstate() returns the chainstate
targeting the current network tip, regardless of what chainstates are being
downloaded or how they are used.
Use to simplify code determining the chainstate leveldb paths. New method is
the now the only code that needs to figure out the storage path, so the path
doesn't need to be constructed multiple places and backed out of leveldb.
Remove hardcoded references to m_ibd_chainstate and m_snapshot_chainstate so
MaybeCompleteSnapshotValidation function can be simpler and focus on validating
the snapshot without dealing with internal ChainstateManager states.
This is a step towards being able to validate the snapshot outside of
ActivateBestChain loop so cs_main is not locked for minutes when the snapshot
block is connected.
Move duplicate code from ChainstateManager::ActivateSnapshot and
ChainstateManager::ActivateExistingSnapshot methods to a new
ChainstateManager::AddChainstate method.
The "AddChainstate" method name doesn't mention snapshots even though it is
only used to add snapshot chainstates now, because it becomes more generalized
in a later commit in this PR ("refactor: Add ChainstateManager::m_chainstates
member")
Get rid of m_disabled/IsUsable members. Instead of marking chains disabled for
different reasons, store chainstate assumeutxo status explicitly and use that
information to determine how chains should be treated.
Make Chainstate objects aware of what block they are targeting. This makes
Chainstate objects more self contained, so it's possible for validation code to
look at one Chainstate object and know what blocks to connect to it without
needing to consider global validation state or look at other Chainstate
objects.
The motivation for this change is to make validation and networking code more
readable, so understanding it just requires knowing about chains and blocks,
not reasoning about assumeutxo download states. This change also enables
simplifications to the ChainstateManager interface in subsequent commits, and
could make it easier to implement new features like creating new Chainstate
objects to generate UTXO snapshots or index UTXO data.
Note that behavior of the MaybeCompleteSnapshotValidation function is not
changing here but some checks that were previously impossible to trigger like
the BASE_BLOCKHASH_MISMATCH case have been turned into asserts.
The following test code never checked anything because the if statement was
always false:
if (cs != &chainman_restarted.ActiveChainstate()) {
BOOST_CHECK_EQUAL(cs->m_chain.Height(), 109);
}
Also, the height of the background chainstate it was intending to check is 110,
not 109. Fix both problems by rewriting the check.
5f5c1ea019 net: Cache -capturemessages setting (Anthony Towns)
cea443e246 net: Pass time to InactivityChecks fuctions (Anthony Towns)
Pull request description:
Cuts out some wasted time in net socket handling. First, only calculates the current time once every 50ms, rather than once for each peer, which given we only care about second-level precision seems more than adequate. Second, caches the value of the `-capturemessages` setting in `CConnman` rather than re-evaluating it every time we invoke `PushMessaage`.
ACKs for top commit:
maflcko:
review ACK 5f5c1ea019🏣
vasild:
ACK 5f5c1ea019
sedited:
ACK 5f5c1ea019
mzumsande:
ACK 5f5c1ea019
Tree-SHA512: 0194143a3a4481c6355ac9eab27ce6ae4bed5db1d483ba5d06288dd92f195ccb9f0f055a9eb9d7e16e9bbf72f145eca1ff17c6700ee9aa42730103a8f047b32c
c1e554d3e5 refactor: consolidate 3 separate locks into one block (Andrew Toth)
41479ed1d2 test: add test for periodic flush inside ActivateBestChain (Andrew Toth)
84820561dc validation: periodically flush dbcache during reindex-chainstate (Andrew Toth)
Pull request description:
After #30611 we periodically do a non-erasing flush of the dbcache to disk roughly every hour during IBD.
The intention was to also do this periodic flush during reindex-chainstate, so we would not risk losing progress during a system failure when reindexing with a high dbcache value.
It was discovered that reindex-chainstate does not perform a PERIODIC flush until it has already reached the tip. Since reindexing to tip usually happens within 24 hours, this behaviour was unnoticed with the previous periodic flush interval. Note that reindex-chainstate still does IF_NEEDED flushes during `ConnectBlock`, so this also would not be noticed when running with a lower dbcache value.
This patch moves the PERIODIC flush from after the outer loop in `ActivateBestChain` to inside the outer loop after we release `cs_main`. This will periodically flush during IBD, reindex-chainstate, and steady state.
ACKs for top commit:
l0rinc:
ACK c1e554d3e5
achow101:
ACK c1e554d3e5
sipa:
utACK c1e554d3e5
Tree-SHA512: c447ad03e16c9978b8ed2c285b38e1b4c56e7778ab93b6f64435116f47b8931017f5f56ab53eb61656693146aaced776f666af573a41ab28e8f2b6d8657fa756
fa89f60e31 scripted-diff: LogPrintLevel(*,BCLog::Level::*,*) -> LogError()/LogWarning() (MarcoFalke)
fa6c7a1954 scripted-diff: LogPrintLevel(*,BCLog::Level::Debug,*) -> LogDebug() (MarcoFalke)
Pull request description:
Errors and warnings should normally not happen. However, if they do happen, it is easier to spot them, if they are all logged in the same format via `LogError` or `LogWarning`.
So do that with a scripted-diff.
This is a minimal behavior change and unifies the log output from:
[net:error] Something bad happened
[net:warning] Something problematic happened
to either
[error] Something bad happened
[warning] Something problematic happened
or, when `-loglevelalways=1` is enabled:
[all:error] Something bad happened
[all:warning] Something problematic happened
Such a behavior change is desired, because all warning and error logs are written in the same style in the source code and they are logged in the same format for log consumers.
Removing the category should be harmless, because warning and error messages should be descriptive and unique anyway.
ACKs for top commit:
ajtowns:
ACK fa89f60e31
stickies-v:
ACK fa89f60e31
rkrux:
lgtm code review ACK fa89f60e31
Tree-SHA512: dafa47ab561609a79005faf008fe188dd714f6e07bb2dfbe4db49290d6636b12eb7ac4a18ed32bcc5526641a9f258dbc37c08e10c223ec068b97976590ff0b52
0ac969cddf validation: don't reallocate cache for short-lived CCoinsViewCache (Lőrinc)
c8f5e446dc coins: reduce lookups in dbcache layer propagation (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
Previously, when the parent coins cache had no entry and the child did, `BatchWrite` performed a find followed by `try_emplace`, which resulted in multiple `SipHash` computations and bucket traversals on the common insert path.
On a different path, these caches were recreated needlessly for every block connection.
### Fix for double fetch
This change uses a single leading `try_emplace` and branches on the returned `inserted` flag. In the `FRESH && SPENT` case (not used in production, only exercised by tests), we erase the just-inserted placeholder (which is constant time with no rehash anyway). Semantics are unchanged for all valid parent/child state combinations.
This change is a minimal version of [bitcoin/bitcoin@`723c49b` (#32128)](723c49b63b) and draws simplification ideas [bitcoin/bitcoin@`ae76ec7` (#30673)](ae76ec7bcf) and https://github.com/bitcoin/bitcoin/pull/30326.
### Fix for temporary cache recreation
Related to parent cache propagation, the second commit makes it possible to avoid destructuring-recreating-destructuring of these short-live parent caches created for each new block.
A few temporary `CCoinsViewCache`'s are destructed right after the `Flush()`, therefore it is not necessary to call `ReallocateCache` to recreate them right before they're killed anyway.
This change was based on a subset of https://github.com/bitcoin/bitcoin/pull/28945, the original authors and relevant commenters were added as coauthors to this version.
-----
Reindex-chainstate indicates ~1% speedup.
<details>
<summary>Details</summary>
```python
COMMITS="647cdb4f7e8041affed887e2325ee03a91078bb1 0b0c3293ffd75afb27dadc0b28426b40132a8c6b"; \
STOP=909090; DBCACHE=4500; \
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 2 \
--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 -DENABLE_IPC=OFF && ninja -C build bitcoind && \
./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 20" \
--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"
647cdb4f7e Merge bitcoin/bitcoin#33311: net: Quiet down logging when router doesn't support natpmp/pcp
0b0c3293ff validation: don't reallocate cache for short-lived CCoinsViewCache
Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 647cdb4f7e)
Time (mean ± σ): 16233.508 s ± 9.501 s [User: 19064.578 s, System: 951.672 s]
Range (min … max): 16226.790 s … 16240.226 s 2 runs
Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 0b0c3293ffd75afb27dadc0b28426b40132a8c6b)
Time (mean ± σ): 16039.626 s ± 17.284 s [User: 18870.130 s, System: 950.722 s]
Range (min … max): 16027.405 s … 16051.848 s 2 runs
Relative speed comparison
1.01 ± 0.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 647cdb4f7e)
1.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 0b0c3293ffd75afb27dadc0b28426b40132a8c6b)
```
</details>
ACKs for top commit:
optout21:
utACK 0ac969cddf
achow101:
ACK 0ac969cddf
andrewtoth:
utACK 0ac969cddf
sedited:
ACK 0ac969cddf
Tree-SHA512: 9fcc3f1a8314368576a4fba96ca72665527eaa3a97964ab5b39491757f3527147d134f79a5c3456f76c1330c7ef862989d23f764236c5e2563be89a81c1cee47
e7ac5a133c doc: add release note for 34031 (fanquake)
c4c70a256e netbase: Remove "tor" as a network specification (Carl Dong)
Pull request description:
"tor" as a network specification was deprecated in 60dc8e4208 in favor of "onion"
and this commit removes it and updates the relevant test.
Previously #16029. This has been warning as being deprecated since `v0.17.0`.
This PR only removes the already deprecated usage of tor as a network specification, the use of tor throughout the codebase, is not deprecated.
ACKs for top commit:
davidgumberg:
crACK e7ac5a133c
laanwj:
Code review ACK e7ac5a133c
janb84:
ACK e7ac5a133c
stickies-v:
ACK e7ac5a133c
Tree-SHA512: f211dec151c21728b4cd2b1716ee68907871beaa85d8c89e2bc17576e701d03c03e5455593de94970d787aa3264fab60d8c6debeeff908e00d8feb48804692e9
d4d184eda9 log: don't rate-limit "new peer" with -debug=net (0xb10c)
Pull request description:
Previously, when `debug=net` is enabled, we log "New [..] peer connected" for new inbound peers with `LogInfo`. However, `LogInfo` will get rate-limited since https://github.com/bitcoin/bitcoin/pull/32604. When we specifically turn on `debug=net`, we don't want these log messages to be rate-limited.
To fix this, use `LogDebug(BCLog::NET, ...)` for potentially high-rate inbound connections. Otherwise use `LogInfo`. This means we don't rate-limit the messages for inbound peers when `debug=net` is turned on but will rate-limit if we created outbound at a high rate as these are logged via `LogInfo`.
The new log messages look similar to:
```
2025-12-08T00:00:00Z [net] New inbound peer connected: transport=v2 version=70016 blocks=0 peer=1
2025-12-08T00:00:00Z New outbound-full-relay peer connected: transport=v2 version=70016 blocks=281738 peer=5
```
--
I ran into this message getting rate-limited on one of my monitoring nodes with `-logsourcelocations=1`: With logsourcelocations, one of these lines is about 338 chars (or 338 bytes) long. We rate-limit after more than 1048576 bytes per hour, which results in about 3100 in- and outbound connections per hour. With evicted and instantly reconnecting connections from an entity like LinkingLion, this can be reached fairly quickly.
ACKs for top commit:
stickies-v:
utACK d4d184eda9
Crypt-iQ:
tACK d4d184eda9
maflcko:
review ACK d4d184eda9🚲
rkrux:
lgtm code review ACK d4d184eda9
glozow:
lgtm ACK d4d184eda9
Tree-SHA512: 14dbf693fa44a74c9822590e7a08167d2deeb1bc6f4b8aeb00c1b035c0df7101087d5c80a3c0d637879d5c52f88b30f0cb4c0577cff6f647d2eb3300f49d8ea3
4b47113698 validation: Reword CheckForkWarningConditions and call it also during IBD and at startup (Martin Zumsande)
2f51951d03 p2p: Add warning message when receiving headers for blocks cached as invalid (Martin Zumsande)
Pull request description:
In case of corruption that leads to a block being marked as invalid that is seen as valid by the rest of the network, the user currently doesn't receive good error messages, but will often be stuck in an endless headers-sync loop with no explanation (#26391).
This PR improves warnings in two ways:
- When we receive a header that is already saved in our disk, but invalid, add a warning. This will happen repeatedly during the headerssync loop (see https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-1291765534 on how to trigger it artificially).
- Removes the IBD check from `CheckForkWarningConditions` and adds a call to the function during init (`LoadChainTip()`). The existing check was added in 55ed3f1475 a long time ago when we had more sophisticated fork detection that could lead to false positives during IBD, but that logic was removed in fa62304c97 so that I don't see a reason to suppress the warning anymore.
Fixes#26391 (We'll still do the endless looping, trying to find a peer with a headers that we can use, but will now repeatedly log warnings while doing so).
ACKs for top commit:
glozow:
ACK `git range-diff 6d2c8ea9dbd77c71051935b5ab59224487509559...4b4711369880369729893ba7baef11ba2a36cf4b`
theStack:
re-ACK 4b47113698
sedited:
ACK 4b47113698
Tree-SHA512: 78bc53606374636d616ee10fdce0324adcc9bcee2806a7e13c9471e4c02ef00925ce6daef303bc153b7fcf5a8528fb4263c875b71d2e965f7c4332304bc4d922
Split the repeated `SipHash` v[0..3] initialization into a small `SipHashState` helper that is used by both `CSipHasher` and `PresaltedSipHasher`.
Added explanatory comments to clarify behavior, documenting the equivalence of `PresaltedSipHasher` `operator()` overloads to `CSipHasher` usage.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Replaces separate `shorttxidk0`/`shorttxidk1` members with a cached `PresaltedSipHasher`, so `GetShortID()` reuses the precomputed `SipHash` state instead of rebuilding it on every call.
`CBlockHeaderAndShortTxIDs` was never intended to be used before `FillShortTxIDSelector()` runs; doing so already relied on indeterminate salt values.
The new `Assert(m_hasher)` just makes this invariant explicit and fails fast if the object is used in an uninitialized state.
Replaces standalone `SipHashUint256` with an `operator()` overload in `PresaltedSipHasher`.
Updates all hasher classes (`SaltedUint256Hasher`, `SaltedTxidHasher`, `SaltedWtxidHasher`) to use `PresaltedSipHasher` internally, enabling the same constant-state caching optimization while keeping behavior unchanged.
Benchmark was also adjusted to cache the salting part.
Aligns test variable naming with the `k0`/`k1` convention used consistently throughout the codebase for `SipHash` keys.
Also splits the single-param `SipHash` test from the one with extra, for clarity.
9d5021a05b script: add SCRIPT_ERR_TAPSCRIPT_EMPTY_PUBKEY (billymcbip)
Pull request description:
We currently have two callsites for `SCRIPT_ERR_PUBKEYTYPE`:
- A pre-tapscript policy error behind the `SCRIPT_VERIFY_STRICTENC` flag: 4de26b111f/src/script/interpreter.cpp (L220)
- A [consensus error](https://github.com/bitcoin/bips/blob/master/bip-0342.mediawiki?plain=1#L93) in Tapscript: 4de26b111f/src/script/interpreter.cpp (L368)
It would be good for readability and testability to have separate errors for both cases, as they are quite distinct (policy vs. consensus, format vs. emptiness).
**This PR adds `SCRIPT_ERR_TAPSCRIPT_EMPTY_PUBKEY` for the consensus error path.**
This change would make our error handling more consistent. We have more granular errors for other pubkey error paths already: `SCRIPT_ERR_WITNESS_PUBKEYTYPE`, `SCRIPT_ERR_DISCOURAGE_UPGRADABLE_PUBKEYTYPE`. We also have separate errors for MINIMAL_IF: `SCRIPT_ERR_MINIMALIF` for the policy error pre-tapscript, and `SCRIPT_ERR_TAPSCRIPT_MINIMALIF` for the consensus error post-tapscript.
Tests:
Added a test case to `script_tests` and ran `build/bin/test_bitcoin --run_test=script_tests --log_level=success`.
```
test/script_tests.cpp:144: info: check '[["aa","#SCRIPT# 0 CHECKSIG","#CONTROLBLOCK#",0.00000001],"","0x51 0x20 #TAPROOTOUTPUT#","P2SH,WITNESS,TAPROOT","TAPSCRIPT_EMPTY_PUBKEY","TAPSCRIPT: OP_CHECKSIG with empty pubkey must fail"] (with flags 165d5d)' has passed
...
```
Ran `DIR_UNIT_TEST_DATA="$(pwd)/../qa-assets/unit_test_data" build/bin/test_bitcoin --run_test=script_assets_tests --log_level=success`.
Updated `feature_taproot.py` and ran `build/test/functional/feature_taproot.py`.
Looking forward to your feedback.
ACKs for top commit:
sedited:
ACK 9d5021a05b
darosior:
utACK 9d5021a05b
sipa:
ACK 9d5021a05b
Tree-SHA512: bc0b7f64454313fe392ffb2d23aa4eca3deadc5ea1d10b3fba0b3ab4cb0575a5ddcb002dc27b4fa7aa3c180840a83d1b3e5c89351009ce7ffe684d58e1980ace
ff06e2468a init: point out -stopatheight may be imprecise (brunoerg)
Pull request description:
`-stopatheight` is used to stop running bitcoind after reaching a given height. However, this feature is imprecise since some blocks can still be processed during the shutdown.
There are some previous discussions around it in https://github.com/bitcoin/bitcoin/pull/13713, https://github.com/bitcoin/bitcoin/pull/13490 and https://github.com/bitcoin/bitcoin/issues/13477. However, I'm not sure if it will get fixed since it's undesirable to burden the validation code further with this and we can bypass this behavior by using `invalidateblock` to wind back. Anyway, since at this moment its behavior is imprecise I think worth mentioning it in documentation.
ACKs for top commit:
rkrux:
re-ACK ff06e2468a
stickies-v:
ACK ff06e2468a
pablomartin4btc:
ACK ff06e2468a
jaonoctus:
re-ACK ff06e2468a
Tree-SHA512: 222d5e89021d5f9a7ce0edca44c4ce20b13f71832413dccea78ad40a01f2a615a061f8cf446d7290ed911023922adbc6fa22f0c88cff306dcd8b4ae14194e9b8
Previously, when `debug=net` is enabled, we log "New [..] peer connected"
for new inbound peers with `LogInfo`. However, `LogInfo` will get
rate-limited since https://github.com/bitcoin/bitcoin/pull/32604.
When we specifically turn on `debug=net`, we don't want these log
messages to be rate-limited.
To fix this, use `LogDebug(BCLog::NET, ...)` for potentially high-
rate inbound connections. Otherwise use `LogInfo`. This means we
don't rate-limit the messages for inbound peers when `debug=net`
is turned on but will rate-limit if we created outbound at a high
rate as these are logged via `LogInfo`.
--
I ran into this message getting rate-limited on one of my monitoring
nodes with `-logsourcelocations=1`: With logsourcelocations, one of
these lines is about 338 chars (or 338 bytes) long. We rate-limit
after more than 1048576 bytes per hour, which results in about
3100 in- and outbound connections per hour. With evicted and
instantly reconnecting connections from an entity like LinkingLion,
this can be reached fairly quickly.
Co-Authored-By: Eugene Siegel <elzeigel@gmail.com>
Co-Authored-By: Anthony Towns <aj@erisian.com.au>
"tor" as a network specification was deprecated in 60dc8e4208 in favor
of "onion" and this commit removes it and updates the relevant test.
Co-authored-by: Mara van der Laan <126646+laanwj@users.noreply.github.com>
This is a minimal behavior change and changes log output from:
[net:error] Something bad happened
[net:warning] Something problematic happened
to either
[error] Something bad happened
[warning] Something problematic happened
or, when -loglevelalways=1 is enabled:
[all:error] Something bad happened
[all:warning] Something problematic happened
Such a behavior change is desired, because all warning and error logs
are written in the same style in the source code and they are logged in
the same format for log consumers.
-BEGIN VERIFY SCRIPT-
sed --regexp-extended --in-place \
's/LogPrintLevel\((BCLog::[^,]*), BCLog::Level::(Error|Warning), */Log\2(/g' \
$( git grep -l LogPrintLevel ':(exclude)src/test/logging_tests.cpp' )
-END VERIFY SCRIPT-
faa23738fc refactor: Enable clang-tidy bugprone-unused-return-value (MarcoFalke)
fa114be27b Add util::Expected (std::expected) (MarcoFalke)
Pull request description:
Some low-level code could benefit from being able to use `std::expected` from C++23:
* Currently, some code is using `std::optional<E>` to denote an optional error. This is fine, but a bit confusing, because `std::optional` is normally used for values, not errors. Using `std::expected<void, E>` is clearer.
* Currently, some code is using `std::variant<V, E>` to denote either a value or an error. This is fine, but a bit verbose, because `std::variant` requires a visitor or get_if/holds_alternative instead of a simple call of the `operator bool` for `std::expected`.
In theory, `util::Result` could be taught to behave similar to `std::expected` (see https://github.com/bitcoin/bitcoin/pull/34005). However, it is unclear if this is the right approach:
* `util::Result` is mostly meant for higher level code, where errors come with translated error messages.
* `std::expected` is mostly meant for lower level code, where errors could be an enum, or any other type.
* https://github.com/bitcoin/bitcoin/pull/25665 aims to minimize the memory footprint of the error by wrapping it in a unique_ptr internally. `std::expected` requires the value and error to be "nested within it" (https://cplusplus.github.io/LWG/issue4141). So from a memory-layout perspective, the two are not compatible.
* `std::expected` also comes with `std::unexpected`, which also does not map cleanly to `util::Result`.
So just add a minimal drop-in port of `std::expected`.
ACKs for top commit:
romanz:
tACK faa23738fc
sedited:
Re-ACK faa23738fc
hodlinator:
ACK faa23738fc
rkrux:
light Code Review ACK faa23738fc
ryanofsky:
Code review ACK faa23738fc, only adding `value_or` implementation and `CheckedReturnTypes` clang-tidy commit since last review.
stickies-v:
ACK faa23738fc
Tree-SHA512: fdbd0f6bf439738ffe6a68da5522f1051537f8df9c308eb90bef6bd2e06931d79f1c5da22d5500765e9cb1d801d5be39e11e10d47c9659fec1a8c8804cb7c872
48840bfc2d refactor: Prefer `<=>` over multiple relational operators (Daniel Pfeifer)
5a0f49bd26 refactor: Remove all `operator!=` definitions (Daniel Pfeifer)
Pull request description:
Remove all `operator!=` definitions and provide `operator<=>` as a replacement where all relational comparison operators were defined before.
The compiler is able to deduce missing comparison operators from `operator!=` and `operator<=>`. The compiler provided operators have the following advantages:
1. less code
2. guaranteed consistency
Refactoring that changes the implementation, or replaces it with `= default` is left for a separate PR.
ACKs for top commit:
optout21:
utACK 48840bfc2d
Chand-ra:
tACK [`48840bf`](48840bfc2d). Built the PR and ran unit tests; everything passes.
maflcko:
review ACK 48840bfc2d🌖
stickies-v:
utACK 48840bfc2d. Pretty straightforward cleanup taking advantage of C++20 improvements, nice.
janb84:
ACK 48840bfc2d
sipa:
ACK 48840bfc2d
Tree-SHA512: 7fedc4abc451c7ad611e3a960ff939a35580667222009cb30ca546e564dc9161e3e8d4d1d7d44c538d961cc8f7adba6e6dbcebcd1be370bf33aef294d06f236b
We run InactivityChecks() for each node everytime poll()/select() every
50ms or so. Rather than calculating the current time once for each node,
just calculate it once and reuse it.
fa4395dffd refactor: Remove unused LogPrintf (MarcoFalke)
fa05181d90 scripted-diff: LogPrintf -> LogInfo (MarcoFalke)
Pull request description:
`LogPrintf` has many issues:
* It does not mention the log severity (info).
* It is a deprecated alias for `LogInfo`, according to the dev notes.
* It wastes review cycles, because reviewers sometimes point out that it is deprecated.
* It makes the code inconsistent, when both versions of the alias are used.
Fix all issues by removing the deprecated alias.
ACKs for top commit:
ajtowns:
ACK fa4395dffd
stickies-v:
ACK fa4395dffd
rkrux:
lgtm ACK fa4395dffd
Tree-SHA512: de95d56df27b9ee33548cc7ee7595e2d253474094473089ee67787ddb171384383c683142672c3e2c1984e19eee629b2c469dc85713640a73391610581edbdbe
This requires some small refactors to silence false-positive warnings.
Also, expand the bugprone-unused-return-value.CheckedReturnTypes option
to include util::Result, and util::Expected.
`ParseByteUnits()` is the only parsing function in `strencodings.cpp`
lacking a fuzz test. Add a test case to check the function against
arbitrary strings and randomized default_multiplier's.
ffcae82a68 test: exercise TransactionMerklePath with empty block; targets the MerkleComputation empty-leaves path that was only reached by fuzz tests (frankomosh)
Pull request description:
As noted in [#32243 (comment)](https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-2988854482), the early return inside `MerkleComputation` when `leaves.size() == 0` was only exercised by fuzz tests.
The existing `merkle_test_empty_block` calls `BlockMerkleRoot`, which uses `ComputeMerkleRoot`, but does not exercise the `TransactionMerklePath` → `ComputeMerklePath` → `MerkleComputation` code path.
Coverage before adding test:
<img width="2459" height="66" alt="before" src="https://github.com/user-attachments/assets/ca94015a-d7c2-4281-ac60-13b22f177b67" />
Coverage after adding test:
<img width="2459" height="66" alt="after" src="https://github.com/user-attachments/assets/b1d4e1bb-af72-46ab-8898-f18db39dd2fb" />
ACKs for top commit:
kevkevinpal:
ACK [ffcae82](ffcae82a68)
maflcko:
lgtm ACK ffcae82a68
brunoerg:
code review ACK ffcae82a68
sedited:
ACK ffcae82a68
Tree-SHA512: d2499d91269c4f4f9a86011f7ad13f675834662a5bd37b0e7cbe887a7d9acf4170e53f0bdc528011fc82866b9c1dec34f4e7e9cd64cc3100591c1580a4df5d00
167df7a98c net: fix use-after-free with v2->v1 reconnection logic (Eugene Siegel)
Pull request description:
`CConnman::Stop()` resets `semOutbound`, yet `m_reconnections` is not cleared in `Stop`. Each `ReconnectionInfo` contains a `grant` member that points to the memory that `semOutbound` pointed to and `~CConnman` will attempt to access the grant field (memory that was already freed) when destroying `m_reconnections`. Fix this by calling `m_reconnections.clear()` in `CConnman::Stop()` and add appropriate annotations.
I was able to reproduce the original issue https://github.com/bitcoin/bitcoin/issues/33615 with the following diff by randomly stopping my node while it was attempting to reconnect (and verified that this patch fixes the issue, at least in my ~40-50 runs):
<details>
<summary> diff </summary>
```diff
diff --git a/src/net.cpp b/src/net.cpp
index ef1c63044a..9c1d161d8b 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1918,8 +1918,8 @@ void CConnman::DisconnectNodes()
{
LOCK(m_nodes_mutex);
- const bool network_active{fNetworkActive};
- if (!network_active) {
+// const bool network_active{fNetworkActive};
+// if (!network_active) {
// Disconnect any connected nodes
for (CNode* pnode : m_nodes) {
if (!pnode->fDisconnect) {
@@ -1927,7 +1927,7 @@ void CConnman::DisconnectNodes()
pnode->fDisconnect = true;
}
}
- }
+// }
// Disconnect unused nodes
std::vector<CNode*> nodes_copy = m_nodes;
@@ -1941,7 +1941,7 @@ void CConnman::DisconnectNodes()
// Add to reconnection list if appropriate. We don't reconnect right here, because
// the creation of a connection is a blocking operation (up to several seconds),
// and we don't want to hold up the socket handler thread for that long.
- if (network_active && pnode->m_transport->ShouldReconnectV1()) {
+ if (true) {
reconnections_to_add.push_back({
.addr_connect = pnode->addr,
.grant = std::move(pnode->grantOutbound),
```
</details>
I'm curious to see if others can reproduce as well.
ACKs for top commit:
dergoegge:
Code review ACK 167df7a98c
darosior:
utACK 167df7a98c
mzumsande:
ACK 167df7a98c
Tree-SHA512: 33fdfb110a7cdae182b5cd5400eea8a271308a62dd56491e0aef8865eff24a9ea908be74e4e2e2ee00ac1cb698e46f270f56dffffe34cf2cfd79e9b1079d6531
b0c706795c Remove unreliable seed from chainparams.cpp, and the associated README (SatsAndSports)
Pull request description:
The DNS seed `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us.` is not returning a representative sample of bitcoin nodes. It currently returns nothing later than 28.1.0, breaching the policy.
This PR removes that seed from the list of DNS seeds
### Rationale
The [policy for seeds](https://github.com/bitcoin/bitcoin/blob/master/doc/dnsseed-policy.md) includes this:
> The DNS seed results must consist exclusively of fairly selected and functioning Bitcoin nodes from the public network
A number of comments below, in response to this PR, include apparent breaches of this policy: [1](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458071231) [2](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3457655364), [3](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3457712557), in particular the first linked comment ([1](https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458071231)) comparing the distribution at this seed to other seeds. This seed is not including anything later than 28.2.0, breaching this policy.
To ensure the policy is followed, and the seeds include a representative sample of Bitcoin nodes, this PR removes this seed from the list
### Data
I ran this:
```
# Get some ip address from that seed:
# Repeated multiple times, to get many different IPs:
dig +short dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us >> dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us
# For each distinct ip gathered from the seed, get basic info about the node, including it's User Agent string:
cat dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us | sort -u | while read ip; do echo ===; echo $ip; nmap -p 8333 --script bitcoin-info "$ip"; done > seed_versions.txt
```
and then summarized the agents with `egrep 'User Agent' seed_versions.txt | sort | uniq -c` and got:
```
1 User Agent: /Satoshi:22.0.0/
1 User Agent: /Satoshi:22.1.0/
5 User Agent: /Satoshi:24.0.1/
1 User Agent: /Satoshi:25.1.0/
30 User Agent: /Satoshi:27.0.0/
1 User Agent: /Satoshi:27.1.0/
1 User Agent: /Satoshi:27.1.0/Knots:20240801/
1 User Agent: /Satoshi:28.0.0/
7 User Agent: /Satoshi:28.1.0/
2 User Agent: /Satoshi:28.1.0/Knots:20250305/
```
ACKs for top commit:
l0rinc:
reACK b0c706795c
delta1:
reACK b0c706795c
Crypt-iQ:
crACK b0c706795c
laanwj:
ACK b0c706795c
murchandamus:
ACK b0c706795c
RandyMcMillan:
ACK b0c7067
wiz:
ACK b0c706795c
dergoegge:
ACK b0c706795c
stickies-v:
re-ACK b0c706795c
mzumsande:
ACK b0c706795c
instagibbs:
ACK b0c706795c
Tree-SHA512: 7230b8dd24560ce6f8247e2e82ae7846ded8b91e230c59cc3643da3f5b9c12b5f025c1bb14490c19ca55f3794e81ce08106b31b3bf883d5c2dced05017123ac4
