This PR adds functional coverage for combinepsbt preserving unknown PSBT fields across global, input, and output maps, as suggested by @Bicaru20 in #34893 (comment).
The test covers both PSBTv0 and PSBTv2 by creating valid base PSBTs with createpsbt, injecting unknown key-value pairs into two copies, combining them, and asserting that all unknown fields are retained in the decoded result.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35310.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | nebula-21 |
| Concept ACK | polespinasa |
| Approach ACK | winterrdog |
| Stale ACK | mercie-ux |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
While thinking about how to implement this test, I saw that in the bip 174 there is an specific example on how to combine two psbt with unknown fields. I think that since this example is given we could use it for this test.
I re-wrote the test for that specific example in case you also think it is better to do it this way:
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index a88b4773f5..f10fdc0b44 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -304,0 +305,8 @@ class PSBTTest(BitcoinTestFramework):
+
+ unknown_psbt_key_a = unknown_key(0xf0, bytes.fromhex("010203040506070809"))
+ unknown_psbt_key_b = unknown_key(0xf0, bytes.fromhex("010203040506070810"))
+
+ unknown_psbt_value= bytes.fromhex("0102030405060708090a0b0c0d0e0f")
+
+ inputs = [{"txid": "ff" * 32, "vout": 0, "sequence":0xffffffff}]
+ outputs = [{"data": "00"}]
@@ -306,17 +313,0 @@ class PSBTTest(BitcoinTestFramework):
- unknown_key_type = 0xda
- global_key_a = unknown_key(unknown_key_type, b"global-a")
- global_key_b = unknown_key(unknown_key_type, b"global-b")
- input_key_a = unknown_key(unknown_key_type, b"input-a")
- input_key_b = unknown_key(unknown_key_type, b"input-b")
- output_key_a = unknown_key(unknown_key_type, b"output-a")
- output_key_b = unknown_key(unknown_key_type, b"output-b")
-
- global_value_a = b"\xaa"
- global_value_b = b"\xdd"
- input_value_a = b"\xbb"
- input_value_b = b"\xee"
- output_value_a = b"\xcc"
- output_value_b = b"\xff"
-
- inputs = [{"txid": "aa" * 32, "vout": 0}]
- outputs = [{self.nodes[0].getnewaddress(): Decimal("1")}]
@@ -327,3 +318,3 @@ class PSBTTest(BitcoinTestFramework):
- psbt1.g.map[global_key_a] = global_value_a
- psbt1.i[0].map[input_key_a] = input_value_a
- psbt1.o[0].map[output_key_a] = output_value_a
+ psbt1.g.map[unknown_psbt_key_a] = unknown_psbt_value
+ psbt1.i[0].map[unknown_psbt_key_a] = unknown_psbt_value
+ psbt1.o[0].map[unknown_psbt_key_a] = unknown_psbt_value
@@ -332,3 +323,3 @@ class PSBTTest(BitcoinTestFramework):
- psbt2.g.map[global_key_b] = global_value_b
- psbt2.i[0].map[input_key_b] = input_value_b
- psbt2.o[0].map[output_key_b] = output_value_b
+ psbt2.g.map[unknown_psbt_key_b] = unknown_psbt_value
+ psbt2.i[0].map[unknown_psbt_key_b] = unknown_psbt_value
+ psbt2.o[0].map[unknown_psbt_key_b] = unknown_psbt_value
@@ -339,2 +330,2 @@ class PSBTTest(BitcoinTestFramework):
- (global_key_a, global_value_a),
- (global_key_b, global_value_b),
+ (unknown_psbt_key_a, unknown_psbt_value),
+ (unknown_psbt_key_b, unknown_psbt_value),
@@ -343,2 +334,2 @@ class PSBTTest(BitcoinTestFramework):
- (input_key_a, input_value_a),
- (input_key_b, input_value_b),
+ (unknown_psbt_key_a, unknown_psbt_value),
+ (unknown_psbt_key_b, unknown_psbt_value),
@@ -347,2 +338,2 @@ class PSBTTest(BitcoinTestFramework):
- (output_key_a, output_value_a),
- (output_key_b, output_value_b),
+ (unknown_psbt_key_a, unknown_psbt_value),
+ (unknown_psbt_key_b, unknown_psbt_value),
PS: Thanks for putting me as co-author :)
Thanks, this makes sense. I adapted the test to use the unknown key/value bytes from the BIP174 example, while keeping the existing PSBTv0 and PSBTv2 coverage.
I did not use the BIP174 serialized PSBTs directly because that vector is PSBTv0-specific; instead the test still creates valid base PSBTs with createpsbt for each supported version, injects the BIP174-style unknown fields into the global/input/output maps, combines them, and checks that they are preserved.
Co-authored-by: Bicaru20 <bicaru2@gmail.com>
Rebased
ACK 658f947
Ran the full rpc_psbt.py test suite locally, and the new test passes for both PSBTv0 and PSBTv2. The test adds unknown fields to the global, input, and output maps of two PSBTs, combines them, and verifies that all fields are retained in the decoded output.
383 | + psbt2 = PSBT.from_base64(base_psbt) 384 | + psbt2.g.map[unknown_key_b] = unknown_value 385 | + psbt2.i[0].map[unknown_key_b] = unknown_value 386 | + psbt2.o[0].map[unknown_key_b] = unknown_value 387 | + 388 | + decoded = self.nodes[0].decodepsbt(self.nodes[0].combinepsbt([psbt1.to_base64(), psbt2.to_base64()]))
nit-feel-free-to-ignore: for readability I rather have combinepsbt(...) outside the arguments of decode.
combined_psbt = self.nodes[0].combinepsbt([psbt1.to_base64(), psbt2.to_base64()])
decoded = self.nodes[0].decodepsbt(combined_sbt)
concept ACK
reviewed 91830ec2659e0786db277a92a6946f3891b74efb
I think re-using the same unknown_value for both psbts and inputs and outputs can be confusing and misses some test coverage.
I think it is better to generate different values for each key, input and output. This way we make sure that there's no combination while keeping the different keys. Or even if you prefer it, we can have both, a test that ensures that same values with different keys are not combined or removed, and one that tests that different values and different keys are also not removed.
Just saw that what I commented was the first approach: a79131facdf9b72a3e06add41141911861be51d7 I prefer that approach even if it's different from the BIP example, or having both.
ACK 91830ec2659e0786db277a92a6946f3891b74efb
I ran the test with this diff to trigger a test failure, and fails as expected:
diff --git a/src/psbt.cpp b/src/psbt.cpp
index 8f2e9ab16f..f102f61954 100644
--- a/src/psbt.cpp
+++ b/src/psbt.cpp
@@ -80,7 +80,6 @@ bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt)
}
m_proprietary.insert(psbt.m_proprietary.begin(), psbt.m_proprietary.end());
- unknown.insert(psbt.unknown.begin(), psbt.unknown.end());
return true;
}
<details> <summary>Execution details</summary>
build/test/functional/rpc_psbt.py
2026-07-24T12:06:41.355236Z TestFramework (INFO): PRNG seed is: 1571865341637773668
2026-07-24T12:06:41.406104Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_x792qap1
2026-07-24T12:06:41.972193Z TestFramework (INFO): Test for invalid maximum transaction weights
2026-07-24T12:06:41.979026Z TestFramework (INFO): Test that a funded PSBT is always faithful to max_tx_weight option
2026-07-24T12:06:43.460657Z TestFramework (INFO): Test walletcreatefundedpsbt fee rate of 10000 sat/vB and 0.1 BTC/kvB produces a total fee at or slightly below -maxtxfee (~0.05290000)
2026-07-24T12:06:43.463544Z TestFramework (INFO): Test min fee rate checks with walletcreatefundedpsbt are bypassed, e.g. a fee_rate under 1 sat/vB is allowed
2026-07-24T12:06:43.465899Z TestFramework (INFO): Test min fee rate checks with walletcreatefundedpsbt are bypassed and that funding non-standard 'zero-fee' transactions is valid
2026-07-24T12:06:43.478575Z TestFramework (INFO): Test invalid fee rate settings
2026-07-24T12:06:43.490689Z TestFramework (INFO): - raises RPC error if both feeRate and fee_rate are passed
2026-07-24T12:06:43.491540Z TestFramework (INFO): - raises RPC error if both feeRate and estimate_mode passed
2026-07-24T12:06:43.492110Z TestFramework (INFO): - raises RPC error if both feeRate and conf_target are passed
2026-07-24T12:06:43.492620Z TestFramework (INFO): - raises RPC error if both fee_rate and conf_target are passed
2026-07-24T12:06:43.495301Z TestFramework (INFO): - raises RPC error if both fee_rate and estimate_mode are passed
2026-07-24T12:06:43.496057Z TestFramework (INFO): - raises RPC error with invalid estimate_mode settings
2026-07-24T12:06:43.498652Z TestFramework (INFO): - raises RPC error with invalid conf_target settings
2026-07-24T12:06:43.506923Z TestFramework (INFO): Test walletcreatefundedpsbt with too-high fee rate produces total fee well above -maxtxfee and raises RPC error
2026-07-24T12:06:43.511977Z TestFramework (INFO): Test various PSBT operations
2026-07-24T12:06:48.665595Z TestFramework (INFO): Test that unknown values are just passed through
2026-07-24T12:06:48.805403Z TestFramework (INFO): Check that non-witness UTXOs are removed for segwit v1+ inputs
2026-07-24T12:06:50.104078Z TestFramework (INFO): Check that PSBT is correctly marked as incomplete after invalid modification
2026-07-24T12:06:51.157623Z TestFramework (INFO): Crafting PSBT using an unconfirmed input
2026-07-24T12:06:51.163858Z TestFramework (INFO): Fail to craft a new PSBT that sends more funds with add_inputs = False
2026-07-24T12:06:51.166012Z TestFramework (INFO): Fail to craft a new PSBT with minconf above highest one
2026-07-24T12:06:51.167358Z TestFramework (INFO): Fail to broadcast a new PSBT with maxconf 0 due to BIP125 rules to verify it actually chose unconfirmed outputs
2026-07-24T12:06:51.172638Z TestFramework (INFO): Craft a replacement adding inputs with highest confs possible
2026-07-24T12:06:53.263901Z TestFramework (INFO): PSBT spending unspendable outputs should have error message and Creator as next
2026-07-24T12:06:53.264479Z TestFramework (INFO): PSBT with invalid values should have error message and Creator as next
2026-07-24T12:06:53.264985Z TestFramework (INFO): PSBT with signed, but not finalized, inputs should have Finalizer as next
2026-07-24T12:06:53.267015Z TestFramework (INFO): Test that we can fund psbts with external inputs specified
2026-07-24T12:06:53.348941Z TestFramework (INFO): Test signing inputs that the wallet has keys for but is not watching the scripts
2026-07-24T12:06:53.417439Z TestFramework (INFO): Test that walletprocesspsbt both updates and signs a non-updated psbt containing Taproot inputs
2026-07-24T12:06:53.434684Z TestFramework (INFO): Test walletprocesspsbt raises if an invalid sighashtype is passed
2026-07-24T12:06:53.435363Z TestFramework (INFO): Test decoding PSBT with per-input preimage types
2026-07-24T12:06:53.436236Z TestFramework (INFO): Test decoding PSBT with MuSig2 per-input and per-output types
2026-07-24T12:06:53.453387Z TestFramework (INFO): Test that combining PSBTs preserves proprietary fields
2026-07-24T12:06:53.454613Z TestFramework (INFO): Test that combining PSBTs preserves unknown fields
2026-07-24T12:06:53.456165Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
File "/home/bitcoin/test/functional/test_framework/test_framework.py", line 143, in main
self.run_test()
File "/home/bitcoin/build/test/functional/rpc_psbt.py", line 1392, in run_test
self.test_combinepsbt_preserves_unknown_fields()
File "/home/bitcoin/build/test/functional/rpc_psbt.py", line 390, in test_combinepsbt_preserves_unknown_fields
assert_equal(decoded["unknown"], unknown_fields(
(unknown_key_a, unknown_value),
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(unknown_key_b, unknown_value),
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
))
^^
File "/home/bitcoin/test/functional/test_framework/util.py", line 79, in assert_equal
raise AssertionError(f"not({thing1!s} == {thing2!s})\n in particular not({d1!s} == {d2!s})")
AssertionError: not({'f0010203040506070809': '0102030405060708090a0b0c0d0e0f'} == {'f0010203040506070809': '0102030405060708090a0b0c0d0e0f', 'f0010203040506070810': '0102030405060708090a0b0c0d0e0f'})
in particular not({} == {'f0010203040506070810': '0102030405060708090a0b0c0d0e0f'})
2026-07-24T12:06:53.510137Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
2026-07-24T12:06:53.510303Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_x792qap1
2026-07-24T12:06:53.510349Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_x792qap1/test_framework.log
2026-07-24T12:06:53.510464Z TestFramework (ERROR):
2026-07-24T12:06:53.510567Z TestFramework (ERROR): Hint: Call /home/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_x792qap1' to consolidate all logs
2026-07-24T12:06:53.510602Z TestFramework (ERROR):
2026-07-24T12:06:53.510633Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2026-07-24T12:06:53.510687Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2026-07-24T12:06:53.510719Z TestFramework (ERROR):
[node 2] Cleaning up leftover process
[node 1] Cleaning up leftover process
[node 0] Cleaning up leftover process
</details>
I think it is better to generate different values for each key, input and output. This way we make sure that there's no combination while keeping the different keys. Or even if you prefer it, we can have both, a test that ensures that same values with different keys are not combined or removed, and one that tests that different values and different keys are also not removed.
I also think it would be nice to have both test cases.
approach ACK
going through the earlier exchange above, i also think it is still worth adding a 2nd test alongside this one that gives every field its own distinct value i.e. separate values for global, input, and output, and separate values for each of the 2 copies being combined. right now, every expected dict compares equal values against equal values. distinct values just make the assertions more precise, avoiding tests that might pass coincidentally. i reckon this is mostly future-proofing instead of fixing a current bug
i think this is roughly what @polespinasa meant:
<details><summary>details</summary>
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index 249bf1b685..0f35e464f1 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -400,6 +400,58 @@ class PSBTTest(BitcoinTestFramework):
(unknown_key_b, unknown_value),
))
+ def test_combinepsbt_preserves_unknown_fields_distinct_values(self):
+ self.log.info("Test that combining PSBTs preserves unknown fields with distinct values per map")
+
+ def unknown_key(key_type, key_data):
+ return bytes([key_type]) + key_data
+
+ def unknown_fields(*entries):
+ return {key.hex(): value.hex() for key, value in entries}
+
+ unknown_key_a = unknown_key(0xf0, bytes.fromhex("010203040506070809"))
+ unknown_key_b = unknown_key(0xf0, bytes.fromhex("010203040506070810"))
+
+ # Distinct values per (psbt, map)
+ global_value_a = bytes.fromhex("0102030405060708090a0b0c0d0e0f")
+ global_value_b = bytes.fromhex("1112131415161718191a1b1c1d1e1f")
+ input_value_a = bytes.fromhex("2122232425262728292a2b2c2d2e2f")
+ input_value_b = bytes.fromhex("3132333435363738393a3b3c3d3e3f")
+ output_value_a = bytes.fromhex("4142434445464748494a4b4c4d4e4f")
+ output_value_b = bytes.fromhex("5152535455565758595a5b5c5d5e5f")
+
+ inputs = [{"txid": "ff" * 32, "vout": 0, "sequence": 0xffffffff}]
+ outputs = [{"data": "00"}]
+ for psbt_version in [0, 2]:
+ base_psbt = self.nodes[0].createpsbt(inputs=inputs, outputs=outputs, psbt_version=psbt_version)
+
+ psbt1 = PSBT.from_base64(base_psbt)
+ psbt1.g.map[unknown_key_a] = global_value_a
+ psbt1.i[0].map[unknown_key_a] = input_value_a
+ psbt1.o[0].map[unknown_key_a] = output_value_a
+
+ psbt2 = PSBT.from_base64(base_psbt)
+ psbt2.g.map[unknown_key_b] = global_value_b
+ psbt2.i[0].map[unknown_key_b] = input_value_b
+ psbt2.o[0].map[unknown_key_b] = output_value_b
+
+ combined_psbt = self.nodes[0].combinepsbt([psbt1.to_base64(), psbt2.to_base64()])
+ decoded = self.nodes[0].decodepsbt(combined_psbt)
+ assert_equal(decoded["psbt_version"], psbt_version)
+ assert_equal(decoded["unknown"], unknown_fields(
+ (unknown_key_a, global_value_a),
+ (unknown_key_b, global_value_b),
+ ))
+ assert_equal(decoded["inputs"][0]["unknown"], unknown_fields(
+ (unknown_key_a, input_value_a),
+ (unknown_key_b, input_value_b),
+ ))
+ assert_equal(decoded["outputs"][0]["unknown"], unknown_fields(
+ (unknown_key_a, output_value_a),
+ (unknown_key_b, output_value_b),
+ ))
+
def test_sighash_mismatch(self):
self.log.info("Test sighash type mismatches")
self.nodes[0].createwallet("sighash_mismatch")
@@ -1391,6 +1443,8 @@ class PSBTTest(BitcoinTestFramework):
self.test_combinepsbt_preserves_unknown_fields()
+ self.test_combinepsbt_preserves_unknown_fields_distinct_values()
+
self.log.info("Test that combining PSBTs with different transactions fails")
tx = CTransaction()
tx.vin = [CTxIn(outpoint=COutPoint(hash=int('aa' * 32, 16), n=0), scriptSig=b"")]
</details>