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.

src/psbt.cpp

@@ -13,6 +13,8 @@
 #include <util/result.h>
 #include <util/strencodings.h>
 
+#include <algorithm>
+
 using common::PSBTError;
 
 PartiallySignedTransaction::PartiallySignedTransaction(const CMutableTransaction& tx, uint32_t version) : m_version(version)
@@ -58,13 +60,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
@@ -85,6 +81,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<uint32_t> PartiallySignedTransaction::ComputeTimeLock() const
 {
     if (GetVersion() >= 2) {

src/psbt.h

@@ -1260,6 +1260,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<uint32_t> ComputeTimeLock() const;

src/rpc/rawtransaction.cpp

@@ -1918,13 +1918,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());
     }