2777300c68 fuzz: Implement connect_block harness (Robin David)
40add915be test: Add reset to CuckooCache (Eugene Siegel)
Pull request description:
Adds a fuzz target that directly calls `ConnectBlock` with `fJustCheck` set to true, so it hits block/transaction validation without writing undo data or updating the chainstate.
This PR is essentially https://github.com/bitcoin/bitcoin/pull/34651 with some minor tweaks and style cleanups. Additional validation harnesses (e.g. https://github.com/bitcoin/bitcoin/pull/34895) could build on this test's setup.
ACKs for top commit:
Crypt-iQ:
ACK 2777300c68
nervana21:
tACK 2777300c68
Tree-SHA512: e2dc74154a6e29e0f3eaec9caeeec53d64bcc96adb0d1739281da97712dd931c3937eaf71f977bfc9c9330f26e35b3633f72788143b76e24e17c37b0a4258ba4
Add a method that clears and resets CuckooCache, intended for
use in tests only. Without this, fuzz tests may reuse the cache
across iterations, resulting in instability.
576a0ebb53 fix: UB sanitizer in mempool estimator logging (rustaceanrob)
Pull request description:
The following is failing in CI, when a block has `m_height` of 64 bit max:
```
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /home/runner/work/_temp/src/policy/fees/mempool_estimator.cpp:210:62
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x1,0x0,0x0,0x0,0x3,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x1,0x0,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x26,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x2f,0x0,0x3,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x7a,0x3f,0x3f,0x0,0x0,0x2f,0x0,0x3,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x7a,0x3f,0x3f,0x3f,0xff,0xff,0xff,0xff,0xff,0x18,0x0,0x0,0x85,0x3f,0xff,0xff,0xff,0xff,0xff,0x18,0x0,0x0,0x85,0xd6,0x1,0x0,0x86,0x0,0x0,0x0,0x2a,0x0,0xff,0xff,0xff,
\001\000\000\000\003\377\377\377\377\377\377\377\377\001\000\377\377\377\377\377\377\377\377\000\000&\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000/\000\003\002\002\002\002\002\002\002\000\000\000\000\000\000\000z??\000\000/\000\003\002\002\002\002\002\002\002\000\000\000\000\000\000\000z???\377\377\377\377\377\030\000\000\205?\377\377\377\377\377\030\000\000\205\326\001\000\206\000\000\000*\000\377\377\377
artifact_prefix='./'; Test unit written to ./crash-b4333d1fe3993fe8610b86654e385682c23050b9
Base64: AQAAAAP//////////wEA//////////8AACYAAAAAAAAAAAAAAAAAAAAAAAAvAAMCAgICAgICAAAAAAAAAHo/PwAALwADAgICAgICAgAAAAAAAAB6Pz8///////8YAACFP///////GAAAhdYBAIYAAAAqAP///w==
⚠️ Failure generated from target with exit code 1: ['/home/runner/work/_temp/build_ ₿🧪_/bin/fuzz', '-runs=1', PosixPath('/home/runner/work/_temp/ci/scratch_ ₿🧪_/qa-assets/fuzz_corpora/policy_estimator_io')]
Check if using libFuzzer ... True
Command '['docker', 'exec', '--env', 'DANGER_RUN_CI_ON_HOST=1', '8100bf684275e706787e07f8ab94431926ba5184562c52bce95c932210f6f38f', '/home/runner/work/_temp/ci/test/03_test_script.sh']' returned non-zero exit status 1.
```
ACKs for top commit:
maflcko:
lgtm ACK 576a0ebb53
marcofleon:
ACK 576a0ebb53
jeanpablojp:
tACK 576a0ebb53
Tree-SHA512: a7533e68a95b2f0200abdcf08ae72a7f3654db03cffe642ed39b0a5aa48d932a8b07ee4e4477b591a488b5358dd19fe85a03015642b663fd41a771b3090bc8a3
fa72de78a9 util: Set Univalue to null after read failure (MarcoFalke)
fa7786592d test: Add UniValue failed read test (MarcoFalke)
Pull request description:
Currently, `UniValue::read()` may leave the value in a dirty/corrupt state after a read failure.
This is perfectly fine, because all production code-paths check the read return value and exit early.
However, it seems nicer and safer to discard the dirty and corrupt state. So do that here.
This refactor doesn't change any production behavior. However, it fixes a fuzz failure in the `rpc` target, which was recently reworked in commit fa895bb77a. Later, adding new fuzz inputs (e.g. `fuzz_corpora/rpc/fa1b0eeaa948a091f022c1ff2d0002a3fa6a631f `) and commit 747cff8424 made it hit this invalid UniValue code path.
ACKs for top commit:
rustaceanrob:
ACK fa72de78a9
hodlinator:
re-ACK fa72de78a9
jeanpablojp:
tACK fa72de78a9
Sjors:
ACK fa72de78a9
l0rinc:
code review ACK fa72de78a9
Tree-SHA512: 6c0597a5ab558dc7d22e1742e89078e07a59a87185228114db4381c3381e19b668b95e42cb7ff80288fa56fb15ea1e0e181f59ba3ceaea8a2a3bff12899c7821
The following is failing in CI, when a block has `m_height` of 64 bit
max:
```
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /home/runner/work/_temp/src/policy/fees/mempool_estimator.cpp:210:62
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x1,0x0,0x0,0x0,0x3,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x1,0x0,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x26,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x2f,0x0,0x3,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x7a,0x3f,0x3f,0x0,0x0,0x2f,0x0,0x3,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x7a,0x3f,0x3f,0x3f,0xff,0xff,0xff,0xff,0xff,0x18,0x0,0x0,0x85,0x3f,0xff,0xff,0xff,0xff,0xff,0x18,0x0,0x0,0x85,0xd6,0x1,0x0,0x86,0x0,0x0,0x0,0x2a,0x0,0xff,0xff,0xff,
\001\000\000\000\003\377\377\377\377\377\377\377\377\001\000\377\377\377\377\377\377\377\377\000\000&\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000/\000\003\002\002\002\002\002\002\002\000\000\000\000\000\000\000z??\000\000/\000\003\002\002\002\002\002\002\002\000\000\000\000\000\000\000z???\377\377\377\377\377\030\000\000\205?\377\377\377\377\377\030\000\000\205\326\001\000\206\000\000\000*\000\377\377\377
artifact_prefix='./'; Test unit written to ./crash-b4333d1fe3993fe8610b86654e385682c23050b9
Base64: AQAAAAP//////////wEA//////////8AACYAAAAAAAAAAAAAAAAAAAAAAAAvAAMCAgICAgICAAAAAAAAAHo/PwAALwADAgICAgICAgAAAAAAAAB6Pz8///////8YAACFP///////GAAAhdYBAIYAAAAqAP///w==
⚠️ Failure generated from target with exit code 1: ['/home/runner/work/_temp/build_ ₿🧪_/bin/fuzz', '-runs=1', PosixPath('/home/runner/work/_temp/ci/scratch_ ₿🧪_/qa-assets/fuzz_corpora/policy_estimator_io')]
Check if using libFuzzer ... True
Command '['docker', 'exec', '--env', 'DANGER_RUN_CI_ON_HOST=1', '8100bf684275e706787e07f8ab94431926ba5184562c52bce95c932210f6f38f', '/home/runner/work/_temp/ci/test/03_test_script.sh']' returned non-zero exit status 1.
```
80eaa6cabf bugfix: give TxDownloadManager its own RNG (Greg Sanders)
Pull request description:
TxDownloadManagerImpl retains a reference to PeerManagerImpl::m_rng,
which is non-thread-safe and guarded by g_msgproc_mutex.
BlockConnected runs on the validation background thread while holding
only m_tx_download_mutex. Reconsidering an orphan with multiple
announcers could therefore use m_rng concurrently with message
processing.
Regression introduced in #35986
Added a regression test on second commit, can remove it from the PR if deemed superfluous.
This is a Project Loupe find.
ACKs for top commit:
maflcko:
review ACK 80eaa6cabf🐓
hodlinator:
ACK 80eaa6cabf
sedited:
ACK 80eaa6cabf
Tree-SHA512: 2dbc4a9298bfa1375dc364ead4b1ec74c2ebe54fb7c311180fa06fc32240406be2979a2dd6ae0e7a23b099ddcd63f5c76c126d84a1405fc7eacd337eb009dd88
5e0d7a286a refactor: Drastically narrow scope of http_bitcoin namespace and rename it to bitcoin_http (Hodlinator)
8f9fd8698a refactor: Make HTTPRemoteClient fields private (Hodlinator)
d72f67fd6c refactor: Expose additional HTTPRemoteClient fields through accessors (Hodlinator)
10bbae302f refactor: Expose HTTPRemoteClient fields to tests through methods (Hodlinator)
5b06d90831 refactor: Replace HTTPServer::MaybeDispatchRequestsFromClient() with HTTPRemoteClient::TryReadRequest() (Hodlinator)
a1183c02aa refactor: Extract Send() and Receive() into HTTPRemoteClient from HTTPServer (Hodlinator)
6d9b61d4f8 refactor: Extract HTTPRemoteClient::MaybeDisconnect() from HTTPServer::DisconnectClients() (Hodlinator)
6fec8d6914 refactor: Make HTTPRequest fields private (Hodlinator)
b8cd77237b refactor: Make HTTPRequest::GetHeader() return saner optional type (Hodlinator)
e5be0dc35e refactor: Make HTTPResponse a struct since all fields are public (Hodlinator)
Pull request description:
The new HTTP server implementation in v32 has `HTTPServer` reaching into and modifying fields of `HTTPRemoteClient` and `HTTPRequest`. This PR encapsulates field data of the latter 2 types which enforces invariants and reduces cognitive load[^1]. Exposing data through accessor methods also implies adding lock annotations.
Commits:
* Makes `HTTPResponse` a struct since it is used that way. (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3336757663) [^2]
* `HTTPRequest`:
* Saner return type for `GetHeader()` (old type was mirroring the now removed libevent-wrapper and made later commits ugly).
* Make fields private.
* Simplifies boolean logic in `HTTPServer::DisconnectClients()`. (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3336757663)
* Extraction of `HTTPServer` functions into `HTTPRemoteClient`:
Refactors `HTTPRemoteClient` to be more self-contained rather than having `HTTPServer` reach into the fields of other objects. (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3339543447, https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3339543447)
* Severely narrows `http_bitcoin` namespace and renames it to `bitcoin_http` (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3264510816)
Follow-up to #35182.
[^1]: Core Guidelines: C.9: Minimize exposure of members - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c9-minimize-exposure-of-members
[^2]: Core Guidelines: C.2: Use class if the class has an invariant; use struct if the data members can vary independently - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c2-use-class-if-the-class-has-an-invariant-use-struct-if-the-data-members-can-vary-independently
ACKs for top commit:
achow101:
ACK 5e0d7a286a
janb84:
ACK 5e0d7a286a
winterrdog:
tACK 5e0d7a286a
Tree-SHA512: e1c5aa067538e31247ca74923e451038c90750ccc941ae16711dd976c8cd750bd1afaee6e4378aeee91440f7727955d9bfb32aa25a0a745613d0d771a674ebc8
747cff8424 rpc: avoid quadratic output lookups (Lőrinc)
Pull request description:
**Problem:** Transaction-creation RPCs currently take quadratic time to parse outputs.
An authenticated RPC client can therefore tie up a worker with a large request.
`sendmany` also holds the wallet lock while parsing, delaying other operations on the same wallet.
**Fix:** Parse transaction outputs in linear time by reading corresponding keys and values by index instead of looking up each value by key.
**Reproducer:** Run `time build/bin/test_bitcoin --run_test=rpc_tests/parse_outputs` before and after the fix:
<details>
<summary>parse_outputs test in `rpc_tests.cpp`</summary>
```cpp
BOOST_AUTO_TEST_CASE(parse_outputs)
{
constexpr size_t OUTPUT_COUNT{10'000};
UniValue outputs{UniValue::VOBJ};
for (size_t i{0}; i < OUTPUT_COUNT; ++i) {
auto destination{EncodeDestination(WitnessV0ScriptHash{CScript{} << i})};
outputs.pushKVEnd(destination, ValueFromAmount(i + 1));
}
const auto parsed_outputs{ParseOutputs(outputs)};
BOOST_REQUIRE_EQUAL(parsed_outputs.size(), OUTPUT_COUNT);
for (size_t i{OUTPUT_COUNT}; i > 0; --i) {
std::pair expected{CTxDestination{WitnessV0ScriptHash{CScript{} << (i - 1)}}, static_cast<CAmount>(i)};
BOOST_CHECK(parsed_outputs[i - 1] == expected);
}
}
```
</details>
E.g. on my M4 Max with `debug` build:
```python
Before ████████████████████ 1.80 s
After █████▒░░░░░░░░░░░░░░ 0.50 s -72%
```
Related to #35889
ACKs for top commit:
achow101:
ACK 747cff8424
jonatack:
ACK 747cff8424
jeanpablojp:
tACK 747cff8424
hodlinator:
ACK 747cff8424
Tree-SHA512: 154c9f583f6e7f4154882aeb1ae11c40b327d0ef04147e12a0fee749494ae95314cdfc56baad78723f3383f225d752654be0ae9ed7458b1c73f7a6a80922e3ef
b8a8893bf2 qa: Lower `-rpcmaxconnections` in `interface_http.py` test (Hennadii Stepanov)
6f4109b448 qa: Reduce `-maxconnections` in the functional test framework (Hennadii Stepanov)
Pull request description:
This PR follows up on bitcoin/bitcoin#35730 and fixes a [regression](https://github.com/bitcoin/bitcoin/pull/35730#issuecomment-5409592163) on NetBSD.
Since bitcoin/bitcoin#35730 the HTTP server reserves file descriptors for its listen sockets and for `-rpcmaxconnections` connected clients (16 by default), so `min_required_fds` in `init.cpp` grew.
On select()-based platforms `available_fds` is capped at FD_SETSIZE, which is 256 on NetBSD. The previous value of 94 no longer fits and every node in the test suite started up with a warning, which the framework treats as unexpected stderr and fails on.
Recompute the value with the new accounting (256 - 179 = 77) and update the comment to match the current variable names in `init.cpp`.
ACKs for top commit:
achow101:
ACK b8a8893bf2
hodlinator:
re-ACK b8a8893bf2
winterrdog:
re-ACK b8a8893bf2
Tree-SHA512: d6200cc334b98148d71992b1d085ca8f72ba68d330b26d7ba373a0917cca56a444b8d89b0c0827e2a56242893b268e9f581bffc8a631a2ff20cc51f10db3255e
7e973cce52 depends: Make tarball creation from local directory reproducible (Hennadii Stepanov)
Pull request description:
This guarantees `$(package)_sha256_hash` reproducibility regardless of the default behavior of `$(build_TAR)` and fixes [caching](https://github.com/bitcoin/bitcoin/pull/36006#issuecomment-5328809116) for the `native_libmultiprocess` package.
Steps to reproduce the issue using the master branch @ 2a9e35d293:
```console
$ mkdir a && cd a && git init
$ git remote add origin https://github.com/bitcoin/bitcoin.git
$ git fetch --depth 1 origin 2a9e35d293
$ git checkout FETCH_HEAD
$ cd depends
$ gmake print-native_libmultiprocess_sha256_hash # Hash A. Compare with Hash B.
native_libmultiprocess_sha256_hash=7dd817bfc0ee23c408299907aff13fefb0bd3a54ec66dc14ea15b0cc38c3d9ce
$ cd ../../ && sleep 2
$ mkdir b && cd b && git init
$ git remote add origin https://github.com/bitcoin/bitcoin.git
$ git fetch --depth 1 origin 2a9e35d293
$ git checkout FETCH_HEAD
$ cd depends
$ gmake print-native_libmultiprocess_sha256_hash # Hash B. Compare with Hash A.
native_libmultiprocess_sha256_hash=34d6f79560c0ff7a4f46bd6bfb4693076546b41f071b6dbf879da45ac8384688
```
ACKs for top commit:
fanquake:
ACK 7e973cce52
willcl-ark:
ACK 7e973cce52
Tree-SHA512: f939cd1b2aca04eaa0f8426858bae3657ee9625f915834980caacfbf80843b952451f4d1c29e27179e533de3e10d31688593e4c25ed32e62392bfbaf9e58dd12
On some systems, such as NetBSD, the non-default
`-rpcmaxconnections=128` is too high, so bitcoind refuses to start:
```
Error: Not enough file descriptors available. 256 available, 290 required.
```
The test only needs a value above the default of 16. Use 64 and lower
`-maxconnections` in that case so the total fits in 256.
Since bitcoin/bitcoin#35730 the HTTP server reserves file descriptors
for its listen sockets and for `-rpcmaxconnections` connected clients
(16 by default), so `min_required_fds` in init.cpp grew.
On select()-based platforms `available_fds` is capped at FD_SETSIZE,
which is 256 on NetBSD. The previous value of 94 no longer fits and
every node in the test suite started up with a warning, which the
framework treats as unexpected stderr and fails on.
Recompute the value with the new accounting (256 - 179 = 77) and
update the comment to match the current variable names in init.cpp.
TxDownloadManagerImpl retains a reference to PeerManagerImpl::m_rng,
which is non-thread-safe and guarded by g_msgproc_mutex.
BlockConnected runs on the validation background thread while holding
only m_tx_download_mutex. Reconsidering an orphan with multiple
announcers could therefore use m_rng concurrently with message
processing.
Regression introduced in 9cc7dc50bd
http_bitcoin was mostly used during #35182 to distinguish from http_libevent counterpart:
- The http_libevent namespace was introduced around the legacy code in 89c54ae4cb.
- The http_bitcoin namespace was introduced in 68b5d289d1 and extended in subsequent commits.
- The http_libevent namespace together with code it contained was removed in 8c1eea0777.
bitcoin_http is a better name as it is Bitcoin Core's implementation of the HTTP protocol, not HTTP protocol's implementation of bitcoin 402 payment required codes or anything like that.
The namespace only remains for a few constants and a type which don't have HTTP in their names.
e4d80e7001 test: close the loop after the network thread has completed (Vasil Dimov)
29fba5ddbb test: close the listeners before terminating the event loop (Vasil Dimov)
Pull request description:
Whenever a test creates a new `P2PInterface` object a new listener is
created inside `NetworkThread.create_listen_server()` by calling
`cls.network_event_loop.create_server()`.
These listeners are never closed which might result in:
```
2026-06-10T22:13:35.3934880Z Task was destroyed but it is pending!
2026-06-10T22:13:35.3936020Z task: <Task pending name='Task-54' coro=<BaseSelectorEventLoop._accept_connection2() done, defined at /opt/homebrew/Cellar/python@3.14/3.14.5/Frameworks/Python.framework/Versions/3.14/lib/python3.14/asyncio/selector_events.py:217> wait_for=<Future finished result=None>>
```
when the event loop is closed.
Fix that by closing the listeners.
Fixes: https://github.com/bitcoin/bitcoin/issues/35508
ACKs for top commit:
andrewtoth:
ACK e4d80e7001
sedited:
ACK e4d80e7001
Tree-SHA512: b93d06526b4eb31ac445a1a0e379e5ec947661f8ea29f2e07ac88b9e4760b0cc5348638b320ab8d60735f34fc163fcdd18eba42b20e0a7726a98a5136433bd64
1cb416397b psbt: avoid duplicate taproot leaf script keys when merging (Shuvam Pandey)
Pull request description:
Follow-up to #35665, which fixed the same combiner defect for `PSBT_GLOBAL_XPUB`. thomasbuilds
and winterrdog asked for this one as its own PR when I reported it there.
`m_tap_scripts` maps a leaf script to a set of control blocks, but is serialized as one record
per control block, keyed by the control block (`SerializeToVector(s, PSBT_IN_TAP_LEAF_SCRIPT,
std::span{control_block})`). `PSBTInput::Merge` unions it by the map key, so two PSBTs that map
the same control block to different leaf scripts merge into an input that serializes the `0x15`
key twice. Duplicate keys make a PSBT invalid, so it is the same `combinepsbt` then
`decodepsbt` failure as the xpub case, at the input level. Present since #22558 (v24.0).
Both decode on their own, and differ only in the leaf script the control block maps to, `OP_1`
against `OP_1 OP_1`:
```
$ A=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAIhXAUJKbdMGgSVS3i0tgNel6XgeKWg8o7JbVR7/ums6AOsACUcAAAA==
$ B=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAIhXAUJKbdMGgSVS3i0tgNel6XgeKWg8o7JbVR7/ums6AOsADUVHAAAA=
$ bitcoin-cli -regtest decodepsbt "$(bitcoin-cli -regtest combinepsbt "[\"$A\",\"$B\"]")"
error code: -22
error message:
TX decode failed Duplicate Key, input key "15c050929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0" already provided: unspecified iostream_category error
```
winterrdog reproduced it on the #35665 thread with another pair.
Merge the records rather than the map entries, keeping the leaf script already there. BIP 174
lets the combiner "pick arbitrarily when conflicts occur", and unknown and proprietary records
already resolve that way. Refusing to combine is the BIP's other option, but that would fail
`combinepsbt` on input it accepts today.
Merging by map key drops records as well. `std::map::insert` leaves existing keys alone, so
when both PSBTs carry the same leaf script with different control blocks, the incoming set was
dropped. Those keys do not conflict, so merging per record keeps them.
The control blocks already present are collected once per merge rather than searched for per
incoming record, which would be quadratic in the size of the two PSBTs `combinepsbt` takes from
the caller.
Since this is the second field with this shape I checked the rest. `m_xpubs` (#35665) and
`m_tap_scripts` are the only two whose record key comes from the value, so two map entries can
serialize the same key. `partial_sigs` is keyed by `CKeyID` and serialized under the pubkey,
but the pubkey determines the `CKeyID`, so those records stay distinct. The others key the
record by the map key, `m_proprietary` included, and `PSBTOutput` has no such field.
The test fails on master on both counts, and covers the merges that do not conflict as well.
I found this with a local assertion in the psbt fuzz target that a combined PSBT must
roundtrip. That assertion can go in a follow-up.
Tested:
```
./build/bin/test_bitcoin --run_test=psbt_tests
./build/bin/test_bitcoin --run_test=psbt_wallet_tests
./build/test/functional/test_runner.py rpc_psbt.py rpc_rawtransaction.py wallet_taproot.py wallet_signer.py feature_taproot.py wallet_basic.py
```
ACKs for top commit:
achow101:
ACK 1cb416397b
winterrdog:
re-ACK 1cb416397b
Tree-SHA512: 2599beffe701ba9b672e8dcc3d43844f3853ceeb3d86fc53798a428d3288aa8edd2e42f338b32a1046fa558d6cc5cfa5d55b3a0aaf960278b0f3002158381623
436921eb46 test: check joinpsbts preserves global xpubs and proprietary fields (Thomas)
011094b282 rpc: preserve global xpubs and proprietary fields in joinpsbts (Thomas)
Pull request description:
`joinpsbts` collects the global xpubs of all the joined PSBTs into `merged_psbt`, but returns a separately constructed `shuffled_psbt` into which only the inputs, outputs, and unknown fields are copied. The collected `PSBT_GLOBAL_XPUB` records are silently dropped, and `PSBT_GLOBAL_PROPRIETARY` records are not collected at all.
The xpub collection was added in #17034, which was written against a `joinpsbts` that still returned `merged_psbt`, but was merged after #16512 had introduced the `shuffled_psbt` rebuild, so the collected xpubs have never reached the result.
Shuffle the inputs and outputs of `merged_psbt` in place instead of rebuilding a new PSBT, so that all global data is preserved, and union the global proprietary records in the merge loop, matching the `combinepsbt` behavior from #34893.
ACKs for top commit:
jpk68:
ACK 436921eb46
achow101:
ACK 436921eb46
winterrdog:
tACK 436921eb46
Tree-SHA512: d9de34c25aecc29b6b4fb80d6584fa919cc5ff9b7ef2f4d8ce35c4043fe7638fefb8af10448f2cd14021f5d25e149f0efc8798c5b8c9bc8b5582c6152010e891
e2ab8ae551 wallet: spkm: Only notify CanGetAddressesChanged on change (David Gumberg)
0892f16f91 refactor: moveonly: Pair CanGetAddressesChanged notifications with desc range. (David Gumberg)
e6adae3db2 wallet: `NotifyCanGetAddressesChanged` when advancing `next_index` (David Gumberg)
Pull request description:
Even though `TopUp()` notifies, advancing `next_index` after can deplete available addresses, so make sure to notify any time it's changed.
This would manifest as users seeing a clickable `Receive` button in the GUI when in fact no address can be generated in some edge cases, e.g. when a user has a watch only wallet with a hardened derivation path and runs out of keys.
This feels like it's begging for:
1) a refactor to make it impossible to modify `next_index` or `range_end` without firing `CanGetAddressesChanged`
2) a test
I banged my head against the keyboard for a bit but I couldn't get either of these to fall out, I also tried massaging a few clankers into doing it but I couldn't get any results that seemed reasonable to me, still seems like a worthwhile fix so opening PR anyway.
I also included a moveonly commit to pair code that can change the result of `CanGetAddresses()` with the notification firing
ACKs for top commit:
achow101:
ACK e2ab8ae551
polespinasa:
ACK e2ab8ae551
furszy:
utACK e2ab8ae551
Tree-SHA512: 5bb00d1ef4909a3e55535d283e5995df75e4288a151647f4a368b2086e2f2f4140693f43cae4f72727eafb49c9050aea8604cd6ddddc8646f7ac47b1357ed287
d837bb38a4 contrib/init: fix unused variables in openrc script (jpk68)
Pull request description:
- Makes it so that `${BITCOIND_BIN}` is actually used as `command=`, rather than the hardcoded `/usr/bin/bitcoind`.
- Passes `BITCOIND_GROUP` to `start-stop-daemon`, so that the daemon process itself runs under it.
ACKs for top commit:
jeanpablojp:
utACK d837bb38a4
thomasbuilds:
ACK d837bb38
winterrdog:
utACK d837bb38a4
Tree-SHA512: 78c237224d65cc47ade606df4808fbf4ca70109d95301c35d1b336eead1b25138a83ce6f2314775b8a11a6745af2220af9027640530b9edb7e0167ccea081b6b
9e115edd39 test: Remove `BOOST_CHECK_CLOSE` in favor of exact comparison (rustaceanrob)
Pull request description:
`max_cache` is known ahead of time in this test as a `size_t` of `10000`, and each of these calculations should be known ahead of time (500.0, 9500.0). This test can truncate the double and assert exact equality rather than use a tolerance. Found in #35713 whereby this is the only use of this macro in the unit tests. IMO it is appropriate to tighten this test and remove the macro.
ACKs for top commit:
maflcko:
lgtm ACK 9e115edd39
josibake:
ACK 9e115edd39
Tree-SHA512: 738f05650bd1426e8e29e94955685e8ad3cd62e57f1c65b7af045161e4ca5af64bbd1f0cfed8da427a1f778620e3c8f7dfb835881f8de169806096213056e863
7ea36e985a test: preserve index crash test state (Lőrinc)
5aa15df60c test: expose missing index crash checkpoint (Lőrinc)
Pull request description:
**Problem:** #35847 moved the unclean-shutdown test into the shared base index tests, but it checked only that each index could reopen and start background sync.
Both checks also pass when the index reopens at height 0, so they do not verify that a height-100 checkpoint was saved before the simulated crash and reloaded afterward.
**Fix:** The first commit records the existing false positive by asserting that each index reopens at height 0 before background sync.
The second commit establishes a durable checkpoint at height 100, drains its setup notification, and changes the same assertion to the pre-crash height.
ACKs for top commit:
jeanpablojp:
tACK 7ea36e985a
mzumsande:
ACK 7ea36e985a
Tree-SHA512: 0dca2bdd978c5df4acbb01692bb2058e74efa70da3d7191687628075a680a57848ddda8087629b5d743d9a1648d7dc849fda9ff487252136a0c91cdfda33ba32
fad1e6bf23 util: refactor: Remove deprecated SetMockTime(i64) alias (MarcoFalke)
faf87c3535 test: refactor: Use FakeNodeClock over manual/global SetMockTime (MarcoFalke)
Pull request description:
The deprecated test-only alias is only used in a few places and required in none.
In fact, it is incorrectly used in two unit tests, so first fixup those, and then remove it.
ACKs for top commit:
rustaceanrob:
ACK fad1e6bf23
Tree-SHA512: 1fa49e363bf8ccad07d61a77d3bc55c84724cd4cf034756b534cdb56614f334c0009d62dbdb08a940ddf46e0143ec6ecd6b4608baa2f8f581f56a6eef0f0abb8
`max_cache` is known ahead of time in this test as a `size_t` of
`10000`, and each of these calculations should be known ahead of time
(500.0, 9500.0). This test can truncate the double and assert exact
equality rather than use a tolerance.
Using SetMockTime in tests is problematic, because it often requires
verbose calls to
`SetMockTime(GetTime<std::chrono::seconds>() + offset)`.
Also, it requires manual `SetMockTime(0);` at the end.
Fix both issues by using FakeNodeClock.
b42f7fade0 descriptor: don't prepend key origins twice (Shuvam Pandey)
7b15e2cb44 descriptor: fix duplicate check for hardened keys (Shuvam Pandey)
Pull request description:
Fixes#34273.
Importing a descriptor that uses the same `musig()` participants twice in one
tapleaf, with different musig subderivations, fails with
`is not sane: contains duplicate public keys`. It only fails when one of the
participants is a private key on a hardened path. The all-xpub version of the
same descriptor imports fine. That's what gave it away.
The duplicate check (`KeyCompare`) resolves each key expression to a pubkey and
compares the results. It does this at index 0, and the old code used an empty
signing provider. With that empty provider, a `musig()` expression can't resolve
when one of its participants is on a hardened path, because deriving that
participant needs its private key, so the whole aggregate key comes back empty.
Two different musig expressions both came back empty, so the check treated them
as duplicates. The fix derives against the signing provider populated during
parsing, which holds the private keys, and only compares the expression strings
when neither side resolves. 151henry151 had suggested looking at the empty
signing provider on the issue.
scgbckbone found a second, separate bug in the same descriptors. When another
expression that reuses those participants is handled in the same expansion, its
participant origin in the PSBT is added twice, so `m/86h/1h/0h` becomes
`m/86h/1h/0h/86h/1h/0h` in both the input and output Taproot BIP32 derivation
maps. `OriginPubkeyProvider::GetPubKey()` now derives into a temporary provider,
merges it, and writes the corrected origin once, so a later expression can't
prepend the same origin again.
Tested:
```
./build/bin/test_bitcoin --run_test=descriptor_tests
./build/bin/test_bitcoin --run_test=miniscript_tests
./build/bin/test_bitcoin --run_test=bip328_tests
./build/bin/test_bitcoin --run_test=psbt_wallet_tests
./build/test/functional/test_runner.py wallet_musig.py --jobs=1
```
ACKs for top commit:
achow101:
ACK b42f7fade0
scgbckbone:
ACK b42f7fade0
Tree-SHA512: ab36caa6bc484fa1fc3289c79e9a3d713278d82f80de478e53e1bdbe645037e07776ac798eba085733abc139c11a9dbf0d3f49d3c0eee9632e4ddf33d2242f92
m_tap_scripts maps a leaf script to a set of control blocks, but is serialized
one record per control block, keyed by the control block. PSBTInput::Merge
unions it by the map key, so two PSBTs that map the same control block to
different leaf scripts merge into an input serializing the 0x15 key twice.
Duplicate keys are invalid, so combinepsbt hands back a PSBT that can no longer
be decoded. Present since #22558 (v24.0).
Merge the records instead of the map entries, keeping the leaf script already
there, as BIP 174 lets the Combiner pick arbitrarily when conflicts occur. The
control blocks already present are collected once rather than searched for per
incoming record, which would be quadratic in the size of the PSBTs.
Control blocks under a leaf script that both PSBTs carry are now kept as well,
where the map level union dropped them.
The test covers conflicting and non-conflicting merges, including an incoming
leaf script whose control blocks only partly conflict, so records that do not
conflict are not dropped alongside those that do.
bd4b1524ea init: do not count file descriptors for HTTPServer if -server=0 (Matthew Zipkin)
b08662060d init: account for maximum file descriptors needed by HTTP (Matthew Zipkin)
cc2acebefb http: configure simultaneous connection limit with -rpcmaxconnections (Matthew Zipkin)
b3d6d2d1a7 http: limit connected clients to 16 (Matthew Zipkin)
86651d8197 scripted-diff: Rename nUserBind, nBind, nMaxConnections to snake_case (Matthew Zipkin)
Pull request description:
Introduces a new configuration option `-rpcmaxconnections` with default value `16`. This is used to limit the number of simultaneous `HTTPClient` connected to the `HTTPServer`. When the limit is reached, new pending connections remain queued in the kernel's socket buffer. Those connections have complete TCP handshakes with the kernel but do not occupy any application memory.
The previous libevent-based HTTP server had no limit on connections but it did have a limit on the kernel socket queue:
e7ff4ef2b4/http.c (L3510)
```c
if (listen(fd, 128) == -1) {
```
The current HTTP server, like the p2p server, uses a platform constant here:
b6becf3534/src/httpserver.cpp (L743)
(on my macOS `SOMAXCONN` is `128` but on my Debian machine it's `4096`)
The default of 16 was chosen as a reasonable upper bound for single-user RPC use cases. Systems designed to handle more simultaneous HTTP connections than this (previously relying on the absence of a limit) can adjust the setting.
## File descriptors
Because of the connection limit, we can now account for the maximum number of file descriptors needed by the HTTP server. This addresses several issues (#11368#11322 maybe #27732) that could have been fixed by a PR waiting in vain for a libevent release (#27731).
## Bonus performance improvement
The new limit is managed in a loop that drains the kernel's socket queue with `accept()`. All pending connections from the queue (up to the limit) are processed in one single call to `SocketHandlerListening()`. The previous code would only accept one connection from the queue on each I/O loop tick, with a `SELECT_TIMEOUT` (50ms) sleep between each.
ACKs for top commit:
fjahr:
tACK bd4b1524ea
janb84:
ACK bd4b1524ea
winterrdog:
tested ACK bd4b1524ea
hodlinator:
Concept ACK bd4b1524ea
willcl-ark:
ACK bd4b1524ea
Tree-SHA512: 2ef7a96da4d7037c7343ec0ea03fda5bb55d10c2a071fce4929141297515923b203d3d338dbcb6599849768f52aa3c9da509fb5d1d6f7c574a1d2034ea2a9e74
5be248341a bugfix: compare real chunk weight against block weight limit (ismaelsadeeq)
fc98790869 test: `TestChunkBlockLimits` uses incorrect weight for comparison (ismaelsadeeq)
Pull request description:
Partially fixes#35596
When assembling a block template, `BlockAssembler::addChunks()` adds chunks of transactions until the block is close to being full. For each chunk, `TestChunkBlockLimits()` checks both the weight and the sigop-cost limits before the chunk is included.
The weight check compared the chunk's **sigops-adjusted** weight against `block_max_weight`:
```cpp
if (nBlockWeight + chunk_feerate.size >= m_options.block_max_weight) {
return false;
}
```
Whereas `nBlockWeight` accumulates the actual chunk weight.
A chunk whose sigop-adjusted weight exceeds the actual weight can be wrongly skipped even though the block sigop limit is enforced independently on the next line, and that could pass. Those chunks pay higher fees, so this could potentially cause miners to needlessly forfeit some fees revenue.
This PR fixes this by passing the chunk's real weight (sum of `GetTxWeight()`, accumulated in the same loop that already sums sigop cost) to `TestChunkBlockLimits()`. The separate sigop-cost check is unchanged.
- The first commit adds `TestSigOpsAdjustedWeightChunkLimit`: it builds one sigop-dense transaction sized to fit by real weight but not by adjusted weight, and asserts that the tx is skipped and only the coinbase is mined.
- The second commit applies the fix and flips the assertion to show the transaction is now included.
ACKs for top commit:
pablomartin4btc:
Code Review ACK 5be248341a
sedited:
ACK 5be248341a
Tree-SHA512: b3fe9bfaa6d83d713d0243c0fc0d0fb8e68e1060bf6d606e43d9a52bd1ec07e42c561a1ba3426f180903726826c4a664bb131ddc5160354a96e3454d538fbf6c
73a94b4545 psbt: avoid aborting on invalid MuSig2 derivations (Lőrinc)
e3d1e75a51 test: characterize MuSig2 derivation aborts (Lőrinc)
Pull request description:
**Problem:** A PSBT may contain MuSig2 derivation metadata with a hardened child index or a path that derives to a different key.
The hardened index aborts during public derivation, while the mismatched key aborts at the result assertion.
`analyzepsbt`, `finalizepsbt`, and `descriptorprocesspsbt` all reach this code without a wallet.
Even the read-only `analyzepsbt` can force a co-signer service to restart its node after unexpected input.
**Fix:** Return failure when a MuSig2 derivation path contains a hardened child index, and skip only the current aggregate when the path derives to a different key so another matching aggregate can still be tried.
This follows [#35154](https://github.com/bitcoin/bitcoin/pull/35154), with the related contributions credited in the commits.
ACKs for top commit:
jeanpablojp:
ACK 73a94b4545
achow101:
ACK 73a94b4545
andrewtoth:
ACK 73a94b4545
Tree-SHA512: d8e28c5a4184154a4427c644ce62423cbcccdc3d82a6293f36fe99055fa04714598bc92c43b526fbcc7c99b231140669c2d0f1b853999d7dc33f949564c90504
Flush the chainstate at the current tip and drain its notification before registering any index.
This lets each end-of-sync `Commit()` persist the pre-crash height and keeps the setup callback out of the simulated crash window.
Replace the TODO-marked `0` expectation with the captured tip height.
The check runs before background sync, so rebuilding cannot hide a missing checkpoint.
`index_unclean_shutdown` previously checked only that each index could reopen and start background sync after the simulated crash.
An empty index at height 0 satisfies both checks, so the test could pass without preserving any pre-crash checkpoint.
Assert the current reopened height before background sync to make the false positive explicit.
