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.
2708@@ -2709,7 +2709,7 @@ static UniValue listunspent(const JSONRPCRequest& request)
2709 " \"scriptPubKey\" : \"key\", (string) the script key\n"
2710 " \"amount\" : x.xxx, (numeric) the transaction output amount in " + CURRENCY_UNIT + "\n"
2711 " \"confirmations\" : n, (numeric) The number of confirmations\n"
2712- " \"redeemScript\" : n (string) The redeemScript if scriptPubKey is P2SH\n"
2713+ " \"redeemScript\" : n (string) The redeemScript if scriptPubKey is P2SH, or witnessScript if the scriptPubKey is P2WSH or P2SH-P2WSH\n"
signrawtransactionwithkey and signrawtransactionwithwallet? The terminology is already quite confusing, so consistency can really help.
re: #14481 (review)
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).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
2818@@ -2819,7 +2819,29 @@ static UniValue listunspent(const JSONRPCRequest& request)
2819 const CScriptID& hash = boost::get<CScriptID>(address);
2820 CScript redeemScript;
2821 if (pwallet->GetCScript(hash, redeemScript)) {
2822- entry.pushKV("redeemScript", HexStr(redeemScript.begin(), redeemScript.end()));
2823+ // First check if the redeemScript is actually a P2WSH script
2824+ CTxDestination witness_destination;
2825+ if (redeemScript.IsPayToWitnessScriptHash() && ExtractDestination(redeemScript, witness_destination)) {
listunspent #14481 (review). It seems like it should be possible to be compatible with signraw without overloading.
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.
@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?
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:
0assert_equal(unspent_output["witnessScript"], p2sh_p2wsh_address["witnessScript"])
1assert_equal(unspent_output["redeemScript"], p2sh_p2wsh_address["redeemScript"])
2assert_equal(unspent_output["redeemScript"], CScript([OP_0, sha256(hex_str_to_bytes(unspent_output["witnessScript"])))
Forgot to update the release notes, listunspent doesn’t overload redeemScript anymore
Agree about addmultisigaddress, and about squashing :+1:
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
0@@ -0,0 +1,9 @@
1+Low-level RPC changes
2+----------------------
3+
4+The `listunspent` RPC has been modified so that it also returns `witnessScript`,
5+the witness script in the case of a P2WSH or P2SH-P2WSH output.
6+
7+The `signrawtransactionwithkey` and `signrawtransactionwithwallet` RPCs have been
8+modified so that they also accept a `witnessScript`, the witness script in the
optionally instead of also
160+ unspent_output = self.nodes[1].listunspent(0, 999999, [p2sh_p2wsh_address["address"]])[0]
161+ assert_equal(unspent_output["witnessScript"], p2sh_p2wsh_address["redeemScript"])
162+ p2sh_redeemScript = CScript([OP_0, sha256(hex_str_to_bytes(p2sh_p2wsh_address["redeemScript"]))])
163+ assert_equal(unspent_output["redeemScript"], bytes_to_hex_str(p2sh_redeemScript))
164+ # Now create and sign a transaction spending that output on node[0], which doesn't know the scripts or keys
165+ spending_tx = self.nodes[0].createrawtransaction([unspent_output], {self.nodes[1].getnewaddress(): 49.998})
Decimal
862+ if (!rs.isNull()) {
863+ std::vector<unsigned char> rsData(ParseHexV(rs, "redeemScript"));
864 CScript redeemScript(rsData.begin(), rsData.end());
865 keystore->AddCScript(redeemScript);
866 // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH).
867+ // This is only for compatibility, it is encouraged to use the explicit witnessScript field instead.
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)
954@@ -945,7 +955,8 @@ static UniValue signrawtransactionwithkey(const JSONRPCRequest& request)
955 {"txid", RPCArg::Type::STR_HEX, /* opt */ false, /* default_val */ "", "The transaction id"},
956 {"vout", RPCArg::Type::NUM, /* opt */ false, /* default_val */ "", "The output number"},
957 {"scriptPubKey", RPCArg::Type::STR_HEX, /* opt */ false, /* default_val */ "", "script key"},
958- {"redeemScript", RPCArg::Type::STR_HEX, /* opt */ true, /* default_val */ "omitted", "(required for P2SH or P2WSH) redeem script"},
959+ {"redeemScript", RPCArg::Type::STR_HEX, /* opt */ true, /* default_val */ "omitted", "(required for P2SH) redeem script"},
In commit “Make listunspent return witness script for P2WSH or P2SH-P2WSH” (0b665ab577ab41e3b7b12d9b7d83c151cf90fdaf)
Could update commit message to mention change to signrawtransactionwithkey
2876@@ -2876,6 +2877,27 @@ static UniValue listunspent(const JSONRPCRequest& request)
2877 CScript redeemScript;
2878 if (pwallet->GetCScript(hash, redeemScript)) {
2879 entry.pushKV("redeemScript", HexStr(redeemScript.begin(), redeemScript.end()));
2880+ // Now check if the redeemScript is actually a P2WSH script
2881+ CTxDestination witness_destination;
2882+ if (redeemScript.IsPayToWitnessScriptHash()) {
2883+ assert(ExtractDestination(redeemScript, witness_destination));
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:
0bool extracted = ExtractDestitionation...;
1assert(extracted);
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
redeemScriptandwitnessScriptaccurately 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.
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
Labels
Wallet
RPC/REST/ZMQ
Milestone
0.18.0