From 3da0e160124ffdc14e9aaa5d02fd04c232d431cc Mon Sep 17 00:00:00 2001
From: Ava Chow <github@achow101.com>
Date: Mon, 22 Jul 2024 17:14:20 -0400
Subject: [PATCH] Replace PSBT.tx with PSBT::GetUnsignedTx and
 PSBT::GetUniqueID

The global unsigned tx is decomposed into separate fields inside of
PSBT, which mirrors what PSBTv2 will do. However, we still need to get
the global unsigned tx so PSBT::GetUnsignedTx is introduced to do that.
In order to also have a stable unique ID, we also introduce
PSBT::GetUniqueID to replace uses of PSBT.tx.GetHash().
---
 src/node/psbt.cpp                     |   8 +-
 src/psbt.cpp                          | 105 +++++++++++++++++++-------
 src/psbt.h                            |  11 +--
 src/rpc/rawtransaction.cpp            |  11 +--
 src/test/fuzz/psbt.cpp                |   6 +-
 src/wallet/test/psbt_wallet_tests.cpp |   1 -
 6 files changed, 99 insertions(+), 43 deletions(-)

diff --git a/src/node/psbt.cpp b/src/node/psbt.cpp
index 5b78001c6d6..699d1d5f7ce 100644
--- a/src/node/psbt.cpp
+++ b/src/node/psbt.cpp
@@ -18,6 +18,13 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
     // Go through each input and build status
     PSBTAnalysis result;
 
+    std::optional<CMutableTransaction> unsigned_tx = psbtx.GetUnsignedTx();
+    if (!unsigned_tx) {
+        result.SetInvalid("PSBT cannot be made into a valid transaction");
+        return result;
+    }
+    CMutableTransaction& mtx = *unsigned_tx;
+
     bool calc_fee = true;
 
     CAmount in_amt = 0;
@@ -116,7 +123,6 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
         result.fee = fee;
 
         // Estimate the size
-        CMutableTransaction mtx(*psbtx.tx);
         CCoinsViewCache view{&CoinsViewEmpty::Get()};
         bool success = true;
 
diff --git a/src/psbt.cpp b/src/psbt.cpp
index d7320682b27..a87079610d6 100644
--- a/src/psbt.cpp
+++ b/src/psbt.cpp
@@ -7,6 +7,7 @@
 #include <common/types.h>
 #include <node/types.h>
 #include <policy/policy.h>
+#include <primitives/transaction.h>
 #include <script/signingprovider.h>
 #include <util/check.h>
 #include <util/result.h>
@@ -14,7 +15,7 @@
 
 using common::PSBTError;
 
-PartiallySignedTransaction::PartiallySignedTransaction(const CMutableTransaction& tx) : tx(tx)
+PartiallySignedTransaction::PartiallySignedTransaction(const CMutableTransaction& tx)
 {
     tx_version = tx.version;
     fallback_locktime = tx.nLockTime;
@@ -30,13 +31,15 @@ PartiallySignedTransaction::PartiallySignedTransaction(const CMutableTransaction
 
 bool PartiallySignedTransaction::IsNull() const
 {
-    return !tx && inputs.empty() && outputs.empty() && unknown.empty();
+    return inputs.empty() && outputs.empty() && unknown.empty();
 }
 
 bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt)
 {
     // Prohibited to merge two PSBTs over different transactions
-    if (tx->GetHash() != psbt.tx->GetHash()) {
+    std::optional<Txid> this_id = GetUniqueID();
+    std::optional<Txid> psbt_id = psbt.GetUniqueID();
+    if (!this_id || !psbt_id || this_id != psbt_id) {
         return false;
     }
 
@@ -58,23 +61,45 @@ bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt)
     return true;
 }
 
+std::optional<CMutableTransaction> PartiallySignedTransaction::GetUnsignedTx() const
+{
+    CMutableTransaction mtx;
+    mtx.version = tx_version;
+    mtx.nLockTime = fallback_locktime.value_or(0);
+    uint32_t max_sequence = CTxIn::SEQUENCE_FINAL;
+    for (const PSBTInput& input : inputs) {
+        CTxIn txin;
+        txin.prevout.hash = input.prev_txid;
+        txin.prevout.n = input.prev_out;
+        txin.nSequence = input.sequence.value_or(max_sequence);
+        mtx.vin.push_back(txin);
+    }
+    for (const PSBTOutput& output : outputs) {
+        CTxOut txout;
+        txout.nValue = output.amount;
+        txout.scriptPubKey = output.script;
+        mtx.vout.push_back(txout);
+    }
+    return mtx;
+}
+
+std::optional<Txid> PartiallySignedTransaction::GetUniqueID() const
+{
+    // Get the unsigned transaction
+    std::optional<CMutableTransaction> mtx = GetUnsignedTx();
+    if (!mtx) {
+        return std::nullopt;
+    }
+    if (GetVersion() >= 2) {
+        for (CTxIn& txin : mtx->vin) {
+            txin.nSequence = 0;
+        }
+    }
+    return mtx->GetHash();
+}
+
 bool PartiallySignedTransaction::AddInput(const PSBTInput& psbtin)
 {
-    if (GetVersion() < 2) {
-        // This is a v0 psbt, so do the v0 AddInput
-        CTxIn txin(COutPoint(psbtin.prev_txid, psbtin.prev_out));
-        if (std::find(tx->vin.begin(), tx->vin.end(), txin) != tx->vin.end()) {
-            // Prevent duplicate inputs
-            return false;
-        }
-        tx->vin.push_back(std::move(txin));
-        inputs.push_back(psbtin);
-        inputs.back().partial_sigs.clear();
-        inputs.back().final_script_sig.clear();
-        inputs.back().final_script_witness.SetNull();
-        return true;
-    }
-
     // Prevent duplicate inputs
     if (std::find_if(inputs.begin(), inputs.end(),
         [psbtin](const PSBTInput& psbt) {
@@ -84,6 +109,15 @@ bool PartiallySignedTransaction::AddInput(const PSBTInput& psbtin)
         return false;
     }
 
+    if (GetVersion() < 2) {
+        // This is a v0 psbt, so do the v0 AddInput
+        inputs.push_back(psbtin);
+        inputs.back().partial_sigs.clear();
+        inputs.back().final_script_sig.clear();
+        inputs.back().final_script_witness.SetNull();
+        return true;
+    }
+
     return false;
 }
 
@@ -91,8 +125,6 @@ bool PartiallySignedTransaction::AddOutput(const PSBTOutput& psbtout)
 {
     if (GetVersion() < 2) {
         // This is a v0 psbt, do the v0 AddOutput
-        CTxOut txout(psbtout.amount, psbtout.script);
-        tx->vout.push_back(txout);
         outputs.push_back(psbtout);
         return true;
     }
@@ -391,10 +423,15 @@ bool PSBTInputSignedAndVerified(const PartiallySignedTransaction& psbt, unsigned
         return false;
     }
 
+    std::optional<CMutableTransaction> unsigned_tx = psbt.GetUnsignedTx();
+    if (!unsigned_tx) {
+        return false;
+    }
+    const CMutableTransaction& tx = *unsigned_tx;
     if (txdata) {
-        return VerifyScript(input.final_script_sig, utxo.scriptPubKey, &input.final_script_witness, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker{&(*psbt.tx), input_index, utxo.nValue, *txdata, MissingDataBehavior::FAIL});
+        return VerifyScript(input.final_script_sig, utxo.scriptPubKey, &input.final_script_witness, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker{&tx, input_index, utxo.nValue, *txdata, MissingDataBehavior::FAIL});
     } else {
-        return VerifyScript(input.final_script_sig, utxo.scriptPubKey, &input.final_script_witness, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker{&(*psbt.tx), input_index, utxo.nValue, MissingDataBehavior::FAIL});
+        return VerifyScript(input.final_script_sig, utxo.scriptPubKey, &input.final_script_witness, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker{&tx, input_index, utxo.nValue, MissingDataBehavior::FAIL});
     }
 }
 
@@ -411,7 +448,11 @@ size_t CountPSBTUnsignedInputs(const PartiallySignedTransaction& psbt) {
 
 void UpdatePSBTOutput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index)
 {
-    CMutableTransaction& tx = *Assert(psbt.tx);
+    std::optional<CMutableTransaction> unsigned_tx = psbt.GetUnsignedTx();
+    if (!unsigned_tx) {
+        return;
+    }
+    CMutableTransaction& tx = *unsigned_tx;
     const CTxOut& out = tx.vout.at(index);
     PSBTOutput& psbt_out = psbt.outputs.at(index);
 
@@ -431,7 +472,11 @@ void UpdatePSBTOutput(const SigningProvider& provider, PartiallySignedTransactio
 
 std::optional<PrecomputedTransactionData> PrecomputePSBTData(const PartiallySignedTransaction& psbt)
 {
-    const CMutableTransaction& tx = *psbt.tx;
+    std::optional<CMutableTransaction> unsigned_tx = psbt.GetUnsignedTx();
+    if (!unsigned_tx) {
+        return std::nullopt;
+    }
+    const CMutableTransaction& tx = *unsigned_tx;
     bool have_all_spent_outputs = true;
     std::vector<CTxOut> utxos;
     for (const PSBTInput& input : psbt.inputs) {
@@ -449,7 +494,11 @@ std::optional<PrecomputedTransactionData> PrecomputePSBTData(const PartiallySign
 PSBTError SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, const common::PSBTFillOptions& options,  SignatureData* out_sigdata)
 {
     PSBTInput& input = psbt.inputs.at(index);
-    const CMutableTransaction& tx = *psbt.tx;
+    std::optional<CMutableTransaction> unsigned_tx = psbt.GetUnsignedTx();
+    if (!unsigned_tx) {
+        return PSBTError::INVALID_TX;
+    }
+    const CMutableTransaction& tx = *unsigned_tx;
 
     if (PSBTInputSignedAndVerified(psbt, index, txdata)) {
         return PSBTError::OK;
@@ -623,7 +672,11 @@ bool FinalizeAndExtractPSBT(PartiallySignedTransaction& psbtx, CMutableTransacti
         return false;
     }
 
-    result = *psbtx.tx;
+    std::optional<CMutableTransaction> unsigned_tx = psbtx.GetUnsignedTx();
+    if (!unsigned_tx) {
+        return false;
+    }
+    result = *unsigned_tx;
     for (unsigned int i = 0; i < result.vin.size(); ++i) {
         result.vin[i].scriptSig = psbtx.inputs[i].final_script_sig;
         result.vin[i].scriptWitness = psbtx.inputs[i].final_script_witness;
diff --git a/src/psbt.h b/src/psbt.h
index 6fe7a8b2fc6..3c58f3c1783 100644
--- a/src/psbt.h
+++ b/src/psbt.h
@@ -1073,7 +1073,6 @@ private:
     std::optional<uint32_t> m_version;
 
 public:
-    std::optional<CMutableTransaction> tx;
     // We use a vector of CExtPubKey in the event that there happens to be the same KeyOriginInfos for different CExtPubKeys
     // Note that this map swaps the key and values from the serialization
     std::map<KeyOriginInfo, std::set<CExtPubKey>> m_xpubs;
@@ -1093,6 +1092,8 @@ public:
     [[nodiscard]] bool Merge(const PartiallySignedTransaction& psbt);
     bool AddInput(const PSBTInput& psbtin);
     bool AddOutput(const PSBTOutput& psbtout);
+    std::optional<CMutableTransaction> GetUnsignedTx() const;
+    std::optional<Txid> GetUniqueID() const;
     explicit PartiallySignedTransaction(const CMutableTransaction& tx);
 
     template <typename Stream>
@@ -1105,7 +1106,7 @@ public:
         SerializeToVector(s, CompactSizeWriter(PSBT_GLOBAL_UNSIGNED_TX));
 
         // Write serialized tx to a stream
-        SerializeToVector(s, TX_NO_WITNESS(*tx));
+        SerializeToVector(s, TX_NO_WITNESS(*GetUnsignedTx()));
 
         // Write xpubs
         for (const auto& xpub_pair : m_xpubs) {
@@ -1168,6 +1169,7 @@ public:
 
         // Read global data
         bool found_sep = false;
+        std::optional<CMutableTransaction> tx;
         while(!s.empty()) {
             // Read the key of format "<keylen><keytype><keydata>" after which
             // "key" will contain "<keytype><keydata>"
@@ -1197,10 +1199,9 @@ public:
                 case PSBT_GLOBAL_UNSIGNED_TX:
                 {
                     ExpectedKeySize("Global Unsigned TX", key, 1);
-                    CMutableTransaction mtx;
                     // Set the stream to serialize with non-witness since this should always be non-witness
-                    UnserializeFromVector(s, TX_NO_WITNESS(mtx));
-                    tx = std::move(mtx);
+                    tx.emplace();
+                    UnserializeFromVector(s, TX_NO_WITNESS(*tx));
                     // Make sure that all scriptSigs and scriptWitnesses are empty
                     for (const CTxIn& txin : tx->vin) {
                         if (!txin.scriptSig.empty() || !txin.scriptWitness.IsNull()) {
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index f6d323fc65d..202a86609fc 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -1074,7 +1074,7 @@ static RPCMethod decodepsbt()
 
     // Add the decoded tx
     UniValue tx_univ(UniValue::VOBJ);
-    TxToUniv(CTransaction(*psbtx.tx), /*block_hash=*/uint256(), /*entry=*/tx_univ, /*include_hex=*/false);
+    TxToUniv(CTransaction(*CHECK_NONFATAL(psbtx.GetUnsignedTx())), /*block_hash=*/uint256(), /*entry=*/tx_univ, /*include_hex=*/false);
     result.pushKV("tx", std::move(tx_univ));
 
     // Add the global xpubs
@@ -1803,12 +1803,13 @@ static RPCMethod joinpsbts()
         const PartiallySignedTransaction& psbtx = *psbt_res;
         psbtxs.push_back(psbtx);
         // Choose the highest version number
-        if (psbtx.tx->version > best_version) {
-            best_version = psbtx.tx->version;
+        if (psbtx.tx_version > best_version) {
+            best_version = psbtx.tx_version;
         }
         // Choose the lowest lock time
-        if (psbtx.tx->nLockTime < best_locktime) {
-            best_locktime = psbtx.tx->nLockTime;
+        uint32_t psbt_locktime = psbtx.fallback_locktime.value_or(0);
+        if (psbt_locktime < best_locktime) {
+            best_locktime = psbt_locktime;
         }
     }
 
diff --git a/src/test/fuzz/psbt.cpp b/src/test/fuzz/psbt.cpp
index 7481d02e5a5..7d0f775b6ed 100644
--- a/src/test/fuzz/psbt.cpp
+++ b/src/test/fuzz/psbt.cpp
@@ -52,11 +52,7 @@ FUZZ_TARGET(psbt)
 
     (void)psbt.IsNull();
 
-    std::optional<CMutableTransaction> tx = psbt.tx;
-    if (tx) {
-        const CMutableTransaction& mtx = *tx;
-        const PartiallySignedTransaction psbt_from_tx{mtx};
-    }
+    (void)psbt.GetUnsignedTx();
 
     for (const PSBTInput& input : psbt.inputs) {
         (void)PSBTInputSigned(input);
diff --git a/src/wallet/test/psbt_wallet_tests.cpp b/src/wallet/test/psbt_wallet_tests.cpp
index 68b5f36ee0b..8d8dc2ce2b0 100644
--- a/src/wallet/test/psbt_wallet_tests.cpp
+++ b/src/wallet/test/psbt_wallet_tests.cpp
@@ -72,7 +72,6 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test)
     BOOST_CHECK_EQUAL(final_hex, "70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd750000000000ffffffff838d0427d0ec650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d0100000000ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb815e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df5546e8742d1d87008f00000000000100bb0200000001aad73931018bd25f84ae400b68848be09db706eac2ac18298babee71ab656f8b0000000048473044022058f6fc7c6a33e1b31548d481c826c015bd30135aad42cd67790dab66d2ad243b02204a1ced2604c6735b6393e5b41691dd78b00f0c5942fb9f751856faa938157dba01feffffff0280f0fa020000000017a9140fb9463421696b82c833af241c78c17ddbde493487d0f20a270100000017a91429ca74f8a08f81999428185c97b5d852e4063f6187650000000104475221029583bf39ae0a609747ad199addd634fa6108559d6c5cd39b4c2183f1ab96e07f2102dab61ff49a14db6a7d02b0cd1fbb78fc4b18312b5b4e54dae4dba2fbfef536d752ae2206029583bf39ae0a609747ad199addd634fa6108559d6c5cd39b4c2183f1ab96e07f10d90c6a4f000000800000008000000080220602dab61ff49a14db6a7d02b0cd1fbb78fc4b18312b5b4e54dae4dba2fbfef536d710d90c6a4f0000008000000080010000800001008a020000000158e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd7501000000171600145f275f436b09a8cc9a2eb2a2f528485c68a56323feffffff02d8231f1b0100000017a914aed962d6654f9a2b36608eb9d64d2b260db4f1118700c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e8876500000001012000c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e88701042200208c2353173743b595dfb4a07b72ba8e42e3797da74e87fe7d9d7497e3b2028903010547522103089dc10c7ac6db54f91329af617333db388cead0c231f723379d1b99030b02dc21023add904f3d6dcf59ddb906b0dee23529b7ffb9ed50e5e86151926860221f0e7352ae2206023add904f3d6dcf59ddb906b0dee23529b7ffb9ed50e5e86151926860221f0e7310d90c6a4f000000800000008003000080220603089dc10c7ac6db54f91329af617333db388cead0c231f723379d1b99030b02dc10d90c6a4f00000080000000800200008000220203a9a4c37f5996d3aa25dbac6b570af0650394492942460b354753ed9eeca5877110d90c6a4f000000800000008004000080002202027f6399757d2eff55a136ad02c684b1838b6556e5f1b6b34282a94b6b5005109610d90c6a4f00000080000000800500008000");
 
     // Mutate the transaction so that one of the inputs is invalid
-    psbtx.tx->vin[0].prevout.n = 2;
     psbtx.inputs[0].prev_out = 2;
 
     // Try to sign the mutated input