psbt: avoid duplicate global xpub keys when merging #35665

thomasbuilds commented at 8:43 AM on July 6, 2026: contributor

Global xpubs are stored in a map of key origin to set of xpubs, while the serialization writes one record per xpub, keyed by the xpub. Merge unions the map origin-by-origin, so when the combined PSBTs provide different key origins for the same xpub, the result serializes the same PSBT_GLOBAL_XPUB key twice. BIP 174 declares PSBTs with duplicate keys invalid and the deserializer rejects them, so combinepsbt returns a PSBT that no RPC can parse again. This affects all releases since the merge loop was added in #17034 (v23.0).

<details><summary>Reproduction on master</summary>

The PSBTs share the unsigned transaction and xpub, and differ only in the master fingerprint of the global xpub record (00000000 vs 11111111):

$ A=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAD/////AQAAAAAAAAAAAAAAAABPAQQ1h88AAAAAAAAAAACHPf+BwC9SViP9H+UWfqw6VaBJ3j0xS7Qu4if/7TfVCAM5o2ATMBWX2u9B++WToCzFE9C1VSfsLfEFDi6P9JyFwgQAAAAAAAAA
$ B=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAD/////AQAAAAAAAAAAAAAAAABPAQQ1h88AAAAAAAAAAACHPf+BwC9SViP9H+UWfqw6VaBJ3j0xS7Qu4if/7TfVCAM5o2ATMBWX2u9B++WToCzFE9C1VSfsLfEFDi6P9JyFwgQRERERAAAA
$ bitcoin-cli -regtest decodepsbt "$(bitcoin-cli -regtest combinepsbt "[\"$A\",\"$B\"]")"
error code: -22
error message:
TX decode failed Duplicate Key, global key "01043587cf00...9c85c2" already provided: iostream error

</details>

Deduplicate by xpub when merging, keeping the origin that is already present: BIP 174 lets the Combiner "pick arbitrarily when conflicts occur", and conflicting unknown and proprietary records are already resolved the same way. The logic is shared between combinepsbt and joinpsbts through a new MergeGlobalXPubs helper. The second commit adds a test that fails on master with the error above, and the last commit removes the global_xpubs tracking set in Unserialize, write-only since the generic duplicate key check introduced in #21283 (1e2d146b47) replaced the explicit one.

Note: the xpub loop in joinpsbts currently has no observable effect, since the collected xpubs never reach the returned PSBT. My #35516 fixes that, so this PR should land first: on its own, #35516 would make the same duplicate key issue reachable through joinpsbts, while with the shared helper in place it never becomes reachable. I will rebase #35516 on top afterwards.

psbt: avoid duplicate global xpub keys when merging

The global xpubs are stored in a map of key origin to set of xpubs,
while the serialization writes one record per xpub, keyed by the xpub.
Merging two PSBTs origin-by-origin can store the same xpub under two
origins, which serializes as a PSBT with duplicate keys that the
deserializer (and BIP 174) reject. Notably, combinepsbt returned a
PSBT that no RPC could parse again when the combined PSBTs provide
different key origins for the same xpub.

Deduplicate by xpub when merging, keeping the origin that is already
present, and share the logic between combinepsbt and joinpsbts.

7c632c0e2a

test: check combinepsbt with conflicting global xpub origins 3b7051c7e3

psbt: remove write-only global xpub tracking set

The explicit duplicate global xpub check was replaced by the generic
duplicate key check in 1e2d146b47, leaving the set unused.

6d387af562

DrahtBot added the label PSBT on Jul 6, 2026

DrahtBot commented at 8:43 AM on July 6, 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/35665.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	winterrdog, Bicaru20

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:

#35516 (rpc: preserve global xpubs and proprietary fields in joinpsbts by thomasbuilds)
#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.

ghost commented at 12:42 AM on July 11, 2026: none

Reproduced the bug on v31.0.0 with the description's vectors (isolated regtest): combinepsbt succeeds, decodepsbt fails with "Duplicate key, global xpub already provided" — and the fix's approach reads correct to us (keep-first matches BIP 174's combiner language and the existing unknown/proprietary handling; the removed global_xpubs set is indeed write-only in master since #21283).

Two small suggestions from our review:

Keep-first makes the surviving master_fingerprint depend on combinepsbt argument order (["A","B"] -> A's, ["B","A"] -> B's). Worth a line in the RPC help and/or a reversed-order assertion in the new test, so the policy is pinned rather than incidental?
joinpsbts shares the new helper but gets no regression test — a twin case seems like cheap insurance.

Disclosure: this review was produced by two AI agents (Claude/Anthropic + Grok/xAI) in a dual-review protocol with a human operator; the reproduction was run by hand on a local node. Full labeled notes: https://github.com/VitaAI-SCG/one-gpu-lab/blob/main/reviews/bitcoin-core-35665.md — posting only because we believe the two items above are actionable; happy to be told otherwise.

pinheadmz commented at 1:02 AM on July 11, 2026: member

AI agents @VitaAI-SCG thanks but I don't think this kind of comment is helpful. LLMs are great tools but it takes human resources to process their output. If you don't understand the code change in the pull request yourself, then you can't be responsible for the review comment, and that means it's just taking up someone else's time.

winterrdog commented at 10:00 AM on July 15, 2026: contributor

<details><summary>Reproduction on master</summary>

The PSBTs share the unsigned transaction and xpub, and differ only in the > master fingerprint of the global xpub record (00000000 vs 11111111):

$ A=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAD/////AQA> AAAAAAAAAAAAAAABPAQQ1h88AAAAAAAAAAACHPf+BwC9SViP9H+UWfqw6VaBJ3j0xS7Qu4if/> 7TfVCAM5o2ATMBWX2u9B++WToCzFE9C1VSfsLfEFDi6P9JyFwgQAAAAAAAAA
$ B=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAD/////AQA> AAAAAAAAAAAAAAABPAQQ1h88AAAAAAAAAAACHPf+BwC9SViP9H+UWfqw6VaBJ3j0xS7Qu4if/> 7TfVCAM5o2ATMBWX2u9B++WToCzFE9C1VSfsLfEFDi6P9JyFwgQRERERAAAA
$ bitcoin-cli -regtest decodepsbt "$(bitcoin-cli -regtest combinepsbt > "[\"$A\",\"$B\"]")"
error code: -22
error message:
TX decode failed Duplicate Key, global key "01043587cf00...9c85c2" already > provided: iostream error

</details>

confirmed!

i was able to reproduce this locally on master@ 7bff765d514e0bdc8e6b2d156d3de0dd3800ab1d as well, so the issue is still present. here is a recording of my reproduction:

in src/psbt.cpp:92 in 6d387af562

  87 | +        for (const CExtPubKey& xpub : xpubs) {
  88 | +            const bool known{std::ranges::any_of(m_xpubs, [&](const auto& entry) { return entry.second.contains(xpub); })};
  89 | +            if (!known) m_xpubs[origin].insert(xpub);
  90 | +        }
  91 | +    }
  92 | +}

winterrdog commented at 11:37 AM on July 15, 2026:

nit: what do you think about pulling this into a small has_xpub lambda? i think it reads a little easier and keeps the loop focused on the merge logic, but please feel free to ignore this if you prefer the current version

sth like this:

diff --git a/src/psbt.cpp b/src/psbt.cpp
index 3d696da7fd..2660b97862 100644
--- a/src/psbt.cpp
+++ b/src/psbt.cpp
@@ -83,10 +83,14 @@ bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt)

 void PartiallySignedTransaction::MergeGlobalXPubs(const PartiallySignedTransaction& psbt)
 {
+    auto has_xpub = [&](const CExtPubKey& xpub) {
+        return std::ranges::any_of(m_xpubs, [&](const auto& entry) {
+            return entry.second.contains(xpub);
+        });
+    };
     for (const auto& [origin, xpubs] : psbt.m_xpubs) {
         for (const CExtPubKey& xpub : xpubs) {
-            const bool known{std::ranges::any_of(m_xpubs, [&](const auto& entry) { return entry.second.contains(xpub); })};
-            if (!known) m_xpubs[origin].insert(xpub);
+            if (!has_xpub(xpub)) m_xpubs[origin].insert(xpub);
         }
     }
 }

thomasbuilds commented at 9:14 AM on July 17, 2026:

I think it's fine as is

in test/functional/rpc_psbt.py:386 in 6d387af562

 381 | +            ).to_base64()
 382 | +
 383 | +        combined = self.nodes[0].combinepsbt([psbt_with_origin(b"\x00\x00\x00\x00"), psbt_with_origin(b"\x11\x11\x11\x11")])
 384 | +        # The same xpub under both origins would serialize as duplicate keys, making the combined PSBT unparseable
 385 | +        decoded = self.nodes[0].decodepsbt(combined)
 386 | +        assert_equal(decoded["global_xpubs"], [{"xpub": xpub, "master_fingerprint": "00000000", "path": "m"}])

winterrdog commented at 11:58 AM on July 15, 2026:

based on the PR description:

The logic is shared between combinepsbt and joinpsbts through a new MergeGlobalXPubs helper.

would it be worth adding a joinpsbts case to the functional test as well, so that both RPCs have regression coverage ?

<details> <summary>i have sketched a possible diff here</summary>

diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index bedab68725..7f0300e654 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -362,15 +362,14 @@ class PSBTTest(BitcoinTestFramework):
     def test_combinepsbt_global_xpub_origin_conflict(self):
         self.log.info("Test that combining PSBTs with conflicting origins for the same xpub keeps a single record")

-        tx = CTransaction()
-        tx.vin = [CTxIn(outpoint=COutPoint(hash=int('aa' * 32, 16), n=0), scriptSig=b"")]
-        tx.vout = [CTxOut(nValue=0, scriptPubKey=b"")]
-
         xpub = "tpubD6NzVbkrYhZ4XgiXtGrdW5XDAPFCL9h7we1vwNCpn8tGbBcgfVYjXyhWo4E1xkh56hjod1RhGjxbaTLV3X4FyWuejifB9jusQ46QzG87VKp"
         xpub_data, xpub_version = base58_to_byte(xpub)
         xpub_key = bytes([PSBT_GLOBAL_XPUB]) + bytes([xpub_version]) + xpub_data

-        def psbt_with_origin(fingerprint):
+        def psbt_with_origin(fingerprint, tx_input_hash_byte=b"a"):
+            tx = CTransaction()
+            tx.vin = [CTxIn(outpoint=COutPoint(hash=int(tx_input_hash_byte.hex() * 32, 16), n=0), scriptSig=b"")]
+            tx.vout = [CTxOut(nValue=0, scriptPubKey=b"")]
             return PSBT(
                 g=PSBTMap({
                     PSBT_GLOBAL_UNSIGNED_TX: tx.serialize(),
@@ -385,6 +384,20 @@ class PSBTTest(BitcoinTestFramework):
         decoded = self.nodes[0].decodepsbt(combined)
         assert_equal(decoded["global_xpubs"], [{"xpub": xpub, "master_fingerprint": "00000000", "path": "m"}])

+        # joinpsbts operates on DIFFERENT transactions (must have distinct inputs)
+        psbt_a = psbt_with_origin(b"\x00\x00\x00\x00", b"a")
+        psbt_b = psbt_with_origin(b"\x11\x11\x11\x11", b"b")  # different input UTXO
+        joined = self.nodes[0].joinpsbts([psbt_a, psbt_b])
+        decoded_joined = self.nodes[0].decodepsbt(joined)
+
+        # TODO::
+        # once [#35516](/bitcoin-bitcoin/35516/) lands, we can uncomment this assertion to check for the "actual" retained origin dictionary and then
+        # remove the assertion (below this one) that checks for an empty list
+        # assert_equal(decoded_joined["global_xpubs"], [{"xpub": xpub, "master_fingerprint": "00000000", "path": "m"}])
+
+        # without [#35516](/bitcoin-bitcoin/35516/), global_xpubs are dropped here, but this verifies it merges without a duplicate key crash.
+        assert_equal(decoded_joined["global_xpubs"], [])
+
     def test_sighash_mismatch(self):
         self.log.info("Test sighash type mismatches")
         self.nodes[0].createwallet("sighash_mismatch")

</details>

a small caveat

since #35516 has not landed yet, joinpsbts currently does not propagate global_xpubs to the returned PSBT. the assertion below is only checking that the merge succeeds without hitting the duplicate-key error. once #35516 lands, this could be updated to assert the retained origin instead.

sth like this (also shown in the diff above):

assert_equal(decoded_joined["global_xpubs"], [{"xpub": xpub, "master_fingerprint": "00000000", "path": "m"}])

what do you think of this ?

thomasbuilds commented at 9:36 AM on July 17, 2026:

This PR should land first though, so I'd rather add the join case in #35516 after the rebase

winterrdog commented at 9:39 AM on July 17, 2026:

Alright 👍

The feeling is mutual

winterrdog commented at 5:23 PM on July 17, 2026: contributor

tACK 6d387af562fc85eee003a269dbea10963e941985

the reproduction from the PR description no longer throws (works as advertised). deduplicating by xpub (rather than origin) feels right since PSBT_GLOBAL_XPUB records are keyed by xpub on the wire. the global_xpubs removal also makes sense since the generic duplicate-key check in Unserialize already catches the same case earlier in the read loop

<details> <summary>now, reproducing the issue gives me this</summary>

$ A=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAD/////AQAAAAAAAAAAAAAAAABPAQQ1h88AAAAAAAAAAACHPf+BwC9SViP9H+UWfqw6VaBJ3j0xS7Qu4if/7TfVCAM5o2ATMBWX2u9B++WToCzFE9C1VSfsLfEFDi6P9JyFwgQAAAAAAAAA
$ B=cHNidP8BADwCAAAAAaqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqAAAAAAD/////AQAAAAAAAAAAAAAAAABPAQQ1h88AAAAAAAAAAACHPf+BwC9SViP9H+UWfqw6VaBJ3j0xS7Qu4if/7TfVCAM5o2ATMBWX2u9B++WToCzFE9C1VSfsLfEFDi6P9JyFwgQRERERAAAA
$ bitcoin-cli -regtest decodepsbt "$(bitcoin-cli -regtest combinepsbt "[\"$A\",\"$B\"]")"
{
  "tx": {
    "txid": "ad8ea7b4fef818e062b2385538661288c2258124108e01b96f8e3e5ec943474c",
    "hash": "ad8ea7b4fef818e062b2385538661288c2258124108e01b96f8e3e5ec943474c",
    "version": 2,
    "size": 60,
    "vsize": 60,
    "weight": 240,
    "locktime": 0,
    "vin": [
      {
        "txid": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
        "vout": 0,
        "scriptSig": {
          "asm": "",
          "hex": ""
        },
        "sequence": 4294967295
      }
    ],
    "vout": [
      {
        "value": 0.00000000,
        "n": 0,
        "scriptPubKey": {
          "asm": "",
          "desc": "raw()#58lrscpx",
          "hex": "",
          "type": "nonstandard"
        }
      }
    ]
  },
  "global_xpubs": [
    {
      "xpub": "tpubD6NzVbkrYhZ4XgiXtGrdW5XDAPFCL9h7we1vwNCpn8tGbBcgfVYjXyhWo4E1xkh56hjod1RhGjxbaTLV3X4FyWuejifB9jusQ46QzG87VKp",
      "master_fingerprint": "00000000",
      "path": "m"
    }
  ],
  "psbt_version": 0,
  "proprietary": [
  ],
  "unknown": {
  },
  "inputs": [
    {
    }
  ],
  "outputs": [
    {
    }
  ]
}

</details>

Bicaru20 commented at 5:41 PM on July 20, 2026: none

tACK 6d387af562.

I reproduced the error described in the description before the changes were made. After the new changes, I tried again and the error was no longer thrown.

In this pr, the MergeGlobalXPubs function is introduced. It essentially looks at the xpub of both psbt when merging and makes sure that there are no duplications with diferent key origin. In the case where the xpub is duplicated with differents origin-key, it just keeps the first key origin and makes sure that the xpub is added only one time to the combined psbt. Thanks to this function, the duplication of global xpub no longer happens. Also, a functional test for combining psbts with conflicting origins is added to ensure there are no duplicated xpub.

I just have one non-blocking question: Do you think is worth adding a warning message so the user can see it has two psbts with the same xpub but a different key origin? I am not sure if it is worth adding as I don't know if there is any case were this warning would be helpful.