Merge #14780: PSBT backports to 0.17

7bee41452b Add test for conversion from non-witness to witness UTXO (Pieter Wuille)
ff56bb9b44 Add regression test for PSBT signing bug #14473 (Glenn Willen)
db445d4e5a Refactor PSBTInput signing to enforce invariant (Glenn Willen)
ad94165db9 Simplify arguments to SignPSBTInput (Glenn Willen)
39ece4fc28 Add bool PSBTInputSigned (Glenn Willen)
70ee1f8709 New PartiallySignedTransaction constructor from CTransction (Glenn Willen)
a9eab081d5 Remove redundant txConst parameter to FillPSBT (Glenn Willen)
cfdd6b2f6c More concise conversion of CDataStream to string (Glenn Willen)
a3fe125490 check that a separator is found for psbt inputs, outputs, and global map (Andrew Chow)

Pull request description:

  This is a backport of #14588, #14377, and #14197's test to 0.17.

Tree-SHA512: 07535ec69a878a63b549e5e463345e233f34662dff805202614cf2ffc896c6d1981363e6d06d02db2e02d815075ad8ebdc5f93f637052cff8c8cbe6c8dfa096a

2 files changed, 66 insertions, 14 deletions

diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index 1ab5051ff7..f97d6a2533 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -233,10 +233,17 @@ bool ProduceSignature(const SigningProvider& provider, const BaseSignatureCreato
     return sigdata.complete;
 }
 
-bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& tx, PSBTInput& input, SignatureData& sigdata, 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, SignatureData& sigdata, int index, int sighash)
+{
+    PSBTInput& input = psbt.inputs.at(index);
+    const CMutableTransaction& tx = *psbt.tx;
+
+    if (PSBTInputSigned(input)) {
         return true;
     }
 
@@ -246,15 +253,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
@@ -272,6 +283,13 @@ bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& t
     // Verify that a witness signature was produced in case one was required.
     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) {
+        input.witness_utxo = utxo;
+        input.non_witness_utxo = nullptr;
+    }
+
     return sig_complete;
 }
 
@@ -491,6 +509,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 245b15410f..f24d77cb31 100644
--- a/src/script/sign.h
+++ b/src/script/sign.h
@@ -286,6 +286,7 @@ struct PSBTInput
     template <typename Stream>
     inline void Unserialize(Stream& s) {
         // Read loop
+        bool found_sep = false;
         while(!s.empty()) {
             // Read
             std::vector<unsigned char> key;
@@ -293,7 +294,10 @@ struct PSBTInput
 
             // the key is empty if that was actually a separator byte
             // This is a special case for key lengths 0 as those are not allowed (except for separator)
-            if (key.empty()) return;
+            if (key.empty()) {
+                found_sep = true;
+                break;
+            }
 
             // First byte of key is the type
             unsigned char type = key[0];
@@ -408,6 +412,10 @@ struct PSBTInput
                     break;
             }
         }
+
+        if (!found_sep) {
+            throw std::ios_base::failure("Separator is missing at the end of an input map");
+        }
     }
 
     template <typename Stream>
@@ -461,6 +469,7 @@ struct PSBTOutput
     template <typename Stream>
     inline void Unserialize(Stream& s) {
         // Read loop
+        bool found_sep = false;
         while(!s.empty()) {
             // Read
             std::vector<unsigned char> key;
@@ -468,7 +477,10 @@ struct PSBTOutput
 
             // the key is empty if that was actually a separator byte
             // This is a special case for key lengths 0 as those are not allowed (except for separator)
-            if (key.empty()) return;
+            if (key.empty()) {
+                found_sep = true;
+                break;
+            }
 
             // First byte of key is the type
             unsigned char type = key[0];
@@ -513,6 +525,10 @@ struct PSBTOutput
                 }
             }
         }
+
+        if (!found_sep) {
+            throw std::ios_base::failure("Separator is missing at the end of an output map");
+        }
     }
 
     template <typename Stream>
@@ -534,6 +550,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)
@@ -588,6 +605,7 @@ struct PartiallySignedTransaction
         }
 
         // Read global data
+        bool found_sep = false;
         while(!s.empty()) {
             // Read
             std::vector<unsigned char> key;
@@ -595,7 +613,10 @@ struct PartiallySignedTransaction
 
             // the key is empty if that was actually a separator byte
             // This is a special case for key lengths 0 as those are not allowed (except for separator)
-            if (key.empty()) break;
+            if (key.empty()) {
+                found_sep = true;
+                break;
+            }
 
             // First byte of key is the type
             unsigned char type = key[0];
@@ -635,6 +656,10 @@ struct PartiallySignedTransaction
             }
         }
 
+        if (!found_sep) {
+            throw std::ios_base::failure("Separator is missing at the end of the global map");
+        }
+
         // Make sure that we got an unsigned tx
         if (!tx) {
             throw std::ios_base::failure("No unsigned transcation was provided");
@@ -689,8 +714,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, SignatureData& sigdata, int index, int sighash = 1);
+bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, SignatureData& sigdata, 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);