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
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
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
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
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
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
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
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
Rather than rebroadcasting forever, bound attempts at
private broadcast, report remaining attempts over RPC
results, and allow exhausted transactions to be
retried when submitted.
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.
Signed-off-by: shaurya2k06 <shaurya2k06@gmail.com>
PSBTv2 permits outputs to be added before inputs.
`UpdatePSBTOutput()` traverses output scripts with a signature creator for input index 0, so ECDSA signing or a miniscript relative-timelock check can access a missing input and abort.
Construct a standalone transaction with one input for metadata traversal.
Continue taking the output from the unsigned PSBT transaction so scripts and key origins remain associated with the correct output while traversal no longer depends on the PSBT input list.
`MutableTransactionSignatureCreator` continues to require callers to provide a valid input index.
Co-authored-by: Ava Chow <github@achow101.com>
Document the scripts and key origins collected for common output descriptors before changing zero-input handling.
Use a valid input for ECDSA-backed outputs because their zero-input paths currently abort.
f280f5eb47 wallet: rpc: deprecate removeprunedfunds (David Gumberg)
e5b7785447 test: wallet: resend: avoid internal behavior via removeprunedfunds (David Gumberg)
Pull request description:
Originally added in https://github.com/bitcoin/bitcoin/pull/7558 as a companion to `importprunedfunds`, this RPC has no known helpful use while being both dangerous and a maintenance burden.
Despite what the name says, it allows the deletion of arbitrary transactions, and `importprunedfunds` does not allow the importing of transactions not belonging to the user, and `listtransactions` does not list transactions not belonging to the wallet, so this RPC can only be used to delete transactions actually belonging to the wallet, and in the unlikely event that transactions not belonging to the wallet are present, they cause no harm except for occupying a few bytes on the users disk.
ACKs for top commit:
achow101:
ACK f280f5eb47
polespinasa:
ACK f280f5eb47
pablomartin4btc:
reACK f280f5eb47
Tree-SHA512: ed9c30c50be514d637999b4c8f3fa9b9b1446a5553e3974703638b45d8f55f1291f5cfb82dd2ead6d0743e424e9e3b1edbd56fc04dae3dcdee4d175e2a1ce061
4e8c4bc794 test: classify SOCKS5 peers via getpeerinfo addrbind (Henry Romp)
Pull request description:
p2p_private_broadcast.py classifies each SOCKS5 connection by scanning the node's debug log for `trying v. connection (...) to <addr>:<port>`, then attaches a fake peer for that type. The helper returned the first match in the whole log, so when a feeler selected a clearnet address that private broadcast had used earlier in the run (in the CI failure, `[50::1]:8333`, about 10 seconds apart), the feeler was labelled private-broadcast, was given the `NoRelayP2PInterface`, and disconnected as a feeler rather than with the expected "connected in vain" message.
Instead of relying on the debug log, identify the connection via the SOCKS5 proxy client socket's source address, which equals the node's `addrbind` for that peer, and read `connection_type` from getpeerinfo. The proxy replies to the SOCKS5 request before invoking `destinations_factory`, so the node has already registered the peer by the time classification runs. This also stops treating debug.log contents as a stable test interface. Dropping the log scrape removes a full re-read of debug.log per SOCKS5 connection; `p2p_private_broadcast.py` goes from ~23s to ~14s locally.
Fixes#35843
Tested with:
`build/test/functional/test_runner.py p2p_private_broadcast.py p2p_private_broadcast_retry_v1.py --timeout-factor=2`, and against the forced-feeler repro from the issue, which no longer mislabels the feeler.
ACKs for top commit:
jeanpablojp:
tACK 4e8c4bc794
andrewtoth:
ACK 4e8c4bc794
mzumsande:
Code Review ACK 4e8c4bc794
Tree-SHA512: ce2db418787d7ecf518bd49b37d7d664748fee5991a2924522dfaf42b27d90ca001caa0611011310636b453d3aada1061d086f20bb866eb66605645935f55c74
While this is usually used where the types are not enforced strictly,
adding the description is both useful to the developer implementing a
client and for potentially using the openrpc output as a basis for
documentation.
```diff
diff interim_dump.json new_dump.json
465c465,466
< }
---
> },
> "description": "A key-value pair. The key (string) is the bitcoin address,\nthe value (float or string) is the amount in BTC"
777c778,779
< }
---
> },
> "description": "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in BTC"
903c905,906
< }
---
> },
> "description": "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in BTC"
12915c12918,12919
< }
---
> },
> "description": "A key-value pair. The key (string) is the bitcoin address,\nthe value (float or string) is the amount in BTC"
13692c13696,13697
< }
---
> },
> "description": "A key-value pair. The key (string) is the bitcoin address,\nthe value (float or string) is the amount in BTC"
14002c14007,14008
< }
---
> },
> "description": "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in BTC"
14254c14260,14261
< }
---
> },
> "description": "The bitcoin address is the key, the numeric amount (can be string) in BTC is the value"
16046c16053,16054
< }
---
> },
> "description": "A key-value pair. The key (string) is the bitcoin address,\nthe value (float or string) is the amount in BTC"
```
Instead of filling them in with empty object and array args be a bit
more friendly to the consumer by giving type hints, while retaining type
flexibility:
Keep the empty object, but fill the array with the hinted at types by
recursing through them. Add an additional argument to the openrpc
functions (`in_skip_type_check`) to keep track of when a loosely typed
argument is under evaluation. Note that the array is evaluated stricter
than before: If it contains items, they need to match the nested
objects.
The change from oneOf->anyOf and removing additionalProperties: false
should still convey to the reader that this is a loosely typed object
and prevent collisions between these loosely-typed schemas.
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, but I think creating a friendlier
output is helpful on its own.
This patch changes the schema exported from CreateTxDoc as follows:
```diff
diff -U6 <(jq '.methods[] | select(.name == "createrawtransaction")' dump.json) \
<(jq '.methods[] | select(.name == "createrawtransaction")' new_dump.json)
@@ -37,13 +37,43 @@
{
"name": "outputs",
"required": true,
"schema": {
"oneOf": [
{
- "type": "array"
+ "type": "array",
+ "items": {
+ "anyOf": [
+ {
+ "type": "object",
+ "additionalProperties": {
+ "oneOf": [
+ {
+ "type": "number"
+ },
+ {
+ "type": "string"
+ }
+ ]
+ }
+ },
+ {
+ "type": "object",
+ "properties": {
+ "data": {
+ "type": "string",
+ "pattern": "^[0-9a-fA-F]+$",
+ "description": "A key-value pair. The key must be \"data\", the value is hex-encoded data that becomes a part of an OP_RETURN output"
+ }
+ },
+ "required": [
+ "data"
+ ]
+ }
+ ]
+ }
},
{
"type": "object"
}
]
},
```
4ca182ca40 doc: clarify alternate_wtxids is empty when only one witness variant (pablomartin4btc)
fa48b5d28e test: assert listsinceblock "removed" reports current canonical wtxid (pablomartin4btc)
9b96ee1288 wallet, test: add unit test for variant txid validation in CWalletTx deserializer (pablomartin4btc)
9de6543cb5 wallet: post-#35501 cleanup in CWalletTx (pablomartin4btc)
Pull request description:
Follow-up cleanups and clarifications after #35501 was merged.
Commit breakdown:
1. _post-[#35501](https://github.com/bitcoin/bitcoin/pull/35501) cleanup in_ `CWalletTx`
- Rename `arg_state` → `new_state` in `Update()` for consistency
- Simplify `RecomputeCanonical()` using `std::ranges::min_element` with a projection lambda (14 lines → 3 lines)
- Add variant txid validation in the `CWalletTx` deserialise constructor: throws `std::runtime_error` if any variant's txid doesn't match the canonical txid deserialized from the stream
- Move `Init()` to `private` and extend it to clear `m_txs` and reset `m_canonical_wtxid`, so a full re-deserialise via `Unserialize()` starts from a clean state
All [suggested](https://github.com/bitcoin/bitcoin/pull/35501#pullrequestreview-4854519083) by ajtowns.
2. _add unit test for variant txid validation in_ `CWalletTx` _deserializer_
3. _assert_ `listsinceblock` "removed" _reports current canonical wtxid_
Documents that removed entries reflect the wallet's current `CWalletTx` state, not a snapshot of the detached block. A future followup could improve this (requires per-block tracking of which witness variant was included).
[Suggested](https://github.com/bitcoin/bitcoin/pull/35501#discussion_r3632044472) by w0xlt.
4. _clarify_ `alternate_wtxids` _is empty when only one witness variant_
[Suggested](https://github.com/bitcoin/bitcoin/pull/35501#discussion_r3632113003) by polespinasa.
ACKs for top commit:
jeanpablojp:
re-ACK 4ca182ca40
achow101:
ACK 4ca182ca40
polespinasa:
ACK 4ca182ca40
Tree-SHA512: 64eadeb11372d904c79edbfd264c4d8dc1b4fe4ce5e3acc301bfeba9e556efb5dce2c684f0e58c7b74e2c687cc7dd77389970662b3fd629ed034a97bcfdfb71c
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.
0390338692 test: check MuSig import private key warnings (woltx)
5e62fbf09c wallet: check descriptor private key completeness on import (woltx)
cd8d01bf47 descriptors: require complete MuSig private keys (woltx)
Pull request description:
`importdescriptors` currently checks whether all private keys are present by expanding the descriptor and verifying that every expanded origin pubkey has a private key.
This is wrong for MuSig descriptors because expansion includes the synthetic aggregate pubkey. There is no individual private key for that aggregate pubkey, so importing a fully private MuSig descriptor such as `rawtr(musig(A_priv,B_priv))` incorrectly returns:
```
Not all private keys provided. Some wallet functionality may return unexpected errors
```
This PR fixes the issue by making descriptor private-key completeness account for MuSig participant keys, and by having `importdescriptors` use `Descriptor::HavePrivateKeys()` instead of duplicating its own manual completeness check.
The functional test covers both cases:
- `rawtr(musig(A_priv,B_priv))` imports without warnings.
- `rawtr(musig(A_priv,B_pub))` still warns that not all private keys were provided.
ACKs for top commit:
achow101:
ACK 0390338692
theStack:
Code-review ACK 0390338692
Tree-SHA512: a55fb084c63f725a0991556acdfb822f3a5a669f745a00b9f0bf0996b639986cdf5e2be2e8d3d0a2ee3fe5744355f20b40df576601792ef3db698e606629ad52
When a block is detached, listsinceblock "removed" entries reflect the
wallet's current CWalletTx rather than a snapshot of the variant that
was actually in the detached block. Add assertions to make this
behaviour explicit. A future followup could improve listsinceblock to
track and report the specific witness variant that was in the
disconnected block (requires per-block tracking of which witness variant was included).
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
ea59f17220 test: cover v0.14.3 wallet migration (w0xlt)
18b8afd093 test: support v0.14.x in dumb_sync_blocks (w0xlt)
Pull request description:
This PR adds test coverage for migrating legacy Bitcoin Core wallets from v0.14.3 (released in 2017) to the descriptor wallet format. The test validates that users can safely upgrade their wallets while preserving all funds, transaction history, and addresses.
This test was originally developed on top of #32977, as it was requested in reviews.
However, since it also increases test coverage, it can be merged independently.
The test covers two wallet migration scenarios:
* Non-HD Wallet Migration - Tests migration of non-HD wallets (created with `-usehd=0`)
* Single Chain HD Wallet Migration - Tests migration of HD wallets from v0.14.3 (`VERSION_HD_BASE`)
The node v0.14.3 cannot be synced using the normal test framework helpers because it does not have the `syncwithvalidationinterfacequeue` RPC, so the test uses `dumb_sync_blocks` to submit blocks from the ancient node to the modern node before migrating the wallet.
Each scenario uses its own dedicated old/new node pair, which keeps the setup isolated and makes this testing best managed in a separate file rather than in the existing migration test files.
On the Windows cross-built CI job, this test is excluded from the main functional test runner and re-run sequentially in an ASCII-only tmpdir, because the v0.14.3 binary cannot handle non-ASCII characters in the temporary directory path.
ACKs for top commit:
furszy:
utACK ea59f17220
pablomartin4btc:
reACK ea59f17220
Tree-SHA512: 35ef7173e10fe52f20db0d6d1f144c5a1343ff406d294ab7d0e346d79546797f3f145f2893d99bb7b57d71aa14580690ab80eae148ec94b91498eb0938b2d15e
089c883c55 test: Add coverage for listreceivedby* excluding "send" addresses (pablomartin4btc)
873c054805 wallet: Exclude non-owned addresses from listreceivedby* (pablomartin4btc)
Pull request description:
Fixes#16159.
`listreceivedbyaddress`/`listreceivedbylabel` with `include_empty=true` walk the entire address book and return every entry that has no matching `mapTally` record — including addresses with a "send" purpose (foreign addresses that got a label via `setlabel`, the GUI, or `addmultisigaddress`) that this wallet never received funds to and doesn't own.
This excludes those via `IsMine()` rather than the address book's `purpose` field, since `purpose` is set inconsistently across several code paths and `IsMine()` is the same check `mapTally` itself is already built from.
Picks up prior work by kouloumos in #25973 and BrandonOdiwuor in #30972, both closed for
inactivity:
- [#25973](https://github.com/bitcoin/bitcoin/pull/25973) filtered on `purpose == "send"` directly. ryanofsky pointed out purpose "is set pretty haphazardly in code" and [suggested](https://github.com/bitcoin/bitcoin/pull/25973#discussion_r1269477246) `IsMine()` instead.
- [#30972](https://github.com/bitcoin/bitcoin/pull/30972) implemented that, then furszy pointed out `IsMine()` only needs to run for addresses missing from `mapTally`, not every one. rkrux further suggested dropping the redundant re-lock in favor of `EXCLUSIVE_LOCKS_REQUIRED` directly on the lambda — matching the existing pattern in `wallet/interfaces.cpp` — and simplifying the branching.
This PR carries that final approach forward on current master. The regression test is a small, standalone addition rather than reviving the test-file "split into subtests" refactor from the earlier PRs, which achow101 [flagged](https://github.com/bitcoin/bitcoin/pull/30972#issuecomment-3688186614) on #30972 as unrelated stylistic churn.
ACKs for top commit:
polespinasa:
lgtm re-ACK 089c883c55
jeanpablojp:
ACK 089c883c55
achow101:
ACK 089c883c55
Tree-SHA512: d45488c93b9294258faaab5d1891ca5e8c4b8d0d4feb298403c7c3f20d6aa08989d548cddd25ccd47a1ed969e4a309ee68ad1541c6121fed39ed534c78c256e7
fad9ab714b test: Append print_suppressions=0 to LSAN_OPTIONS, and suppress bitcoin-qt (MarcoFalke)
Pull request description:
(see commit msg for rationale and background).
To test, one should be able to use the cmake options such as `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' --preset=dev-mode -DBUILD_GUI=ON -DSANITIZERS=address` on e.g. Fedora. Then see that the current suppressions file is insufficient, and also confirm that `print_suppressions=0` is required.
ACKs for top commit:
fanquake:
ACK fad9ab714b
Tree-SHA512: 1830b4aeb072fa18b76522a124a268073675da14255e469a6d86ee5de52cd08d5613d0c3bd8a66465b0c4636345c9e967923cd1fb516906a58b614fe0e700033
75f5851927 doc: add release note for REST cache-control headers (w0xlt)
bbe21ac29f doc: document REST cache-control defaults (w0xlt)
862a179556 http: add no-store to dispatcher-generated error responses (w0xlt)
acf45c44c0 rest: add Cache-Control headers to REST responses (w0xlt)
Pull request description:
This PR adds explicit Cache-Control headers to REST responses.
The policy is:
- Immutable data gets: `Cache-Control: public, immutable, max-age=86400`
- Mutable, node-local, and error responses get: `Cache-Control: no-store`
Important details:
- `/block` and `/block/notxdetails` bin/hex, `/blockpart`, `/blockfilter`, `/spenttxouts`, and `/deploymentinfo/<blockhash>.json` are treated as immutable.
- `/block` and `/block/notxdetails` JSON, all `/tx` formats, `/headers`, `/blockfilterheaders`, `/blockhashbyheight`, `/chaininfo`, `/mempool`, `/getutxos`, and `/deploymentinfo.json` are no-store.
- REST errors and HTTP dispatcher-generated errors are no-store.
- Unmatched `/rest` 404s also return no-store, including paths like `/rest/tx`, `/rest/does-not-exist`, and `/rest?x=1`.
Tests were added in `interface_rest.py` to cover successful responses, behavior across a newly mined block, REST errors, and unmatched REST 404s.
Docs were added to `REST-interface.md`, including guidance for overriding the defaults in a reverse proxy or CDN.
Closes#33809
ACKs for top commit:
stickies-v:
re-ACK 75f5851927
pinheadmz:
ACK 75f5851927
sedited:
ACK 75f5851927
Tree-SHA512: 292ccd06ddfc9272c17fa720ce1ea8bb05462337af6460488f70003d3daf31fcf262e68c264522a911bba65ae2b25fc88a1fd422e5664583daf64070231cb062
The print_suppressions=0 is required to avoid a CI failure when the
suppressions were used. E.g:
```
$ LSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/lsan:print_suppressions=1" ./bld-cmake/test/functional/interface_gui.py
2026-08-08T10:53:45.864160Z TestFramework (INFO): PRNG seed is: 8358096631255493262
2026-08-08T10:53:45.914748Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_5zx5343v
2026-08-08T10:53:47.029997Z TestFramework (INFO): Test that bitcoin-gui starts up and can be stopped via RPC
2026-08-08T10:53:47.431761Z TestFramework (ERROR): Unexpected exception:
File "./test/functional/test_framework/test_node.py", line 534, in is_node_stopped
raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
AssertionError: Unexpected stderr -----------------------------------------------------
Suppressions used:
count bytes template
2 181 bitcoin-qt
----------------------------------------------------- !=
```
The general suppression of the qt executables is required to avoid CI
failures for i386 builds. E.g:
```
test 2026-08-05T08:54:08.370427Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
File "/ci_container_base/ci/scratch_ ₿🧪_/build-i686-pc-linux-gnu/test/functional/interface_gui.py", line 34, in run_test
self.stop_node(0)
~~~~~~~~~~~~~~^^^
File "/ci_container_base/test/functional/test_framework/test_node.py", line 525, in is_node_stopped
assert return_code in expected_ret_code, self._node_msg(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: [node 0] Node returned unexpected exit code (1) vs ((0,)) when stopping
node0 stderr =================================================================
==73449==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 386 byte(s) in 17 object(s) allocated from:
#0 0x5df28a2d in malloc (/ci_container_base/ci/scratch_ ₿🧪_/build-i686-pc-linux-gnu/bin/bitcoin-qt+0x1d0da2d) (BuildId: 496a5df531df278fe395724ae917f266331b3f81)
#1 0xee03c1d1 (<unknown module>)
Indirect leak of 12 byte(s) in 1 object(s) allocated from:
#0 0x5df28a2d in malloc (/ci_container_base/ci/scratch_ ₿🧪_/build-i686-pc-linux-gnu/bin/bitcoin-qt+0x1d0da2d) (BuildId: 496a5df531df278fe395724ae917f266331b3f81)
#1 0xee03c1d1 (<unknown module>)
SUMMARY: AddressSanitizer: 398 byte(s) leaked in 18 allocation(s).
```
Regression test for #16159: an address labeled via setlabel by a
wallet that doesn't own it is assigned a "send" purpose and must not
appear in listreceivedbyaddress/listreceivedbylabel results, even
with include_empty=true.
Co-authored-by: Andreas Kouloumos <kouloumosa@gmail.com>
Use derivehdkey instead of extracting each participant xpub (and
derivation info) from the listdescriptors output.
Additionally use the new <0;1> descriptor syntax.
Finally this commits adds a few debug log lines, and expand the
explanation for why we use m/44h/1h/0h.
Add an UnusedKey filter to GetHDPubKeys() so the new RPC can prefer
unused(KEY) descriptors before falling back to active descriptors.
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
264555af3c rpc: avoid descriptor range counter overflow (Lőrinc)
143a13fb2b test: characterize descriptor range endpoint (Lőrinc)
Pull request description:
**Problem:** The authenticated `scantxoutset`, `scanblocks`, `getdescriptoractivity`, `utxoupdatepsbt`, and `descriptorprocesspsbt` RPCs share a descriptor expansion helper that iterates inclusive `int64_t` ranges with an `int` counter.
A ranged descriptor with an explicit `[begin, end]` range ending at `2^31 - 1` expands that valid position, then overflows when advancing the counter to exit the loop.
Trap-enabled builds terminate, while other builds invoke undefined behavior.
**Fix:** Use `int64_t` for loop control so the one-past-the-end value is representable and every position passed to `Descriptor::Expand()` remains within its existing `int` range.
Related: [#26275](https://github.com/bitcoin/bitcoin/pull/26275) fixed the same endpoint overflow in `deriveaddresses`.
ACKs for top commit:
achow101:
ACK 264555af3c
polespinasa:
ACK 264555af3c
sedited:
ACK 264555af3c
Tree-SHA512: 4326182b5897b6f6672e5f7c7296eafdbb6e3b5ed901d61e8fa2cff9b19d372bb8adb88902368dc520ed400e12dd5264ea68677d9ce0feec76fa2ef55fa0d2f4
1278a5970d net_processing: process unique tx INVs only (Gregory Sanders)
Pull request description:
There is no reason we should process conflicting
advertisements for transactions, as they cannot be both accepted into our mempool.
Avoid processing these and doing spurious work.
Should be no change in observable behavior.
ACKs for top commit:
ajtowns:
ACK 1278a5970d
fjahr:
ACK 1278a5970d
l0rinc:
ACK 1278a5970d
Tree-SHA512: c62ceed2cc634c8c99509a8495e5f9bb6d4d8d050942f709a6539ae4dfe1ec628ce7ab0ec1392656809e0d827d1fecf2e8fb9bb4600d13a5bf9a34c9e9e3ad6e
There is no reason we should process conflicting
advertisements for transactions, as they cannot be both
accepted into our mempool.
Avoid processing these and doing spurious work.
