partial Merge bitcoin#25595: Verify PSBT inputs rather than check for fields being empty

BACKPORT NOTE:
witness-scenario is omitted for backport; the new test provides the OP_TRUE prevout as non_witness_utxo, since Dash's PSBT has no witness_utxo field, and the expected hex is computed because the input spends a real prev-tx.
The segwit-based extractor vector was removed from rpc_psbt.json in a preceding commit, since its final scriptSig can no longer be script-verified.
-------------
e133264 Add test for PSBT input verification (Greg Sanders)
d256992 Verify PSBT inputs rather than check for fields being empty (Greg Sanders)

Pull request description:

  In a few keys spots, PSBT finality is checked by looking for non-empty witness data.

  This complicates a couple things:
  1) Empty data can be valid in certain cases
  2) User may be passed bogus final data by a counterparty during PSBT work happening, and end up with incorrect signatures that they may not be able to check in other contexts if the UTXO doesn't exist yet in chain/mempool, timelocks, etc.

  On the whole I think these heavier checks are worth it in case someone is actually assuming the signatures are correct if our API is saying so.

ACKs for top commit:
  achow101:
    ACK e133264

Tree-SHA512: 9de4fbb0be1257b081781f5df908fd55666e3acd5c4e36beb3b3f2f5a6aed69ff77068c44cde6127e159e773293fd9ced4c0bb47e693969f337e74dc8af030da

Author: fanquake

src/node/psbt.cpp

@@ -58,7 +58,7 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
         }
 
         // Check if it is final
-        if (!utxo.IsNull() && !PSBTInputSigned(input)) {
+        if (!PSBTInputSignedAndVerified(psbtx, i, &txdata)) {
             input_analysis.is_final = false;
 
             // Figure out what is missing

src/psbt.cpp

@@ -181,6 +181,33 @@ void PSBTOutput::Merge(const PSBTOutput& output)
     if (redeem_script.empty() && !output.redeem_script.empty()) redeem_script = output.redeem_script;
 }
 
+bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, unsigned int input_index, const PrecomputedTransactionData* txdata)
+{
+    CTxOut utxo;
+    assert(psbt.inputs.size() >= input_index);
+    const PSBTInput& input = psbt.inputs[input_index];
+
+    if (input.non_witness_utxo) {
+        // If we're taking our information from a non-witness UTXO, verify that it matches the prevout.
+        COutPoint prevout = psbt.tx->vin[input_index].prevout;
+        if (prevout.n >= input.non_witness_utxo->vout.size()) {
+            return false;
+        }
+        if (input.non_witness_utxo->GetHash() != prevout.hash) {
+            return false;
+        }
+        utxo = input.non_witness_utxo->vout[prevout.n];
+    } else {
+        return false;
+    }
+
+    if (txdata) {
+        return VerifyScript(input.final_script_sig, utxo.scriptPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker{&(*psbt.tx), input_index, utxo.nValue, *txdata, MissingDataBehavior::FAIL});
+    } else {
+        return VerifyScript(input.final_script_sig, utxo.scriptPubKey, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker{&(*psbt.tx), input_index, utxo.nValue, MissingDataBehavior::FAIL});
+    }
+}
+
 size_t CountPSBTUnsignedInputs(const PartiallySignedTransaction& psbt) {
     size_t count = 0;
     for (const auto& input : psbt.inputs) {
@@ -211,6 +238,7 @@ void UpdatePSBTOutput(const SigningProvider& provider, PartiallySignedTransactio
     // Put redeem_script, key paths, into PSBTOutput.
     psbt_out.FromSignatureData(sigdata);
 }
+
 bool PSBTInputSigned(const PSBTInput& input)
 {
     return !input.final_script_sig.empty();
@@ -238,7 +266,7 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
     PSBTInput& input = psbt.inputs.at(index);
     const CMutableTransaction& tx = *psbt.tx;
 
-    if (PSBTInputSigned(input)) {
+    if (PSBTInputSignedAndVerified(psbt, index, txdata)) {
         return true;
     }
 

src/psbt.h

@@ -865,9 +865,12 @@ std::string PSBTRoleName(PSBTRole role);
 /** Compute a PrecomputedTransactionData object from a psbt. */
 PrecomputedTransactionData PrecomputePSBTData(const PartiallySignedTransaction& psbt);
 
-/** Checks whether a PSBTInput is already signed. */
+/** Checks whether a PSBTInput is already signed by checking for non-null finalized fields. */
 bool PSBTInputSigned(const PSBTInput& input);
 
+/** Checks whether a PSBTInput is already signed by doing script verification using final fields. */
+bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, unsigned int input_index, const PrecomputedTransactionData* txdata);
+
 /** Signs a PSBTInput, verifying that all provided data matches what is being signed.
  *
  * txdata should be the output of PrecomputePSBTData (which can be shared across

test/functional/rpc_psbt.py

@@ -24,7 +24,9 @@
     PSBT_IN_SHA256,
     PSBT_IN_HASH160,
     PSBT_IN_HASH256,
+    PSBT_IN_NON_WITNESS_UTXO,
 )
+from test_framework.script import CScript, OP_TRUE
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_approx,
@@ -637,6 +639,21 @@ def test_psbt_input_keys(psbt_input, keys):
         assert_raises_rpc_error(-8, "PSBTs not compatible (different transactions)", self.nodes[0].combinepsbt, [psbt1, psbt2])
         assert_equal(self.nodes[0].combinepsbt([psbt1, psbt1]), psbt1)
 
+        self.log.info("Test that PSBT inputs are being checked via script execution")
+        acs_prev_tx = CTransaction()
+        acs_prev_tx.vin = [CTxIn(outpoint=COutPoint(hash=int('dd' * 32, 16), n=0), scriptSig=b"")]
+        acs_prev_tx.vout = [CTxOut(nValue=0, scriptPubKey=CScript([OP_TRUE]))]
+        acs_prev_tx.rehash()
+        tx = CTransaction()
+        tx.vin = [CTxIn(outpoint=COutPoint(hash=acs_prev_tx.sha256, n=0), scriptSig=b"")]
+        tx.vout = [CTxOut(nValue=0, scriptPubKey=b"")]
+        psbt = PSBT()
+        psbt.g = PSBTMap({PSBT_GLOBAL_UNSIGNED_TX: tx.serialize()})
+        psbt.i = [PSBTMap({bytes([PSBT_IN_NON_WITNESS_UTXO]) : acs_prev_tx.serialize()})]
+        psbt.o = [PSBTMap()]
+        assert_equal(self.nodes[0].finalizepsbt(psbt.to_base64()),
+            {'hex': tx.serialize().hex(), 'complete': True})
+
 
 if __name__ == '__main__':
     PSBTTest().main()