Add P2SH-P2WSH support to listunspent RPC #14481

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

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.

Sjors · 2018-10-17T08:40:16Z

src/wallet/rpcwallet.cpp

Why not a separate witnessScript field like in #14454?

@Sjors because this has to be compatible with how signrawtransaction works, so it can be passed directly in. signrawtransaction only accepts the witness script as "redeemScript" and automatically wraps it

Would it be super involved to add support for witnessScript to signrawtransactionwithkey and signrawtransactionwithwallet? The terminology is already quite confusing, so consistency can really help.

Agreed with @Sjors I'd much rather we fix terminology instead of compounding confusion.

re: #14481 (comment)

Being consistent with terminology from https://bitcoincore.org/en/segwit_wallet_dev/#creation-of-p2sh-p2wsh-address and importmulti seems good. Making it possible to call signrawtransaction directly also seems good. Can't we do both by updating signrawtransaction to accept a witnessScript field?

If we did this, I guess there would still be question of whether signrawtransaction should remain backwards compatible and continue accepting overloaded redeemScript's introduced in #12427, or if that feature should be deprecated. I'd probably opt not to deprecate. Nicolas made decent usability arguments in #11708 (comment), #11708 (comment) that overloading redeemScript can make calling signraw less painful, and that it doesn't introduce ambiguities that would let callers shoot themselves in the foot (which I guess would not be the case with importmulti).

DrahtBot · 2018-10-20T09:57:37Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)

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.

ryanofsky

Code utACK c789fef8ca4739e2fb3c930bacad77543650b781, but I agree with others about not wanting to overload redeemScript in listunspent #14481 (comment). It seems like it should be possible to be compatible with signraw without overloading.

ryanofsky · 2018-11-14T16:03:24Z

src/wallet/rpcwallet.cpp

re: #14481 (comment)

Being consistent with terminology from https://bitcoincore.org/en/segwit_wallet_dev/#creation-of-p2sh-p2wsh-address and importmulti seems good. Making it possible to call signrawtransaction directly also seems good. Can't we do both by updating signrawtransaction to accept a witnessScript field?

If we did this, I guess there would still be question of whether signrawtransaction should remain backwards compatible and continue accepting overloaded redeemScript's introduced in #12427, or if that feature should be deprecated. I'd probably opt not to deprecate. Nicolas made decent usability arguments in #11708 (comment), #11708 (comment) that overloading redeemScript can make calling signraw less painful, and that it doesn't introduce ambiguities that would let callers shoot themselves in the foot (which I guess would not be the case with importmulti).

ryanofsky · 2018-11-14T16:56:43Z

src/wallet/rpcwallet.cpp

IsPayToWitnessScriptHash succeeding and ExtractDestination failing should never happen, right? I think it would be better to trigger an actual error in this case than to silently fall back to the non-witness case.

sipa · 2018-11-14T23:48:31Z

I wonder if it isn't just easier to support passing in a descriptor in signrawtransactionwithkey and using #14477 to get to get descriptors from listunspent etc? The various ways of passing around scripts in different RPCs looks like hard to solve mess.

ryanofsky · 2018-12-04T20:32:21Z

@meshcollider is there an update on the status of this PR?

c789fef8ca4739e2fb3c930bacad77543650b781 seems like an improvement to me, and has my ACK. But I do think it would be an easy win to replace "redeemScript" with "witnessScript" there, and tweak signrawtransaction to accept "witnessScript", so all RPCs could use these terms consistently.

Or if we want to abandon this approach and jump to descriptors instead as suggested in #14481 (comment), this seems like another good option. Do you have thoughts on it? Does it require more work? Is it blocked on anything now that #14477 is merged?

meshcollider · 2018-12-06T20:01:23Z

@ryanofsky I plan on modifying this for terminology as above for now, and then I think we should look at RPC descriptor support as a whole after that. But I will come back to this PR in the next few days

meshcollider · 2019-02-05T20:35:21Z

I've rebased this and addressed the terminology comments above, moving to a new witnessScript field with backwards compatibility

Sjors

bd2b1f65e949a1768c6f989677ea24b746384ca6 looks OK, but...

I have my doubt if backwards compatibility with something that was already buggy is worth the confusion of overloading redeemScript for listunspent. I'd rather have it null if signrawtransaction can infer it anyway, or even just the P2SH wrapper redeem script.

It might be better to squash the first and last commit.

I think addmultisigaddress should also return both redeemScript and witnessScript in the correct manner, so you you can just test:

assert_equal(unspent_output["witnessScript"], p2sh_p2wsh_address["witnessScript"])
assert_equal(unspent_output["redeemScript"], p2sh_p2wsh_address["redeemScript"])
assert_equal(unspent_output["redeemScript"], CScript([OP_0, sha256(hex_str_to_bytes(unspent_output["witnessScript"])))

meshcollider · 2019-02-11T19:48:28Z

Forgot to update the release notes, listunspent doesn't overload redeemScript anymore

Agree about addmultisigaddress, and about squashing 👍

meshcollider · 2019-02-11T23:09:26Z

I've squashed and updated the release notes, but modifying addmultisigaddress to return a witnessScript as well as redeemScript would be a breaking change, because currently the witnessScript is returned as "redeemScript". It would be more confusing to use different terminology so unless a breaking change is worth it here, which I don't think it is, I'll leave it as is

laanwj · 2019-02-12T09:20:09Z

doc/release-notes-14481.md

maybe optionally instead of also

laanwj · 2019-02-12T09:24:51Z

test/functional/rpc_signrawtransaction.py

Please pass fractional amounts as a Decimal

Sjors

to return a witnessScript as well as redeemScript would be a breaking change, because currently the witnessScript is returned as "redeemScript"

I could be wrong, but I suspect very few people use addmultisigaddress with SegWit in an automated setup (maybe @alecalve has stats on how much SegWit multisig is used in general). So it might still be early enough for a breaking change to use redeemScript and witnessScript accurately and consistently throughout the RPC.

(I can live with not doing it though)

Sjors · 2019-02-12T10:08:10Z

src/rpc/rawtransaction.cpp

I was also referring to this overloading.

ryanofsky

utACK 77c0c8e246e1a7f8c403c19d13c8bb699dba8cf4. Left some comments but I think this is good to merge as-is. Only changes since last review are rebase and following up on previous suggestions (main one being to switch from redeemScript to witnessScript). Thanks for adopting these!

change to use redeemScript and witnessScript accurately and consistently throughout the RPC

Would agree with Sjors in preferring to use these terms consistently, and breaking backwards compatibility in what seems to be a minor edge case. This could be done in a followup PR, and would be easy thing to explain in release notes.

ryanofsky · 2019-02-12T22:01:28Z

src/rpc/rawtransaction.cpp

In commit "Make listunspent return witness script for P2WSH or P2SH-P2WSH" (0b665ab577ab41e3b7b12d9b7d83c151cf90fdaf)

Could update commit message to mention change to signrawtransactionwithkey

ryanofsky · 2019-02-12T22:11:42Z

src/wallet/rpcwallet.cpp

In commit "Make listunspent return witness script for P2WSH or P2SH-P2WSH" (0b665ab577ab41e3b7b12d9b7d83c151cf90fdaf)

This would be broken if compiled with NDEBUG, which even though we don't support it, isn't really great for readability because you don't expect to see code with side effects inside an assert. Would be a little better to write something like:

bool extracted = ExtractDestitionation...; assert(extracted);

maflcko · 2019-02-12T23:54:55Z

Needs rebase (blame me)

meshcollider · 2019-02-13T01:27:09Z

Rebased and addressed comments above. @MarcoFalke I expect a review from you then ;)

addmultisigaddress change can be done in a followup PR to avoid adding extra review burden here so close to merge-readiness

ryanofsky

utACK 6ca836a. Changes since last review: rebase due to #14918 and making various requested changes (updating release notes and first commit message, removing assert side-effect, using python Decimal).

maflcko · 2019-02-14T20:36:29Z

utACK 6ca836a

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

meshcollider mentioned this pull request Oct 15, 2018

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

Closed

meshcollider added Wallet RPC/REST/ZMQ labels Oct 15, 2018

Sjors reviewed Oct 17, 2018

View reviewed changes

DrahtBot mentioned this pull request Oct 20, 2018

Rpc help helper class #14502

Closed

This was referenced Oct 20, 2018

docs: Consistent type names in RPC help descriptions #14459

Closed

RPC: Add ancestor{count,size,fees} to listunspent output #12677

Merged

practicalswift mentioned this pull request Oct 22, 2018

Enable flake8 rule E225 which checks for missing whitespace around op… #14534

Closed

DrahtBot mentioned this pull request Oct 29, 2018

[rpc] Descriptions: Consistent arg labels for types 'object', 'array', 'boolean', and 'string' #14601

Closed

ryanofsky approved these changes Nov 14, 2018

View reviewed changes

meshcollider added this to the 0.18.0 milestone Feb 8, 2019

Sjors reviewed Feb 11, 2019

View reviewed changes

laanwj reviewed Feb 12, 2019

View reviewed changes

doc/release-notes-14481.md Outdated

Copy link

Member

laanwj Feb 12, 2019

Choose a reason for hiding this comment

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

maybe optionally instead of also

laanwj reviewed Feb 12, 2019

View reviewed changes

test/functional/rpc_signrawtransaction.py Outdated

Copy link

Member

laanwj Feb 12, 2019

Choose a reason for hiding this comment

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

Please pass fractional amounts as a Decimal

Sjors reviewed Feb 12, 2019

View reviewed changes

src/rpc/rawtransaction.cpp Outdated

Copy link

Member

Sjors Feb 12, 2019

Choose a reason for hiding this comment

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

I was also referring to this overloading.

ryanofsky approved these changes Feb 12, 2019

View reviewed changes

maflcko added the Needs rebase label Feb 12, 2019

Make listunspent and signrawtransaction RPCs support witnessScript

314784a

meshcollider added 2 commits February 13, 2019 14:35

Add test for P2SH-P2WSH in signrawtransactionwithkey and listunspent

928beae

Add release note for listunspent P2WSH change

6ca836a

ryanofsky approved these changes Feb 13, 2019

View reviewed changes

DrahtBot removed the Needs rebase label Feb 13, 2019

laanwj merged commit 6ca836a into bitcoin:master Feb 14, 2019

meshcollider deleted the 201810_listunspent_wsh branch February 15, 2019 00:28

meshcollider mentioned this pull request Feb 16, 2019

importmulti changes not backward compatible #15392

Closed

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

Add P2SH-P2WSH support to listunspent RPC #14481

Add P2SH-P2WSH support to listunspent RPC #14481

Uh oh!

Conversation

meshcollider commented Oct 15, 2018 • 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

DrahtBot commented Oct 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

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!

sipa commented Nov 14, 2018

Uh oh!

ryanofsky commented Dec 4, 2018

Uh oh!

meshcollider commented Dec 6, 2018

Uh oh!

meshcollider commented Feb 5, 2019

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Feb 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

maflcko commented Feb 12, 2019

Uh oh!

meshcollider commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

meshcollider commented Oct 15, 2018 •

edited

Loading

meshcollider Oct 17, 2018 •

edited

Loading

Sjors Oct 17, 2018 •

edited

Loading

DrahtBot commented Oct 20, 2018 •

edited

Loading

meshcollider commented Feb 11, 2019 •

edited

Loading

meshcollider commented Feb 13, 2019 •

edited

Loading