Add P2SH-P2WSH support to signrawtransaction and listunspent RPC

meshcollider commented at 2:18 am on November 17, 2017: contributor

Currently signrawtransaction works with P2SH-P2WSH which are already in wallet (e.g. addmultisigaddress -> addwitnessaddress). But when using signrawtransaction with keys which aren’t in the wallet, there is currently only a redeemScript key so you cannot enter both the P2SH redeemScript and the witness script. There is an undocumented workaround by including the same input twice (suggested on StackExchange here), once with each script, but that is unnecessary and hacky.

This simply allows the optional inclusion of a witnessScript key in the JSON input to signrawtransaction. Because it uses JSON, this is a non-breaking change.

Also, as discussed on IRC (see here), we add a witnessScript output to the listunspent RPC for P2SH-P2WSH addresses because gmaxwell pointed out signrawtransaction should be able to get most of the needed info from listunspent.

Closes #11693

TODO:

~Needs tests + release notes~

fanquake added the label RPC/REST/ZMQ on Nov 17, 2017

fanquake added the label Wallet on Nov 17, 2017

NicolasDorier commented at 5:21 am on November 17, 2017: contributor

NACK, redeemScript + scriptPubKey are sufficient information to know whether you are talking about P2SH-P2WSH, P2SH, P2WSH. Having one more “witness script” is just redundant information which can go wrong.

If you enter the witness script in “redeemScript” field, you can easily find out in code if it mean P2SH or Witness by comparing with the scriptPubKey, there is no ambiguity.

sipa commented at 5:35 am on November 17, 2017: member

@NicolasDorier How so? The redeemscript only contains a hash of the witness script in the case of P2SH-P2WSH, which is not sufficient to sign with.

It would be possible to require passing only the witness script, and automatically also add the v0 P2WSH redeemscript (and any future versions?). But just the redeemscript is not enough.

meshcollider commented at 6:18 am on November 17, 2017: contributor

Indeed, IMO its much cleaner implementation and user experience to have the witnessScript provided in addition to the redeemScript, not only because that avoids the ambiguous witness version issue in a nice way but also because then it is clear to the user which script the redeemScript actually refers to (because the redeemScript output of other RPCs in general would refer to the wrapper P2SH redeemScript not the witnessScript which would introduce confusion).

in src/wallet/rpcwallet.cpp:2812 in 9e5c9ebe02 outdated

2818-            "    \"redeemScript\" : n        (string) The redeemScript if scriptPubKey is P2SH\n"
2819-            "    \"spendable\" : xxx,        (bool) Whether we have the private keys to spend this output\n"
2820-            "    \"solvable\" : xxx,         (bool) Whether we know how to spend this output, ignoring the lack of keys\n"
2821-            "    \"safe\" : xxx              (bool) Whether this output is considered safe to spend. Unconfirmed transactions\n"
2822+            "    \"txid\" : \"txid\",            (string) the transaction id \n"
2823+            "    \"vout\" : n,                   (numeric) the vout value\n"

jonasschnelli commented at 6:48 pm on November 17, 2017:

Watch out with the indentation,… the \" result in one char. It may look unaligned in the source code but is aligned in the help output.

in src/wallet/rpcwallet.cpp:2931 in 9e5c9ebe02 outdated

2923@@ -2923,6 +2924,26 @@ UniValue listunspent(const JSONRPCRequest& request)
2924                 CScript redeemScript;
2925                 if (pwallet->GetCScript(hash, redeemScript)) {
2926                     entry.push_back(Pair("redeemScript", HexStr(redeemScript.begin(), redeemScript.end())));
2927+                    CTxDestination witness_destination;
2928+                    if(redeemScript.IsPayToWitnessScriptHash() && ExtractDestination(redeemScript, witness_destination)) {
2929+                        const WitnessV0ScriptHash& whash = boost::get<WitnessV0ScriptHash>(witness_destination);
2930+                        CScriptID id;
2931+                        CRIPEMD160().Write(whash.begin(), 32).Finalize(id.begin());

jonasschnelli commented at 9:04 pm on November 17, 2017:

nit: replace 32 with sizeof()?

jonasschnelli commented at 9:10 pm on November 17, 2017: contributor

Concept ACK. Can you add a test?

meshcollider commented at 10:57 pm on November 17, 2017: contributor

@jonasschnelli yep will do, see TODO section :) Just trying to work out the cleanest way to test it wrt all the existing tests in segwit.py

in src/wallet/rpcwallet.cpp:2943 in a0b90c9edf outdated

2938+            }
2939+
2940+            if (scriptPubKey.IsPayToWitnessScriptHash()) {
2941+                const WitnessV0ScriptHash& whash = boost::get<WitnessV0ScriptHash>(address);
2942+                CScriptID id;
2943+                CRIPEMD160().Write(whash.begin(), 32).Finalize(id.begin());

jonasschnelli commented at 7:42 am on November 18, 2017:

nit: the whash.size() should also go here… :P

meshcollider commented at 7:52 am on November 18, 2017:

Heh missed that, thanks :P

meshcollider commented at 10:24 am on November 18, 2017: contributor

Test and release notes added, @jonasschnelli’s nits addresses

NicolasDorier commented at 5:04 am on November 20, 2017: contributor

This is how I am doing with NBitcoin for long. @sipa if you have redeemscript and scriptPubKey you can know exactly wether redeemScript is a Segwit or P2SH redeem.

If scriptPubKey is P2WSH, then you know redeemScript is a segwit redeem.

If scriptPubKey is P2SH, you know redeemScript is either a P2SH redeem or a Segwit Redeem.

If Hash(redeemScript) is contained inside scriptPubKey, then it is a P2SH redeem else it is a segwit redeem. If it is a P2SH redeem, and inside is a segwit program, you must return error to the user.

You can then Hash(Hash(redeemScript)) and verify that it is indeed into ScriptPubKey to be sure the user did not messed up.

You can derive the P2SH redeem from the Segwit redeem, so no need to specify both.

Again, strong NACK on this one. This is not needed.

sipa commented at 5:08 am on November 20, 2017: member

@NicolasDorier Ah, I’m using redeemscript exclusively for P2SH.

Yes, if you have the witness script, and can correctly guess it is a V0 P2WSH embedded in P2SH, you can derive the redeemscript.

I’m not convinced guessing is the best approach.

NicolasDorier commented at 5:11 am on November 20, 2017: contributor

Well the thing is that this field is called redeemScript which does not suppose it is P2SH or Segwit redeem, and it does not have to.

If you insist having both redeem, then I would suggest to rename redeemScript as p2shRedeem.

But then, the logic of this RPC method will need to ensure that all, witnessScript, p2shRedeem and scriptPubKey are coherent.

I am convinced specifying both is just wrong as there is no ambiguity, no way to guess incorrectly. Just more way for the user to get wrong and confused, and more code to ensure everything is coherent on both side.

sipa commented at 6:59 am on November 20, 2017: member

@NicolasDorier That’s the fair point, the naming would be inconsistent in that case.

We also don’t actually need to distinguish for our implementation. @MeshCollider perhaps we can just permit redeemScript to be a list, in which case multiple scripts gets loaded with AddCScript?

meshcollider commented at 11:19 am on November 20, 2017: contributor

Yep I’m happy with that approach too, I just definitely want to avoid guessing the witness version as mentioned. Although re: terminology, I’ve only ever seen redeemScript used to refer exclusively to P2SH, even the bitcoincore.org segwit wallet dev guide refers to the witness redeem script as witnessScript only.

NicolasDorier commented at 2:34 am on November 21, 2017: contributor

The fact that “redeemScript” can refer to all type of PS2SH*-P2WSH* redeems makes migration from P2SH to Segwit easier, as there is no additional fields to pass around that did not existed before.

In NBitcoin, there is a type called ScriptCoin which was used to represent a P2SH + Redeem. When I updated the library for Segwit, I only needed to instruct my users that if they want to migrate to P2SH-P2WSH they just need a different way of calculating the ScriptPubKey when generating a new address. The majority of their code did not changed. (No additional field, old non-segwit UTXO were still working fine, no additional type to know about, no if/else logic for handling both way in their code)

That said I would be fine with redeemScript being an array as well.

meshcollider commented at 7:06 am on November 23, 2017: contributor

I’ve made the modification to include the witnessScript in a redeem script array in the case of P2SH-P2WSH, will squash those modification commits if that approach is acceptable

in src/rpc/rawtransaction.cpp:672 in 8cb0b73b72 outdated

668@@ -669,6 +669,7 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
669             "         \"vout\":n,                  (numeric, required) The output number\n"
670             "         \"scriptPubKey\": \"hex\",   (string, required) script key\n"
671             "         \"redeemScript\": \"hex\",   (string, required for P2SH or P2WSH) redeem script\n"
672+            "         \"witnessScript\": \"hex\",  (string, required for P2SH-P2WSH) witness script\n"

NicolasDorier commented at 6:58 am on November 24, 2017:

you are not using that anymore

in src/rpc/rawtransaction.cpp:653 in 8cb0b73b72 outdated

649@@ -650,7 +650,7 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
650 
651     if (request.fHelp || request.params.size() < 1 || request.params.size() > 4)
652         throw std::runtime_error(
653-            "signrawtransaction \"hexstring\" ( [{\"txid\":\"id\",\"vout\":n,\"scriptPubKey\":\"hex\",\"redeemScript\":\"hex\"},...] [\"privatekey1\",...] sighashtype )\n"
654+            "signrawtransaction \"hexstring\" ( [{\"txid\":\"id\",\"vout\":n,\"scriptPubKey\":\"hex\",\"redeemScript\":\"hex\",\"witnessScript\":\"hex\"},...] [\"privatekey1\",...] sighashtype )\n"

NicolasDorier commented at 6:58 am on November 24, 2017:

you are not using that anymore

meshcollider commented at 11:42 pm on November 28, 2017: contributor

Fixed, thanks @NicolasDorier Squashed fixups too

in test/functional/signrawtransactions.py:165 in 43fc622821 outdated

157+        self.nodes[0].generate(1)
158+        self.sync_all()
159+
160+        # Find the UTXO for the transaction node[1] should have received, check witnessScript matches
161+        unspent_output = self.nodes[1].listunspent(0, 999999, [p2sh_p2wsh_address])[0]
162+        assert_equal(unspent_output["redeemScript"][1], bytes_to_hex_str(witness_script))

NicolasDorier commented at 3:18 am on November 29, 2017:

nit: check if the redeemScript[0] is the p2sh redeem.

meshcollider commented at 10:49 pm on December 12, 2017:

Done

NicolasDorier commented at 3:25 am on November 29, 2017: contributor

Code review ACK

meshcollider commented at 10:50 pm on December 12, 2017: contributor

Rebased and added the extra check as suggested by @NicolasDorier. That also fixed the unused OP_0 warning.

NicolasDorier commented at 2:32 am on December 13, 2017: contributor

reACK

ryanofsky commented at 4:17 pm on December 13, 2017: member

Should passing a redeem script array to signrawtransaction be possible with bitcoin-cli? It looks this might not work currently because the param does not have an entry in vRPCConvertParams. You might be able to work around this with something like:

0if (redeem_script.isStr() && !IsHex(redeem_script.get_str())) {
1    redeem_script.read(redeem_script.get_str().substr());
2    RPCTypeCheckArgument(redeem_script, UniValue::VARR);
3}

in src/rpc/rawtransaction.cpp:827 in 82ef24d7f8 outdated

824@@ -825,17 +825,18 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
825             // if redeemScript given and not using the local wallet (private keys
826             // given), add redeemScript to the tempKeystore so it can be signed:
827             if (fGivenKeys && (scriptPubKey.IsPayToScriptHash() || scriptPubKey.IsPayToWitnessScriptHash())) {

ryanofsky commented at 4:58 pm on December 13, 2017:

Not directly related to this PR, but seems like it might good to return an error if this condition is false instead of skipping validation of the redeemscript parameter.

in src/rpc/rawtransaction.cpp:837 in 82ef24d7f8 outdated

843+                } else if (rs.getType() == UniValue::VARR) {
844+                    // If redeemScript is an array, iterate over all scripts provided
845+                    scripts = rs.getValues();
846+                }
847+                for (const UniValue& script : scripts) {
848+                    if (script.getType() != UniValue::VSTR) continue;

ryanofsky commented at 5:01 pm on December 13, 2017:

Maybe drop this line. It prevents get_str() below from throwing an exception if the wrong type is passed.

in src/wallet/rpcwallet.cpp:2852 in 83a418da22 outdated

2858+            "    \"address\" : \"address\",      (string) the bitcoin address\n"
2859+            "    \"account\" : \"account\",      (string) DEPRECATED. The associated account, or \"\" for the default account\n"
2860+            "    \"scriptPubKey\" : \"key\",     (string) the script key\n"
2861+            "    \"amount\" : x.xxx,           (numeric) the transaction output amount in " + CURRENCY_UNIT + "\n"
2862+            "    \"confirmations\" : n,        (numeric) The number of confirmations\n"
2863+            "    \"redeemScript\" : \"script\" | [\"script\", ... ],  (string or array) The redeemScript and/or witnessScript if using P2SH/P2WSH\n"

ryanofsky commented at 5:22 pm on December 13, 2017:

It might be good to explicitly say this will be an array for p2wsh and nested p2wsh outputs, a string for traditional p2sh outputs, and unset in other cases.

in src/wallet/rpcwallet.cpp:3046 in 83a418da22 outdated

2980+            if (scriptPubKey.IsPayToWitnessScriptHash()) {
2981+                const WitnessV0ScriptHash& whash = boost::get<WitnessV0ScriptHash>(address);
2982+                CScriptID id;
2983+                CRIPEMD160().Write(whash.begin(), whash.size()).Finalize(id.begin());
2984+                CScript witnessScript;
2985+                if (pwallet->GetCScript(id, witnessScript)) {

ryanofsky commented at 5:43 pm on December 13, 2017:

IIUC, this should never be false? Would be helpful to have a comment saying this, or else mentioning when it would be expected.

ryanofsky commented at 6:34 pm on December 14, 2017: member

utACK a1dec739839053ce095f84ea1d281744124896e6

ryanofsky commented at 2:09 pm on December 18, 2017: member

This PR has two code review acks (from me and NicolasDorier) so maybe it is close to being merged. The remaining comments that haven’t been responded to are minor and shouldn’t hold this up.

meshcollider commented at 11:12 pm on December 19, 2017: contributor

Addressed @ryanofsky feedback, thanks!

in src/rpc/rawtransaction.cpp:854 in 5749f261df outdated

847+                if (rs.getType() == UniValue::VSTR) {
848+                    scripts.push_back(rs);
849+                } else if (rs.getType() == UniValue::VARR) {
850+                    // If redeemScript is an array, iterate over all scripts provided
851+                    scripts = rs.getValues();
852+                }

promag commented at 11:19 pm on December 19, 2017:

Missing error when not string or array? Suggestion (scripts is a vector of strings):

0if (rs.getType() == UniValue::VARR) {
1    for (auto r : rs.getValues()) {
2         scripts.push_back(r.get_str());
3    }
4} else {
5    scripts.push_back(rs.get_str());
6}

meshcollider commented at 0:44 am on December 20, 2017:

Nice idea, done, thanks :)

meshcollider commented at 0:29 am on December 21, 2017: contributor

Feedback addressed, fixup commits squashed into one cleanup commit

in test/functional/rawtransactions.py:183 in 50074643f2 outdated

179@@ -180,7 +180,7 @@ def run_test(self):
180                 break
181 
182         bal = self.nodes[0].getbalance()
183-        inputs = [{ "txid" : txId, "vout" : vout['n'], "scriptPubKey" : vout['scriptPubKey']['hex'], "redeemScript" : mSigObjValid['hex']}]
184+        inputs = [{ "txid" : txId, "vout" : vout['n'], "scriptPubKey" : vout['scriptPubKey']['hex']}]

meshcollider commented at 0:30 am on December 21, 2017:

This redeemScript parameter was being silently ignored until now, because scripts are not used by signrawtransaction if no keys are passed in (the internal wallet is used instead). New code returns an error instead of silently ignoring it, so it was removed.

in src/rpc/rawtransaction.cpp:844 in 50074643f2 outdated

851+                    scripts.push_back(rs.get_str());
852                 }
853-                for (const UniValue& script : scripts) {
854-                    if (script.getType() != UniValue::VSTR) continue;
855-                    std::vector<unsigned char> script_data(ParseHexV(script.get_str(), "redeemScript"));
856+                for (const std::string script : scripts) {

ryanofsky commented at 3:49 pm on December 21, 2017:

Could use const reference to avoid string copy here (though there are more copies below anyway).

in src/rpc/rawtransaction.cpp:838 in 50074643f2 outdated

842+                }
843+                std::vector<std::string> scripts;
844+                if (rs.getType() == UniValue::VARR) {
845                     // If redeemScript is an array, iterate over all scripts provided
846-                    scripts = rs.getValues();
847+                    for (auto r : rs.getValues()) {

ryanofsky commented at 3:51 pm on December 21, 2017:

Could use const auto& to avoid univalue copies

in src/rpc/rawtransaction.cpp:832 in 50074643f2 outdated

835+            if (rs.getType() != UniValue::VNULL) {
836+                if (!(scriptPubKey.IsPayToScriptHash() || scriptPubKey.IsPayToWitnessScriptHash())) {
837+                    // if this isnt P2SH or P2WSH, scripts should not be provided
838+                    throw JSONRPCError(RPC_INVALID_PARAMETER, "Scripts should not be provided if P2SH or P2WSH are not being used");
839+                } else if (!fGivenKeys) {
840+                    // if private keys aren't given, assume we are using the local wallet, so we should not accept scripts

ryanofsky commented at 4:00 pm on December 21, 2017:

Maybe s/should not accept/don’t currently support accepting/. This seems more like a reasonable limitation of the current implementation than a desirable feature of it.

ryanofsky commented at 4:01 pm on December 21, 2017: member

utACK 50074643f20cc36e6ffbbec22f1c850c6e9a3207. Only change since last review is the new cleanup commit. Feel free to ignore very minor review suggestions below.

meshcollider commented at 2:32 am on December 24, 2017: contributor

Fixed @ryanofsky’s comments, thanks :) Haven’t tested CLI yet though

fanquake added this to the "In progress" column in a project

ryanofsky commented at 5:57 pm on January 3, 2018: member

utACK 22397a43a6601e76582053dc95381196f520a610. Sorry for the delay, only changes since last review were the suggested comment & for loop tweaks.

meshcollider commented at 0:51 am on January 17, 2018: contributor

Rebased to fix merge conflict in rawtransactions.py

ryanofsky commented at 7:54 pm on January 17, 2018: member

utACK 73b582dc6624ffc27dc65cf7f60e6ff8829b6691. No changes since last review except for rebase and simple conflict resolution in rawtransactions.py.

sipa commented at 11:53 pm on February 12, 2018: member

Concept ACK. Rebase/squash last commit?

meshcollider commented at 8:10 am on February 13, 2018: contributor

@sipa done :)

Allow array of redeemScripts in signrawtransaction RPC 063fbae04b

Make listunspent output array of redeemScripts for P2SH-P2WSH b3f3ad41aa

Add test for P2SH-P2WSH signrawtransaction 98c29fd458

Add release notes for signrawtransaction/listunspent witnessScript b85cc9c60c

Check that redeemScript for P2SH-P2WSH from listunspent is correct 576624ce95

laanwj referenced this in commit 3fa556aee2 on Feb 15, 2018

in src/wallet/rpcwallet.cpp:3033 in 576624ce95

3029+                        UniValue scripts(UniValue::VARR);
3030+                        scripts.push_back(HexStr(redeemScript.begin(), redeemScript.end()));
3031+                        if (pwallet->GetCScript(id, witnessScript)) {
3032+                            scripts.push_back(HexStr(witnessScript.begin(), witnessScript.end()));
3033+                        }
3034+                        entry.push_back(Pair("redeemScript", scripts));

luke-jr commented at 4:47 pm on March 7, 2018:

IIRC, Pair push_backs were just cleaned up… :/

meshcollider commented at 2:16 pm on March 8, 2018:

Yes, this needs a large rebase and cleanup anyway now signrawtransaction was split up, things like this will be done at the same time when I get onto this

in doc/release-notes.md:71 in 576624ce95

62@@ -63,6 +63,14 @@ RPC changes
63 
64 - The `fundrawtransaction` rpc will reject the previously deprecated `reserveChangeKey` option.
65 
66+- The `redeemScript` for each of the `prevtxs` given to `signrawtransaction` can now optionally be an array
67+  of scripts, so both the redeemScript and witnessScript for P2SH-P2WSH addresses can be passed in.
68+
69+- The `listunspent` output will now return an array of scripts as the `redeemScript` in the case of P2SH-P2WSH,
70+  for compatibility with the `signrawtransaction` change above. In other cases, `listunspent` will continue to return
71+  the only `redeemScript` as a string, as before.

luke-jr commented at 6:09 pm on March 12, 2018:

Is there a reason to break the API here? Why not add a new key for the witnessScript?

meshcollider commented at 11:13 pm on March 12, 2018:

The API only breaks in the case of P2SH-P2WSH which was not supported at all before anyway. Adding a new key for witnessScript to listunspent would make its output incompatible with the signrawtransaction input which expects a list of scripts in this case

MarcoFalke added the label Needs rebase on Jun 6, 2018

MarcoFalke commented at 4:35 pm on August 25, 2018: member

Needs rebase

in src/rpc/rawtransaction.cpp:840 in 576624ce95

849-                    CScript redeemScript(rsData.begin(), rsData.end());
850+            // if redeemScript given add redeemScript to the tempKeystore so it can be signed:
851+            UniValue rs = find_value(prevOut, "redeemScript");
852+            if (rs.getType() != UniValue::VNULL) {
853+                if (!(scriptPubKey.IsPayToScriptHash() || scriptPubKey.IsPayToWitnessScriptHash())) {
854+                    // if this isnt P2SH or P2WSH, scripts should not be provided

practicalswift commented at 6:25 pm on September 2, 2018:

Typo found by codespell: “isnt” should be “isn’t” or “is not” :-)

meshcollider commented at 5:49 am on October 15, 2018: contributor

Closing this, replaced by #14481

meshcollider closed this on Oct 15, 2018

Rspigler commented at 11:30 pm on November 6, 2018: contributor

@fanquake This should be removed from ‘In Progress’ in ‘Segwit’ and #14481 perhaps added?

fanquake removed this from the "In progress" column in a project

laanwj referenced this in commit 3facd9fdc4 on Feb 14, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021

Add P2SH-P2WSH support to signrawtransaction and listunspent RPC #11708