Continuing the work from #35310 gere in agreement with w0xlt
This PR adds functional coverage for combinepsbt preserving unknown PSBT fields across global, input, and output maps, as suggested here.
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/36261.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | nebula-21, winterrdog, w0xlt, polespinasa |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
There are currently four test cases:
I added all the tests cases in the same function in the test. I can separate each one into different function if the reviwers think it is clearer this way.
517 | + )) 518 | + assert_equal(decoded["outputs"][0]["unknown"], unknown_fields( 519 | + (unknown_key_a, global_value_a) 520 | + )) 521 | + 522 | + # Combining the PSBTs with the same unknown keys values and distinct fields values
# Combining PSBTs with the same unknown keys but distinct values (first PSBT wins).
Fixed!
575 | + assert_equal(decoded["outputs"][0]["unknown"], unknown_fields( 576 | + (unknown_key_a, output_value_a), 577 | + (unknown_key_b, output_value_b), 578 | + )) 579 | + 580 | def test_sighash_mismatch(self):
Just a style nit preference, it is good as is.
The same blocks with asserts are repeated over and over, I think a helper function could help hiding the asserts and just keeping the minimal information to make each case clear, improving readability.
<details> <summary>diff</summary>
$ git diff
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index 96ada3386e..6d71784a75 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -471,7 +471,7 @@ class PSBTTest(BitcoinTestFramework):
])
def test_combinepsbt_preserves_unknown_fields(self):
- self.log.info("Test that combining PSBTs preserves unknown fields with the same and with distinct values per map")
+ self.log.info("Test that combining PSBTs preserves unknown fields with the same and with distinct values per map, and when they are missing from some maps")
def unknown_key(key_type, key_data):
return bytes([key_type]) + key_data
@@ -486,6 +486,16 @@ class PSBTTest(BitcoinTestFramework):
psbt.o[0].map[key] = output_value
return psbt
+ def check_combined_unknowns(*psbts, global_unknown, input_unknown, output_unknown):
+ # Combine the PSBTs and check that the unknown fields of the global map, the
+ # first input and the first output are preserved, along with the PSBT version
+ combined_psbt = self.nodes[0].combinepsbt([psbt.to_base64() for psbt in psbts])
+ decoded = self.nodes[0].decodepsbt(combined_psbt)
+ assert_equal(decoded["psbt_version"], psbt_version)
+ assert_equal(decoded["unknown"], global_unknown)
+ assert_equal(decoded["inputs"][0]["unknown"], input_unknown)
+ assert_equal(decoded["outputs"][0]["unknown"], output_unknown)
+
unknown_key_a = unknown_key(0xf0, bytes.fromhex("010203040506070809"))
unknown_key_b = unknown_key(0xf0, bytes.fromhex("010203040506070810"))
@@ -503,79 +513,59 @@ class PSBTTest(BitcoinTestFramework):
base_psbt = self.nodes[0].createpsbt(inputs=inputs, outputs=outputs, psbt_version=psbt_version)
# Combining the PSBTs with unknown keys and fields with the same values
- combined_duplicated_psbt = self.nodes[0].combinepsbt([
- build_psbt(unknown_key_a, global_value_a, global_value_a, global_value_a).to_base64(),
- build_psbt(unknown_key_a, global_value_a, global_value_a, global_value_a).to_base64(),
- ])
- decoded = self.nodes[0].decodepsbt(combined_duplicated_psbt)
- assert_equal(decoded["psbt_version"], psbt_version)
- assert_equal(decoded["unknown"], unknown_fields(
- (unknown_key_a, global_value_a)
- ))
- assert_equal(decoded["inputs"][0]["unknown"], unknown_fields(
- (unknown_key_a, global_value_a)
- ))
- assert_equal(decoded["outputs"][0]["unknown"], unknown_fields(
- (unknown_key_a, global_value_a)
- ))
+ check_combined_unknowns(
+ build_psbt(unknown_key_a, global_value_a, global_value_a, global_value_a),
+ build_psbt(unknown_key_a, global_value_a, global_value_a, global_value_a),
+ global_unknown=unknown_fields((unknown_key_a, global_value_a)),
+ input_unknown=unknown_fields((unknown_key_a, global_value_a)),
+ output_unknown=unknown_fields((unknown_key_a, global_value_a)),
+ )
# Combining the PSBTs with the same unknown keys values and distinct fields values
# First PSBT wins
- combined_psbt_same_keys = self.nodes[0].combinepsbt([
- build_psbt(unknown_key_a, global_value_a, global_value_a, global_value_a).to_base64(),
- build_psbt(unknown_key_a, global_value_b, global_value_b, global_value_b).to_base64(),
- ])
- decoded = self.nodes[0].decodepsbt(combined_psbt_same_keys)
- assert_equal(decoded["psbt_version"], psbt_version)
- assert_equal(decoded["unknown"], unknown_fields(
- (unknown_key_a, global_value_a)
- ))
- assert_equal(decoded["inputs"][0]["unknown"], unknown_fields(
- (unknown_key_a, global_value_a)
- ))
- assert_equal(decoded["outputs"][0]["unknown"], unknown_fields(
- (unknown_key_a, global_value_a)
- ))
+ check_combined_unknowns(
+ build_psbt(unknown_key_a, global_value_a, global_value_a, global_value_a),
+ build_psbt(unknown_key_a, global_value_b, global_value_b, global_value_b),
+ global_unknown=unknown_fields((unknown_key_a, global_value_a)),
+ input_unknown=unknown_fields((unknown_key_a, global_value_a)),
+ output_unknown=unknown_fields((unknown_key_a, global_value_a)),
+ )
# Combining PSBTs with unknown fields with the same values
- combined_psbt_same_fields = self.nodes[0].combinepsbt([
- build_psbt(unknown_key_a, global_value_a, global_value_a, global_value_a).to_base64(),
- build_psbt(unknown_key_b, global_value_a, global_value_a, global_value_a).to_base64(),
- ])
- decoded = self.nodes[0].decodepsbt(combined_psbt_same_fields)
- assert_equal(decoded["psbt_version"], psbt_version)
- assert_equal(decoded["unknown"], unknown_fields(
- (unknown_key_a, global_value_a),
- (unknown_key_b, global_value_a),
- ))
- assert_equal(decoded["inputs"][0]["unknown"], unknown_fields(
- (unknown_key_a, global_value_a),
- (unknown_key_b, global_value_a),
- ))
- assert_equal(decoded["outputs"][0]["unknown"], unknown_fields(
- (unknown_key_a, global_value_a),
- (unknown_key_b, global_value_a),
- ))
+ check_combined_unknowns(
+ build_psbt(unknown_key_a, global_value_a, global_value_a, global_value_a),
+ build_psbt(unknown_key_b, global_value_a, global_value_a, global_value_a),
+ global_unknown=unknown_fields(
+ (unknown_key_a, global_value_a),
+ (unknown_key_b, global_value_a),
+ ),
+ input_unknown=unknown_fields(
+ (unknown_key_a, global_value_a),
+ (unknown_key_b, global_value_a),
+ ),
+ output_unknown=unknown_fields(
+ (unknown_key_a, global_value_a),
+ (unknown_key_b, global_value_a),
+ ),
+ )
# Combining PSBTs preserves unknown fields with distinct values
- combined_psbt = self.nodes[0].combinepsbt([
- build_psbt(unknown_key_a, global_value_a, input_value_a, output_value_a).to_base64(),
- build_psbt(unknown_key_b, global_value_b, input_value_b, output_value_b).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),
- ))
+ check_combined_unknowns(
+ build_psbt(unknown_key_a, global_value_a, input_value_a, output_value_a),
+ build_psbt(unknown_key_b, global_value_b, input_value_b, output_value_b),
+ global_unknown=unknown_fields(
+ (unknown_key_a, global_value_a),
+ (unknown_key_b, global_value_b),
+ ),
+ input_unknown=unknown_fields(
+ (unknown_key_a, input_value_a),
+ (unknown_key_b, input_value_b),
+ ),
+ output_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")
</details>
I'd rather keep it as is. In my opinion it is clearer this way. If more reviwers think you way is better I can change it.
I'd rather keep it as is
+1
i lean in this direction too. although it is verbose, i find it straightforward to read
Concept ACK
I would add another test case. What if one of the two PSBTs does not have a value for the unknown field. So pass None.
<details> <summary>diff</summary>
$ git diff
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index 96ada3386e..76250ae69b 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -471,7 +471,7 @@ class PSBTTest(BitcoinTestFramework):
])
def test_combinepsbt_preserves_unknown_fields(self):
- self.log.info("Test that combining PSBTs preserves unknown fields with the same and with distinct values per map")
+ self.log.info("Test that combining PSBTs preserves unknown fields with the same and with distinct values per map.")
def unknown_key(key_type, key_data):
return bytes([key_type]) + key_data
@@ -480,10 +480,14 @@ class PSBTTest(BitcoinTestFramework):
return {key.hex(): value.hex() for key, value in entries}
def build_psbt(key, global_value, input_value, output_value):
+ # A value of None leaves the corresponding map without the unknown field
psbt = PSBT.from_base64(base_psbt)
- psbt.g.map[key] = global_value
- psbt.i[0].map[key] = input_value
- psbt.o[0].map[key] = output_value
+ if global_value is not None:
+ psbt.g.map[key] = global_value
+ if input_value is not None:
+ psbt.i[0].map[key] = input_value
+ if output_value is not None:
+ psbt.o[0].map[key] = output_value
return psbt
unknown_key_a = unknown_key(0xf0, bytes.fromhex("010203040506070809"))
@@ -577,6 +581,24 @@ class PSBTTest(BitcoinTestFramework):
(unknown_key_b, output_value_b),
))
+ # Combining PSBTs with unknown fields missing from the maps of the other PSBT
+ # The fields of each map are preserved regardless of which PSBT they come from
+ combined_psbt = self.nodes[0].combinepsbt([
+ build_psbt(unknown_key_a, global_value_a, None, None).to_base64(),
+ build_psbt(unknown_key_a, None, input_value_b, output_value_b).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)
+ ))
+ assert_equal(decoded["inputs"][0]["unknown"], unknown_fields(
+ (unknown_key_a, input_value_b)
+ ))
+ assert_equal(decoded["outputs"][0]["unknown"], unknown_fields(
+ (unknown_key_a, output_value_b)
+ ))
+
def test_sighash_mismatch(self):
self.log.info("Test sighash type mismatches")
self.nodes[0].createwallet("sighash_mismatch")
</details>
Co-authored-by: bicaru20 <bicaru2@gmail.com>
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
Co-authored-by: polespinasa <pol.espinasa@uab.cat>
Added the new test case that was suggested and addressed the nits.
Also added @polespinasa as Co-author.
Approach ACK
559 | + (unknown_key_a, global_value_a), 560 | + (unknown_key_b, global_value_a), 561 | + )) 562 | + 563 | + # Combining PSBTs preserves unknown fields with distinct values 564 | + combined_psbt = self.nodes[0].combinepsbt([
Nit:
- combined_psbt = self.nodes[0].combinepsbt([
+ combined_psbt_distinct_fields = self.nodes[0].combinepsbt([
580 | + (unknown_key_b, output_value_b), 581 | + )) 582 | + 583 | + # Combining PSBTs with unknown fields missing from the maps of the other PSBT 584 | + # The fields of each map are preserved regardless of which PSBT they come from 585 | + combined_psbt = self.nodes[0].combinepsbt([
Nit:
- combined_psbt = self.nodes[0].combinepsbt([
+ combined_psbt_one_empty = self.nodes[0].combinepsbt([
ACK 21e4ea8134d8ca95416e065490d99f82be66bd67
Prior to this PR, this diff would pass all the tests:
diff --git a/src/psbt.cpp b/src/psbt.cpp
index 51fb19591a..509bc251b3 100644
--- a/src/psbt.cpp
+++ b/src/psbt.cpp
@@ -68,7 +68,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;
}
After adding coverage with this PR, the test fails as expected with the diff:
<details>
2026-09-17T15:30:35.196205Z TestFramework (INFO): Test that combining PSBTs preserves unknown fields with the same and with distinct values per map.
2026-09-17T15:30:35.199960Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
File "bitcoin/test/functional/test_framework/test_framework.py", line 145, in main
self.run_test()
~~~~~~~~~~~~~^^
File "bitcoin/build/test/functional/rpc_psbt.py", line 1700, in run_test
self.test_combinepsbt_preserves_unknown_fields()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "bitcoin/build/test/functional/rpc_psbt.py", line 550, in test_combinepsbt_preserves_unknown_fields
assert_equal(decoded["unknown"], unknown_fields(
~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(unknown_key_a, global_value_a),
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(unknown_key_b, global_value_a),
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
))
</details>
536 | + assert_equal(decoded["inputs"][0]["unknown"], unknown_fields( 537 | + (unknown_key_a, global_value_a) 538 | + )) 539 | + assert_equal(decoded["outputs"][0]["unknown"], unknown_fields( 540 | + (unknown_key_a, global_value_a) 541 | + ))
would it be worth also testing the "same unknown keys but distinct values" case.. but with the PSBTs reversed ?
the current test confirms that the first PSBT wins, but only for one ordering.
reversing them would confirm that the merge fully honours the PSBT ordering
thus verifying the ordering full circle, back to back: A + B => A, then
B + A => B.
<details> <summary>suggested diff </summary>
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index 3e24e125c7..612adbecfa 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -539,8 +539,26 @@ class PSBTTest(BitcoinTestFramework):
assert_equal(decoded["outputs"][0]["unknown"], unknown_fields(
(unknown_key_a, global_value_a)
))
+ # Combining reversed PSBTs with the same unknown keys but distinct values (first PSBT wins).
+ # Similar to the test case above but with reversed values
+ combined_rev_psbt_same_keys = self.nodes[0].combinepsbt([
+ build_psbt(unknown_key_a, global_value_b, global_value_b, global_value_b).to_base64(),
+ build_psbt(unknown_key_a, global_value_a, global_value_a, global_value_a).to_base64(),
+ ])
+ decoded = self.nodes[0].decodepsbt(combined_rev_psbt_same_keys)
+ assert_equal(decoded["psbt_version"], psbt_version)
+ assert_equal(decoded["unknown"], unknown_fields(
+ (unknown_key_a, global_value_b)
+ ))
+ assert_equal(decoded["inputs"][0]["unknown"], unknown_fields(
+ (unknown_key_a, global_value_b)
+ ))
+ assert_equal(decoded["outputs"][0]["unknown"], unknown_fields(
+ (unknown_key_a, global_value_b)
+ ))
+
# Combining PSBTs with unknown fields with the same values
combined_psbt_same_fields = self.nodes[0].combinepsbt([
build_psbt(unknown_key_a, global_value_a, global_value_a, global_value_a).to_base64(),
build_psbt(unknown_key_b, global_value_a, global_value_a, global_value_a).to_base64(),
</details>
thoughts ?
Since combinepsbt processes inputs sequentially, testing A + B => A already covers the order of operations for merging. Adding the same case with another order, I think it is a bit redundant.
tACK 21e4ea8134d8ca95416e065490d99f82be66bd67
successfully built and tested on this toolchain: Debian/clang++-18/x86_64.
the tested cases look great to me. i left a single suggestion
ACK 21e4ea8134d8ca95416e065490d99f82be66bd67
ACK 21e4ea8134d8ca95416e065490d99f82be66bd67