b3ff9c4d68 iwyu: Fix warnings in `src/interfaces` and treat them as errors (Hennadii Stepanov)
d564b0255f iwyu: Add temporary mapping to work around upstream bug (Hennadii Stepanov)
Pull request description:
This PR continues the ongoing effort to enforce IWYU warnings.
See [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu).
ACKs for top commit:
maflcko:
review ACK b3ff9c4d68 🖋
Tree-SHA512: 9359854c7b6e6ec93022947e78ef57fc68728326df874371028997a550fd1b6ef08e4b09271163b9d5c921c9e01deadf92b0eb5ae3f620d797e4f9bdea8f4727
d5e64b01e1 doc: note -blocknotify is not run during IBD/reindex in help text (Guillermo Fernandes)
Pull request description:
Adds a note to the `-blocknotify` help output that it is not executed for blocks connected during initial block download or reindexing (it only fires once the node reaches the post-init sync state). Suggested by sedited in review of #35518.
ACKs for top commit:
sedited:
ACK d5e64b01e1
Tree-SHA512: 06b4a3c6354689e9846b5af6c210c3bb360b917477ac9506814c0238a1afb78392c4b6a6bbd51a4b9d98c841a1bc86686e90072155ce9a4f5e8f875cfe288c0c
558e26e66e test: cover OP_SUCCESSx bypassing the initial stack element size limit (ViniciusCestarii)
Pull request description:
BIP-342 specifies that the initial stack resource checks happen after OP_SUCCESSx processing, and explicitly notes the checks "can be bypassed using OP_SUCCESSx". Core implements this correctly, but there are currently no tests covering this behavior. This means a consensus-breaking change to the ordering could pass the test suite undetected. Verified this on local commit 68d24d7430, which mutates to incorrectly implement the order and CI still turns green.
Add a new test at feature_taproot.py to cover OP_SUCCESSx bypassing the initial stack element size limit.
Verified that the new test catches the mutant: f8f42a13a8.
ACKs for top commit:
instagibbs:
ACK 558e26e66e
Tree-SHA512: 66d7bbbf286bf7e5c5762704e8c0f835c6a8026d7d604263e9debcd7e71df80506c0238b8a15cda3d4ba245d9c2bc46e79a1986d66583e571e208c8dfbe66156
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
CDBWrapper::Read() errors have no test coverage or documentation.
Currently, the function can return false when deserialization fails
(indistinguishable from a missing key) and throw dbwrapper_error
on an internal database error.
This commit introduces tests that pin both behaviors so we can work
on improvements in the next commits without worrying about introducing
a behavior change.
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
