rpc: Improve signrawtransaction error reporting

ajtowns commented at 12:45 pm on June 20, 2019: member

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

fanquake added the label RPC/REST/ZMQ on Jun 20, 2019

DrahtBot commented at 12:50 pm on June 20, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16841 (Replace GetScriptForWitness with GetScriptForDestination by meshcollider)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ajtowns commented at 12:51 pm on June 20, 2019: member

With this patch, partial signature error message looks like:

"error": "CHECK(MULTI)SIG failing with non-zero signature (possibly need more signatures)"

instead of:

"error": "Signature must be zero for failed CHECK(MULTI)SIG operation"

Mismatching scripts give a new error of either:

error code: -3
error message:
redeemScript/witnessScript does not match scriptPubKey

or

error code: -3
error message:
redeemScript does not correspond to witnessScript

instead of an apparently successful operation (but unchanged rawtx) with embedded error of:

"error": "Witness program was passed an empty witness"

ajtowns force-pushed on Jun 25, 2019

ajtowns force-pushed on Jul 3, 2019

ajtowns marked this as ready for review on Jul 3, 2019

in src/rpc/rawtransaction_util.cpp:211 in 01174596e6 outdated

220@@ -221,6 +221,9 @@ UniValue SignTransaction(CMutableTransaction& mtx, const UniValue& prevTxsUnival
221                     // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH).
222                     keystore->AddCScript(GetScriptForWitness(witnessScript));
223                 }
224+                if (rs.isNull() && ws.isNull()) {
225+                    throw JSONRPCError(RPC_INVALID_PARAMETER, "Missing redeemScript/witnessScript");
226+                }

MarcoFalke commented at 2:55 pm on July 3, 2019:

I feel like this has already been merged

ajtowns commented at 3:03 pm on July 3, 2019:

Yeah, I just rebased it ( ajtowns force-pushed the ajtowns:201906-signrawerror branch from 073a2f8 to f7e4676) but github’s not picking up the new HEAD?

MarcoFalke commented at 3:07 pm on July 3, 2019:

It is well known for GitHub to show outdated branches in the web view (see also #15847 (comment))

I guess you have to force push twice?

ajtowns commented at 3:10 pm on July 3, 2019:

Looks like it’s doing the right thing now (I pushed a dummy commit and force-pushed it out of existence). You should be seeing f7e4676591c9027a6fe4dd1ebc9e1ed35b27574f as HEAD.

ajtowns force-pushed on Jul 3, 2019

meshcollider commented at 8:57 am on July 30, 2019: contributor

Concept ACK, looks right to me after an initial read

DrahtBot added the label Needs rebase on Sep 7, 2019

ajtowns force-pushed on Sep 7, 2019

DrahtBot removed the label Needs rebase on Sep 7, 2019

in src/rpc/rawtransaction_util.cpp:248 in a4b351a6c6 outdated

259+                    const CTxDestination p2wsh{WitnessV0ScriptHash(script)};
260+                    // plain p2wsh; could throw an error if script
261+                    // was specified by redeemScript rather than
262+                    // witnessScript (ie, ws.IsNull() == true), but
263+                    // accept it for backwards compat
264+

instagibbs commented at 2:10 pm on September 9, 2019:

nit: extra newline

ajtowns commented at 5:42 am on September 10, 2019:

wow, that is nitty! fixed

in src/rpc/rawtransaction_util.cpp:219 in a4b351a6c6 outdated

230+                std::vector<unsigned char> scriptData(!ws.isNull() ? ParseHexV(ws, "witnessScript") : ParseHexV(rs, "redeemScript"));
231+                CScript script(scriptData.begin(), scriptData.end());
232+                keystore->AddCScript(script);
233+                // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH).
234+                // This is done for redeemScript only for compatibility, it is encouraged to use the explicit witnessScript field instead.
235+                CScript witnessScript{GetScriptForWitness(script)};

instagibbs commented at 2:13 pm on September 9, 2019:

this is the witness output script aka script with version number and witness program, not witnessScript, right?

meshcollider commented at 4:46 am on September 10, 2019:

Yes, it is technically a P2SH redeemScript in the form of a P2WSH scriptPubKey, but I don’t think the current name is bad

ajtowns commented at 5:42 am on September 10, 2019:

fixed

ajtowns force-pushed on Sep 10, 2019

signrawtransactionwithkey: better error messages for bad redeemScript/witnessScript

This adds checks to ensure the redeemScript/witnessScript actually
correspond to the provided scriptPubKey, and, if both are provided,
that they are sensibly related to each other.

Thanks to github user passionofvc for raising this issue.

3c481f8921

signrawtransaction*: improve error for partial signing

Thanks to Danial Jaffy (tipu) for reporting this issue.

ec4c79326b

ajtowns force-pushed on Sep 10, 2019

instagibbs commented at 5:48 pm on September 10, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/16251/commits/ec4c79326bb670c2cc1757ecfb1900f8460c5257

meshcollider commented at 8:22 pm on September 10, 2019: contributor

utACK ec4c79326bb670c2cc1757ecfb1900f8460c5257

I really like this change

achow101 commented at 4:09 am on September 11, 2019: member

Code Review ACK ec4c79326bb670c2cc1757ecfb1900f8460c5257

fanquake renamed this:
~~Improve signrawtransaction error reporting~~
rpc: Improve signrawtransaction error reporting
on Sep 11, 2019

fanquake referenced this in commit 2296fe65f5 on Sep 11, 2019

fanquake merged this on Sep 11, 2019

fanquake closed this on Sep 11, 2019

fanquake referenced this in commit d052f5e6b7 on Aug 15, 2020

sidhujag referenced this in commit 75936a7014 on Aug 16, 2020

DrahtBot locked this on Dec 16, 2021

rpc: Improve signrawtransaction error reporting #16251

Conflicts