fa75480c84ffecc856c2d76b1143b14ebce85d0b test: Detect truncated download in get_previous_releases.py (MarcoFalke)
Pull request description:
Without this, and end-of-stream is not detected and will just lead to an immediate exit, instead of a re-try.
E.g. https://github.com/bitcoin/bitcoin/actions/runs/20089133013/job/57633839315?pr=34038#step:12:201:
```
...
Downloading: [##--------------------------------------] 5.4%
Downloading: [##--------------------------------------] 5.4%
Downloading: [##--------------------------------------] 5.5%
Downloading: [##--------------------------------------] 5.6%
Checksum dd02eab18f9154604e38135ef3f98fd310ba3c748074aeb83a71118cd2cd1367 did not match
Error: Process completed with exit code 1.
```
Also, remove the `0` fallback value, because if the fallback was ever hit, the program would fail anyway with `division by zero` error.
ACKs for top commit:
Sjors:
utACK fa75480c84ffecc856c2d76b1143b14ebce85d0b
rkrux:
Looks fine, ACK fa75480c84ffecc856c2d76b1143b14ebce85d0b
l0rinc:
code review ACK fa75480c84ffecc856c2d76b1143b14ebce85d0b
Tree-SHA512: 230eaf155701ed833636b401118f11ff5c6521c61bf4f3a01fcf390a71a508ba6a570eea855ef659134e118b74f75e3d5772ec8a261db23ebfe4ac7ec87cab5a
e7ac5a133cc354b1e60cb9c3660da03e79c50173 doc: add release note for 34031 (fanquake)
c4c70a256ed82c0e25d9bf32eda218f31e1523c8 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 e7ac5a133cc354b1e60cb9c3660da03e79c50173
janb84:
ACK e7ac5a133cc354b1e60cb9c3660da03e79c50173
stickies-v:
ACK e7ac5a133cc354b1e60cb9c3660da03e79c50173
Tree-SHA512: f211dec151c21728b4cd2b1716ee68907871beaa85d8c89e2bc17576e701d03c03e5455593de94970d787aa3264fab60d8c6debeeff908e00d8feb48804692e9
a1f76230209629c5141984aaac1cc4ba7b4f761a qa: Only complain about expected messages that were not found (Hodlinator)
1e54125e2e0063e4586265b957912896c056ba0a refactor(qa): Avoid unnecessary string operations (Hodlinator)
a9021101dc63ee7fb1e1f342387ab7d3f0e3cefc qa: Replace always-escaped regexps with "X in Y" (Hodlinator)
5c16e4631c009056d571d83fe17062c107f71798 doc: Remove no longer correct comment (Hodlinator)
Pull request description:
* Remove incorrect docstring in `busy_wait_for_debug_log()`.
* Replace nerfed regex searches with `X in Y` expressions.
* Only compute the log string to be printed on failure *when we actually fail* instead of every 0.05s.
* As we find each needle (expected message) in the haystack (log output), stop searching for it. **If we fail and time out, we will only complain about the needles (expected messages) we didn't find. On master we also include found needles, which is less helpful.**
Found while developing a new test case in https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351892330
ACKs for top commit:
l0rinc:
Code review ACK a1f76230209629c5141984aaac1cc4ba7b4f761a
maflcko:
review ACK a1f76230209629c5141984aaac1cc4ba7b4f761a 💨
Tree-SHA512: 191ea7647b0ea8b4220e37c62d176861c2fd0e3737aee3641b262915d9118f48953cf1204767c93a93a8fc78a44c2c29206fb390b44c59d99fc2aa7d12bf4889
d4d184eda9c0f73bc31ece07d5001d887b5c6914 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 d4d184eda9c0f73bc31ece07d5001d887b5c6914
Crypt-iQ:
tACK d4d184eda9c0f73bc31ece07d5001d887b5c6914
maflcko:
review ACK d4d184eda9c0f73bc31ece07d5001d887b5c6914 🚲
rkrux:
lgtm code review ACK d4d184eda9c0f73bc31ece07d5001d887b5c6914
glozow:
lgtm ACK d4d184eda9c0f73bc31ece07d5001d887b5c6914
Tree-SHA512: 14dbf693fa44a74c9822590e7a08167d2deeb1bc6f4b8aeb00c1b035c0df7101087d5c80a3c0d637879d5c52f88b30f0cb4c0577cff6f647d2eb3300f49d8ea3
4b4711369880369729893ba7baef11ba2a36cf4b validation: Reword CheckForkWarningConditions and call it also during IBD and at startup (Martin Zumsande)
2f51951d03cc1f8917e0fc931dce674f9bfedaf5 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 4b4711369880369729893ba7baef11ba2a36cf4b
sedited:
ACK 4b4711369880369729893ba7baef11ba2a36cf4b
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.
a7c96f874de13ace9814da92fd6160a126a97ebf tests: Add witness commitment if we have a witness transaction in FullBlockTest.update_block() (Chris Stewart)
Pull request description:
This is useful for test cases where we want to test logic invalid blocks that contain witness transactions. If we don't add the witness commitment as per [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#user-content-Commitment_structure), blocks will be rejected with the error [`Block mutated`](fb0ada982a/src/validation.cpp (L4180)).
This change was needed in https://github.com/ajtowns/bitcoin/pull/13 which is a soft fork proposal to disallow 64 byte transactions. We want to test that 64 byte transactions serialized without the witness are invalid. If we do not have this change, we cannot directly test the logic that rejects 64 byte transactions.
I decided to PR this upstream as many soft fork proposals may not see the light of day, but this functionality seems strictly additive to the test framework.
ACKs for top commit:
theStack:
ACK a7c96f874de13ace9814da92fd6160a126a97ebf
sedited:
ACK a7c96f874de13ace9814da92fd6160a126a97ebf
glozow:
ACK a7c96f874de13ace9814da92fd6160a126a97ebf
Tree-SHA512: 7c185838abaf068bc96b425c3c971b73f75dfcb41dacc8b2f2543c7602f23f19d908633278b93738f18049e6bd8c845c152cfb93b289bef501c7e86ed8dae0ab
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.
9d5021a05bd33c73276909eec961777867ddb412 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 9d5021a05bd33c73276909eec961777867ddb412
darosior:
utACK 9d5021a05bd33c73276909eec961777867ddb412
sipa:
ACK 9d5021a05bd33c73276909eec961777867ddb412
Tree-SHA512: bc0b7f64454313fe392ffb2d23aa4eca3deadc5ea1d10b3fba0b3ab4cb0575a5ddcb002dc27b4fa7aa3c180840a83d1b3e5c89351009ce7ffe684d58e1980ace
b5a7a685bba312a780eddcb4a53ce2c26a937854 ci: Make the max number of commits tested explicit (Hodlinator)
Pull request description:
Gives less of a false sense of security.
ACKs for top commit:
maflcko:
lgtm ACK b5a7a685bba312a780eddcb4a53ce2c26a937854
rkrux:
crACK b5a7a685bba312a780eddcb4a53ce2c26a937854
janb84:
ACK b5a7a685bba312a780eddcb4a53ce2c26a937854
glozow:
lgtm ACK b5a7a685bba312a780eddcb4a53ce2c26a937854
Tree-SHA512: 9f50a86f440d6a551a0c1ff547e61b61b829e98cd0cd2d5ca65966af0b48d40582f698bcb039a7467c4b71166920413c334eac0e9e4f0141c3e02cd68555865b
ff06e2468a5d3eeebeffe781904c34c9d1b44385 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 ff06e2468a5d3eeebeffe781904c34c9d1b44385
stickies-v:
ACK ff06e2468a5d3eeebeffe781904c34c9d1b44385
pablomartin4btc:
ACK ff06e2468a5d3eeebeffe781904c34c9d1b44385
jaonoctus:
re-ACK ff06e2468a5d3eeebeffe781904c34c9d1b44385
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-
As pointed out by Sjors in
https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598844209 and
https://github.com/bitcoin/bitcoin/pull/34003#discussion_r2598858386 the
original intention of having waitNext and waitTipChanged calls in the test was
to ensure that if new blocks were connected or fees were increased *during* the
waits, that the calls would wake up and return.
But the tests were written incorrectly, to generate blocks and transactions
before the wait calls instead of during the calls. So the tests were less
meaningful then they should be.
There was also a similar problem in the interruptWait test. The test was
intended to test the interruptWait method, but it was never actually calling
the method due to a missing await keyword. Instead it was testing that
miniwallet.send_self_transfer would interrupt the wait.
This commit fixes these issues by introducing a wait_and_do() helper function
to start parallel tasks and trigger an action after a wait call is started.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
There are a few things that are incorrect or messy in the interface_ipc.py test.
This commit tries to clean them up:
- isTestChain and isInitialBlockDownload asserts were not checking the results
of those calls, only that calls were, made because they were not checking the
responses' .result member.
- A lot of result accesses like `template.result` `mining.result` were repeated
unnecessarily because variables like `template` and `mining` were assigned
response objects instead of result objects. These variables are now changed
to point directly to results.
- Some coroutine calls were assigned to temporary `wait` before being awaited.
This was unnecessarily confusing and would make code not run in top-down
order.
- `to_dict` calls were being made to check if result variables were unset. This
was inefficient and indirect because it iterates over all fields in response
structs instead of just checking whether the result field is present. The
to_dict calls are now replaced with more direct `_has('result')` calls.
- The `res` variables used to hold various responses did not have descriptive
names. These are replaced with clearer names.
Co-authored-by: rkrux <rkrux.connect@gmail.com>
faa23738fc2576e412edb04a4004fab537a3098e refactor: Enable clang-tidy bugprone-unused-return-value (MarcoFalke)
fa114be27b17ed32c1d9a7106f313a0df8755fa2 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 faa23738fc2576e412edb04a4004fab537a3098e
hodlinator:
ACK faa23738fc2576e412edb04a4004fab537a3098e
rkrux:
light Code Review ACK faa23738fc2576e412edb04a4004fab537a3098e
ryanofsky:
Code review ACK faa23738fc2576e412edb04a4004fab537a3098e, only adding `value_or` implementation and `CheckedReturnTypes` clang-tidy commit since last review.
stickies-v:
ACK faa23738fc2576e412edb04a4004fab537a3098e
Tree-SHA512: fdbd0f6bf439738ffe6a68da5522f1051537f8df9c308eb90bef6bd2e06931d79f1c5da22d5500765e9cb1d801d5be39e11e10d47c9659fec1a8c8804cb7c872
48840bfc2d7beeac0ddf56a3c26b243156ec8936 refactor: Prefer `<=>` over multiple relational operators (Daniel Pfeifer)
5a0f49bd2661d82efe13740856764e4e17fc1d06 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 48840bfc2d7beeac0ddf56a3c26b243156ec8936
Chand-ra:
tACK [`48840bf`](48840bfc2d). Built the PR and ran unit tests; everything passes.
maflcko:
review ACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936 🌖
stickies-v:
utACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936. Pretty straightforward cleanup taking advantage of C++20 improvements, nice.
janb84:
ACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936
sipa:
ACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936
Tree-SHA512: 7fedc4abc451c7ad611e3a960ff939a35580667222009cb30ca546e564dc9161e3e8d4d1d7d44c538d961cc8f7adba6e6dbcebcd1be370bf33aef294d06f236b
41e657aacfa605ac2de08ef26f2b536810f3e55a guix: add bitcoin-qt runtime libs doc in symbol-check (fanquake)
ef4ce19a1545d81dcf95b07e04628557a57451e2 depends: freetype 2.11.1 (fanquake)
Pull request description:
Update freetype to `2.11.1`.
Updating fontconfig (currently `2.12.6`) to `2.13.1` requires what looks like a hard dep on gperf; leaving that as-is for now.
Document expectations in `symbol-check.py`.
Closes#29977 (changes are based on discussion there).
ACKs for top commit:
sedited:
ACK 41e657aacfa6
Tree-SHA512: 71c4ccc442df0b90bebc475003eb325564111b8312c42bc7d7a9c81a2fc166fdc0814c9ddde3cfe562c3c835556e7f97107458b02a07b981b1a199bf65d5ac1d
7b90b4f5bb10e2156709b07e3996f867e2421232 guix: reduce allowed exported symbols (fanquake)
Pull request description:
Need to double-check, but pretty sure this is atleast partly from #33181.
ACKs for top commit:
sedited:
Nice, ACK 7b90b4f5bb10e2156709b07e3996f867e2421232
Tree-SHA512: 538c03dc32aab9b3e18100e8ffa0d664aea5ceba6aafee9e8e0894c2d02eea3b3fb09733cf7b5bd0aefb6b56d0ac3b92f28da932e135b23f55404efd8f43664a
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.
fa4395dffd432b999002dfd24eb6f8d7384fbcbe refactor: Remove unused LogPrintf (MarcoFalke)
fa05181d904d620cd8944123e64e3931b21298f9 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 fa4395dffd432b999002dfd24eb6f8d7384fbcbe
stickies-v:
ACK fa4395dffd432b999002dfd24eb6f8d7384fbcbe
rkrux:
lgtm ACK fa4395dffd432b999002dfd24eb6f8d7384fbcbe
Tree-SHA512: de95d56df27b9ee33548cc7ee7595e2d253474094473089ee67787ddb171384383c683142672c3e2c1984e19eee629b2c469dc85713640a73391610581edbdbe
57b888ce0ebdeb34d866fd1511052fd740cc5ab8 fuzz: Add a test case for `ParseByteUnits()` (Chandra Pratap)
Pull request description:
`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`.
ACKs for top commit:
maflcko:
lgtm ACK 57b888ce0ebdeb34d866fd1511052fd740cc5ab8
dergoegge:
utACK 57b888ce0ebdeb34d866fd1511052fd740cc5ab8
marcofleon:
crACK 57b888ce0ebdeb34d866fd1511052fd740cc5ab8
Tree-SHA512: c16557442987437e5e0c9d9a8b016df93e513e34acb78242a1f73dabc4482632ec57eb35cb4c84f9a1ea838fa6bda2094f2a8b52ace431f8064a79fad96e9a52
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.
710031ebef838d2f0a1effa19170bef7b130bbeb Revert "guix: sqlite wants tcl" (Hennadii Stepanov)
4cf5ea6c3d2a5990aafd59ea5137d99f050840f4 depends: Propagate native C compiler to `sqlite` package (Hennadii Stepanov)
Pull request description:
This PR:
1. Ensures that autosetup can build the local bootstrap `jimsh0` when neither `jimsh` nor `tclsh` is available on the system.
2. Removes the `tcl` package from the Guix manifest.
This is an alternative to https://github.com/bitcoin/bitcoin/pull/33975.
ACKs for top commit:
fanquake:
ACK 710031ebef838d2f0a1effa19170bef7b130bbeb
sedited:
ACK 710031ebef838d2f0a1effa19170bef7b130bbeb
Tree-SHA512: bdaa29af977799669bfc2aa3a8d0a4a688263b99c5f06b1582fbefb71ef77be0ee6223903e8357e51a9e0a7744807174b94262c2f4a3afd9f39737b61b00863e
fd4ce55121e7b0fe0e5b1ecf648dc3178ed37fd8 contrib: Count entry differences in asmap-tool diff summary (Fabian Jahr)
Pull request description:
Currently the output of `asmap-tool.py diff` returns the total number of addresses that has changed at the end of the list.
Example output currently:
```
2602:feda:c0::/48 AS1029 # was AS43126
2604:7c00:100::/40 AS29802 # was AS40244
# 0 IPv4 addresses changed; 79552154633921058212365205504 (2^96.01) IPv6 addresses changed
```
This is good indicator but in case of a longer list I would like the number of changed entries as well, since that is an easier number to parse and for debugging of certain issues also the more relevant value. This PR adds the count of changed entries to this summary output at the end. There as also a bit more structure so it's easier to parse as well.
Example new output:
```
2602:feda:c0::/48 AS1029 # was AS43126
2604:7c00:100::/40 AS29802 # was AS40244
# Summary
IPv4: 0 entries with 0 addresses changed
IPv6: 12 entries with 79552154633921058212365205504 (2^96.01) addresses changed
```
ACKs for top commit:
jurraca:
utACK [`fd4ce55121`](fd4ce55121)
janb84:
utACK fd4ce55121e7b0fe0e5b1ecf648dc3178ed37fd8
hodlinator:
ACK fd4ce55121e7b0fe0e5b1ecf648dc3178ed37fd8
Tree-SHA512: 97cc543eaba80a33f0291b20630411bda869d3b8d1b35ed7f36792064cb1edccc8fe4740b7229b5451a88b7bd8d68c42f96829ce4255ecac3e29d70b68061608
`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.
ffcae82a68104c1992964b26c592b62cbca391bf 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 ffcae82a68104c1992964b26c592b62cbca391bf
brunoerg:
code review ACK ffcae82a68104c1992964b26c592b62cbca391bf
sedited:
ACK ffcae82a68104c1992964b26c592b62cbca391bf
Tree-SHA512: d2499d91269c4f4f9a86011f7ad13f675834662a5bd37b0e7cbe887a7d9acf4170e53f0bdc528011fc82866b9c1dec34f4e7e9cd64cc3100591c1580a4df5d00
167df7a98c8514da6979d45e58fcdcbd0733b8fe 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 167df7a98c8514da6979d45e58fcdcbd0733b8fe
darosior:
utACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe
mzumsande:
ACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe
Tree-SHA512: 33fdfb110a7cdae182b5cd5400eea8a271308a62dd56491e0aef8865eff24a9ea908be74e4e2e2ee00ac1cb698e46f270f56dffffe34cf2cfd79e9b1079d6531
b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e 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 b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
delta1:
reACK b0c706795c
Crypt-iQ:
crACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
laanwj:
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
murchandamus:
ACK b0c706795c
RandyMcMillan:
ACK b0c7067
wiz:
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
dergoegge:
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
stickies-v:
re-ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
mzumsande:
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
instagibbs:
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
Tree-SHA512: 7230b8dd24560ce6f8247e2e82ae7846ded8b91e230c59cc3643da3f5b9c12b5f025c1bb14490c19ca55f3794e81ce08106b31b3bf883d5c2dced05017123ac4
866bbb98fd365962840ee99df0d9f7ad557cc025 cmake, test: Improve locality of `bitcoin_ipc_test` library description (Hennadii Stepanov)
ae2e438b257f2b5322080087ed7dc8843d4f9cca cmake: Move IPC tests to `ipc/test` (Hennadii Stepanov)
Pull request description:
This PR follows up on https://github.com/bitcoin/bitcoin/pull/33445 and:
1. Organizes the IPC tests in the same way as the wallet tests.
2. Removes no longer needed `src/test/.clang-tidy.in`.
See the previous discussion:
- https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379651340
- https://github.com/bitcoin/bitcoin/pull/33445#pullrequestreview-3411868329
Additionally, the locality of the `bitcoin_ipc_test` build target description has been improved.
ACKs for top commit:
Sjors:
ACK 866bbb98fd365962840ee99df0d9f7ad557cc025
janb84:
ACK 866bbb98fd365962840ee99df0d9f7ad557cc025
ryanofsky:
Code review ACK 866bbb98fd365962840ee99df0d9f7ad557cc025, just adding back the suggested comment, and also fixing bad include arguments passed to target_capnp_sources. It would probably be a little better if the include fix was done in an earlier commit, since it's not really related to the other changes in the last commit, but would also be ok to make both changes at the same time.
Tree-SHA512: ed7cc817ccb88595d8516978bff0ea2560048d35b3f548e7913aec7d58b8d6ac550e230e992c527fb747bef175580be92dc4df6342e4485f3a9870dba0a25cba
dcd42d6d8f160ae8bc12c152099a6e6473658e30 [test] wallet send 3 generation TRUC (glozow)
e753fadfd01cb8a4a8de3bddc7391481551cca89 [wallet] never try to spend from unconfirmed TRUC that already has ancestors (glozow)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3319935660
There is not an explicit check that the to-be-created wallet transaction would be within the {TRUC, normal} ancestor limits. This means that the wallet may create a transaction that violates these limits, but fail to broadcast it in `CommitTransaction`.
This appears to be expected behavior for the normal ancestor limits (and any other situation in which the wallet creates a tx that was rejected by mempool) and AFAIK the transaction will be rebroadcast at some point after the ancestors confirm.
1ed00a0d39/test/functional/wallet_basic.py (L502-L506)
It's a bit complex to address this for the normal ancestor limit, and probably unrealistic for the wallet to check all possible mempool policies in coin selection, but it's quite trivial for TRUC: just skip any unconfirmed UTXOs that have any ancestors. I think it would be much more helpful to the user to say there are insufficient funds.
ACKs for top commit:
achow101:
ACK dcd42d6d8f160ae8bc12c152099a6e6473658e30
monlovesmango:
ACK dcd42d6d8f160ae8bc12c152099a6e6473658e30
rkrux:
lgtm ACK dcd42d6d8f160ae8bc12c152099a6e6473658e30
Tree-SHA512: b4cf9685bf0593c356dc0d6644835d53e3d7089f42b65f647795257dc7f5dac90c5ee493b41ee30a1c1beb880a859db8e049d3c64a43d5ca9b3e6482ff6bddd5
Use context managers to destroy block templates. Previously, block templates
were not being destroyed before disconnecting because the destroy coroutines
were called but never awaited. It's not necessary to explicitly destroy the
templates since they will get garbage collected asynchronously, but it's good
to destroy them to make the test more predictable, and to make the destroy
calls that are present actually do something.
This change also removes `await waitnext` expressions without changing
behavior, because the previous code was misleading about what order waitNext
calls were executed.
This change is easiest to review ignoring whitespace.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
e9536faaee2bdb53807aa7c48aafa269d18f19d3 contrib: fix manpage generation (fanquake)
Pull request description:
0972f5504021b482b27523fd3bcb8036cf6b439c from #33229 broke manpage generation, because the assumption that the last word in the line containing the version number, was the version number, no-longer holds for some binaries. i.e `bitcoind`.
ACKs for top commit:
janb84:
re ACK e9536faaee2bdb53807aa7c48aafa269d18f19d3
rkrux:
re-ACK e9536faaee2bdb53807aa7c48aafa269d18f19d3
Tree-SHA512: 471b1800beeec3ea70d722ac2dcc26073805c8fcdf0418ceb79728cc001eb7c2f11a3d832b54a7ae68d26fe5c97934a9c87eedae7601515857e660fac7532c0a
fa6db79302d28e6b31b07b24580430614ffcd0e8 test: Avoid shutdown race in NetworkThread (MarcoFalke)
Pull request description:
Locally, I am seeing rare intermittent exceptions in the network thread:
```
stderr:
Exception in thread NetworkThread:
Traceback (most recent call last):
File "/python3.10/threading.py", line 1016, in _bootstrap_inner
self.run()
File "./test/functional/test_framework/p2p.py", line 744, in run
self.network_event_loop.run_forever()
File "/python3.10/asyncio/base_events.py", line 603, in run_forever
self._run_once()
File "/python3.10/asyncio/base_events.py", line 1871, in _run_once
event_list = self._selector.select(timeout)
AttributeError: 'NoneType' object has no attribute 'select'
```
I can reproduce this intermittently via `while ./bld-cmake/test/functional/test_runner.py $(for i in {1..400}; do echo -n "tool_rpcauth "; done) -j 400 ; do true ; done`.
I suspect this is a race where the shutdown starts the close of the network thread while it is starting.
A different exception showing this race can be reproduced via:
```diff
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index 610aa4ccca..64561e157c 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -741,6 +741,7 @@ class NetworkThread(threading.Thread):
def run(self):
"""Start the network thread."""
+ import time;time.sleep(.1)
self.network_event_loop.run_forever()
def close(self, *, timeout=10):
```
It is trivial to reproduce via any test (e.g. `./bld-cmake/test/functional/tool_rpcauth.py`) and shows a similar traceback to the one above:
```
Exception in thread NetworkThread:
Traceback (most recent call last):
File "/python3.10/threading.py", line 1016, in _bootstrap_inner
self.run()
File "./test/functional/test_framework/p2p.py", line 745, in run
self.network_event_loop.run_forever()
File "/python3.10/asyncio/base_events.py", line 591, in run_forever
self._check_closed()
File "/python3.10/asyncio/base_events.py", line 515, in _check_closed
raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed
```
So fix the second runtime error in hope of fixing the first one as well.
ACKs for top commit:
brunoerg:
code review ACK fa6db79302d28e6b31b07b24580430614ffcd0e8
Tree-SHA512: ca352ebf7929456ea2bbfcfe4f726adcbfcfb3dc0edeaddae7f6926f998888f0bd8b987ddef60308266eeab6bffa7ebdc32f5908db9de5404df95635dae4a8f6
0972f5504021b482b27523fd3bcb8036cf6b439c from #33229 broke manpage
generation, because the assumption that the last word in the line
containing the version number, was the version number, no-longer holds
for some binaries. i.e bitcoind.
