6d387af562 psbt: remove write-only global xpub tracking set (Thomas)
3b7051c7e3 test: check combinepsbt with conflicting global xpub origins (Thomas)
7c632c0e2a psbt: avoid duplicate global xpub keys when merging (Thomas)
Pull request description:
Global xpubs are stored in a map of key origin to set of xpubs, while the serialization writes one record per xpub, keyed by the xpub. `Merge` unions the map origin-by-origin, so when the combined PSBTs provide different key origins for the same xpub, the result serializes the same `PSBT_GLOBAL_XPUB` key twice. BIP 174 declares PSBTs with duplicate keys invalid and the deserializer rejects them, so `combinepsbt` returns a PSBT that no RPC can parse again. This affects all releases since the merge loop was added in #17034 (v23.0).
<details><summary>Reproduction on master</summary>
The PSBTs share the unsigned transaction and xpub, and differ only in the master fingerprint of the global xpub record (`00000000` vs `11111111`):
```
$ A=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAD/////AQAAAAAAAAAAAAAAAABPAQQ1h88AAAAAAAAAAACHPf+BwC9SViP9H+UWfqw6VaBJ3j0xS7Qu4if/7TfVCAM5o2ATMBWX2u9B++WToCzFE9C1VSfsLfEFDi6P9JyFwgQAAAAAAAAA
$ B=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAD/////AQAAAAAAAAAAAAAAAABPAQQ1h88AAAAAAAAAAACHPf+BwC9SViP9H+UWfqw6VaBJ3j0xS7Qu4if/7TfVCAM5o2ATMBWX2u9B++WToCzFE9C1VSfsLfEFDi6P9JyFwgQRERERAAAA
$ bitcoin-cli -regtest decodepsbt "$(bitcoin-cli -regtest combinepsbt "[\"$A\",\"$B\"]")"
error code: -22
error message:
TX decode failed Duplicate Key, global key "01043587cf00...9c85c2" already provided: iostream error
```
</details>
Deduplicate by xpub when merging, keeping the origin that is already present: BIP 174 lets the Combiner "pick arbitrarily when conflicts occur", and conflicting unknown and proprietary records are already resolved the same way. The logic is shared between `combinepsbt` and `joinpsbts` through a new `MergeGlobalXPubs` helper. The second commit adds a test that fails on master with the error above, and the last commit removes the `global_xpubs` tracking set in `Unserialize`, write-only since the generic duplicate key check introduced in #21283 (1e2d146b47) replaced the explicit one.
Note: the xpub loop in `joinpsbts` currently has no observable effect, since the collected xpubs never reach the returned PSBT. My #35516 fixes that, so this PR should land first: on its own, #35516 would make the same duplicate key issue reachable through `joinpsbts`, while with the shared helper in place it never becomes reachable. I will rebase #35516 on top afterwards.
ACKs for top commit:
Bicaru20:
tACK 6d387af562.
achow101:
ACK 6d387af562
winterrdog:
tACK 6d387af562
Tree-SHA512: e2a9e02617eeec22a9240d7cf9386ee880a5f3639b143df7de4d8ea3e7b808f8c123f0b0410ff4a22e9a564bd86b2335a5c4aa3b2281af47d111484a6f1fd108
cf36df070b Wallet: Check crypter return values (benthecarman)
b76afff274 Wallet: Use unsigned KDF iteration count (benthecarman)
Pull request description:
CMasterKey::nDeriveIterations values are deserialized from wallet files
as unsigned 32-bit integers, but key derivation narrowed the count to a
signed int. A count above INT_MAX became negative in the conversion, and
the derivation loop counter then overflowed, which is undefined
behavior.
Keep the count unsigned through the derivation path to match the
serialized type, and add tests for zero and normal counts.
Also check key-derivation calibration failures and validate calculated
iteration counts before conversion. Keep the output master key unchanged
until derivation and encryption succeed, and mark fallible crypter
methods as [[nodiscard]].
ACKs for top commit:
l0rinc:
code review ACK cf36df070b
achow101:
ACK cf36df070b
Tree-SHA512: 95d5db2655fef8ca499af7da0f0258b4bee90975468286cb88e424257c4c5bf36407d2b2816b538e2ac3227e2a3a75c8d775211c65fe3ec6d12189da0b05fba1
777aee77d1 refactor: deduplicate keypath element parsing (pythcoiner)
7d8fddfba2 refactor: define BIP32_HARDENED and BIP32_UNHARDENED constants (pythcoiner)
Pull request description:
The codebase used raw `0x80000000` (and implicit `0`) as the bip32 hardened / unhardened flag.
`ParseHDKeypath` and `ParseKeyPathNum` were two separate parsers for BIP32 keypath elements, #32784 aligned their rules (both accept ' and h as hardened marker and reject indexes > 0x7FFFFFFF), but the parsing logic itself was still duplicated.
This PR:
- Define `BIP32_HARDENED_FLAG` / `BIP32_UNHARDENED_FLAG` constants to replace magic `0x80000000` and `0` literals.
- Add `ParseKeyPathElement` as bip32 parsing util and use it consistantly in `ParseHDKeyPath` and the descriptor keypath parser.
ACKs for top commit:
Sjors:
ACK 777aee77d1
achow101:
ACK 777aee77d1
Tree-SHA512: fb096eef82bb5a90baa7de41f5562b935665ee4b64c41586cf89e06c5633be44063a35a89d01a1432c8e63222d94bd06840bafc2a837f252013e317de0f5837a
465bca734e contrib: reject divergent verify-commits history (Lőrinc)
b3d1dca338 contrib: fail on verify-commits ancestry errors (Lőrinc)
Pull request description:
**Problem:** `verify-commits.py` checks a Git commit's history for trusted signatures and tree hashes back to configured roots.
The documented workflow runs this check after fetching a commit and before checkout, proceeding only when the script succeeds.
A commit that is an ancestor of a configured root is intentionally accepted without checking earlier history.
The script also takes this success path after Git errors or for divergent commits, even though neither establishes that relationship.
**Fix:** Require Git to prove the ancestor relationship before taking this success path.
**Reproducers:** Each commit can be validated manually.
<details><summary>Manual reproducer: Git error</summary>
Run this on `master` and at this PR's head:
```bash
contrib/verify-commits/verify-commits.py 0000000000000000000000000000000000000000 && echo ❌ || echo ✅
```
`master` exits successfully without verifying the missing commit, while the PR head rejects the Git error.
</details>
<details><summary>Manual reproducer: divergent history</summary>
On `master` and at this PR's head, create an unreferenced sibling of the trusted root and run the verifier:
```bash
root=$(head -n1 contrib/verify-commits/trusted-git-root)
divergent_commit=$(git commit-tree "$root^{tree}" -p "$root^" -m 'divergent commit')
contrib/verify-commits/verify-commits.py "$divergent_commit" && echo ❌ || echo ✅
```
`master` exits successfully without verifying the sibling commit, while the PR head rejects divergent history.
</details>
This issue was also found and disclosed responsibly by the Red Team 🟥.
ACKs for top commit:
151henry151:
tACK 465bca734e
jeanpablojp:
tACK 465bca734e
achow101:
ACK 465bca734e
sedited:
ACK 465bca734e
maflcko:
review ACK 465bca734e🥜
Tree-SHA512: 72b8cd9902d881e59a1d99fda8e5d511806826fa27c05a2c21a7d2eb62b2a5a0b1b6bdc67e8d19d57f9171278f4858fd019eb7890b960df0475ba4713683f0ac
2c16efbb7b psbt: Remove unused IsNull() methods (nebula-21)
Pull request description:
This PR removes the `IsNull()` methods from `PartiallySignedTransaction`, `PSBTInput`, and `PSBTOutput`, along with their calls from the fuzz target.
This methods have no production callers, their only callers are the fuzz target. As such, keeping these methods seems not useful.
The motivation for this PR came from jeanpablojp's comment on [#35848](https://github.com/bitcoin/bitcoin/pull/35848#issuecomment-5274013825), added him as coauthor.
ACKs for top commit:
maflcko:
review ACK 2c16efbb7b🥑
vicjuma:
ACK 2c16efbb7b
sedited:
ACK 2c16efbb7b
Tree-SHA512: 129933ae9803a2d053e340ee2a85efd1e5d9e5e38833fac0a0a9cbd467fec3157087959c464a8eeedb04ea99b1e5d7eba5a2fafde2e78a4fabcd247fac855477
4e5327bc98 fuzz: refactor: scope fake clocks to target phases (Hao Xu)
e33410d888 fuzz: document arbitrary mocktimes (Hao Xu)
Pull request description:
Follow-up to #35482 (https://github.com/bitcoin/bitcoin/pull/35482#discussion_r3612852792), addressing a remaining issue with the lifetime of the mock node clock.
This replaces the process-wide `FakeNodeClock` accessor with scoped clocks in the affected fuzz target initialization and input-processing phases, following the existing `FakeSteadyClock` pattern. The active clock is passed to `ResetChainmanAndMempool()` by reference.
Tested the affected fuzz targets with `-runs=1`:
- `cmpctblock`
- `process_message`
- `process_messages`
- `utxo_snapshot`
- `utxo_snapshot_invalid`
ACKs for top commit:
maflcko:
review ACK 4e5327bc98🚉
nervana21:
re-ACK 4e5327bc98
Tree-SHA512: 7763bb2a06e3f33bcae3ad7b43f6d30a231e39197e8d274eadd496da1194fc0178b27cf016b451f36309d9277ce414c85daf734604c85a9deb3803b26b968e6a
8454fb2bd7 test: sync funding block before isolating nodes (shaurya2k06)
Pull request description:
Fixes#35967
test_alternate_witness_tx mines the taproot funding output on node0 with
sync_fun=self.no_op and immediately disconnects. node1 later includes the
script-path spend via generateblock. If the funding block has not reached
node1, that call fails with bad-txns-inputs-missingorspent.
Drop the no_op so generate() uses the default sync_all before the partition.
Later generate* calls keep no_op because the nodes are then disconnected.
Seen twice this week in hebasto bitcoin-core-nightly NetBSD jobs:
https://github.com/hebasto/bitcoin-core-nightly/actions/runs/31350308484/job/93339698854https://github.com/hebasto/bitcoin-core-nightly/actions/runs/31765546925/job/94660585799
The modified test is test/functional/wallet_listtransactions.py. I ran it
locally three times with build/test/functional/wallet_listtransactions.py.
ACKs for top commit:
achow101:
ACK 8454fb2bd7
furszy:
utACK 8454fb2bd7
Tree-SHA512: 6b8fdcdc9ce9c57c2939caff34850e88450864c909fe226ba9b6e02ffcefa3625623589b8ecd2f09b9ca16bcaec3a64bd07642ff2c47ae6e8d250de478d34b53
1156ce6754 test: Tighten `Coin` equality and add debug output (rustaceanrob)
Pull request description:
If the `==` operator on two `Coin` fails, the developer should also see the conditions under which it failed. All that is required is adding a `<<` operator, moving the `==` out of the namespace, and switching `==` sites to `BOOST_TEST`.
Here we also tighten what it means for a coin to be "equal."
This is a pre-requiste for https://github.com/bitcoin/bitcoin/pull/35713 but seems to be a benefit on its own.
ACKs for top commit:
josibake:
reACK 1156ce6754
maflcko:
review ACK 1156ce6754🔋
Tree-SHA512: de5c612998518371ded3d25abdf1c96640e33d9961902dc4765a7e8f5088d8698a66ad63bc0a9822ec2b53e41e72dc95f78196822429af30b2ec29baa31c1ed1
ec61a1af62 depends: Hash included makefiles in package checksums (Hennadii Stepanov)
Pull request description:
This PR fixes an issue where modifications in included files (e.g. `packages/qt_details.mk`) do not trigger a rebuild of the parent packages (`native_qt` and `qt`).
This addresses an oversight in 248613eb3e from https://github.com/bitcoin/bitcoin/pull/30997.
### Reproduction
On master (@ 595504a432), modifying the included makefile does not change the build ID:
```
$ cd depends
$ gmake print-qt_build_id HOST=x86_64-w64-mingw32
qt_build_id=b2ce790473c
$ gmake print-native_qt_build_id HOST=x86_64-w64-mingw32
native_qt_build_id=70e1e5164c5
$ echo "" >> packages/qt_details.mk
$ gmake print-qt_build_id HOST=x86_64-w64-mingw32
qt_build_id=b2ce790473c
$ gmake print-native_qt_build_id HOST=x86_64-w64-mingw32
native_qt_build_id=70e1e5164c5
```
### With this patch
The checksum calculation now parses `include` directives and adds those files to the hash. The IDs now update correctly:
```
$ cd depends
$ gmake print-qt_build_id HOST=x86_64-w64-mingw32
qt_build_id=9a6ebf79cb3
$ gmake print-native_qt_build_id HOST=x86_64-w64-mingw32
native_qt_build_id=6ad78a3f644
$ echo "" >> packages/qt_details.mk
$ gmake print-qt_build_id HOST=x86_64-w64-mingw32
qt_build_id=ca820665c52
$ gmake print-native_qt_build_id HOST=x86_64-w64-mingw32
native_qt_build_id=082e4cb2364
```
ACKs for top commit:
sedited:
ACK ec61a1af62
Tree-SHA512: 9425d606dcf003ef9342560a1d0def3d591279ed261a6af0cfb2bf7b2fb1864bf7937877a1119e4c694e6c74a46074eb5e84e6198a90f4d1cd49010088077f92
de9b436ba3 depends: Switch from multilib to platform-specific toolchains (Hennadii Stepanov)
Pull request description:
Using the multilib GCC toolchain, as currently documented in [`depends/README.md`](4c1906a500/depends/README.md), has several issues, such as:
1. The [`g++-multilib`](https://packages.ubuntu.com/noble/g++-multilib) package conflicts with platform-specific cross-compiler packages. This means it is not possible to cross compile for `i686` and other platforms using the same set of installed packages.
2. The [`g++-multilib`](https://packages.ubuntu.com/noble/g++-multilib) package is not available for `arm64`:
```sh
$ sudo apt install g++-multilib
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
E: Unable to locate package g++-multilib
```
3. Managing the multilib GCC toolchain requires additional code in both depends and Guix scripts.
This PR addresses all the issues mentioned above by switching from multilib to platform-specific toolchains.
Also see https://github.com/bitcoin/bitcoin/pull/22456.
---
Here are examples of building for different scenarions:
- Linux, `x86_64` or `arm64`, building with depends natively:
```sh
$ gmake -C depends -j $(nproc)
$ cmake -B build --toolchain depends/$(./depends/config.sub $(./depends/config.guess))/toolchain.cmake
$ cmake --build build -j $(nproc)
```
- Linux, `x86_64` or `arm64`, cross compiling for `i686-pc-linux-gnu`:
```sh
$ sudo apt install g++-i686-linux-gnu binutils-i686-linux-gnu
$ export HOST=i686-linux-gnu
$ gmake -C depends -j $(nproc)
$ cmake -B build-${HOST} --toolchain depends/${HOST}/toolchain.cmake
$ cmake --build build-${HOST} -j $(nproc)
```
- Linux, `x86_64`, cross compiling for `arm64`:
```sh
$ sudo apt install g++-aarch64-linux-gnu binutils-aarch64-linux-gnu
$ export HOST=aarch64-linux-gnu
$ gmake -C depends -j $(nproc)
$ cmake -B build-${HOST} --toolchain depends/${HOST}/toolchain.cmake
$ cmake --build build-${HOST} -j $(nproc)
```
- Linux, `arm64`, cross compiling for `x86_64`:
```sh
$ sudo apt install g++-x86-64-linux-gnu binutils-x86-64-linux-gnu
$ export HOST=x86_64-linux-gnu
$ gmake -C depends -j $(nproc)
$ cmake -B build-${HOST} --toolchain depends/${HOST}/toolchain.cmake
$ cmake --build build-${HOST} -j $(nproc)
```
ACKs for top commit:
fanquake:
ACK de9b436ba3
BrandonOdiwuor:
ACK de9b436ba3
Tree-SHA512: 453b4744974cdf56d6edfdbe93bb11e3bae3f9bc9cd99b9c57aee74e65fcdd3ac011a1dcc19f485ea3be427f4e9c6c6b0d704881369f719620f0cb299123e561
Avoid exposing a process-wide FakeNodeClock accessor from the test
utility module. Initialize separate scoped clocks for target setup and
input processing, and pass the active clock to ResetChainmanAndMempool
by reference.
ResetChainmanAndMempool sets each scoped clock to the selected chain's
genesis time. Avoid hard-coding the mainnet genesis timestamp when
constructing these clocks, because the targets use REGTEST parameters
and the value is overwritten during reset.
Initialize each clock from the fuzz harness's existing mock time until
ResetChainmanAndMempool sets the REGTEST genesis time.
This commit does not change behavior.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: nervana21 <205626986+nervana21@users.noreply.github.com>
If the `==` operator on two `Coin` fails, the developer should also see
the conditions under which it failed. All that is required is adding a
`<<` operator, moving the `==` out of the namespace, and switching `==`
sites to `BOOST_TEST`.
Here we also tighten what it means for a coin to be "equal."
This is a pre-requiste for #35713 but seems to be a benefit on its own.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
ec5d19665b wallet: WalletBatch->WriteVersion respect argument. (David Gumberg)
Pull request description:
> Previously would use global `CLIENT_VERSION` no matter what, but this is one sense a refactor since all of the places where WriteVersion is called currently call it with `CLIENT_VERSION` anyways. The `client_version` argument is kept since future test code may want to write other versions.
> Addresses a review comment from [#32636](https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2356299627):
This was originally pointed out in https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2356299627, and the followup (#34490) was never merged. However I think it's confusing to have functions that take arguments but ignore them (and it's dead code), so I've cherry-picked the fix up from #34490.
ACKs for top commit:
achow101:
ACK ec5d19665b
pablomartin4btc:
ACK ec5d19665b
w0xlt:
ACK ec5d19665b
Tree-SHA512: 3ad82d979493ac14704975bef504c791ac72fe7910c25097f0019bf61e3d384b0e1646f90c730d2f655733428b14ccd0196c333df2d6153ff5b55ab41918a6dd
fada80192b test: Print os exit code on failure (MarcoFalke)
Pull request description:
Printing the exit code (like printing the stderr) seems independently useful, but should also help to debug the Windows CI failures, which have an empty stderr and truncated combined log:
* https://github.com/bitcoin/bitcoin/issues/34925
* https://github.com/bitcoin/bitcoin/issues/34367
* ...
ACKs for top commit:
sedited:
tACK fada80192b
Tree-SHA512: 085201532ccce9da27cf996136d48436b7800b00a8c8011977d37fcfe152ca029e093428b06ba058759ae10f6dd8e94500b34df712f13d6064f6a7499539bcdc
fc0dcf950f kernel: keep range iterators tied to their owner (Lőrinc)
0936c55f62 test: characterize kernel range iterators (Lőrinc)
Pull request description:
**Problem:** Kernel wrapper methods return `Range` views by value, but their iterators point to the `Range` object.
Saving an iterator from a temporary view, such as `block.Transactions().begin()`, leaves it pointing to the destroyed view, so later use has undefined behavior.
**Fix:** Make range iterators point to the underlying Kernel wrapper object and use the range's compile-time getter for element access.
Remove `operator->`, which returned elements by value and could not easily support arrow expressions.
ACKs for top commit:
purpleKarrot:
ACK fc0dcf950f
yuvicc:
ACK fc0dcf950f
sedited:
ACK fc0dcf950f
Tree-SHA512: 85ae1f8d4c62a762a10c546ebb312f126efc5ef349557dd235c81b585bef92cbb6c4a565734d85d315641a169f48b6032b859f5a9f070bc347b1424b05473e5d
fe7d475d45 private broadcast: bound broadcast attempts per tx to 1k (Gregory Sanders)
Pull request description:
Since rebroacasts introduce additional state, bound the state growth by capping the number of rebroadcasts. With ~72 bytes per record, 10k transactions rebroadcasting for ~42 hours will result about 703 MiB allocated with overhead.
ACKs for top commit:
andrewtoth:
ACK fe7d475d45
frankomosh:
ReACK fe7d475d45
sedited:
ACK fe7d475d45
Tree-SHA512: e4ec5156b90ad24d68b561df03ad09bdf0ac7535886ff56891cb698cf64ff0e1e484075b76040bba6194baf874c9237028c82debf7405136447ba5b5faee589c
5548818115 guix: build glibc with --enable-kernel=3.17.0 (fanquake)
Pull request description:
Our minimum required kernel version is documented as `3.17.0`. Pass `--enable-kernel=3.17.0` when building glibc, so that version is reflected in the binary, and the version checked in the symbol-check script, aligns with the expected minimum.
ACKs for top commit:
hebasto:
ACK 5548818115, tested on Ubuntu 24.04:
willcl-ark:
ACK 5548818115
Tree-SHA512: fc23561d77da80f53cf7564bdb87f5f3c0b23401de79e0801168190d7a99c96e6a60e9437779680b43173572aededf4242b0fba0fe255970f83e6529e3149870
979a42ec17 http: Make HTTPRequest::m_client a weak_ptr (Hodlinator)
Pull request description:
Removes the need for `HTTPClient::ReleaseRequest()` as the client<->request cycle is broken. Not having to remember to call `ReleaseRequest()` reduces cognitive load.
Follow-up to #35735.
ACKs for top commit:
pinheadmz:
untested ACK 979a42ec17
Tree-SHA512: b740a765ffe0592055819e71df8654614e9e140bb77e4a6d146045255f1db9b470ae4a1a77aa16a0d3f3519b7c0d1e9f8c9fbc4c29aab28a2d55ea828ca912c6
c079288967 psbt: update output metadata without inputs (Lőrinc)
4f5712476a test: characterize P2WSH miniscript output (Lőrinc)
e24e8fa2a6 test: characterize PSBT output metadata (Lőrinc)
Pull request description:
**Problem:** PSBTv2 permits outputs to be added before inputs.
An authenticated `descriptorprocesspsbt` request can abort the node while updating metadata for one of those outputs because `UpdatePSBTOutput()` traverses the output script with a signature creator for input index 0.
ECDSA signing or a miniscript timelock check can then access the missing input.
**Fix:** Make `UpdatePSBTOutput()` traverse output scripts with a temporary one-input transaction while continuing to take the output from the PSBT's unsigned transaction.
`MutableTransactionSignatureCreator` continues to require a valid input index.
Output metadata traversal still records scripts and key origins, allowing outputs to be updated before inputs are added.
ACKs for top commit:
jeanpablojp:
tACK c079288967
achow101:
ACK c079288967
w0xlt:
ACK c079288967
polespinasa:
ACK c079288967
Tree-SHA512: 0d8cda74b8a56c0f4713b2669e5a3e5b0551ecda4fdfceb38a80e5b98a9d208d447f2045a1cc9fee74fe33b2fc8f7a60997cd60b2961de5cea871f53831895fe
9cc7dc50bd p2p: reconsider orphans when missing inputs are mined (Greg Sanders)
Pull request description:
We reconsider for mempool entry of missing inputs, we should reconsider for mining of them too.
ACKs for top commit:
yuvicc:
ACK 9cc7dc50bd
l0rinc:
Lightly tested code review ACK 9cc7dc50bd
marcofleon:
ACK 9cc7dc50bd
Tree-SHA512: 9acfb6898e3b286ce23bc2ca3369ae951fadee5f175baad634a6bd23039108d97283814e47972fb857ae459505535f621866f2514b705e94cf841979c37a3933
de2adc308a qa: Disable Qt's glib event dispatcher for GUI tests on OpenBSD (Hennadii Stepanov)
Pull request description:
When `bitcoin-gui` is built against OpenBSD's system Qt packages (which have GLib support), shutdown emits "GLib-CRITICAL **: g_main_context_pop_thread_default: assertion 'stack != NULL' failed" messages on `stderr`, which the test framework treats as a failure.
Set `QT_NO_GLIB=1` so Qt falls back to its poll-based event dispatcher, which avoids the GLib thread-default context entirely.
Fixes https://github.com/bitcoin/bitcoin/issues/35851.
See the CI log here: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/31510478034.
ACKs for top commit:
maflcko:
lgtm ACK de2adc308a
Tree-SHA512: edc991c7a174bc304a4da0ca29ec97bcaece463289de3da5350a046f1133ce6d830c37d5188536ca2ce238d462e56de8f2167fdeeb1d1e5ecca38d60c8495cce
beefda21be doc : update cjdns docs to discourage using onlynet option (naiyoma)
Pull request description:
Currently, the number of CJDNS addresses is still very small and may not be sufficient to fill all outbound connection slots. When running with the `-cjdnsonly` and `-cjdnsreachable` options, `ThreadOpenConnections()` repeatedly calls `Select()`, which returns the same few addresses over and over. The connection attempts may fail, the addresses remain in `AddrMan`, and the loop continually restarts.
The documentation does mention running CJDNS alongside other networks, but we should explicitly explain why using only CJDNS is discouraged, since running with these options alone is supported.
ACKs for top commit:
achow101:
ACK beefda21be
jonatack:
ACK beefda21be
hodlinator:
ACK beefda21be
brunoerg:
ACK beefda21be
Tree-SHA512: 46126291ceb1c36384f9b3deaccdcf20baf8cd4a68491d2ad9fc4a35ec71ad4194b38c4a22c9853bd3f329e2ecb9a0452813d1eea121eb3712876e73dfd4b3f0
158efbc723 doc: fix outdated URL in hash_tests.cpp (cyb3ralbert)
Pull request description:
The old URL still 301-redirects, but to the site root, not to the file — and the file itself is gone: the same path on the new domain returns 404. The author's page at https://www.aumasson.jp/siphash/ says that the SipHash page and documentation have moved to github.com/veorq/SipHash.
The vectors now live in vectors.h in that repository. All 64 values match the array below.
ACKs for top commit:
maflcko:
lgtm ACK 158efbc723
l0rinc:
tested ACK 158efbc723
Tree-SHA512: e45d6872787474e50d51f408ae496e816adbaa8ff263f34df6af7070cfc94bc4f289c5d43cf79461055a0e9738340eb3088837e42991640a253853a5c9984ec6
e07d826e0e rpc: Fix type in ApplyTypeStrOverride (Shuvam Pandey)
c94074fa1b rpc: Surface OBJ_USER_KEYS description for openrpc (sedited)
c020c21d54 rpc: Handle skip type args for openrpc (sedited)
Pull request description:
This was initially motivated by testing the dump of the schema against open-rpc-generator, which crashed with:
```
open-rpc-generator generate -t client -l rust -n bitcoin_client -d ./openrpc.gen.json -o ./generated
There was error at generator runtime:
TypeError: Cannot convert undefined or null to object
```
The changes here fix this crash (albeit perfectly valid existing schema), but I think creating a more complete output is helpful on its own. The openrpc schema dumps can eventually be re-used for the rpc docs and to track rpc interface changes more accurately. Adding the CreateTxDoc outputs section seems useful for that.
Also includes a type tightening from number to integer in `ApplyTypeStrOverride` to reflect the actual behaviour in the rpc calls, where only integers are accepted.
ACKs for top commit:
achow101:
ACK e07d826e0e
willcl-ark:
ACK e07d826e0e
Tree-SHA512: d0454a71b4f1dab1daf8a0d5b1e5bf1c1b8f1a16d26638d4a64a2652402ad74230366cabf0cf4135a16d0bdab584d3d4b2a47f2968a4eff605348ce85e8dbadb
This removes the need for HTTPClient::ReleaseRequest() as the client<->request cycle is broken. Not having to remember to call ReleaseRequest() reduces cognitive load.
Mark CCrypter's fallible methods and crypto helpers as nodiscard, and
handle key-derivation calibration failures.
Keep the output master key unchanged until encryption succeeds. Validate
calibrated iteration counts before integer conversion. This prevents a
failed calibration from leaving mismatched parameters and ciphertext or
triggering undefined conversion behavior.
Extract ParseKeyPathElement() as a shared helper in util/bip32 that
parses a single BIP32 key path element (e.g. "0", "0'", "0h").
Both ParseHDKeypath() and the descriptor parser's ParseKeyPathNum()
now delegate to it, eliminating duplicated parsing logic.
Previously would use global `CLIENT_VERSION` no matter what, but this is
one sense a refactor since all of the places where WriteVersion is
called currently call it with `CLIENT_VERSION` anyways. The
`client_version` argument is kept since future test code may want to
write other versions.
Addresses a review comment from #32636:
https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2356299627
01dde6b205 fuzz: Fix assertion in txorphan (marcofleon)
Pull request description:
`EraseTx()` calls `LimitOrphans()`, which may evict announcements from a peer that didn't announce the erased transaction, causing that peer's usage to decrease. Relax the assertion in the `EraseTx()` branch that claimed usage of a non-announcer peer should be unchanged. Also, add assertions for the other cases.
ACKs for top commit:
dergoegge:
utACK 01dde6b205
instagibbs:
ACK 01dde6b205
Tree-SHA512: 2e597b85fd41058c2fa79fa55f0d37e12505065b5e27aba7b9680e0c249a5450e6fa97b45394d6ffe1318f42538134ffa9c423b126c455f6f8e6d8ca59eed4b6
9954aa7728 http: don't parse any new requests from a client if m_req_busy = true (Matthew Zipkin)
c7db3ae1f9 test: cover HTTPRequest state machine (Matthew Zipkin)
90676e24ad Add state to HTTPRequest to avoid duplicate work over I/O cycles (Matthew Zipkin)
507e528e84 http: reuse HTTPHeaders to parse chunked trailer (Matthew Zipkin)
902d8908c9 http: only read one HTTPRequest at a time per client (Matthew Zipkin)
Pull request description:
This PR reduces the memory consumption of the HTTP Server when reading data from connected clients, and improves performance especially when requests are large (i.e. requiring multiple TCP packets).
In https://github.com/bitcoin/bitcoin/pull/35182 the server copies as much data as it can from the socket into application memory, and then tries to parse as many complete HTTP requests as possible from that data. If a request is discovered to be incomplete, the in-progress request is abandoned. The server tries again on the next I/O cycle to read the same data from the buffer, duplicating work as many times as it takes before the client finishes sending the request (or times out).
This PR implements two improvements to this:
1. Only parse one request at a time from the receive buffer. The server processes requests from each client in series anyway.
2. Add state to `HTTPRequest` so it can be filled with data from the receive buffer over multiple I/O loop iterations without losing progress.
If a client sends large or multiple requests, that data will sit in the kernel's socket buffer instead of the application memory. Eventually the socket buffer will fill up and TCP backpressure will kick in, dropping the TCP window to 0 and blocking the client from sending any more.
A state machine for `HTTPRemoteClient` was [discussed previously](https://github.com/bitcoin/bitcoin/pull/35182#pullrequestreview-4322490068) to control resource consumption. Another nice benefit of this model (for a follow-up PR) will be to insert the RPC authentication check after reading 8kB-limited headers but before the 32MB-limited request body.
ACKs for top commit:
winterrdog:
re-ACK 9954aa7728
janb84:
re ACK 9954aa7728
frankomosh:
ACK 9954aa7728.
fjahr:
ACK 9954aa7728
Tree-SHA512: b7c913114283fbf1f360b40f6c65a01390a26731bf3b166f460ec260f9206f25d738b3a06887bfa839911c1c6aaf634448181da47a752a9a881aebd907e44868
fabe100c2b test: Use throwing config parser getters without fallback (MarcoFalke)
fa8acd57cd test: Write true/false values in config.ini (MarcoFalke)
Pull request description:
Currently, the called `getboolean` member function is *not* the throwing https://docs.python.org/3/library/configparser.html#configparser.ConfigParser.getboolean, but a non-throwing member function on a dict-like proxy object.
This is confusing and brittle, because tests shouldn't silently skip when a config key is missing. Instead, tests should loudly fail, e.g. when the config key is renamed in one place, but not the other.
ACKs for top commit:
jeanpablojp:
tACK fabe100c2b
willcl-ark:
ACK fabe100c2b
Tree-SHA512: a970d74ad285372b8adcce8e2a52b01f5a3b563899dfc5262e6ffbf3d8aba43e72f7b03111e8d5188924c7d3d789992407cddb5d182d9be5e42f07896d8ad4a3
b3d77ea027 test: Speedup fee estimation functional test with batching (sedited)
Pull request description:
The fee estimation functional test is currently the slowest one by a good margin. It is a bit annoying, because it also increases the total runtime of the functional tests.
It seems like most of the slowness comes from the transactions propagating between the nodes. This patch helps them do that by submitting them directly to all the nodes. Also take this opportunity to batch the transaction submissions.
On my machine this speeds up the fee estimation functional test from around 71 seconds to 25 seconds.
ACKs for top commit:
151henry151:
tACK b3d77ea027
maflcko:
review ACK b3d77ea027🐇
ismaelsadeeq:
ACK b3d77ea027
Tree-SHA512: f76415dca7997577ca39ac6b95dfdf32b930dd64b4311e3da34e34adb16107ff4ea2d9fa679f3ca50540e80c38af7f9390b44f21ad1b8107fbbc3586edb2ef19
25bed560be test: add forward-compat functional test for txindex (sedited)
703304ed8c doc: add release notes for txindex disk usage and downgrading (Andrew Toth)
8e5320a2d2 tests: cover txindex hash prefix collisions and legacy fallback (Andrew Toth)
b75efa19ba txindex: skip bloom filters and legacy lookups for new databases (Andrew Toth)
004d7c098c txindex: hash key prefixes and pack block positions (Andrew Toth)
5a255970fd refactor: move txindex db constants and legacy key to txindex_key.h (Andrew Toth)
327660134c txindex: pass the full block to DB::WriteTxs (Andrew Toth)
42771e7998 txindex: use a new block locator for downgrade safety (Andrew Toth)
4b08baed72 txindex: return optional tx and block hash from FindTx (Andrew Toth)
Pull request description:
The current txindex uses the full 32-byte txid as keys, which takes up about 66 GB of disk space today on mainnet. Using a 5-byte key prefix instead drops the disk usage to 26 GB - cutting the size to less than half.
Using the full 32-bytes is unnecessary since a 5-byte salted siphash will produce collisions in about 1 in 1.1 trillion. Some collisions will occur, but the penalty is just an extra disk read, deserialization and hash.
The tx position can be appended to the key instead of used as a value, and a LevelDB iterator can seek to the prefix and then scan for the correct tx. This is an almost identical approach to `txospenderindex`.
Also instead of storing the file position of the block, we can store only the sequence of the connected block and offset of the transaction in the block. This can be packed into a 6-byte key suffix using 3-byte representations of the sequence and offset in the block. The block file can be recovered by the CBlockIndex that is already in memory. The sequence is mapped to the block hash in the db, so we can lookup the block hash to find the CBlockIndex during reads.
If a tx is not found with this method, we fallback to looking up the legacy entry. With this method a user with an existing db can opt to erase the `indexes/txindex` folder and reindex, or keep the current index and new entries will be appended with the smaller footprint.
The time to index was faster on my machine with this method, 1h19m vs current 1h50m.
Lookups are roughly the same, around 0.2ms per lookup with `getrawtransaction`.
When testing on mainnet, I got 894,549 2-way collisions, 395 3-way collision, and 1 4-way collision that worst case could cause an extra 3 false positives when reading.
ACKs for top commit:
l0rinc:
diff reACK 25bed560be
sedited:
ACK 25bed560be
ajtowns:
ACK 25bed560be
Tree-SHA512: a25c79ca7e722e2f372b65f5fc11c8b194ad49f2240b4881c7e606306aabbd3604aede3f1c33606b467486affac3a3f503638f513c896935cebbc02709cb60d8
75a4e6c678 gui: fix allow restore wallets without .dat file extension (Pol Espinasa)
6ed7e05e20 gui: fix add .dat file extension automatically when exporting watchonly (Pol Espinasa)
Pull request description:
fixes https://github.com/bitcoin-core/gui/issues/956
Unlike `backup wallet`, `export watch-only wallet` was not automatically adding the file extension to the exported file, making restoring difficult if the user doesn't manually add the file extension after exporting.
Allows also to restore a wallet from a non specified `.dat` file extension. This is achieved by removing the filter in the select file screen, matching the RPC behavior.
ACKs for top commit:
hebasto:
ACK 75a4e6c678.
Tree-SHA512: 7c45d51205f9abf2b67233e8abd3297e49a4230eb32aa4118b37ab9da0a8d692aae4b67a8880881e5fab42256d1cf52ccf1b289b8f23f85930354130c192b33d
c3945bfd2b doc: use derivehdkey in multisig tutorial (Sjors Provoost)
3662e33669 test: use derivehdkey in M-of-N multisig demo (Sjors Provoost)
d9570f0838 rpc: add derivehdkey (Sjors Provoost)
62da9f9614 wallet: add GetExtKey helper (Sjors Provoost)
aaf1548475 wallet: generalize GetActiveHDPubKeys helper (Sjors Provoost)
3821452c4a refactor: add hardened derivation helper (Sjors Provoost)
0ab61caafd rpc: ParsePathBIP32 helper (Sjors Provoost)
e36c4b76e1 util: reject out-of-range BIP32 keypath indices (Sjors Provoost)
ba78c31a00 fuzz: check ParseHDKeypath/WriteHDKeypath round-trip (Sjors Provoost)
8cce969085 Have ParseHDKeypath handle h derivation marker (Sjors Provoost)
fc53077762 test: move parse_hd_keypath test to bip32_tests (Sjors Provoost)
dab525eb77 key: add DeriveExtKey() helper (Sjors Provoost)
Pull request description:
Adds a `derivehdkey` RPC that returns an xpub, or optionally the xprv, at an arbitrary BIP32 path (with at least one hardened step), derived from a wallet HD key.
The main use case is coordinating a multisig setup, where each participant shares an xpub derived at a hardened path (e.g. `m/87h/0h/0h`) distinct from their default single-signature descriptors. See the (updated) `doc/multisig-tutorial.md` and (updated) functional test to see how that workflow improves.
The first commits are some helpful helpers:
- _key: add DeriveExtKey() helper_ - performs the actual derivation
- _test: move parse_hd_keypath test to bip32_tests_ - from `psbt_wallet_tests`
- _Have ParseHDKeypath handle h derivation marker_
- _util: reject out-of-range BIP32 keypath indices_ - `ParseHDKeypath` would previously map overflowing values without `h` to hardened.
- _fuzz: check ParseHDKeypath/WriteHDKeypath round-trip_
- _rpc: ParsePathBIP32 helper_
- _refactor: add hardened derivation helper_ - `HasHardenedDerivation()`, to enforce the "at least one hardened step" rule
- _wallet: generalize GetActiveHDPubKeys helper_ - extracts code from `gethdkeys` which `derivehdkey` needs
- _wallet: add GetExtKey helper_ - reconstruct an xprv from a wallet xpub (analog of `GetKey()`); behavior-preserving prep, also simplifies `gethdkeys`.
Meat and potatoes:
- _rpc: add derivehdkey_ - the RPC itself, plus the `UnusedKey` filter on `GetHDPubKeys` that drives key selection.
- _test: use derivehdkey in M-of-N multisig demo_ - rewrites the functional multisig test to use the RPC and `<0;1>` syntax.
- _doc: use derivehdkey in multisig tutorial_ - same for the prose tutorial.
ACKs for top commit:
pseudoramdom:
code review ACK c3945bfd2b
achow101:
ACK c3945bfd2b
w0xlt:
That being the case, ACK c3945bfd2b
Tree-SHA512: 661f17c9bfe26017eb14c27ba7af37093387100d3baa25f5d29bba9c1aedc40d19afe1bdfc126a18d018857bb02f1fc84386f10b8f4f4b8e9d6f4b0691d9e302
`verify-commits.py` must not authorize checkout for a commit whose history diverges from configured trust roots.
Require proof that the commit is an ancestor of a root before skipping checks, and identify the failing root in errors.
Co-authored-by: Rob Hamilton <6456095+Rob1Ham@users.noreply.github.com>
`verify-commits.py` must not authorize checkout when Git cannot inspect the requested commit or its ancestry.
Reject ancestry command errors and validate the exact trusted root through Git before reporting success.
Co-authored-by: Rob Hamilton <6456095+Rob1Ham@users.noreply.github.com>
