psbt: preserve proprietary fields when combining PSBTs #34893

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.

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

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

Reviewers, this pull request conflicts with the following ones:

• #35310 (test: cover PSBT unknown field merging by w0xlt)

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

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.

The CI error seems unrelated.

The CI error seems unrelated.

Yea it's #34782.

Concept ACK

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

Concept ACK

tACK eb76e953acc0bcb392dfe3e943f2843f5e462732

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>

re-ACK da769855d0ce60c4a5ded844a4d4c43917a6e027

only rebased off master since prior tACK

@Bicaru20 Good suggestion. Since this test is not directly related to the changes introduced here, I opened a separate PR for it and added you as co-author. https://github.com/bitcoin/bitcoin/pull/35310

re-ACK da769855d0

Code-review ACK da769855d0ce60c4a5ded844a4d4c43917a6e027

ACK da769855d0ce60c4a5ded844a4d4c43917a6e027