test: cover PSBT unknown field merging #35310

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-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35310.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

<!--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.

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.

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.