Add P2SH-P2WSH support to signrawtransaction and listunspent RPC by meshcollider · Pull Request #11708 · bitcoin/bitcoin

meshcollider · 2017-11-17T02:18:43Z

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~~

NicolasDorier · 2017-11-17T05:21:45Z

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 · 2017-11-17T05:35:09Z

@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 · 2017-11-17T06:18:56Z

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).

jonasschnelli · 2017-11-17T18:48:26Z

src/wallet/rpcwallet.cpp

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.

jonasschnelli · 2017-11-17T21:04:34Z

src/wallet/rpcwallet.cpp

nit: replace 32 with sizeof()?

jonasschnelli · 2017-11-17T21:10:02Z

Concept ACK. Can you add a test?

meshcollider · 2017-11-17T22:57:27Z

@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

jonasschnelli · 2017-11-18T07:42:13Z

src/wallet/rpcwallet.cpp

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

Heh missed that, thanks :P

meshcollider · 2017-11-18T10:24:04Z

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

NicolasDorier · 2017-11-20T05:04:04Z

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 · 2017-11-20T05:08:07Z

@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 · 2017-11-20T05:11:08Z

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 · 2017-11-20T06:59:15Z

@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 · 2017-11-20T11:19:06Z

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 · 2017-11-21T02:34:47Z

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 · 2017-11-23T07:06:04Z

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

NicolasDorier · 2017-11-24T06:58:31Z

src/rpc/rawtransaction.cpp

you are not using that anymore

NicolasDorier · 2017-11-24T06:58:37Z

src/rpc/rawtransaction.cpp

you are not using that anymore

meshcollider · 2017-11-28T23:42:56Z

Fixed, thanks @NicolasDorier
Squashed fixups too

NicolasDorier · 2017-11-29T03:18:22Z

test/functional/signrawtransactions.py

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

NicolasDorier · 2017-11-29T03:25:23Z

Code review ACK

meshcollider · 2017-12-12T22:50:54Z

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

NicolasDorier · 2017-12-13T02:32:15Z

reACK

ryanofsky · 2017-12-13T16:17:32Z

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:

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

ryanofsky

utACK a1dec739839053ce095f84ea1d281744124896e6

ryanofsky · 2017-12-13T16:58:08Z

src/rpc/rawtransaction.cpp

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.

ryanofsky · 2017-12-13T17:01:55Z

src/rpc/rawtransaction.cpp

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

ryanofsky · 2017-12-13T17:22:00Z

src/wallet/rpcwallet.cpp

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.

ryanofsky · 2017-12-13T17:43:17Z

src/wallet/rpcwallet.cpp

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

ryanofsky · 2017-12-18T14:09:31Z

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 · 2017-12-19T23:12:33Z

Addressed @ryanofsky feedback, thanks!

promag · 2017-12-19T23:19:33Z

src/rpc/rawtransaction.cpp

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

if (rs.getType() == UniValue::VARR) { for (auto r : rs.getValues()) { scripts.push_back(r.get_str()); } } else { scripts.push_back(rs.get_str()); }

Nice idea, done, thanks :)

meshcollider · 2017-12-21T00:29:27Z

Feedback addressed, fixup commits squashed into one cleanup commit

meshcollider · 2017-12-21T00:30:43Z

test/functional/rawtransactions.py

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.

ryanofsky

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

ryanofsky · 2017-12-21T15:49:43Z

src/rpc/rawtransaction.cpp

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

ryanofsky · 2017-12-21T15:51:23Z

src/rpc/rawtransaction.cpp

Could use const auto& to avoid univalue copies

ryanofsky · 2017-12-21T16:00:19Z

src/rpc/rawtransaction.cpp

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.

meshcollider · 2017-12-24T02:32:59Z

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

ryanofsky

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

meshcollider · 2018-01-17T00:51:31Z

Rebased to fix merge conflict in rawtransactions.py

ryanofsky

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

sipa · 2018-02-12T23:53:02Z

Concept ACK. Rebase/squash last commit?

meshcollider · 2018-02-13T08:10:11Z

@sipa done :)

5f605e1 Make signrawtransaction accept P2SH-P2WSH redeemscripts (Pieter Wuille) Pull request description: This is a quick fix for #12418, which is a regression in 0.16. It permits specifying just the inner redeemscript to let `signrawtransaction` succeed. This inner redeemscript is already reported by `addmultisigaddress` & co. #11708 uses a different approach, where `listunspent` reports both inner & outer redeemscript, but requires both to be provided to `signrawtransaction`. Part of #11708 is still needed even in combination with this PR however, as currently the inner redeemscript isn't reported by `listunspent`. Tree-SHA512: a6fa2b2661ce04db25cf029dd31da39c0b4811d43692f816dfe0f77b4159b5e2952051664356a579f690ccd58a626e0975708afcd7ad5919366c490944e3a9a5

luke-jr · 2018-03-07T16:47:01Z

src/wallet/rpcwallet.cpp

+                        if (pwallet->GetCScript(id, witnessScript)) {
+                            scripts.push_back(HexStr(witnessScript.begin(), witnessScript.end()));
+                        }
+                        entry.push_back(Pair("redeemScript", scripts));


IIRC, Pair push_backs were just cleaned up... :/

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

luke-jr · 2018-03-12T18:09:03Z

doc/release-notes.md

+
+- The `listunspent` output will now return an array of scripts as the `redeemScript` in the case of P2SH-P2WSH,
+  for compatibility with the `signrawtransaction` change above. In other cases, `listunspent` will continue to return
+  the only `redeemScript` as a string, as before.


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

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

maflcko · 2018-08-25T16:35:17Z

Needs rebase

practicalswift · 2018-09-02T18:25:47Z

src/rpc/rawtransaction.cpp

+            UniValue rs = find_value(prevOut, "redeemScript");
+            if (rs.getType() != UniValue::VNULL) {
+                if (!(scriptPubKey.IsPayToScriptHash() || scriptPubKey.IsPayToWitnessScriptHash())) {
+                    // if this isnt P2SH or P2WSH, scripts should not be provided


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

meshcollider · 2018-10-15T05:49:21Z

Closing this, replaced by #14481

Rspigler · 2018-11-06T23:30:39Z

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

6ca836a Add release note for listunspent P2WSH change (MeshCollider) 928beae Add test for P2SH-P2WSH in signrawtransactionwithkey and listunspent (MeshCollider) 314784a Make listunspent and signrawtransaction RPCs support witnessScript (MeshCollider) Pull request description: This is a reworked version of #11708 after #12427 and the `signrawtransaction` split. For a P2WSH address, listunspent should return the witness script, and for a P2SH-P2WSH address, it should also return the inner witness script (because SignTransaction will automatically wrap it in P2SH if required). Includes a test which also tests the behaviour of #12427, and release note. Tree-SHA512: a8e72cf16930312bf48ec47e44a68f8d7e26664043c1b4cc0983eb25aec4087e511188ff9a0f181cd7b8a0c068c60d7f1e7e3f226b79e8c48890039dcf57f7b7

fanquake added RPC/REST/ZMQ Wallet labels Nov 17, 2017

jonasschnelli reviewed Nov 17, 2017

View reviewed changes

src/wallet/rpcwallet.cpp Outdated

Copy link

Contributor

jonasschnelli Nov 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: replace 32 with sizeof()?

jonasschnelli reviewed Nov 18, 2017

View reviewed changes

NicolasDorier reviewed Nov 24, 2017

View reviewed changes

src/rpc/rawtransaction.cpp Outdated

Copy link

Contributor

NicolasDorier Nov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are not using that anymore

NicolasDorier reviewed Nov 24, 2017

View reviewed changes

src/rpc/rawtransaction.cpp Outdated

Copy link

Contributor

NicolasDorier Nov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are not using that anymore

NicolasDorier reviewed Nov 29, 2017

View reviewed changes

ajtowns mentioned this pull request Dec 10, 2017

Add Travis check for unused Python imports #11835

Merged

ryanofsky reviewed Dec 14, 2017

View reviewed changes

promag reviewed Dec 19, 2017

View reviewed changes

meshcollider commented Dec 21, 2017

View reviewed changes

ryanofsky reviewed Dec 21, 2017

View reviewed changes

ryanofsky reviewed Jan 3, 2018

View reviewed changes

ryanofsky reviewed Jan 17, 2018

View reviewed changes

bitcoinhodler mentioned this pull request Feb 12, 2018

signrawtransaction unable to sign p2sh-segwit multisig transaction #12418

Closed

sipa mentioned this pull request Feb 13, 2018

Make signrawtransaction accept P2SH-P2WSH redeemscripts #12427

Merged

meshcollider added 5 commits February 15, 2018 17:48

Allow array of redeemScripts in signrawtransaction RPC

063fbae

Make listunspent output array of redeemScripts for P2SH-P2WSH

b3f3ad4

Add test for P2SH-P2WSH signrawtransaction

98c29fd

Add release notes for signrawtransaction/listunspent witnessScript

b85cc9c

Check that redeemScript for P2SH-P2WSH from listunspent is correct

576624c

luke-jr reviewed Mar 7, 2018

View reviewed changes

luke-jr reviewed Mar 12, 2018

View reviewed changes

maflcko added the Needs rebase label Jun 6, 2018

practicalswift reviewed Sep 2, 2018

View reviewed changes

meshcollider mentioned this pull request Oct 15, 2018

Add P2SH-P2WSH support to listunspent RPC #14481

Merged

meshcollider closed this Oct 15, 2018

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

meshcollider commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Nov 17, 2017

Uh oh!

meshcollider commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Nov 17, 2017

Uh oh!

meshcollider commented Nov 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Nov 20, 2017

Uh oh!

NicolasDorier commented Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Nov 20, 2017

Uh oh!

meshcollider commented Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Nov 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Nov 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Nov 29, 2017

Uh oh!

meshcollider commented Dec 12, 2017

Uh oh!

NicolasDorier commented Dec 13, 2017

Uh oh!

ryanofsky commented Dec 13, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Dec 18, 2017

Uh oh!

meshcollider commented Dec 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Nov 17, 2017 •

edited

Loading

NicolasDorier commented Nov 17, 2017 •

edited

Loading

meshcollider commented Nov 17, 2017 •

edited

Loading

meshcollider commented Nov 18, 2017 •

edited

Loading

NicolasDorier commented Nov 20, 2017 •

edited

Loading

NicolasDorier commented Nov 20, 2017 •

edited

Loading

meshcollider commented Nov 20, 2017 •

edited

Loading

NicolasDorier commented Nov 21, 2017 •

edited

Loading

meshcollider Dec 21, 2017 •

edited

Loading

meshcollider commented Dec 24, 2017 •

edited

Loading

meshcollider commented Jan 17, 2018 •

edited

Loading