psbt: preserve proprietary fields when combining PSBTs #34893

w0xlt commented at 7:56 AM on March 22, 2026: contributor

BIP 174 proprietary fields are currently parsed, serialized, and exposed by decodepsbt, but they are not preserved by combinepsbt.

The reason is that the merge paths in PartiallySignedTransaction::Merge(), PSBTInput::Merge(), and PSBTOutput::Merge() union unknown, but never union m_proprietary.

This means application-specific PSBT metadata can be lost during combination, even though BIP 174 treats proprietary records as normal PSBT key-value pairs for private or application-specific use.

This PR fixes that by preserving proprietary fields in all three merge paths.

DrahtBot added the label PSBT on Mar 22, 2026

DrahtBot commented at 7:56 AM on March 22, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	nervana21, Bicaru20
Concept ACK	theStack

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34765 (rpc: detect invalid signatures in analyzepsbt by overcookedpanda)
#21283 (Implement BIP 370 PSBTv2 by achow101)

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.

rkrux commented at 11:49 AM on March 23, 2026: contributor

Good catch, will review. Surprised that merging proprietary fields was missed but merging unknown fields wasn't.

In the commit message of test: add PSBT proprietary merge regression coverage:

I'm confused by this statement.

These tests document the expected behavior before the implementation fix, so the follow-up commit can be reviewed by dropping it and observing the failure.

This is the second commit in the PR, the fix is in the preceding commit not in the follow-up one.

in src/test/psbt_tests.cpp:36 in 8cd41daf9a

  31 | +    const auto global_left = MakeProprietary(PSBT_GLOBAL_PROPRIETARY, 1, 0xaa);
  32 | +    const auto global_right = MakeProprietary(PSBT_GLOBAL_PROPRIETARY, 2, 0xbb);
  33 | +    const auto input_left = MakeProprietary(PSBT_IN_PROPRIETARY, 3, 0xcc);
  34 | +    const auto input_right = MakeProprietary(PSBT_IN_PROPRIETARY, 4, 0xdd);
  35 | +    const auto output_left = MakeProprietary(PSBT_OUT_PROPRIETARY, 5, 0xee);
  36 | +    const auto output_right = MakeProprietary(PSBT_OUT_PROPRIETARY, 6, 0xff);

achow101 commented at 9:16 PM on March 23, 2026:

In 8cd41daf9a5ad9ca460c2ed9b2050d21008ecf6e "test: add PSBT proprietary merge regression coverage"

It's not necessary to have unique fields for each scope. This could just be 2 PSBTProprietary that is reused across the scopes.

w0xlt commented at 11:00 PM on March 24, 2026:

Done. Thanks.

in src/test/psbt_tests.cpp:12 in 8cd41daf9a

   7 | +
   8 | +#include <boost/test/unit_test.hpp>
   9 | +
  10 | +BOOST_FIXTURE_TEST_SUITE(psbt_tests, BasicTestingSetup)
  11 | +
  12 | +static PSBTProprietary MakeProprietary(uint8_t scope_type, uint8_t unique_tag, uint8_t value_tag)

achow101 commented at 9:16 PM on March 23, 2026:

In 8cd41daf9a5ad9ca460c2ed9b2050d21008ecf6e "test: add PSBT proprietary merge regression coverage"

Calling the subtype and value "tag" is weird, that is not terminology used anywhere else in PSBTs.

w0xlt commented at 11:00 PM on March 24, 2026:

Done. Thanks.

in src/test/psbt_tests.cpp:17 in 8cd41daf9a

  12 | +static PSBTProprietary MakeProprietary(uint8_t scope_type, uint8_t unique_tag, uint8_t value_tag)
  13 | +{
  14 | +    return PSBTProprietary{
  15 | +        .subtype = unique_tag,
  16 | +        .identifier = {'p', 's', 'b', 't'},
  17 | +        .key = {scope_type, unique_tag},

achow101 commented at 9:18 PM on March 23, 2026:

In 8cd41daf9a5ad9ca460c2ed9b2050d21008ecf6e "test: add PSBT proprietary merge regression coverage"

Using the proprietary type as part of key data is odd. key can be any garbage, and having it be specifically these values is a bit confusing.

w0xlt commented at 11:00 PM on March 24, 2026:

Done. Thanks.

in test/functional/rpc_psbt.py:1175 in 8cd41daf9a outdated

1173 | @@ -1170,6 +1174,64 @@ def test_psbt_input_keys(psbt_input, keys):
1174 |  
1175 |          self.test_decodepsbt_musig2_input_output_types()

luke-jr commented at 11:34 PM on March 23, 2026:

Would probably be good to follow the method-for-test pattern here

w0xlt commented at 11:00 PM on March 24, 2026:

Done. Thanks.

psbt: preserve proprietary fields when combining PSBTs

CombinePSBTs currently preserves unknown records but drops proprietary records at the global, input, and output levels because the Merge() paths never union m_proprietary.

Preserve proprietary records in PartiallySignedTransaction::Merge(), PSBTInput::Merge(), and PSBTOutput::Merge() so combine/merge keeps all PSBT key-value data.

5e9b722494

w0xlt force-pushed on Mar 24, 2026

test: add PSBT proprietary merge regression coverage

Add unit and functional regression tests asserting that combine/merge preserves proprietary fields at the global, input, and output scopes.

eb76e953ac

w0xlt force-pushed on Mar 24, 2026

DrahtBot added the label CI failed on Mar 24, 2026

w0xlt commented at 1:14 AM on March 25, 2026: contributor

The CI error seems unrelated.

fanquake commented at 1:16 AM on March 25, 2026: member

The CI error seems unrelated.

Yea it's #34782.

DrahtBot removed the label CI failed on Mar 25, 2026

luke-jr referenced this in commit d2538a0b50 on Apr 13, 2026

luke-jr referenced this in commit f3aef7ae21 on Apr 13, 2026

nervana21 commented at 11:58 PM on April 19, 2026: contributor

Concept ACK

small nit: psbt_tests.cpp should be moved to after private_broadcast_tests.cpp for alphabetical ordering

theStack commented at 1:28 PM on April 23, 2026: contributor

Concept ACK

bitcoin deleted a comment on Apr 30, 2026

nervana21 commented at 8:08 PM on May 3, 2026: contributor

tACK eb76e953acc0bcb392dfe3e943f2843f5e462732

DrahtBot requested review from theStack on May 3, 2026

DrahtBot added the label Needs rebase on May 5, 2026

DrahtBot commented at 1:42 PM on May 5, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Bicaru20 commented at 4:23 PM on May 6, 2026: none

ACK eb76e953acc0bcb392dfe3e943f2843f5e462732. I tested the code by combining two psbt and the proprietary fields were correctly combined.

While reviewing the the code, I notcied there is no functional test to check that the unknown fields are correctly combined. Would it make sense to add a test for this?

<details> <summary>Following the same pattern, I though of something like this:</summary>

      def unkown_key(type_byte, keydata=b""):
          return bytes([type_byte]) + keydata
          
      psbt3 = PSBT(
          g=PSBTMap({
              PSBT_GLOBAL_UNSIGNED_TX: tx.serialize(),
              unkown_key(0xda,b"\x01"): b"\xaa",
          }),
          i=[PSBTMap({
              unkown_key(0xda,b"\x03"): b"\xbb",
          })],
          o=[PSBTMap({
              unkown_key(0xda,b"\x05"): b"\xcc",
          })],
      ).to_base64()
      
      psbt4 = PSBT(
          g=PSBTMap({
              PSBT_GLOBAL_UNSIGNED_TX: tx.serialize(),
              unkown_key(0xda,b"\x02"): b"\xaa",
          }),
          i=[PSBTMap({
              unkown_key(0xda,b"\x04"): b"\xbb",
          })],
          o=[PSBTMap({
              unkown_key(0xda,b"\x06"): b"\xcc",
          })],
      ).to_base64()

      decoded = self.nodes[0].decodepsbt(self.nodes[0].combinepsbt([psbt3, psbt4]))
      assert_equal(...)

</details>