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

p2wsh and p2sh-p2wsh address in decodescript #12321

Merged
merged 2 commits into from Apr 26, 2018

Conversation

Projects
None yet
6 participants
@fivepiece
Contributor

fivepiece commented Feb 1, 2018

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

@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Feb 1, 2018

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.

@MeshCollider

This comment has been minimized.

Member

MeshCollider commented Feb 1, 2018

@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!

@fivepiece fivepiece changed the title from p2wsh address in decodescript to [WIP] p2wsh address in decodescript Feb 1, 2018

@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Feb 1, 2018

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

@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Feb 1, 2018

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?

@instagibbs

This comment has been minimized.

Member

instagibbs commented Feb 1, 2018

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?

@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Feb 1, 2018

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.

@fivepiece fivepiece changed the title from [WIP] p2wsh address in decodescript to p2wsh address in decodescript Feb 3, 2018

@fivepiece fivepiece changed the title from p2wsh address in decodescript to p2wsh and p2sh-p2wsh address in decodescript Feb 8, 2018

@promag

Missing tests for new keys.

Doesn't make sense to move this to ScriptPubKeyToUniv?

@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Feb 8, 2018

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

@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Feb 9, 2018

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.
@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Feb 16, 2018

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!

@instagibbs

This comment has been minimized.

Member

instagibbs commented Feb 16, 2018

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.

@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Feb 16, 2018

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

@instagibbs

This comment has been minimized.

Member

instagibbs commented Feb 16, 2018

in python 'segwit' not in rpc_result ?

@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Feb 16, 2018

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

@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Feb 17, 2018

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

# 3) multisig scriptPubKey
# <m> <A pubkey> <B pubkey> <C pubkey> <n> OP_CHECKMULTISIG
# just imagine that the pub keys used below are different.
# for our purposes here it does not matter that they are the same even though it is unrealistic.
rpc_result = self.nodes[0].decodescript('52' + push_public_key + push_public_key + push_public_key + '53ae')
assert_equal('2 ' + public_key + ' ' + public_key + ' ' + public_key + ' 3 OP_CHECKMULTISIG', rpc_result['asm'])
# multisig in P2WSH
assert_equal('0 0f320decca000221b611117448134c78dd220cd22384ac80025b59e43c8698e8', rpc_result['segwit']['asm'])

This comment has been minimized.

@instagibbs

instagibbs Mar 6, 2018

Member

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

@@ -103,6 +109,8 @@ def decodescript_script_pub_key(self):
# lock until block 500,000
rpc_result = self.nodes[0].decodescript('63' + push_public_key + 'ad670320a107b17568' + push_public_key + 'ac')
assert_equal('OP_IF ' + public_key + ' OP_CHECKSIGVERIFY OP_ELSE 500000 OP_CHECKLOCKTIMEVERIFY OP_DROP OP_ENDIF ' + public_key + ' OP_CHECKSIG', rpc_result['asm'])
# CLTV script in P2WSH
assert_equal('0 01f79ceb012cbcefb251f7843b827a3e285f40ddc5014d35e2486716cbacf5f1', rpc_result['segwit']['asm'])

This comment has been minimized.

@instagibbs

instagibbs Mar 6, 2018

Member

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

@instagibbs

This comment has been minimized.

Member

instagibbs commented Mar 6, 2018

utACK 785bd02

I think you can squash almost all these commits.

@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Mar 6, 2018

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.

@instagibbs

This comment has been minimized.

Member

instagibbs commented Mar 6, 2018

whatever's easier to you, 1 or 2

@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Mar 6, 2018

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

@instagibbs

utACK

if ((which_type == TX_PUBKEY) || (which_type == TX_MULTISIG)) {
for (const auto& solution : solutions_data) {
if ((solution.size() != 1) && !CPubKey(solution).IsCompressed()) {
fFoundUncompressedPubkeys = true;

This comment has been minimized.

@instagibbs

instagibbs Mar 6, 2018

Member

just return r here. then remove the conditional later?

This comment has been minimized.

@fivepiece

fivepiece Mar 6, 2018

Contributor

Ah I see what you mean, looking into this.

@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Mar 6, 2018

Good call. All in a single commit now.

@instagibbs

This comment has been minimized.

Member

instagibbs commented Mar 6, 2018

re-utACK 4f933b3

@@ -563,6 +563,38 @@ UniValue decodescript(const JSONRPCRequest& request)
// P2SH cannot be wrapped in a P2SH. If this script is already a P2SH,
// don't return the address for a P2SH of the P2SH.
r.pushKV("p2sh", EncodeDestination(CScriptID(script)));
// P2SH and witness programs cannot be wrapped in P2WSH, if this script
// is a witness program, don't return addresses for a segwit programs.
if (type.get_str().find("witness") == std::string::npos) {

This comment has been minimized.

@laanwj

laanwj Apr 7, 2018

Member

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"
@fivepiece

This comment has been minimized.

Contributor

fivepiece commented Apr 7, 2018

Updated to address @laanwj's comment

@laanwj

This comment has been minimized.

Member

laanwj commented Apr 8, 2018

Thank you
utACK 41ff967

@laanwj laanwj merged commit 41ff967 into bitcoin:master Apr 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 26, 2018

Merge #12321: p2wsh and p2sh-p2wsh address in decodescript
41ff967 list the types of scripts we should consider for a witness program (fivepiece)
4f933b3 p2wpkh, p2wsh and p2sh-nested scripts in decodescript (fivepiece)

Pull request description:

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

Tree-SHA512: eb47f094c1a4c2ad2bcf27a8032307e43cf787d50bf739281aeb4101d97316a2f307b05118bf138298c937fa34e15f91436443a9b313f809fad2c43e94cd1831
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment