efb4eae338 clusterlin: avoid recomputing intersections in MergeChunks (Pieter Wuille)
4b91ad149f clusterlin: reserve the suboptimal-chunk queue up front (Pieter Wuille)
e6ca996255 clusterlin: avoid heap allocations in GetLinearization (Pieter Wuille)
Pull request description:
These are a few minor and easy-to-review code changes to the SFL algorithm, which net a few % speedup (~2.6% speedup on `LinearizeOptimally*` benchmarks, ~5% on the `Historical` ones).
<details><summary>LLM benchmark results:</summary>
| Class | Ratio | 95% CI |
|-------------------|--------|------------------|
| HistoricalTotal | 0.9499 | [0.9312, 0.9709] |
| SyntheticTotal | 0.9760 | [0.9597, 0.9949] |
| HistoricalPerCost | 0.9545 | [0.9238, 0.9845] |
| SyntheticPerCost | 0.9934 | [0.9688, 1.0175] |
| **All (60)** | **0.9737** | **[0.9563, 0.9913]** |
Methology: One release-mode `bench_bitcoin` binary (GCC 15.2, `-O2`) was built for the base commit and one for the branch tip, and run as `bench_bitcoin -filter='.*LinearizeOptimally.*' -min-time=100` in 85 strictly alternating pairs of fresh process launches (170 launches, ~20 minutes total), each pinned to the same core of an otherwise idle Zen5 machine. For each of the 60 benchmarks, the per-launch ns/op values were averaged over each binary's 85 launches; the optimized/base ratios of these means were aggregated as a geometric mean per benchmark class, with 95% confidence intervals from 1000 bootstrap resamplings of the launches.
</details>
Disclosure: this code, comments, benchmarks, and selection of optimizations were done by Claude Fable 5. I reviewed the commits, and wrote the PR description.
ACKs for top commit:
optout21:
ACK efb4eae338
instagibbs:
light review ACK efb4eae338
marcofleon:
crACK efb4eae338
Tree-SHA512: 6bd523e8bc56dbc8ff2540ef26355e79895a8a11a26d08e22918aea0c002ee8f96916654d5a2b792515ef12ee022c65d71ab9a778f937a264aaf6e4ed6167b71
75929b11ed doc: add release note for submitSolution IPC changes (woltx)
ed75d70fdb refactor: centralize SubmitBlock result handling (w0xlt)
cbaa1696f3 mining: add reason and debug output to submitSolution (w0xlt)
83f3bc002d mining: clarify SubmitBlock result handling (w0xlt)
Pull request description:
`BlockTemplate.submitSolution` currently returns only a boolean, so IPC mining clients cannot determine why a submission failed without inspecting Bitcoin Core's debug log.
Returning `reason` and `debug`, as `Mining.submitBlock` already does, lets callers distinguish a concrete block rejection from a duplicate or inconclusive result. Here, `inconclusive` means the method returns failure, but validation did not determine that the submitted block is invalid.
This follow-up was suggested during the review of #34644:
https://github.com/bitcoin/bitcoin/pull/34644#discussion_r2853758006
This PR:
- Extracts a shared `SubmitBlock` helper that wraps `ProcessNewBlock` with `SubmitBlockStateCatcher` to capture `BlockValidationState`
- Adds `reason` and `debug` output parameters to `submitSolution`, matching `submitBlock`
- Makes both methods delegate to the same helper, eliminating duplicated logic
ACKs for top commit:
optout21:
ACK 75929b11ed
achow101:
light ACK 75929b11ed
Sjors:
ACK 75929b11ed
enirox001:
ACK 75929b11ed
sedited:
ACK 75929b11ed
Tree-SHA512: 31b1c305c20aaebdfa2d887665d9927830d0f97ba3c3469e2792148ad799d5a400a000cc0ca0b9add071d314e27c9da44d55228c442533a32a7c031678b78a55
7e19ce200b rpc: Fix descriptorprocesspsbt internal bug on invalid signatures (b-l-u-e)
Pull request description:
Fixes#32849
descriptorprocesspsbt crashes with an internal bug assertion when given a PSBT whose inputs looked “finalized” that is non-empty final_script_sig / witness but whose signatures did not verify like invalid Schnorr bytes with unusual sighash combinations such as SIGHASH_SINGLE | ANYONECANPAY.
Completeness was inferred with PSBTInputSigned, which only checks that final fields are present, not that they pass script verification. That let the RPC treat the PSBT as complete and call FinalizeAndExtractPSBT, where a failing invariant surfaced as CHECK_NONFATAL instead of a normal incomplete outcome.
This PR determines complete the same way as the wallet path: after ProcessPSBT, it recomputes PrecomputedTransactionData and sets complete only if every input passes PSBTInputSignedAndVerified.
Invalid signatures then yield complete: false and no hex, without going through the finalize path that asserted.
ACKs for top commit:
achow101:
ACK 7e19ce200b
rkrux:
lgtm re-ACK 7e19ce200b
Tree-SHA512: c534fb5bd3401ee3ac0bc27eaedaaaaf23e5e8510ed9b96a747bb0e41e2bd3d2031b6b08e933e139bb426e97bd05fab6e91397205f4be1db9d17630eea49a8bf
fa0c8337a8 test: Nudge toward QT_STYLE_OVERRIDE=fusion on macOS (MarcoFalke)
faa50c08b1 refactor: Run clang-format on qt test_main.cpp (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/35771
Using the Fusion style works around QTBUG-49686.
Otherwise, there could be an invalid call under the QMacStyle.
Can be tested via:
```
QT_STYLE_OVERRIDE=macOS build/bin/test_bitcoin-qt # fails
QT_STYLE_OVERRIDE=fusion build/bin/test_bitcoin-qt # passes
```
ACKs for top commit:
hebasto:
ACK fa0c8337a8, tested on macOS Tahoe 26.5.2 (Intel).
Tree-SHA512: 3fd4e65f708ce5acd5243747661b76a5cf25e6810b49586dee5971f5f8bad13a907ee27d6830124c8c6d2b7e807661a59404f387ff59c1feeacfaa4b276bf270
075e7f4218 net: Simplify `AddressPosition` comparitor (rustaceanrob)
Pull request description:
Picked from #35713 because this appears unintentional. There were no cases where the source attempted to compare `AddressPosition` by const reference, but such a comparison is valid. This may be fixed by simplifying the comparison operator here, which also avoids copying the value.
Found in #35713:
```
<AddressPosition>' requested here
817 | BOOST_CHECK(addr_pos1 == addr_pos2);
| ^
/bitcoin-core/bitcoin/src/addrman.h:76:10: note: candidate function not viable: 'this' argument has type 'const AddressPosition', but method is not marked const
76 | bool operator==(AddressPosition other) {
```
ACKs for top commit:
maflcko:
lgtm ACK 075e7f4218
sedited:
Re-ACK 075e7f4218
Tree-SHA512: 4fc95026e6f9ec23757c97fc36a7fa94faf54a018727d0d2606215fc5705f3f1817607d8ff72544664cedc666914484ed490c6f75af63db1ce1a337be2e2949f
cf0f2aeae0 p2p: Assume v2transport for addresses from seeds (Martin Zumsande)
Pull request description:
gmaxwell noted in https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-5035178818 that addresses loaded from dns seeds and fixed seeds are still assumed to be v1, so a new node won't use `v2transport` for the first few connections it makes.
By now, the vast majority of reachable nodes (~80% according to https://bitnod.es/) in the network supports BIP324, and even if the optimistic guess would turn out to be wrong for a given node, we would just reconnect with v1.
This would also be necessary for a v2-only option (#30951), but I think it makes sense to change the default regardless of that PR.
Note that `-seednode` and `addr-fetch` connections already use `v2transport` by default.
ACKs for top commit:
w0xlt:
ACK cf0f2aeae0
sedited:
ACK cf0f2aeae0
willcl-ark:
ACK cf0f2aeae0
stratospher:
tested ACK cf0f2aea.
Tree-SHA512: 921dfcf56960f66c37346f0a4d07e8ad8aa617d4cd1c4db36474e8275c47d5eb70a34c32d1378cbcfce356348b12c36c8fedcdaee3c90c2c4963256857407db4
There were no cases where the source attempted to compare
`AddressPosition` by const reference, but such a comparison is valid.
This may be fixed by simplifying the comparison operator here, which
also avoids copying the value.
Found in #35713:
```
<AddressPosition>' requested here
817 | BOOST_CHECK(addr_pos1 == addr_pos2);
| ^
/bitcoin-core/bitcoin/src/addrman.h:76:10: note: candidate function not viable: 'this' argument has type 'const AddressPosition', but method is not marked const
76 | bool operator==(AddressPosition other) {
```
226e6388b7 depends: Update Qt to 6.8.4 (Hennadii Stepanov)
Pull request description:
Release notes: https://code.qt.io/cgit/qt/qtreleasenotes.git/about/qt/6.8.4/release-note.md
`depends/patches/qt/qtbase_platformsupport.patch` has been dropped as the fix was backported upstream in 5afcc64fd7.
ACKs for top commit:
fanquake:
ACK 226e6388b7
Tree-SHA512: 20f371384c1fcfd5018218ef262b10568bd009e2f1fafca9fa42788aca4683ec706b7a4e35816b884b294b5f9bdb1aefda0bad75921d9e426558cccd80f913cd
51d36dfd07 qt: Fix `-Wsfinae-incomplete` warnings when building with GCC 16.x (Hennadii Stepanov)
Pull request description:
According to the CMake documentation for [`AUTOMOC`](https://cmake.org/cmake/help/latest/prop_tgt/AUTOMOC.html), all `moc` output files that are not included in a source file are aggregated into the CMake-generated `<AUTOGEN_BUILD_DIR>/mocs_compilation.cpp`, which is added to the target's sources.
Within that single translation unit, `moc`-generated code checks the completeness of a signal or slot parameter type while it is still only forward-declared, and the type is completed later, when a subsequently included `moc_*.cpp` file pulls in the header that defines it. GCC 16.x diagnoses this pattern with the `-Wsfinae-incomplete` warning, which is enabled by default.
Including the `moc` output files at the end of the corresponding source files excludes them from `mocs_compilation.cpp`, so each one is compiled in a translation unit where the relevant types are complete.
FWIW, Qt itself uses the same approach throughout its own codebase. Also see https://www.youtube.com/watch?v=Cx_m-qVnEjo.
---
Steps to reproduce on the master branch @ 18c05d9301 on Fedora 44 (GCC 16.1.1):
```console
$ cmake --preset dev-mode
$ cmake --build build_dev_mode -t bitcoind
$ cmake --build build_dev_mode -t bitcoin-qt
[166/172] Building CXX object src/qt/CMakeFiles/bitcoinqt.dir/bitcoinqt_autogen/mocs_compilation.cpp.o
In file included from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/EWIEGA46WW/moc_bitcoingui.cpp:9,
from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/mocs_compilation.cpp:9:
/home/hebasto/dev/bitcoin-gui/src/qt/bitcoingui.h:67:7: warning: defining ‘BitcoinGUI’, which previously failed to be complete in a SFINAE context [-Wsfinae-incomplete=]
67 | class BitcoinGUI : public QMainWindow
| ^~~~~~~~~~
In file included from /usr/include/qt6/QtCore/qobject.h:19,
from /usr/include/qt6/QtWidgets/qwidget.h:10,
from /usr/include/qt6/QtWidgets/qdialog.h:9,
from /usr/include/qt6/QtWidgets/QDialog:1,
from /home/hebasto/dev/bitcoin-gui/src/qt/addressbookpage.h:8,
from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/EWIEGA46WW/moc_addressbookpage.cpp:9,
from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/mocs_compilation.cpp:2:
/usr/include/qt6/QtCore/qmetatype.h:344:64: note: here. Use ‘-Wsfinae-incomplete=2’ for a diagnostic at that point
344 | static auto check(U *) -> std::integral_constant<bool, sizeof(U) != 0>;
| ^~~~~~~~~
In file included from /home/hebasto/dev/bitcoin-gui/src/qt/paymentserver.h:35,
from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/EWIEGA46WW/moc_paymentserver.cpp:9,
from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/mocs_compilation.cpp:27:
/home/hebasto/dev/bitcoin-gui/src/qt/sendcoinsrecipient.h:15:7: warning: defining ‘SendCoinsRecipient’, which previously failed to be complete in a SFINAE context [-Wsfinae-incomplete=]
15 | class SendCoinsRecipient
| ^~~~~~~~~~~~~~~~~~
/usr/include/qt6/QtCore/qmetatype.h:344:64: note: here. Use ‘-Wsfinae-incomplete=2’ for a diagnostic at that point
344 | static auto check(U *) -> std::integral_constant<bool, sizeof(U) != 0>;
| ^~~~~~~~~
In file included from /home/hebasto/dev/bitcoin-gui/src/qt/psbtoperationsdialog.h:13,
from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/EWIEGA46WW/moc_psbtoperationsdialog.cpp:9,
from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/mocs_compilation.cpp:30:
/home/hebasto/dev/bitcoin-gui/src/qt/walletmodel.h:48:7: warning: defining ‘WalletModel’, which previously failed to be complete in a SFINAE context [-Wsfinae-incomplete=]
48 | class WalletModel : public QObject
| ^~~~~~~~~~~
/usr/include/qt6/QtCore/qmetatype.h:344:64: note: here. Use ‘-Wsfinae-incomplete=2’ for a diagnostic at that point
344 | static auto check(U *) -> std::integral_constant<bool, sizeof(U) != 0>;
| ^~~~~~~~~
In file included from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/EWIEGA46WW/moc_qvalidatedlineedit.cpp:9,
from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/mocs_compilation.cpp:32:
/home/hebasto/dev/bitcoin-gui/src/qt/qvalidatedlineedit.h:13:7: warning: defining ‘QValidatedLineEdit’, which previously failed to be complete in a SFINAE context [-Wsfinae-incomplete=]
13 | class QValidatedLineEdit : public QLineEdit
| ^~~~~~~~~~~~~~~~~~
/usr/include/qt6/QtCore/qmetatype.h:344:64: note: here. Use ‘-Wsfinae-incomplete=2’ for a diagnostic at that point
344 | static auto check(U *) -> std::integral_constant<bool, sizeof(U) != 0>;
| ^~~~~~~~~
In file included from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/EWIEGA46WW/moc_rpcconsole.cpp:9,
from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/mocs_compilation.cpp:37:
/home/hebasto/dev/bitcoin-gui/src/qt/rpcconsole.h:43:7: warning: defining ‘RPCConsole’, which previously failed to be complete in a SFINAE context [-Wsfinae-incomplete=]
43 | class RPCConsole: public QWidget
| ^~~~~~~~~~
/usr/include/qt6/QtCore/qmetatype.h:344:64: note: here. Use ‘-Wsfinae-incomplete=2’ for a diagnostic at that point
344 | static auto check(U *) -> std::integral_constant<bool, sizeof(U) != 0>;
| ^~~~~~~~~
In file included from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/EWIEGA46WW/moc_sendcoinsentry.cpp:9,
from /home/hebasto/dev/bitcoin-gui/build_dev_mode/src/qt/bitcoinqt_autogen/mocs_compilation.cpp:39:
/home/hebasto/dev/bitcoin-gui/src/qt/sendcoinsentry.h:26:7: warning: defining ‘SendCoinsEntry’, which previously failed to be complete in a SFINAE context [-Wsfinae-incomplete=]
26 | class SendCoinsEntry : public QWidget
| ^~~~~~~~~~~~~~
/usr/include/qt6/QtCore/qmetatype.h:344:64: note: here. Use ‘-Wsfinae-incomplete=2’ for a diagnostic at that point
344 | static auto check(U *) -> std::integral_constant<bool, sizeof(U) != 0>;
| ^~~~~~~~~
[172/172] Linking CXX executable bin/bitcoin-qt
```
ACKs for top commit:
maflcko:
review ACK 51d36dfd07🦅
Tree-SHA512: fe48e4925aaccb833bc065aa7f988f22482ef83d3d555e601d3955109f840b221be971c560668eac79506f567999d4e1b2a464ec3717b5cecb9a3eaaa1dbc01b
7298281ba8 bitcoin-util: replace netmagic command with getchainparams command (Anthony Towns)
Pull request description:
This is a follow-up to #35610. It replaces the `netmagic` command with a more versatile `getchainparams` command, as suggested in https://github.com/bitcoin/bitcoin/pull/35610#issuecomment-4974474640.
ACKs for top commit:
maflcko:
re-ACK 7298281ba8📓
ajtowns:
Coauthor ACK 7298281ba8
sedited:
ACK 7298281ba8
Tree-SHA512: 153e97eb8d6bc6d98d925ce9718239c6ff5d7a80b760e7f3b807fc36f78e8da8550033c31016e1d1aee11484392dd80b76f9b1496a8dc6e47825910c170a0cd1
faada35f9c fuzz: [refactor] Use 100'000 digit separator in __AFL_LOOP (MarcoFalke)
fae067ec4a fuzz: Avoid dangling prevoutfetch threads after AFL fork (MarcoFalke)
Pull request description:
Presumably fixes https://issues.oss-fuzz.com/issues/536943806
This is a bit confusing, because the issue was already fixed in commit f608a409f7, by removing the AFL forkserver.
However, OSS-Fuzz doesn't go through the AFL_LOOP, but through the AFL libFuzzer driver:
```
#0 0x7e055245baab in __pthread_clockjoin_ex /build/glibc-B3wQXB/glibc-2.31/nptl/pthread_join_common.c:89:6
#1 0x5a27e904dcdd in operator() /src/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:293:14
#2 0x5a27e904dcdd in Join<(lambda at /src/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:292:44)> /src/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_thread_arg_retval.h:75:9
#3 0x5a27e904dcdd in ___interceptor_pthread_join /src/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:292:25
#4 0x5a27e90f4044 in std::__1::thread::join()
#5 0x5a27e9366277 in ThreadPool::Stop() [bitcoin-core/src/util/threadpool.h:146](7d8137c141/src/util/threadpool.h (L146)):53
#6 0x5a27e9365db9 in ThreadPool::~ThreadPool() [bitcoin-core/src/util/threadpool.h:94](7d8137c141/src/util/threadpool.h (L94)):9
...
#32 0x5a27e95a9506 in (anonymous namespace)::ResetChainman(TestingSetup&) (.12669) [bitcoin-core/src/test/fuzz/process_messages.cpp:44](7d8137c141/src/test/fuzz/process_messages.cpp (L44)):27
#33 0x5a27e95a8c60 in process_messages_fuzz_target(std::__1::span<unsigned char const, 18446744073709551615ul>) [bitcoin-core/src/test/fuzz/process_messages.cpp:141](7d8137c141/src/test/fuzz/process_messages.cpp (L141)):9
...
#36 0x5a27e97b7190 in test_one_input(std::__1::span<unsigned char const, 18446744073709551615ul>) bitcoin-core/src/test/fuzz/fuzz.cpp:86:5
#37 0x5a27e97b7190 in LLVMFuzzerTestOneInput bitcoin-core/src/test/fuzz/fuzz.cpp:214:5
#38 0x5a27e90ada19 in LLVMFuzzerRunDriver /src/aflplusplus/utils/aflpp_driver/aflpp_driver.c:427:13
#39 0x5a27e90ad69b in main /src/aflplusplus/utils/aflpp_driver/aflpp_driver.c:323:10
#40 0x7e055223b082 in __libc_start_main /build/glibc-B3wQXB/glibc-2.31/csu/libc-start.c:308:16
#41 0x5a27e8fc602d in _start
```
So the correct fix would be to set `AFL_DRIVER_DONT_DEFER=1`. Ref: ad5304010a/utils/aflpp_driver/aflpp_driver.c (L161)
However, I don't know how to do this on OSS-Fuzz, so just drop the threads for now, because there are dedicated fuzz targets to test the multi-threaded case anyway.
ACKs for top commit:
l0rinc:
ACK faada35f9c
andrewtoth:
lgtm ACK faada35f9c
sedited:
ACK faada35f9c
Tree-SHA512: c249d7267f789084968f8510531f60fc71c9fbd6b4e574a181fd6da1af19a3cb7c03ba60c3ca70244b56ce44c602293517cdb6b0e969b4a2cc9d1afaa49ab0f8
f3f302150b ci: Put space and non-ASCII char in `BASE_BUILD_DIR` (Hennadii Stepanov)
a7e980af31 build: Quote host paths in NSIS installer template (Hennadii Stepanov)
Pull request description:
The CI scratch directory contains a space and non-ASCII symbols to test path handling (see #34614). However, the GHA workflows [override](18c05d9301/.github/actions/configure-environment/action.yml (L10)) `BASE_BUILD_DIR` to `${{ runner.temp }}/build` via the `configure-environment` action, bypassing the `$BASE_SCRATCH_DIR/build-$HOST` [default](18c05d9301/ci/test/03_test_script.sh (L109-L110)) from `03_test_script.sh`.
The first commit fixes the NSIS template, which otherwise breaks the `deploy` target when paths contain spaces.
Related to #35356.
ACKs for top commit:
maflcko:
re-ACK f3f302150b🥘
l0rinc:
code review ACK f3f302150b
Tree-SHA512: 5785dc13961d656e73759888d653967e43d6a9e1eec46a12301c65d042bc53a824f2dd9cdaa602c1bfb1afb52c4753099a705eab3a337f88ac6a68893e2de7f9
3bfdcbd7ee coins: reuse cache hasher for txid set (Lőrinc)
2beab94896 coins: use SipHash-1-3-UJ for `CCoinsMap` (Lőrinc)
7ff55cc650 bench: add fixed-width SipHash benchmarks (Lőrinc)
3aea85411f test: add SipHash-1-3-UJ coverage (Pieter Wuille)
a0ccd4ad17 crypto: add fixed-width SipHash-1-3-UJ (Pieter Wuille)
c2d7931b5c crypto: add generic SipHash-1-3-UJ (Pieter Wuille)
25bfca06d6 refactor: simplify adding SipHash-1-3-UJ (Lőrinc)
af50ba8500 test: add shared SipHash vectors (Lőrinc)
Pull request description:
**Problem:** The in-memory UTXO cache hashes `COutPoint` keys containing a 32-byte txid and a 32-bit output index.
SipHash-2-4 processes the txid as four independent 64-bit blocks, so its optimized 32-byte and 36-byte paths both take 14 SipRounds.
This also matters for hash-prefix index work such as [#35531](https://github.com/bitcoin/bitcoin/pull/35531): once a persisted key format chooses a hash function, changing it later requires reindexing.
**Fix:** Add `SipHasher13UJ`, a custom block-oriented variant combining Pieter Wuille's jumbo-block suggestion with SipHash-1-3, the reduced-round variant discussed in the [SipHash analysis](https://eprint.iacr.org/2012/351.pdf).
It provides inline `Hash` overloads for the fixed-width inputs used here.
Use a dedicated `SaltedCoinsCacheHasher` for `CCoinsMap` and `CoinsViewOverlay`'s temporary earlier-txid set, while other outpoint tables remain on SipHash-2-4.
The salted hash values vary between restarts and are never persisted or sent over the network.
**Design:** `SipHasher13UJ` accepts normal 64-bit blocks and 256-bit jumbo blocks.
For hash-table use, cryptographic hash outputs must make up all but a small bounded number of retained jumbo blocks.
The construction mixes all four limbs around one SipRound, omits byte-oriented padding, and uses an `"unpadded"` finalizer distinct from standard SipHash-1-3.
The fixed-width paths take four rounds for one `uint256` jumbo block and five when followed by one normal block.
For outpoints, the 32-bit output index is zero-extended into a normal 64-bit block.
Retained `CCoinsMap` entries identify real transaction outputs, so their keys contain computed txids.
Missing-input validation may probe arbitrary claimed prevouts, but `FetchCoin()` immediately erases their temporary entries when the backend lookup fails, so non-hash keys cannot accumulate.
The assumeutxo loader assumes snapshot txids are valid while loading and verifies the complete snapshot's content hash before activation.
Every entry in the temporary earlier-txid set is a computed transaction hash, and the set is bounded by the block's transaction count.
This construction is limited to local hash tables and is not a general-purpose or protocol SipHash replacement.
Pieter discussed the construction with [SipHash co-author Jean-Philippe Aumasson](https://github.com/bitcoin/bitcoin/pull/35215#issuecomment-4385336928), whose preliminary analysis did not find an easier collision construction and supported SipHash-1-3 for this hash-table use.
<img width="2100" height="860" alt="siphash_compare_updated" src="https://github.com/user-attachments/assets/cefec6f8-5ec0-450a-a0a2-f946de9ef36d" />
**Structure:** Shared vectors first cover the existing generic and fixed SipHash-2-4 paths in C++, the generic path in Python, and their randomized equivalence in the fuzzer.
A behavior-neutral refactor then moves the round, compression, and finalization logic into inline `SipHashState` methods; assembly inspection shows that the fixed-width paths retain their instruction counts, while the generic byte loop retains its prior code generation through a local state copy.
Three Pieter-authored commits add the generic UJ specification, fixed-width implementation, and shared correctness coverage.
Benchmarks follow that coverage, then separate commits change `CCoinsMap`'s hasher and reuse it for the temporary earlier-txid set.
**Tests:** The shared JSON supplies the same byte sequences to the generic C++ and Python SipHash-2-4 implementations, with applicable fixed-width paths checked against the same expected output.
The SipHash-2-4 rows include the 64 official vectors for inputs from 0 to 63 bytes and cases that vary input chunking.
The UJ outputs were generated by an independent implementation and are checked using normal blocks, equivalent zero-extended jumbo blocks, and applicable fixed-width `Hash` overloads.
The integer fuzzer extends these comparisons to arbitrary values and mixed normal/jumbo block encodings.
[Counting the dbcache buckets](https://gist.github.com/l0rinc/d68f56c3ed89f76f56da6632ef6f2d92) indicates the new outpoint hasher retains the uniform bucket distribution expected by `CCoinsMap`:
<img width="1200" height="750" alt="ccoinsmap-collisions" src="https://github.com/user-attachments/assets/eeedec81-acdc-4adf-a9c8-bfce089700da" />
**Benchmarks:** Fixed-width microbenchmarks compare SipHash-2-4 with SipHash-1-3-UJ for 32-byte hashes and inputs containing a 32-byte hash plus a 32-bit index.
Reported aarch64 measurements and an [independent x86_64 run](https://github.com/bitcoin/bitcoin/pull/35215#issuecomment-4400609637) show the outpoint path is about 2x faster.
<details><summary>Benchmark runner</summary>
```bash
for COMPILER in gcc clang; do \
if [ "$COMPILER" = gcc ]; then CC=gcc; CXX=g++; else CC=clang; CXX=clang++; fi; \
cmake -B "build-bench-$COMPILER" -DCMAKE_BUILD_TYPE=Release -DBUILD_BENCH=ON -DBUILD_TESTS=OFF -DBUILD_GUI=OFF -DENABLE_WALLET=OFF -DCMAKE_C_COMPILER="$CC" -DCMAKE_CXX_COMPILER="$CXX" >/dev/null 2>&1 && \
cmake --build "build-bench-$COMPILER" --target bench_bitcoin -j"$(nproc)" >/dev/null 2>&1 && \
echo "" && echo "$(date -I) | SipHash fixed-width microbench | $("$CXX" --version | head -1) | $(hostname) | $(uname -m) | $(lscpu | awk -F: '/Model name/{print $2; exit}' | xargs) | $(nproc) cores | $(free -h | awk '/^Mem:/{print $2}') RAM" && \
"build-bench-$COMPILER/bin/bench_bitcoin" -filter='SipHash.*32b|SipHash.*36b' -min-time=10000; \
done
```
</details>
A two-run GCC `-reindex-chainstate` comparison of the same `CCoinsMap` hot path through height 957,759 with `-dbcache=2000` on a Ryzen 7 3700X/SSD reduced mean wall time from 11,278 s to 10,759 s, a ~5% validation speedup.
ACKs for top commit:
achow101:
light ACK 3bfdcbd7ee
sipa:
ACK 3bfdcbd7ee (to the extent the code/ideas aren't my own)
andrewtoth:
ACK 3bfdcbd7ee
optout21:
ACK 3bfdcbd7ee
Tree-SHA512: c3c66051cb1ebdb0cddbc8b8bed2297c524842f92960531de23d9586c5bb950b302c33d06fc15c2320cbbf97273b22ec3f17fd0e7ddcc7df4a8132a47a606277
6aa5d8d948 blockencodings: fix extra transaction count (Lőrinc)
be4e64d9e4 test: characterize extra transaction miscount (Lőrinc)
Pull request description:
A short ID collision can invalidate a mempool-sourced transaction after an unrelated transaction was found in extra_txn.
Track each slot's source so extra_count is decremented only when the invalidated slot came from extra_txn. Retain the source after a collision to preserve the rule that later candidates do not refill the slot.
ACKs for top commit:
l0rinc:
retested ACK 6aa5d8d948
andrewtoth:
ACK 6aa5d8d948
sedited:
ACK 6aa5d8d948
Tree-SHA512: d4407dca7ca2b46795e52a5d611b66072d75ba966416d4ede27fa776ac0a7cbde52c8abae2daec532ec5f24d88e608622168ca066c0d1dbc133e098c8e6fe85a
The GHA workflows override `BASE_BUILD_DIR`, so the build tree no longer
lives under `BASE_SCRATCH_DIR` and its word-splitting and UTF-8 coverage
is bypassed on CI. Restore it by putting a space and a non-ASCII symbol
in the externally defined path as well.
By now, the vast majority of nodes in the network supports BIP324.
Even if the optimistic guess would turn out to be wrong for a given
node, we would just reconnect with v1.
This is better than making v1 connections with peers when both nodes support v2.
0b0785daa0 guix: split macOS and win builds (fanquake)
008a3e29c8 guix: split builds into Linux(gui) and macOS/Windows (fanquake)
a3b5dc0572 cmake: Add `GenerateWindowsInstaller` script (Hennadii Stepanov)
Pull request description:
The next change split out of #25573, which splits the Guix build into separate Linux, Linux GUI, macOS, macOS GUI and Windows and Windows GUI builds.
Closes#29914.
ACKs for top commit:
hebasto:
ACK 0b0785daa0.
Tree-SHA512: 6222024c891d1bec69bb93ae28864142d7e75aae31968b727a3f7226e0debe89f2dc6954a0c26ccfccf2ce8a28d002aa0284a778c2df6c330c5262d41057602d
5d57f2cefe test: cover unused mempool space in coins cache (woltx)
Pull request description:
This PR extends the existing unit test for `Chainstate::GetCoinsCacheSizeState()`.
`Chainstate::GetCoinsCacheSizeState()` calculates when the UTXO/coins cache is too large and should be flushed. Part of that calculation includes unused `-maxmempool` space. For example, if the mempool limit is 300 MiB but the mempool is mostly empty, some of that unused space can be counted toward the coins cache limit.
The existing `validation_flush_tests.cpp` test checks this calculation by calling:
```c++
chainstate.GetCoinsCacheSizeState(MAX_COINS_BYTES, max_mempool_size_bytes)
```
This change extends the test to also check the no-argument call:
```c++
chainstate.GetCoinsCacheSizeState()
```
That is the call used by validation code during normal operation.
The test grows the coins cache above the coins-only limit, then checks that:
- calling the explicit helper with `max_mempool_size_bytes=0` reports `CRITICAL`
- calling the normal no-argument method reports `OK`, because it includes unused mempool space
This makes sure future refactors do not accidentally drop unused mempool space from the normal cache-size calculation.
ACKs for top commit:
l0rinc:
ACK 5d57f2cefe
sedited:
ACK 5d57f2cefe
Tree-SHA512: c4131cdedd7bd48224d3323fdc1bbce668f171105939c8aa3c452080bbe22819f3910d2a5ce3f46a5a1b7eb625b63240a8c0b7c26fbd5f30927712545b0f765f
1fc9277a1c test: cover disconnect on private broadcast peer with relay=false (Bruno Garcia)
Pull request description:
Currently, there is no test case to verify the disconnection of a private broadcast connection when the peer does not support transaction relay. This PR addresses it.
Can be tested with:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 27f0a63c55..5d9a8f9c06 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3741,7 +3741,7 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
} else {
LogDebug(BCLog::PRIVBROADCAST, "Disconnecting: does not support transaction relay (connected in vain), %s",
pfrom.LogPeer());
- pfrom.fDisconnect = true;
+ pfrom.fDisconnect = false;
}
return;
}
```
ACKs for top commit:
Herb-ops:
ACK 1fc9277a1c
w0xlt:
ACK 1fc9277a1c
nebula-21:
ACK 1fc9277a1c
Tree-SHA512: 1fd7ad3a5fc690a25276a9654e297858d95607fa9c696a0a6f53223aeb3dde7f80e836d2cbf835a39487f953b04f73b4b8af0c79a2ee8f58c894259f7c9383ff
fd59d68c26 qt, test: Enable tests on macOS with `minimal` QPA plugin (Hennadii Stepanov)
c8b2aeb226 qt: Avoid implicit `NSApplication` instantiation (Hennadii Stepanov)
Pull request description:
On macOS, `test_bitcoin-qt` has skipped `AppTests`, `WalletTests`, and `AddressBookTests` under the `minimal` QPA plugin since a3197c5294 (2018, Qt 5.11), pointing to [QTBUG-49686](https://qt-project.atlassian.net/browse/QTBUG-49686).
The recent re-evaluation of the Bitcoin Core codebase showed that the Qt bug report is no longer relevant. The remaining causes were in our own code, and they are fixed in the first commit.
ACKs for top commit:
maflcko:
lgtm ACK fd59d68c26
pablomartin4btc:
tACK fd59d68c26
Tree-SHA512: 9fae09668cffe8b207736c6a202af01666a06ae1a8adbd3061717b92a692c0b65b6fe853f192cdc8793073b1c02c3f6a61350a5974397ac016ff8924a51e084a
6eca11175b lint: remove E731 Ruff ignore (will)
b52454538b lint: remove E712 Ruff ignore (will)
Pull request description:
Follows up on #34547 by dropping (and fixing) most remaining default Ruff rule ignores:
- [E712](https://docs.astral.sh/ruff/rules/true-false-comparison/) avoids noisy `== True/False` comparisons and makes boolean intent clearer.
- [E731](https://docs.astral.sh/ruff/rules/lambda-assignment/) named `def` helpers are easier to debug, trace, type-check, and extend than assigned lambdas.
Dropping the rules means these issues cannot be re-introduced.
E501 remains as this has 8909 callsites to fix, and IMO it's not clear that having an arbitrary-line length improves anything about the codebase, or for developers.
E741 remains based on review feedback that it's unnecessary to enforce.
ACKs for top commit:
stickies-v:
ACK 6eca11175b
sedited:
ACK 6eca11175b
Tree-SHA512: 763ad88b66fbe1df09c88c688865b7ebdcc4b431858776b5557eb7eec61b98795fb6946b32549f2e631648eae03a6a2882fcd2a673948e1a04d8b2ea358d83ce
`[NSApplication sharedApplication]` creates the shared application
object if it does not yet exist. When running with the `minimal` or
`offscreen` QPA plugins, which is common for testing purposes, the Cocoa
platform plugin never creates it, so these call sites were instantiating
`NSApplication` as a side effect.
Use the `NSApp` global instead and return early when it is `nil`.
d24d3cbad0 fuzz: add p2p_private_broadcast harness (frankomosh)
Pull request description:
Add a fuzz harness for `ConnectionType::PRIVATE_BROADCAST`, a privacy-preserving transaction relay mechanism whose p2p code paths had no meaningful fuzz coverage.
Current `process_message` touches it but is insufficient in exercising it. It creates `PRIVATE_BROADCAST` nodes via `ConsumeNode()`, but some structural problems prevent it from covering the relevant logic:
1. `m_tx_for_private_broadcast` is never seeded, `PushPrivateBroadcastTx` always takes the immediate disconnect path (7 accidental hits, all on lines 3559–3562). Lines 3564–3570 (the actual INV send) had 0 hits.
2. `ALL_NET_MESSAGE_TYPES` is used as the message pool. `CConnman::PushMessage` silently drops anything outside the four-type allowlist for private broadcast connections, wasting most iterations.
3. Connection types are picked randomly, hence private broadcast coverage is accidental.
To solve the issues above;
- this harness explicitly constructs nodes with `ConnectionType::PRIVATE_BROADCAST`
- seeds `m_tx_for_private_broadcast` via `InitiateTxBroadcastPrivate` before the peer connects, so `PushPrivateBroadcastTx` reaches the transaction send path
- constrains the message pool to the four types permitted by handshake or private-broadcast filter in PeerManagerImpl::ProcessMessage
- passes `{NODE_NONE}` to `InitializeNode`, matching what `PushNodeVersion` advertises for private broadcast peers.
ACKs for top commit:
instagibbs:
ACK d24d3cbad0
brunoerg:
code review ACK d24d3cbad0
andrewtoth:
ACK d24d3cbad0
Tree-SHA512: 0d2ff9a79aa87a6eb7d7efdbe03ebba545abad1d1e995dfe617e5bd8fd7a5098fd44d92fa6eac1695aa81f38e762b30ed7a7fd0db6c85aaa32bd1d068c946e38
According to the CMake documentation for `AUTOMOC`, all `moc` output
files that are not included in a source file are aggregated into the
CMake-generated `<AUTOGEN_BUILD_DIR>/mocs_compilation.cpp`, which is
added to the target's sources.
Within that single translation unit, `moc`-generated code checks the
completeness of a signal or slot parameter type while it is still only
forward-declared, and the type is completed later, when a subsequently
included `moc_*.cpp` file pulls in the header that defines it. GCC 16.x
diagnoses this pattern with the `-Wsfinae-incomplete` warning, which is
enabled by default.
Including the `moc` output files at the end of the corresponding source
files excludes them from `mocs_compilation.cpp`, so each one is
compiled in a translation unit where the relevant types are complete.
Additionally:
1. Some of the `BitcoinGUI` class's private members are gated with
`#ifdef ENABLE_WALLET` to prevent `-Wunused-private-field` warnings
when building with `-DENABLE_WALLET=OFF`.
2. `test/lint/lint-includes.py` is adjusted to allow new `#include`
statements.
The new script is parameterized by paths to executables. It can be used
to generate the installer from the install tree rather than the build
tree. This capability will be used by the Guix scripts in subsequent
commits.
Use `SaltedCoinsCacheHasher` for the temporary set of earlier txids in `CoinsViewOverlay`, and in existing overlay tests to exercise the new `Txid` overload.
Every entry is a computed transaction hash, and the set is limited to a few thousand elements per block, satisfying the SipHash-1-3-UJ jumbo-input requirements.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Use the fixed-width `SipHasher13UJ::Hash` path for `CCoinsMap`, while keeping other `SaltedOutpointHasher` users on SipHash-2-4.
The salted outputs are process-local and must not be persisted, serialized, or compared across processes.
Retained cache entries identify real transaction outputs and therefore contain computed txids.
Missing-input validation may probe arbitrary claimed prevouts, but `FetchCoin()` erases each temporary entry immediately when the backend lookup fails, so non-hash keys cannot accumulate.
The assumeutxo loader assumes snapshot txids are valid while loading and verifies the complete snapshot content hash before activation.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Benchmark 32-byte hashes and inputs consisting of a 32-byte hash plus a 32-bit index with SipHash-2-4 and SipHash-1-3-UJ.
The UJ benchmark zero-extends the index into one 64-bit normal block.
Keep all four measurements together after shared correctness coverage and before changing `CCoinsMap`'s hasher.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Add SipHash-1-3-UJ outputs to the shared vectors for sequences of 8- and 32-byte blocks.
Check generic writes and applicable fixed-width `Hash` overloads against those outputs, and fuzz their equivalence including mixed normal/jumbo encodings.
The outputs were generated by an independent implementation that Claude Opus 4.8 produced using only the `SipHasher13UJ` class comment as its prompt.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Add const `SipHasher13UJ::Hash` overloads for one `uint256` jumbo block, optionally followed by one 64-bit normal block.
Reuse the generic hasher's initialized state and define these short paths in the header because the new hasher benefits from inlining.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Add `SipHasher13UJ`, a block-oriented implementation accepting arbitrarily mixed 64-bit normal blocks and 256-bit jumbo blocks.
For hash-table use, cryptographic hash outputs must make up all but a small bounded number of retained jumbo blocks.
The generic interface serves as the executable specification for the fixed-width overloads added next.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Co-authored-by: Jean-Philippe Aumasson <jeanphilippe.aumasson@gmail.com>
Move the SipHash round, compression, and finalization operations shared by `CSipHasher` and `PresaltedSipHasher` into inline `SipHashState` methods.
This centralizes state mutation, preserves the existing byte-path code generation, and keeps the security-sensitive follow-up focused on its changed block compression, round counts, and finalizer.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Lock SipHash-2-4 behavior into shared vectors before refactoring its round and finalization code.
Store inputs as ordered hex byte blocks so `CSipHasher` and the independent Python implementation hash the same byte sequence, with applicable `PresaltedSipHasher` overloads checked against the same vectors.
Add the 64 official SipHash-2-4 vectors alongside block-partition and empty-block cases for the generic path.
Move randomized generic/fixed comparisons to the integer fuzzer.
SipHash-1-3-UJ coverage can add expected outputs for compatible 8- and 32-byte block sequences.
The Python test reads a build-tree copy so functional-test staging behaves consistently when files are symlinked or copied.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
6ee05c4b18 test: wallet: BnB incomplete result on attempt-limit success (Bruno Garcia)
Pull request description:
BnB can return a valid selection before exhausting the search tree, then hit `TOTAL_TRIES` while continuing to look for a better one. Add a unit test for that path using a known exhaustion fixture plus an exact-match coin, and assert the result is marked incomplete via `GetAlgoCompleted() == false`.
It kills the following mutant:
```diff
diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp
index 8d69957c30..e4d07415f3 100644
--- a/src/wallet/coinselection.cpp
+++ b/src/wallet/coinselection.cpp
@@ -212,7 +212,7 @@ util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool
if (curr_try >= TOTAL_TRIES) {
// Solution is not guaranteed to be optimal if `curr_try` hit TOTAL_TRIES
- result.SetAlgoCompleted(false);
+ result.SetAlgoCompleted(true);
break;
}
```
ACKs for top commit:
yashbhutwala:
tACK 6ee05c4b18
achow101:
ACK 6ee05c4b18
murchandamus:
ACK 6ee05c4b18
Tree-SHA512: 32d0cc6da7586abdd195b87459da0a6ebbdcf8115bef41f5e625864bfa9ce8231a58f8d4b61708279ebeaef372e4f3b5fb18eab28a01dad53f043ba083330b3d
BnB can return a valid selection before exhausting
the search tree, then hit TOTAL_TRIES while continuing
to look for a better one. Add a unit test for that path
using a known exhaustion fixture plus an exact-match
coin, and assert the result is marked incomplete via
GetAlgoCompleted() == false.
Co-authored-by: Murch <murch@murch.one>
A short ID collision can invalidate a mempool-sourced transaction after an unrelated transaction was found in extra_txn.
Track each slot's source so extra_count is decremented only when the invalidated slot came from extra_txn. Mark collided slots explicitly so later candidates do not refill them.
3e8e21b2ef txgraph: avoid moving primitive members (Lőrinc)
d9f94aa882 rpc: avoid moving RPC enum types (Lőrinc)
b67baed4e7 coins: avoid moving `COutPoint` values (Lőrinc)
Pull request description:
Inspired by https://github.com/bitcoin/bitcoin/pull/34320#discussion_r2751764873.
**Problem:** A few code paths use rvalue references or `std::move()` for types where moving provides no benefit.
`EmplaceCoinInternalDANGER` took `COutPoint&&`, forcing callers to pass trivially copyable outpoints as rvalues even though the cache stores its own key.
Some call sites also use `std::move()` on enum and primitive values, where it only adds noise.
> [!NOTE]
> `CheckTriviallyCopyableMove` remains `false` since `std::move()` on trivially copyable types can still be useful as intent documentation, for example to signal that a value should not be reused after a call.
**Fix:** Take trivially copyable arguments by const reference where the callee only needs to store its own copy, and pass existing values directly at the call sites.
Also remove `std::move()` from enum and primitive assignments where it has no semantic effect.
ACKs for top commit:
maflcko:
review ACK 3e8e21b2ef 🖇
hodlinator:
re-ACK 3e8e21b2ef
andrewtoth:
ACK 3e8e21b2ef
hebasto:
ACK 3e8e21b2ef, I have reviewed the code and it looks OK.
Tree-SHA512: cbe55b13290ae261bba359dc6e5a3bbdfb7ae9d31bdf8e0da2eef65a0df776e4081ceeac3c82731a631bda656a86b3789651fa1f4d87875cee1dc96351bdfd7c
