Merge #16251: Improve signrawtransaction error reporting

ec4c79326bb670c2cc1757ecfb1900f8460c5257 signrawtransaction*: improve error for partial signing (Anthony Towns)
3c481f8921bbc587cf287329f39243abe703b868 signrawtransactionwithkey: better error messages for bad redeemScript/witnessScript (Anthony Towns)

Pull request description:

  Two fixes for `signrawtransactionwith{key,wallet}` (in addition to #16250): one that checks redeemScript/witnessScript matches scriptPubKey (and if both are provided that they match each other sanely), and the other changes the warning when some-but-not-all the signatures for a CHECKMULTISIG are provided to something that suggests more signatures may be all that's required.

  Fixes: #13218
  Fixes: #14823

ACKs for top commit:
  instagibbs:
    utACK https://github.com/bitcoin/bitcoin/pull/16251/commits/ec4c79326bb670c2cc1757ecfb1900f8460c5257
  achow101:
    Code Review ACK ec4c79326bb670c2cc1757ecfb1900f8460c5257
  meshcollider:
    utACK ec4c79326bb670c2cc1757ecfb1900f8460c5257

Tree-SHA512: 0c95c91d498e85b834662b9e5c83f336ed5fd306be7701ce1dbfa0836fbeb448a267a796585512f7496e820be668b07c2a0a2f45e52dc23f09ee7d9c87e42b35

2 files changed, 81 insertions, 16 deletions

diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp
index 697c6d45c4..f1d176ba4d 100644
--- a/src/rpc/rawtransaction_util.cpp
+++ b/src/rpc/rawtransaction_util.cpp
@@ -196,32 +196,73 @@ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keyst
             }
 
             // if redeemScript and private keys were given, add redeemScript to the keystore so it can be signed
-            if (keystore && (scriptPubKey.IsPayToScriptHash() || scriptPubKey.IsPayToWitnessScriptHash())) {
+            const bool is_p2sh = scriptPubKey.IsPayToScriptHash();
+            const bool is_p2wsh = scriptPubKey.IsPayToWitnessScriptHash();
+            if (keystore && (is_p2sh || is_p2wsh)) {
                 RPCTypeCheckObj(prevOut,
                     {
                         {"redeemScript", UniValueType(UniValue::VSTR)},
                         {"witnessScript", UniValueType(UniValue::VSTR)},
                     }, true);
                 UniValue rs = find_value(prevOut, "redeemScript");
-                if (!rs.isNull()) {
-                    std::vector<unsigned char> rsData(ParseHexV(rs, "redeemScript"));
-                    CScript redeemScript(rsData.begin(), rsData.end());
-                    keystore->AddCScript(redeemScript);
-                    // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH).
-                    // This is only for compatibility, it is encouraged to use the explicit witnessScript field instead.
-                    keystore->AddCScript(GetScriptForWitness(redeemScript));
-                }
                 UniValue ws = find_value(prevOut, "witnessScript");
-                if (!ws.isNull()) {
-                    std::vector<unsigned char> wsData(ParseHexV(ws, "witnessScript"));
-                    CScript witnessScript(wsData.begin(), wsData.end());
-                    keystore->AddCScript(witnessScript);
-                    // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH).
-                    keystore->AddCScript(GetScriptForWitness(witnessScript));
-                }
                 if (rs.isNull() && ws.isNull()) {
                     throw JSONRPCError(RPC_INVALID_PARAMETER, "Missing redeemScript/witnessScript");
                 }
+
+                // work from witnessScript when possible
+                std::vector<unsigned char> scriptData(!ws.isNull() ? ParseHexV(ws, "witnessScript") : ParseHexV(rs, "redeemScript"));
+                CScript script(scriptData.begin(), scriptData.end());
+                keystore->AddCScript(script);
+                // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH).
+                // This is done for redeemScript only for compatibility, it is encouraged to use the explicit witnessScript field instead.
+                CScript witness_output_script{GetScriptForWitness(script)};
+                keystore->AddCScript(witness_output_script);
+
+                if (!ws.isNull() && !rs.isNull()) {
+                    // if both witnessScript and redeemScript are provided,
+                    // they should either be the same (for backwards compat),
+                    // or the redeemScript should be the encoded form of
+                    // the witnessScript (ie, for p2sh-p2wsh)
+                    if (ws.get_str() != rs.get_str()) {
+                        std::vector<unsigned char> redeemScriptData(ParseHexV(rs, "redeemScript"));
+                        CScript redeemScript(redeemScriptData.begin(), redeemScriptData.end());
+                        if (redeemScript != witness_output_script) {
+                            throw JSONRPCError(RPC_INVALID_PARAMETER, "redeemScript does not correspond to witnessScript");
+                        }
+                    }
+                }
+
+                if (is_p2sh) {
+                    const CTxDestination p2sh{ScriptHash(script)};
+                    const CTxDestination p2sh_p2wsh{ScriptHash(witness_output_script)};
+                    if (scriptPubKey == GetScriptForDestination(p2sh)) {
+                        // traditional p2sh; arguably an error if
+                        // we got here with rs.IsNull(), because
+                        // that means the p2sh script was specified
+                        // via witnessScript param, but for now
+                        // we'll just quietly accept it
+                    } else if (scriptPubKey == GetScriptForDestination(p2sh_p2wsh)) {
+                        // p2wsh encoded as p2sh; ideally the witness
+                        // script was specified in the witnessScript
+                        // param, but also support specifying it via
+                        // redeemScript param for backwards compat
+                        // (in which case ws.IsNull() == true)
+                    } else {
+                        // otherwise, can't generate scriptPubKey from
+                        // either script, so we got unusable parameters
+                        throw JSONRPCError(RPC_INVALID_PARAMETER, "redeemScript/witnessScript does not match scriptPubKey");
+                    }
+                } else if (is_p2wsh) {
+                    // plain p2wsh; could throw an error if script
+                    // was specified by redeemScript rather than
+                    // witnessScript (ie, ws.IsNull() == true), but
+                    // accept it for backwards compat
+                    const CTxDestination p2wsh{WitnessV0ScriptHash(script)};
+                    if (scriptPubKey != GetScriptForDestination(p2wsh)) {
+                        throw JSONRPCError(RPC_INVALID_PARAMETER, "redeemScript/witnessScript does not match scriptPubKey");
+                    }
+                }
             }
         }
     }
@@ -268,6 +309,9 @@ UniValue SignTransaction(CMutableTransaction& mtx, const SigningProvider* keysto
             if (serror == SCRIPT_ERR_INVALID_STACK_OPERATION) {
                 // Unable to sign input and verification failed (possible attempt to partially sign).
                 TxInErrorToJSON(txin, vErrors, "Unable to sign input, invalid stack size (possibly missing key)");
+            } else if (serror == SCRIPT_ERR_SIG_NULLFAIL) {
+                // Verification failed (possibly due to insufficient signatures).
+                TxInErrorToJSON(txin, vErrors, "CHECK(MULTI)SIG failing with non-zero signature (possibly need more signatures)");
             } else {
                 TxInErrorToJSON(txin, vErrors, ScriptErrorString(serror));
             }
diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py
index 62f3843756..056e193d55 100755
--- a/test/functional/rpc_createmultisig.py
+++ b/test/functional/rpc_createmultisig.py
@@ -134,6 +134,27 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
 
         assert_raises_rpc_error(-8, "Missing redeemScript/witnessScript", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
 
+        # if witnessScript specified, all ok
+        prevtx_err["witnessScript"] = prevtxs[0]["redeemScript"]
+        node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
+
+        # both specified, also ok
+        prevtx_err["redeemScript"] = prevtxs[0]["redeemScript"]
+        node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
+
+        # redeemScript mismatch to witnessScript
+        prevtx_err["redeemScript"] = "6a" # OP_RETURN
+        assert_raises_rpc_error(-8, "redeemScript does not correspond to witnessScript", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
+
+        # redeemScript does not match scriptPubKey
+        del prevtx_err["witnessScript"]
+        assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
+
+        # witnessScript does not match scriptPubKey
+        prevtx_err["witnessScript"] = prevtx_err["redeemScript"]
+        del prevtx_err["redeemScript"]
+        assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err])
+
         rawtx2 = node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs - 1], prevtxs)
         rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [self.priv[-1]], prevtxs)