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.
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
Serialization parameters should be embedded into the object being
serialized rather than passed as a separate argument. This works here
because only serialization is performed and no new object needs to be
constructed.
Replace assigned lambdas with local functions so Ruff can enforce E731.
For platform-specific immutable file cleanup, store the command as data
instead of creating conditional callbacks.
dab7f2c984 test: cover -externalip/onlynet interaction in functional test (will)
657a5aa3f3 test: cover -externalip bypassing -onlynet (will)
8c87e32bd3 net: let -externalip bypass -onlynet (will)
f4af02e827 net: add an add_even_if_unreachable argument to AddLocal (will)
Pull request description:
`-onlynet` is documented to restrict automatic outbound connections, but it also currently prevents `-externalip` addresses from being advertised when their network is not in the `-onlynet` set. This happens because `AddLocal()` rejects addresses outside `g_reachable_nets`, regardless of whether the address was explicitly configured by the user.
Previous attempts to fix this (#24835 and #25690) removed the `g_reachable_nets` check from `AddLocal()`.
This PR instead adds an explicit `add_even_if_unreachable` argument to `AddLocal()`. The argument defaults to `false`, and is set to `true` only when adding addresses from `-externalip`.
As a result, explicitly configured `-externalip` addresses can still be advertised even when their network is excluded by `-onlynet`, while discovered, mapped, bound, Tor control, and I2P SAM addresses continue to use the existing reachable-network filter.
This keeps the fix scoped to `-externalip` and addresses the concern raised in #25690:
> I think it might also be weird for a user to activate -onlynet and keep on advertising their clearnet address to the network
The branch adds unit coverage for `AddLocal()` and functional coverage in `p2p_addr_selfannouncement.py` for `-onlynet=ipv4 -externalip=<onion>`.
Fixes: #25336Fixes: #25669
ACKs for top commit:
achow101:
ACK dab7f2c984
mzumsande:
re-ACK dab7f2c984
w0xlt:
ACK dab7f2c984
Tree-SHA512: a4ac9334b85da8b6902d3850e21d3a1c9d7dce70bcb79182448c8d5684e24462cd6e440385af7aa4420d9582e4dff9dc9e827ca7a6da0363fff2d3c531784d9b
4c9de7d5b3 external_signer: validate fingerprint from enumerate response (Kyle 🐆)
Pull request description:
`enumeratesigners` takes the `fingerprint` field from the external signer's `enumerate` output and stores it without checking it. That value is later handed back to the signer command as `--fingerprint <value>` (e.g. in `displayaddress`), so a malformed value propagates unchecked.
A master key fingerprint is 4 bytes, i.e. 8 hex characters. This adds a check that the reported fingerprint is exactly 8 hex characters and throws a clear error otherwise. A functional test covers empty, wrong-length, and non-hex inputs.
ACKs for top commit:
Sjors:
utACK 4c9de7d5b3
achow101:
ACK 4c9de7d5b3
sedited:
ACK 4c9de7d5b3
Tree-SHA512: 7c3303b24e234e13a4c20c0b93552145b9ccffc29d1bae42ce8a2faf548377f051e52f8ffb3924679065b27d15fc7bf3859e5ae32a2bb185738cc29bc0ade486
72db4accbf coins: drop stale cursor null checks (Lőrinc)
3d2f2d8de0 coins: pass UTXO stats view by reference (Lőrinc)
35aedb2823 coins: drop cursor from base view (Lőrinc)
c6fbe2f66c coins: pass DB view to cursor users (Lőrinc)
Pull request description:
**Problem:** `CCoinsView::Cursor()` makes cursor iteration look like a generic coins view operation, but cursor iteration is only supported by the DB-backed coins view.
The cache override only threw, and the `coins_view` fuzz target only asserted that deterministic unsupported throw path.
**Fix:** Make cursor iteration a `CCoinsViewDB` operation.
Cursor users now take the DB-backed view directly, `CCoinsView` no longer exposes `Cursor()`, and the fuzz target keeps DB-backed cursor coverage while dropping the unsupported cache throw probe.
The UTXO stats path is also tightened to pass the non-null DB view by reference, and stale null handling for DB cursors is removed.
This was extracted from review discussion in https://github.com/bitcoin/bitcoin/pull/35295#discussion_r3420576781 and extended based on https://github.com/bitcoin/bitcoin/pull/35562#issuecomment-4746585893.
ACKs for top commit:
achow101:
ACK 72db4accbf
sedited:
Re-ACK 72db4accbf
w0xlt:
ACK 72db4accbf
andrewtoth:
ACK 72db4accbf
Tree-SHA512: 12a81330a6ec1b91a7e4393f3761ea9ed4702ecb24312f1defa5a9a079a396ce921fc52f74fe296e5ac7ab20d5b5a8a84e858c96847f333c58b7fa9de9e8143e
3ae3a94f2b wallet: avoid call bumpfeediscount with negative values (Pol Espinasa)
Pull request description:
in https://github.com/bitcoin/bitcoin/issues/34232 dergoegge reported an assertion fail in `SetBumpFeeDiscount`.
**Context**
The bump-fee discount in the savings that we can have when multiple UTXOs that we selected for our transaction share a common unconfirmed ancestor transaction.
We know we can have some savings because we first calculate the `summed_bump_fees` and then compare to the `combined_bump_fee`.
For context:
- `bump_fees`: Extra fees that the new transaction must pay to contribute to get his ancestor confirmed.
- `summed_bump_fees`: The sum of each input ancestor `bump_fee`. If we have two inputs with unconfirmed ancestors A and B and both have a `bump_fee = 100` then `summed_bump_fees = 200`.
- `combined_bump_fee`: Is the total `bump_fees` summed of all inputs, but taking into account shared ancestors. If A and B share the transaction ancestor then `combined_bump_fee = 100` not `200` as the transaction must be bumped only once.
If `summed_bumpfees` > `combined_bump_fee` we are overestimating the `bump_fee` as we are counting multiple times the same ancestors so we can discount it using `SetBumpFeeDiscount(summed_bump_fees - combined_bump_fees)`.
**Problem**
To calculate `summed_bump_fees` and `combined_bump_fee` we use two different fresh MiniMiner snapshots of the mempool. Because they are called in different moments the two snapshots of the mempool might be different. An artificial feerate decrease of an ancestor using `prioritizesettransaction` can make `combined_bump_fee > summed_bump_fees` creating a negative discount. This cause calling `SetBumpFeeDiscount` with a negative vaule triggering an assertion `discount >= 0`.
**Fix**
This PR fixes it by ensuring not only that a discount exist but also that is greater the 0.
**Test**
It is hard to manually trigger this race condition. dergoegge coded a patch and test to trigger it that can be used to test the fix.
5320e2fd21
ACKs for top commit:
achow101:
ACK 3ae3a94f2b
pablomartin4btc:
ACK 3ae3a94f2b
Tree-SHA512: e76693eb66c4883ed3ef5edf1baa972657b0d532487803706312d486b4d4a4997f5dcbc64781a1a21b7b5628b06fcc97a6633382509061734d0f5615e851bef1
Remove the `Mutex` from the `coins_view` and `coinscache_sim`
pool startup helpers. Fuzz targets are entered sequentially within a
process and parallel fuzzing uses separate processes/forks, which each
have their own copy of the global thread pool. Therefore, a mutex to
prevent two in-process callers from racing to start the pool isn't needed.
fab8eeed82 fuzz: clang-format LIMITED_WHILE (MarcoFalke)
fa0d777ce2 fuzz: Clang-format LIMITED_WHILE like while (MarcoFalke)
fa1a9bde5a fuzz: Remove unused workaround after fix in libmultiprocess byte-span serializer (MarcoFalke)
fa55385ab3 fuzz: Use LIMITED_WHILE over for-loop with consumed size integral (MarcoFalke)
6d5f753921 Squashed 'src/ipc/libmultiprocess/' changes from 28e056576a..e8de5c7b68 (MarcoFalke)
Pull request description:
Includes several changes, to first update the subtree. Then, modify the fuzz test to address review comments:
* https://github.com/bitcoin/bitcoin/pull/35118#discussion_r3506566815
* https://github.com/bitcoin/bitcoin/pull/35118#discussion_r3523175145
ACKs for top commit:
ryanofsky:
Code review ACK fab8eeed82. Just fuzz test clang-format cleanups added since last review, which seem nice
Tree-SHA512: 0836628f8ee54adf02571025456211a74f63d05058b72280b10111ecfbb93d30945f4a48f30cc790774de4e2b489907313e86b0fa6729f3480c64850c94848b4
6667dc4eec kernel: expose scriptSig for btck_TransactionInput (Peter Zafonte)
e6de3a2d3c kernel: expose witness stack for btck_TransactionInput (Peter Zafonte)
Pull request description:
Silent payments scanning needs the public key from every input. For SegWit inputs it is in the witness stack. For P2PKH inputs it is in scriptSig. Without these new functions, callers must deserialize the raw transaction themselves to reach that data, which is difficult and error-prone.
Introduces a `btck_WitnessStack` type and adds the following functions:
**Witness stack:**
`btck_transaction_input_get_witness_stack`: returns a non-owning `const btck_WitnessStack* `view
`btck_witness_stack_count_items `: item count
`btck_witness_stack_get_item_at `: single item by index via btck_WriteBytes
`btck_witness_stack_copy` / `btck_witness_stack_destroy`: lifecycle for owned copies
**scriptSig:**
`btck_transaction_input_get_script_sig`: full scriptSig via btck_WriteBytes
All functions are exposed in the C++ wrapper via `WitnessStackView`, `WitnessStack`, and `WitnessStackApi` , and `GetScriptSig()`.
ACKs for top commit:
sedited:
ACK 6667dc4eec
musaHaruna:
ACK [6667dc](6667dc4eec)
stickies-v:
ACK 6667dc4eec
Tree-SHA512: b5e9d32ec87a5f5a9fea5652ed69737eae1d1f9cfb777544b96d703bbd057c114e0b7afae4e9e9d09b8b546694b14fb923b747c04a8f0211ab6caa015d06967d
