diff --git a/src/psbt.cpp b/src/psbt.cpp index d1a688e6fbb..9c595abc450 100644 --- a/src/psbt.cpp +++ b/src/psbt.cpp @@ -13,6 +13,8 @@ #include #include +#include + using common::PSBTError; PartiallySignedTransaction::PartiallySignedTransaction(const CMutableTransaction& tx, uint32_t version) : m_version(version) @@ -53,13 +55,7 @@ bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt) return false; } } - for (auto& xpub_pair : psbt.m_xpubs) { - if (!m_xpubs.contains(xpub_pair.first)) { - m_xpubs[xpub_pair.first] = xpub_pair.second; - } else { - m_xpubs[xpub_pair.first].insert(xpub_pair.second.begin(), xpub_pair.second.end()); - } - } + MergeGlobalXPubs(psbt); if (fallback_locktime == std::nullopt && psbt.fallback_locktime != std::nullopt) fallback_locktime = psbt.fallback_locktime; // Set m_tx_modifiable only if either PSBT had it set @@ -80,6 +76,16 @@ bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt) return true; } +void PartiallySignedTransaction::MergeGlobalXPubs(const PartiallySignedTransaction& psbt) +{ + 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); + } + } +} + std::optional PartiallySignedTransaction::ComputeTimeLock() const { if (GetVersion() >= 2) { diff --git a/src/psbt.h b/src/psbt.h index 91182230ca7..a6f189d0151 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -1257,6 +1257,9 @@ public: /** Merge psbt into this. The two psbts must have the same underlying CTransaction (i.e. the * same actual Bitcoin transaction.) Returns true if the merge succeeded, false otherwise. */ [[nodiscard]] bool Merge(const PartiallySignedTransaction& psbt); + /** Merge the global xpubs of psbt into this, keeping the existing origin for an xpub + * seen again with a different one, as the serialized records are keyed by xpub. */ + void MergeGlobalXPubs(const PartiallySignedTransaction& psbt); bool AddInput(const PSBTInput& psbtin); bool AddOutput(const PSBTOutput& psbtout); std::optional ComputeTimeLock() const; @@ -1354,9 +1357,6 @@ public: // Used for duplicate key detection std::set> key_lookup; - // Track the global xpubs we have already seen. Just for sanity checking - std::set global_xpubs; - // Read global data bool found_sep = false; std::optional tx; @@ -1459,7 +1459,6 @@ public: if (!xpub.pubkey.IsFullyValid()) { throw std::ios_base::failure("Invalid pubkey"); } - global_xpubs.insert(xpub); // Read in the keypath from stream KeyOriginInfo keypath; DeserializeHDKeypath(s, keypath); diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 701916c6148..388a7fd9167 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1927,13 +1927,7 @@ static RPCMethod joinpsbts() for (const PSBTOutput& output : psbt.outputs) { merged_psbt.AddOutput(output); } - for (auto& xpub_pair : psbt.m_xpubs) { - if (!merged_psbt.m_xpubs.contains(xpub_pair.first)) { - merged_psbt.m_xpubs[xpub_pair.first] = xpub_pair.second; - } else { - merged_psbt.m_xpubs[xpub_pair.first].insert(xpub_pair.second.begin(), xpub_pair.second.end()); - } - } + merged_psbt.MergeGlobalXPubs(psbt); merged_psbt.unknown.insert(psbt.unknown.begin(), psbt.unknown.end()); } diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index b107dd05169..445a44f3ef1 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -8,6 +8,7 @@ from decimal import Decimal from itertools import product from random import randbytes +from test_framework.address import base58_to_byte from test_framework.blocktools import ( MAX_STANDARD_TX_WEIGHT, ) @@ -28,6 +29,7 @@ from test_framework.psbt import ( PSBT_GLOBAL_PROPRIETARY, PSBT_GLOBAL_UNSIGNED_TX, PSBT_GLOBAL_VERSION, + PSBT_GLOBAL_XPUB, PSBT_IN_RIPEMD160, PSBT_IN_SHA256, PSBT_IN_SIGHASH_TYPE, @@ -359,6 +361,32 @@ class PSBTTest(BitcoinTestFramework): proprietary_entry(key=output_key_b, value=b"\xff", identifier=b"out", subtype=6), ]) + 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): + return PSBT( + g=PSBTMap({ + PSBT_GLOBAL_UNSIGNED_TX: tx.serialize(), + xpub_key: fingerprint, + }), + i=[PSBTMap({})], + o=[PSBTMap({})], + ).to_base64() + + combined = self.nodes[0].combinepsbt([psbt_with_origin(b"\x00\x00\x00\x00"), psbt_with_origin(b"\x11\x11\x11\x11")]) + # The same xpub under both origins would serialize as duplicate keys, making the combined PSBT unparseable + decoded = self.nodes[0].decodepsbt(combined) + assert_equal(decoded["global_xpubs"], [{"xpub": xpub, "master_fingerprint": "00000000", "path": "m"}]) + def test_sighash_mismatch(self): self.log.info("Test sighash type mismatches") self.nodes[0].createwallet("sighash_mismatch") @@ -1380,6 +1408,7 @@ class PSBTTest(BitcoinTestFramework): self.test_decodepsbt_musig2_input_output_types() self.test_combinepsbt_preserves_proprietary_fields() + self.test_combinepsbt_global_xpub_origin_conflict() self.log.info("Test that combining PSBTs with different transactions fails") tx = CTransaction()