Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add P2SH-P2WSH support to listunspent RPC #14481

Merged
merged 3 commits into from Feb 14, 2019

Conversation

@meshcollider
Copy link
Member

@meshcollider meshcollider commented Oct 15, 2018

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.

@@ -2709,7 +2709,7 @@ static UniValue listunspent(const JSONRPCRequest& request)
" \"scriptPubKey\" : \"key\", (string) the script key\n"
" \"amount\" : x.xxx, (numeric) the transaction output amount in " + CURRENCY_UNIT + "\n"
" \"confirmations\" : n, (numeric) The number of confirmations\n"
" \"redeemScript\" : n (string) The redeemScript if scriptPubKey is P2SH\n"
" \"redeemScript\" : n (string) The redeemScript if scriptPubKey is P2SH, or witnessScript if the scriptPubKey is P2WSH or P2SH-P2WSH\n"
Copy link
Member

@Sjors Sjors Oct 17, 2018

Why not a separate witnessScript field like in #14454?

Copy link
Member Author

@meshcollider meshcollider Oct 17, 2018

@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

Copy link
Member

@Sjors Sjors Oct 17, 2018

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.

Copy link
Member

@instagibbs instagibbs Oct 17, 2018

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

Copy link
Contributor

@ryanofsky ryanofsky Nov 14, 2018

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 DrahtBot mentioned this pull request Oct 20, 2018
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Oct 20, 2018

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.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Code utACK c789fef, 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.

@@ -2709,7 +2709,7 @@ static UniValue listunspent(const JSONRPCRequest& request)
" \"scriptPubKey\" : \"key\", (string) the script key\n"
" \"amount\" : x.xxx, (numeric) the transaction output amount in " + CURRENCY_UNIT + "\n"
" \"confirmations\" : n, (numeric) The number of confirmations\n"
" \"redeemScript\" : n (string) The redeemScript if scriptPubKey is P2SH\n"
" \"redeemScript\" : n (string) The redeemScript if scriptPubKey is P2SH, or witnessScript if the scriptPubKey is P2WSH or P2SH-P2WSH\n"
Copy link
Contributor

@ryanofsky ryanofsky Nov 14, 2018

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

entry.pushKV("redeemScript", HexStr(redeemScript.begin(), redeemScript.end()));
// First check if the redeemScript is actually a P2WSH script
CTxDestination witness_destination;
if (redeemScript.IsPayToWitnessScriptHash() && ExtractDestination(redeemScript, witness_destination)) {
Copy link
Contributor

@ryanofsky ryanofsky Nov 14, 2018

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
Copy link
Member

@sipa sipa commented Nov 14, 2018

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
Copy link
Contributor

@ryanofsky ryanofsky commented Dec 4, 2018

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

c789fef 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
Copy link
Member Author

@meshcollider meshcollider commented Dec 6, 2018

@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 meshcollider force-pushed the 201810_listunspent_wsh branch 2 times, most recently from 6ce92c0 to bd2b1f6 Feb 5, 2019
@meshcollider
Copy link
Member Author

@meshcollider meshcollider commented Feb 5, 2019

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

@meshcollider meshcollider added this to the 0.18.0 milestone Feb 8, 2019
Copy link
Member

@Sjors Sjors left a comment

bd2b1f6 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
Copy link
Member Author

@meshcollider meshcollider commented Feb 11, 2019

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

Agree about addmultisigaddress, and about squashing 👍

@meshcollider meshcollider force-pushed the 201810_listunspent_wsh branch from bd2b1f6 to 77c0c8e Feb 11, 2019
@meshcollider
Copy link
Member Author

@meshcollider meshcollider commented Feb 11, 2019

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

the witness script in the case of a P2WSH or P2SH-P2WSH output.

The `signrawtransactionwithkey` and `signrawtransactionwithwallet` RPCs have been
modified so that they also accept a `witnessScript`, the witness script in the
Copy link
Member

@laanwj laanwj Feb 12, 2019

maybe optionally instead of also

p2sh_redeemScript = CScript([OP_0, sha256(hex_str_to_bytes(p2sh_p2wsh_address["redeemScript"]))])
assert_equal(unspent_output["redeemScript"], bytes_to_hex_str(p2sh_redeemScript))
# Now create and sign a transaction spending that output on node[0], which doesn't know the scripts or keys
spending_tx = self.nodes[0].createrawtransaction([unspent_output], {self.nodes[1].getnewaddress(): 49.998})
Copy link
Member

@laanwj laanwj Feb 12, 2019

Please pass fractional amounts as a Decimal

Copy link
Member

@Sjors Sjors left a comment

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)

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.
Copy link
Member

@Sjors Sjors Feb 12, 2019

I was also referring to this overloading.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

utACK 77c0c8e. 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.

@@ -945,7 +955,8 @@ static UniValue signrawtransactionwithkey(const JSONRPCRequest& request)
{"txid", RPCArg::Type::STR_HEX, /* opt */ false, /* default_val */ "", "The transaction id"},
{"vout", RPCArg::Type::NUM, /* opt */ false, /* default_val */ "", "The output number"},
{"scriptPubKey", RPCArg::Type::STR_HEX, /* opt */ false, /* default_val */ "", "script key"},
{"redeemScript", RPCArg::Type::STR_HEX, /* opt */ true, /* default_val */ "omitted", "(required for P2SH or P2WSH) redeem script"},
{"redeemScript", RPCArg::Type::STR_HEX, /* opt */ true, /* default_val */ "omitted", "(required for P2SH) redeem script"},
Copy link
Contributor

@ryanofsky ryanofsky Feb 12, 2019

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

Could update commit message to mention change to signrawtransactionwithkey

// Now check if the redeemScript is actually a P2WSH script
CTxDestination witness_destination;
if (redeemScript.IsPayToWitnessScriptHash()) {
assert(ExtractDestination(redeemScript, witness_destination));
Copy link
Contributor

@ryanofsky ryanofsky Feb 12, 2019

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

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Feb 12, 2019

Needs rebase (blame me)

@meshcollider meshcollider force-pushed the 201810_listunspent_wsh branch from 77c0c8e to 9492a6d Feb 13, 2019
@meshcollider
Copy link
Member Author

@meshcollider meshcollider commented Feb 13, 2019

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

@meshcollider meshcollider force-pushed the 201810_listunspent_wsh branch from 9492a6d to 6ca836a Feb 13, 2019
Copy link
Contributor

@ryanofsky ryanofsky left a comment

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Feb 14, 2019

utACK 6ca836a

@laanwj laanwj merged commit 6ca836a into bitcoin:master Feb 14, 2019
2 checks passed
laanwj added a commit that referenced this issue Feb 14, 2019
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 meshcollider deleted the 201810_listunspent_wsh branch Feb 15, 2019
@meshcollider meshcollider moved this from In progress to Done in Segwit Feb 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants