Merge #14588: Refactor PSBT signing logic to enforce invariant and fix signing bug

e13fea975d Add regression test for PSBT signing bug #14473 (Glenn Willen)
565500508a Refactor PSBTInput signing to enforce invariant (Glenn Willen)
0f5bda2bd9 Simplify arguments to SignPSBTInput (Glenn Willen)
53e6fffb8f Add bool PSBTInputSigned (Glenn Willen)
65166d4cf8 New PartiallySignedTransaction constructor from CTransction (Glenn Willen)
4f3f5cb4b1 Remove redundant txConst parameter to FillPSBT (Glenn Willen)
fe5d22bc67 More concise conversion of CDataStream to string (Glenn Willen)

Pull request description:

  As discussed in the comments on #14473, I think that bug was caused primarily by failure to adhere to the invariant that a PSBTInput always has exactly one of the two utxo fields present -- an invariant that is already enforced by PSBTInput::IsSane, but which we were temporarily suspending during signing.

  This refactor repairs the invariant, also fixing the bug. It also simplifies some other code, and removes redundant parameters from some related functions.

  fixes #14473

Tree-SHA512: cbad3428175e30f9b7bac3f600668dd1a8f9acde16b915d27a940a2fa6d5149d4fbe236d5808fd590fb20a032274c99e8cac34bef17f79a53fdf69a5948c0fd0

2 files changed, 34 insertions, 21 deletions

diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index 89cc7c808c..69ee08ffd7 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -239,10 +239,17 @@ bool ProduceSignature(const SigningProvider& provider, const BaseSignatureCreato
     return sigdata.complete;
 }
 
-bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& tx, PSBTInput& input, int index, int sighash)
+bool PSBTInputSigned(PSBTInput& input)
 {
-    // if this input has a final scriptsig or scriptwitness, don't do anything with it
-    if (!input.final_script_sig.empty() || !input.final_script_witness.IsNull()) {
+    return !input.final_script_sig.empty() || !input.final_script_witness.IsNull();
+}
+
+bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, int sighash)
+{
+    PSBTInput& input = psbt.inputs.at(index);
+    const CMutableTransaction& tx = *psbt.tx;
+
+    if (PSBTInputSigned(input)) {
         return true;
     }
 
@@ -253,15 +260,19 @@ bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& t
     // Get UTXO
     bool require_witness_sig = false;
     CTxOut utxo;
+
+    // Verify input sanity, which checks that at most one of witness or non-witness utxos is provided.
+    if (!input.IsSane()) {
+        return false;
+    }
+
     if (input.non_witness_utxo) {
         // If we're taking our information from a non-witness UTXO, verify that it matches the prevout.
-        if (input.non_witness_utxo->GetHash() != tx.vin[index].prevout.hash) return false;
-        // If both witness and non-witness UTXO are provided, verify that they match. This check shouldn't
-        // matter, as the PSBT deserializer enforces only one of both is provided, and the only way both
-        // can be present is when they're added simultaneously by FillPSBT (in which case they always match).
-        // Still, check in order to not rely on callers to enforce this.
-        if (!input.witness_utxo.IsNull() && input.non_witness_utxo->vout[tx.vin[index].prevout.n] != input.witness_utxo) return false;
-        utxo = input.non_witness_utxo->vout[tx.vin[index].prevout.n];
+        COutPoint prevout = tx.vin[index].prevout;
+        if (input.non_witness_utxo->GetHash() != prevout.hash) {
+            return false;
+        }
+        utxo = input.non_witness_utxo->vout[prevout.n];
     } else if (!input.witness_utxo.IsNull()) {
         utxo = input.witness_utxo;
         // When we're taking our information from a witness UTXO, we can't verify it is actually data from
@@ -280,18 +291,10 @@ bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& t
     if (require_witness_sig && !sigdata.witness) return false;
     input.FromSignatureData(sigdata);
 
+    // If we have a witness signature, use the smaller witness UTXO.
     if (sigdata.witness) {
-        assert(!utxo.IsNull());
         input.witness_utxo = utxo;
-    }
-
-    // If both UTXO types are present, drop the unnecessary one.
-    if (input.non_witness_utxo && !input.witness_utxo.IsNull()) {
-        if (sigdata.witness) {
-            input.non_witness_utxo = nullptr;
-        } else {
-            input.witness_utxo.SetNull();
-        }
+        input.non_witness_utxo = nullptr;
     }
 
     return sig_complete;
@@ -513,6 +516,12 @@ bool IsSolvable(const SigningProvider& provider, const CScript& script)
     return false;
 }
 
+PartiallySignedTransaction::PartiallySignedTransaction(const CTransaction& tx) : tx(tx)
+{
+    inputs.resize(tx.vin.size());
+    outputs.resize(tx.vout.size());
+}
+
 bool PartiallySignedTransaction::IsNull() const
 {
     return !tx && inputs.empty() && outputs.empty() && unknown.empty();
diff --git a/src/script/sign.h b/src/script/sign.h
index d47aada17d..e71f43f96d 100644
--- a/src/script/sign.h
+++ b/src/script/sign.h
@@ -566,6 +566,7 @@ struct PartiallySignedTransaction
     bool IsSane() const;
     PartiallySignedTransaction() {}
     PartiallySignedTransaction(const PartiallySignedTransaction& psbt_in) : tx(psbt_in.tx), inputs(psbt_in.inputs), outputs(psbt_in.outputs), unknown(psbt_in.unknown) {}
+    explicit PartiallySignedTransaction(const CTransaction& tx);
 
     // Only checks if they refer to the same transaction
     friend bool operator==(const PartiallySignedTransaction& a, const PartiallySignedTransaction &b)
@@ -729,8 +730,11 @@ bool ProduceSignature(const SigningProvider& provider, const BaseSignatureCreato
 bool SignSignature(const SigningProvider &provider, const CScript& fromPubKey, CMutableTransaction& txTo, unsigned int nIn, const CAmount& amount, int nHashType);
 bool SignSignature(const SigningProvider &provider, const CTransaction& txFrom, CMutableTransaction& txTo, unsigned int nIn, int nHashType);
 
+/** Checks whether a PSBTInput is already signed. */
+bool PSBTInputSigned(PSBTInput& input);
+
 /** Signs a PSBTInput, verifying that all provided data matches what is being signed. */
-bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& tx, PSBTInput& input, int index, int sighash = SIGHASH_ALL);
+bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, int sighash = SIGHASH_ALL);
 
 /** Extract signature data from a transaction input, and insert it. */
 SignatureData DataFromTransaction(const CMutableTransaction& tx, unsigned int nIn, const CTxOut& txout);