p2wsh and p2sh-p2wsh address in decodescript #12321

pull fivepiece wants to merge 2 commits into bitcoin:master from fivepiece:decodescript-p2wsh changing 2 files +89 −3
  1. fivepiece commented at 1:16 AM on February 1, 2018: contributor

    Attempts to address #12244 . p2wsh addresses are returned only for scripts that are neither p2sh nor any witness program.

  2. fivepiece commented at 1:21 AM on February 1, 2018: contributor

    Again I'm only considering the edge cases after clicking "create pull request".. What if the script contains a checksig with an uncompressed pubkey? Obviously not ready.

  3. fanquake added the label RPC/REST/ZMQ on Feb 1, 2018
  4. meshcollider commented at 1:30 AM on February 1, 2018: contributor

    @fivepiece feel free to add [WIP] to the title of your PR if its work in progress :) It's usually fine to open up a PR before its completely ready because other contributors might like to give feedback on the approach or high level concept ACK/NACKs regardless. Thanks for helping out!

  5. fivepiece renamed this:
    p2wsh address in decodescript
    [WIP] p2wsh address in decodescript
    on Feb 1, 2018
  6. fivepiece commented at 1:34 AM on February 1, 2018: contributor

    Cheers, title changed (and now changed back :) )

  7. fivepiece commented at 8:24 PM on February 1, 2018: contributor

    Ideas welcome, returning a p2wsh for a p2pk or multisig with an uncompressed pubkey can result in fund loss, then again decodescript already returns p2sh addresses even for complete garbage. What should this change to decodescript take into account when deciding whether to post a p2wsh address to the user?

  8. instagibbs commented at 10:14 PM on February 1, 2018: member

    concept ACK

    I think returning garbage is fine. It can be something unprovably unspendable even, and I don't think it's the job of this tool to check.

    Wonder if it's worth it to have it also return a p2sh-p2wsh version, in other words interpreting the script as a witnessscript, if applicable?

  9. fivepiece commented at 11:10 PM on February 1, 2018: contributor

    Agreed. Keeping the simple check that this is not a bare witness program seems to be in line with the check above it not letting p2sh be re-encoded into p2sh. This commit adds p2sh-p2wsh as well.

  10. fivepiece renamed this:
    [WIP] p2wsh address in decodescript
    p2wsh address in decodescript
    on Feb 3, 2018
  11. fivepiece renamed this:
    p2wsh address in decodescript
    p2wsh and p2sh-p2wsh address in decodescript
    on Feb 8, 2018
  12. promag commented at 12:08 PM on February 8, 2018: member

    Missing tests for new keys.

    Doesn't make sense to move this to ScriptPubKeyToUniv?

  13. fivepiece commented at 2:38 PM on February 8, 2018: contributor

    @promag wouldn't moving this to ScriptPubKeyToUniv cause script translation to all these types of addresses to fire on decoderawtransaction as well? Sorry if I misunderstood your comment.

    I will be looking into adding tests as well.

  14. fivepiece commented at 7:31 PM on February 9, 2018: contributor

    I've made some changes to this:

    1. Segwit stuff is handled in a generic way using GetScriptForWitness(). This allows to return a more appropriate p2wpkh for p2pkh and p2pk scriptpubkeys, and (currently) p2wsh for all the rest.
    2. The data returned for the segwit scripts is its own univalue object (@promag suggestion), so it includes more useful info like the segwit scriptpubkey and type, along with the bech32 address.

    Various examples of the output can be seen here : https://gist.github.com/fivepiece/e25b0fcdf8303fdbfd56363ae50dec15

    Caveats :

    1. For the segwit scriptpubkeys, we always have "reqSigs": 1.
    2. It's still possible to get a segwit program address for scripts using checksigs with uncompressed pubkeys. This is because GetScriptForWitness() doesn't check vSolutions for presense of such pubkeys. It's probably be possible to change GetScriptForWitness to check for that when bare pubkeys are invloved in the script, but out of scope for this PR.
  15. fivepiece force-pushed on Feb 16, 2018
  16. fivepiece commented at 2:39 PM on February 16, 2018: contributor

    This is pretty much a complete rewrite taking into account @sipa's suggestion (on irc) to use the same logic from validateaddress in here.

    1. Uncompressed pubkeys are handled correctly - meaning standard scripts containing them will not be encoded in segwit scriptpubkeys.
    2. Added a few tests on top of the existing rpc_decodescript.py. I couldn't find a way to easily test that segwit output is not returned for standard scripts with uncompressed pubkeys. Suggestions on how to do that are welcome.
    3. Updated https://gist.github.com/fivepiece/e25b0fcdf8303fdbfd56363ae50dec15 with examples showing segwit output is disabled for uncompressed pubkey scripts.

    ~(please restart the travis job if possible, it hasn't failed, just cancelled before it began)~ Done, thanks!

  17. instagibbs commented at 2:44 PM on February 16, 2018: member

    kicked travis.

    I couldn't find a way to easily test that segwit output is not returned for standard scripts with uncompressed pubkeys.

    hm? Your gist has examples that you could use.

  18. fivepiece commented at 2:48 PM on February 16, 2018: contributor

    Specifically, I don't know how to assert that a value (say rpc_result['segwit']) does not exist in the returned univalue.

  19. instagibbs commented at 2:51 PM on February 16, 2018: member

    in python 'segwit' not in rpc_result ?

  20. fivepiece commented at 2:54 PM on February 16, 2018: contributor

    Ah, I was trying to bend assert_array_result to do the check but your way works.

  21. fivepiece commented at 9:12 AM on February 17, 2018: contributor

    Added tests for checking 'segwit' value is not returned when inappropriate.

  22. in test/functional/rpc_decodescript.py:79 in d0164ba9ce outdated 
      74 |          # just imagine that the pub keys used below are different.
  75 |          # for our purposes here it does not matter that they are the same even though it is unrealistic.
  76 |          rpc_result = self.nodes[0].decodescript('52' + push_public_key + push_public_key + push_public_key + '53ae')
  77 |          assert_equal('2 ' + public_key + ' ' + public_key + ' ' + public_key +  ' 3 OP_CHECKMULTISIG', rpc_result['asm'])
  78 | +        # multisig in P2WSH
  79 | +        assert_equal('0 0f320decca000221b611117448134c78dd220cd22384ac80025b59e43c8698e8', rpc_result['segwit']['asm'])
    instagibbs commented at 2:05 PM on March 6, 2018:

    could you generate this dynamically instead of hardcoding the long hex?

  23. in test/functional/rpc_decodescript.py:113 in d0164ba9ce outdated 
     108 | @@ -103,6 +109,8 @@ def decodescript_script_pub_key(self):
 109 |          # lock until block 500,000
 110 |          rpc_result = self.nodes[0].decodescript('63' + push_public_key + 'ad670320a107b17568' + push_public_key + 'ac')
 111 |          assert_equal('OP_IF ' + public_key + ' OP_CHECKSIGVERIFY OP_ELSE 500000 OP_CHECKLOCKTIMEVERIFY OP_DROP OP_ENDIF ' + public_key + ' OP_CHECKSIG', rpc_result['asm'])
 112 | +        # CLTV script in P2WSH
 113 | +        assert_equal('0 01f79ceb012cbcefb251f7843b827a3e285f40ddc5014d35e2486716cbacf5f1', rpc_result['segwit']['asm'])
    instagibbs commented at 2:05 PM on March 6, 2018:

    could you generate this dynamically instead of hardcoding the long hex?

  24. instagibbs commented at 2:06 PM on March 6, 2018: member

    utACK https://github.com/bitcoin/bitcoin/pull/12321/commits/785bd02cda896c92190a8a9bba3d1b40fd2812b9

    I think you can squash almost all these commits.

  25. fivepiece commented at 7:39 PM on March 6, 2018: contributor

    Cool, I'll squash it all into a single commit if this current change looks good, or two if it's better to separate work done on core from the tests. Let me know which is better.

  26. instagibbs commented at 7:41 PM on March 6, 2018: member

    whatever's easier to you, 1 or 2

  27. fivepiece force-pushed on Mar 6, 2018
  28. fivepiece commented at 7:55 PM on March 6, 2018: contributor

    Two commits now. No problem changing later on if needed.

  29. in src/rpc/rawtransaction.cpp:578 in f474907243 outdated 
     573 | +            // If the script contains an uncompressed pubkey, skip encoding of a segwit program.
 574 | +            bool fFoundUncompressedPubkeys = false;
 575 | +            if ((which_type == TX_PUBKEY) || (which_type == TX_MULTISIG)) {
 576 | +                for (const auto& solution : solutions_data) {
 577 | +                    if ((solution.size() != 1) && !CPubKey(solution).IsCompressed()) {
 578 | +                        fFoundUncompressedPubkeys = true;
    instagibbs commented at 8:00 PM on March 6, 2018:

    just return r here. then remove the conditional later?

    fivepiece commented at 8:20 PM on March 6, 2018:

    Ah I see what you mean, looking into this.

  30. instagibbs approved
  31. instagibbs commented at 8:02 PM on March 6, 2018: member

    utACK

  32. p2wpkh, p2wsh and p2sh-nested scripts in decodescript
    plus tests
    4f933b3d23
  33. fivepiece force-pushed on Mar 6, 2018
  34. fivepiece commented at 9:11 PM on March 6, 2018: contributor

    Good call. All in a single commit now.

  36. list the types of scripts we should consider for a witness program 41ff9675a9
  37. in src/rpc/rawtransaction.cpp:568 in 4f933b3d23 outdated 
     562 | @@ -563,6 +563,38 @@ UniValue decodescript(const JSONRPCRequest& request)
 563 |          // P2SH cannot be wrapped in a P2SH. If this script is already a P2SH,
 564 |          // don't return the address for a P2SH of the P2SH.
 565 |          r.pushKV("p2sh", EncodeDestination(CScriptID(script)));
 566 | +        // P2SH and witness programs cannot be wrapped in P2WSH, if this script
 567 | +        // is a witness program, don't return addresses for a segwit programs.
 568 | +        if (type.get_str().find("witness") == std::string::npos) {
    laanwj commented at 7:19 PM on April 7, 2018:

    Searching for a string in the type name seems brittle - if we have to work with strings here (there seems to be no other accessible way) let's explicitly list the options. e.g. according to current selection that would be

    "nonstandard"
"pubkey"
"pubkeyhash"
"scripthash"
"multisig"
"nulldata"
  38. fivepiece commented at 10:03 PM on April 7, 2018: contributor

    Updated to address @laanwj's comment

  40. laanwj merged this on Apr 26, 2018
  41. laanwj closed this on Apr 26, 2018
  42. laanwj referenced this in commit eac067ad59 on Apr 26, 2018
  43. PastaPastaPasta referenced this in commit 00d00ee3a7 on Jun 27, 2021
  44. MarcoFalke locked this on Sep 8, 2021
Contributors
fivepiecemeshcolliderinstagibbspromaglaanwj
Labels
RPC/REST/ZMQ
Linked (view graph)
#12244 decodescript only returns non-segwit p2sh for redeemScript/witnessScript
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me