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 SegWit support to importmulti #14454

Merged
merged 5 commits into from Oct 31, 2018

Conversation

@MeshCollider
Copy link
Member

MeshCollider commented Oct 10, 2018

Add support for segwit to importmulti, supports P2WSH, P2WPKH, P2SH-P2WPKH, P2SH-P2WSH. Adds a new witnessscript parameter which must be used for the witness scripts in the relevant situations.

Also includes some tests for the various import types.

Also makes the change in #14019 redundant, but cherry-picks the test from that PR to test the behavior (@achow101).

Fixes #12253, also addresses the second point in #12703, and fixes #14407

@MeshCollider MeshCollider force-pushed the MeshCollider:201810_importmulti_segwit_support branch from bd4ab9a to 59f8e54 Oct 10, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 10, 2018

Reviewers, this pull request conflicts with the following ones:
  • #14303 (rpc: Early call once CWallet::MarkDirty in import calls by promag)
  • #14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)

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.


CTxDestination pubkey_dest = pubKey.GetID();
CTxDestination pubkey_dest = pubKey.GetID();

This comment has been minimized.

@practicalswift

practicalswift Oct 13, 2018

Member

No longer needed?

if (pubKeys.size() && keys.size() == 0) {
const std::string& strPubKey = pubKeys[0].get_str();
// Generate the scripts
std::vector<unsigned char> vData(ParseHex(strWitnessScript));

This comment has been minimized.

@practicalswift

practicalswift Oct 13, 2018

Member

Use another variable name to avoid shadowing existing local variable vData :-)

@MeshCollider MeshCollider force-pushed the MeshCollider:201810_importmulti_segwit_support branch from 5b84a05 to 938eccd Oct 14, 2018

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Oct 14, 2018

Thanks for reviewing @practicalswift, both comments addressed :)

@achow101
Copy link
Member

achow101 left a comment

Concept ACK

59f8e54 could be squashed into 24092b3

acdba74 seems to be an unrelated change

if (isP2SH && !IsHex(strRedeemScript)) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid redeem script");
// Force users to provide the witness script in its field rather than redeemscript
if (!strRedeemScript.empty() && script.IsPayToWitnessScriptHash()) {

This comment has been minimized.

@achow101

achow101 Oct 14, 2018

Member

Instead of checking that strRedeemScript is not empty, this should check that strWitnessScript is empty. Or it could check both: !strRedeemScript.empty() && strWitnessScript.empty().

This comment has been minimized.

@MeshCollider

MeshCollider Oct 14, 2018

Member

It should be possible to provide a P2WSH scriptPubKey without providing the witness script for it, e.g. watch only, so I don't think that's right

This comment has been minimized.

@achow101

achow101 Oct 15, 2018

Member

I think checking both covers that case.

This comment has been minimized.

@MeshCollider

MeshCollider Oct 15, 2018

Member

Checking both would mean that if witnessScript and redeemScript were both present, it would allow it, which makes no sense if its not a P2WSH address

This comment has been minimized.

@Sjors

Sjors Oct 17, 2018

Member

P2WSH scriptPubKey without providing the witness script for it, e.g. watch only

Do we already support that? And test it? If not, maybe it's easier to just treat it as P2SH until we know the witnessScript.

This comment has been minimized.

@sipa

sipa Oct 17, 2018

Member

@Sjors Yes, of course, just importaddress it. It probably works fine without this PR even.

This comment has been minimized.

@Sjors

Sjors Oct 18, 2018

Member

@sipa so we currently support importing a 3... address as watch-only, but the wallet doesn't know or care if it's SegWit or not. The new code here is for a scenario where you do know it's SegWit script.IsPayToWitnessScriptHash(), which means you know the strRedeemScript, but you don't know the strWitnessScript. That seems a strange situation. Why would anyone give you the legacy redeem script but not the Segwit witness script?

This comment has been minimized.

@sipa

sipa Oct 18, 2018

Member

@Sjors I have no idea what you're talking about. This is about the case where you're importing a bech32 P2WSH address (no P2SH), without revealing the script inside - which is a totally reasonable thing if you don't care about solvability.

This comment has been minimized.

@Sjors

Sjors Oct 22, 2018

Member

@sipa I see, I may have gotten confused because Github is showing if (isP2SH && !IsHex(strRedeemScript as the deleted line, but this thread is about an earlier code branch.
schermafbeelding 2018-10-22 om 14 16 57

// Import redeem script.
std::vector<unsigned char> vData(ParseHex(strRedeemScript));
CScript redeemScript = CScript(vData.begin(), vData.end());
CScriptID redeem_id(redeemScript);
CScript redeemDestination = GetScriptForDestination(redeem_id);

This comment has been minimized.

@achow101

achow101 Oct 14, 2018

Member

nit: snake_case


// Optional fields.
const std::string& strRedeemScript = data.exists("redeemscript") ? data["redeemscript"].get_str() : "";
const std::string& strWitnessScript = data.exists("witnessscript") ? data["witnessscript"].get_str() : "";

This comment has been minimized.

@achow101

achow101 Oct 14, 2018

Member

nit: snake_case

const std::string& strPubKey = pubKeys[0].get_str();
// Generate the scripts
std::vector<unsigned char> vData(ParseHex(strWitnessScript));
CScript witnessScript = CScript(vData.begin(), vData.end());

This comment has been minimized.

@achow101

achow101 Oct 14, 2018

Member

nit: snake_case

std::vector<unsigned char> vData(ParseHex(strWitnessScript));
CScript witnessScript = CScript(vData.begin(), vData.end());
CScriptID witness_id(witnessScript);
CScript witnessDestination = GetScriptForDestination(WitnessV0ScriptHash(witnessScript));

This comment has been minimized.

@achow101

achow101 Oct 14, 2018

Member

nit: snake_case

// P2PK/P2PKH/P2WPKH
} else if (dest.type() == typeid(CKeyID) || dest.type() == typeid(WitnessV0KeyHash)) {
if (keys.size() > 1 || pubKeys.size() > 1) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "More than private key given for one address");

This comment has been minimized.

@achow101

achow101 Oct 14, 2018

Member

This error wouldn't make sense if only pubkeys were given.

}
pubkey = pubkey_temp;
}
if (pubkey.size()) {

This comment has been minimized.

@achow101

achow101 Oct 14, 2018

Member

Instead of checking pubkey size, check validity? (e.g. use IsValid() or IsFullyValid())

This comment has been minimized.

@MeshCollider

MeshCollider Oct 14, 2018

Member

IsValid() just performs the same size check anyway, but yep I'll do that for the sake of clarity, I don't think we need an IsFullyValid() check here

@MeshCollider MeshCollider force-pushed the MeshCollider:201810_importmulti_segwit_support branch 4 times, most recently from 17619ef to aecb400 Oct 14, 2018

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Oct 14, 2018

Thanks @achow101, addressed all your points

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 15, 2018

@MeshCollider I don't think it's necessary to import the pubkeys involved separately anymore since #14424, and in fact that sounds very dangerous (you could be tricked into importing a 2-of-3 multisig where you have 2 of the keys, but then receiving a payment to a P2PKH of the third key, and seeing it treated towards your watch-only balance). NACK until that is fixed (or at least restricted to not have that issue, but I believe that rebasing on 14424 will be sufficient).

@MeshCollider MeshCollider force-pushed the MeshCollider:201810_importmulti_segwit_support branch 5 times, most recently from 6baa217 to dc4768b Oct 15, 2018

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Oct 16, 2018

@sipa good point, fixed, and rebased on master to include the public key fix

@MeshCollider MeshCollider force-pushed the MeshCollider:201810_importmulti_segwit_support branch from dc4768b to d227d10 Oct 16, 2018

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 17, 2018

I think there are a few things that need fixing:

  • There doesn't seem to be code for P2SH-P2WPKH (which matters at least when passing in a pubkey in that case, which won't be imported).
  • The added tests only cover whether importmulti returns true, not whether it actually imported anything.
  • The added tests are restricted to watchonly and spendable cases, but no solvable-but-not-spendable ones.

@MeshCollider MeshCollider force-pushed the MeshCollider:201810_importmulti_segwit_support branch from 1d64756 to 65ecf2d Oct 17, 2018

@DrahtBot DrahtBot referenced this pull request Oct 20, 2018

Closed

Rpc help helper class #14502

@MeshCollider MeshCollider force-pushed the MeshCollider:201810_importmulti_segwit_support branch from 3c52566 to 7c0268a Oct 20, 2018

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Oct 20, 2018

Comments addressed + squashed some commits together

if (IsValidDestination(dest)) {
pwallet->SetAddressBook(dest, label, "receive");
// Import into the wallet
if (!pwallet->AddWatchOnly(witness_script, timestamp)) {

This comment has been minimized.

@sipa

sipa Oct 23, 2018

Member

A small issue still: for P2SH-P2WSH it's unnecessary (and undesirable, I think) to also import the inner P2WSH script as watch-only.

It's not necessary because we're already marking the P2SH-P2WSH script as watch-only, and for solvability all we need is AddCScript, not AddWatchOnly.

It's undesirable because it will cause us to treat payments to the P2WSH address as watchonly IsMine (instead of just the P2SH-P2WSH version).

This comment has been minimized.

@MeshCollider

MeshCollider Oct 23, 2018

Member

Good point, fixed

@Sjors
Copy link
Member

Sjors left a comment

I added a few more questions inline, hopefully merely to reduce my own confusion.

My above concerns are addressed in e4a66bb, modulo two tests that @sipa requested and are either missing or I overlooked them.

The added tests only cover whether importmulti returns true, not whether it actually imported anything.

This is done now by checking properties via getaddressinfo.

The added tests are restricted to watchonly and spendable cases, but no solvable-but-not-spendable ones.

Some of the ['solvable'], True) tests now check that ['ismine'], False, but none check for iswatchonly'],False].

Address based import (without key/script) of P2WSH-multisig

Covered in # P2WSH multisig address without scripts or keys and # P2SH-P2WSH 1-of-1 multisig + redeemscript with no private key

For all of P2WPKH, P2WSH-multisig, P2SH-P2WPKH, P2SH-P2WSH-multisig, versions with script/pubkey but no private key, and test that the result is solvable.

I think this one is still missing for P2WSH multisig, between the # P2WSH multisig address without scripts or keys and # Same P2WSH multisig address as above, but now with witnessscript + private keys tests?

P2WPKH with private key, result must be spendable (not just watchonly)

Where "spendable" means ("ismine": true and "iswatchonly": false). This is done.

P2WSH-multisig and P2SH-multisig with more than 1 key, where some of the keys are specified as private keys, and some as public keys.

Missing?

I made a commit that checks solvable for all legacy scenarios as well: Sjors@02d553b

assert_equal(result[0]['success'], True)
address_assert = self.nodes[1].getaddressinfo(address['address'])
assert_equal(address_assert['iswatchonly'], True)
assert_equal(address_assert['solvable'], False)

This comment has been minimized.

@Sjors

Sjors Oct 23, 2018

Member

@sipa can you remind me why a Native Pay-to-Witness-Public-Key-Hash isn't solvable, even though legacy Pay-to-Public-Key-Hash addresses are? Is that just to reduce evil magic like in the P2SH-P2PKH case?
If so, maybe add a comment in the test or in the RPC doc?

This comment has been minimized.

@sipa

sipa Oct 23, 2018

Member

In this example it's not solvable because the public key isn't included. It has nothing to do with the script/witness hashing type.

In general, solvability is very easy. Start at the scriptPubKey. Whenever it includes a script hash, we must receive the actual script, and recurse into it. Whenever you encounter a pubkey hash, we must receive the actual public key. A scriptPubKey is solvable whenever you can descend all the way to scripts and pubkeys without any hashes of unknown things.

The only exception to that is that whenever you have a private key X, we also automatically import the P2WPKH and P2SH-P2WPKH versions of it, so no separate importing is needed in that case.

This comment has been minimized.

@Sjors

Sjors Oct 23, 2018

Member

In this example it's not solvable because the public key isn't included.

OK, that makes sense. Actually I misread the legacy test: it does add the public key, which why it's solvable.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 23, 2018

utACK e4a66bb. As @Sjors points out there are some combinations for which tests can be added, but I think that can be done later.

Please squash fixups?

@MeshCollider MeshCollider force-pushed the MeshCollider:201810_importmulti_segwit_support branch 2 times, most recently from 2f4e5cd to 86a9e99 Oct 24, 2018

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Oct 24, 2018

Squashed fixups into their respective commits, thanks for the review :)

@MeshCollider MeshCollider force-pushed the MeshCollider:201810_importmulti_segwit_support branch from 86a9e99 to c11875c Oct 24, 2018

@sipa sipa referenced this pull request Oct 25, 2018

Merged

Overhaul importmulti logic #14565

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 25, 2018

utACK c11875c

@instagibbs
Copy link
Member

instagibbs left a comment

utACK

}])
assert_equal(result[0]['success'], True)
address_assert = self.nodes[1].getaddressinfo(multi_sig_script['address'])
assert_equal(address_assert['solvable'], False)

This comment has been minimized.

@instagibbs

instagibbs Oct 25, 2018

Member

nit: if "sigsrequired" and "ismine" aren't in results can we assert their absence here?

}])
assert_equal(result[0]['success'], True)
address_assert = self.nodes[1].getaddressinfo(multi_sig_script['address'])
assert_equal(address_assert['solvable'], True)

This comment has been minimized.

@instagibbs

instagibbs Oct 25, 2018

Member

nit: ismine?

@@ -3538,6 +3538,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
" \"address\" : \"address\", (string) The bitcoin address validated\n"
" \"scriptPubKey\" : \"hex\", (string) The hex-encoded scriptPubKey generated by the address\n"
" \"ismine\" : true|false, (boolean) If the address is yours or not\n"
" \"solvable\" : true|false, (boolean) If the address is solvable by the wallet\n"

This comment has been minimized.

@promag

promag Oct 26, 2018

Member

Could have release note.

pwallet->UpdateTimeFirstKey(timestamp);
}
// (P2SH-)P2PK/P2PKH/P2WPKH
if (dest.type() == typeid(CKeyID) || dest.type() == typeid(WitnessV0KeyHash)) {

This comment has been minimized.

@promag

promag Oct 26, 2018

Member

First time using RTTI? Maybe it could be avoided?

BTW, didn't test but I don't see variant::type() in boost 1.47 documentation.

This comment has been minimized.

@MeshCollider

MeshCollider Oct 29, 2018

Member

@promag it seems to be there: https://www.boost.org/doc/libs/1_47_0/doc/html/boost/variant.html
I could also do dest.which() == 1 || dest.which() == 4 but that's very unclear, or define a new enum of the types, but that seemed over the top unless there's a good reason to avoid RTTI?

This comment has been minimized.

@promag

promag Oct 29, 2018

Member

Indeed it's in 1.47. Anyway this is removed in #14565.

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Oct 30, 2018

utACK 201451b

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 31, 2018

looks good to me, adding the solvable to getaddressinfo makes a lot of sense, and tests also look good to me
utACK c11875c

@laanwj laanwj merged commit c11875c into bitcoin:master Oct 31, 2018

2 checks passed

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

laanwj added a commit that referenced this pull request Oct 31, 2018

Merge #14454: Add SegWit support to importmulti
c11875c Add segwit address tests for importmulti (MeshCollider)
201451b Make getaddressinfo return solvability (MeshCollider)
1753d21 Add release notes for importmulti segwit change (MeshCollider)
353c064 Fix typo in test_framework/blocktools (MeshCollider)
f6ed748 Add SegWit support to importmulti with some ProcessImport cleanup (MeshCollider)

Pull request description:

  Add support for segwit to importmulti, supports P2WSH, P2WPKH, P2SH-P2WPKH, P2SH-P2WSH. Adds a new `witnessscript` parameter which must be used for the witness scripts in the relevant situations.

  Also includes some tests for the various import types.

  ~Also makes the change in #14019 redundant, but cherry-picks the test from that PR to test the behavior (@achow101).~

  Fixes #12253, also addresses the second point in #12703, and fixes #14407

Tree-SHA512: 775a755c524d1c387a99acddd772f677d2073876b72403dcfb92c59f9b405ae13ceedcf4dbd2ee1d7a8db91c494f67ca137161032ee3a2071282eeb411be090a

@MeshCollider MeshCollider deleted the MeshCollider:201810_importmulti_segwit_support branch Oct 31, 2018

@laanwj laanwj removed this from Blockers in High-priority for review Nov 1, 2018

@joemphilips joemphilips referenced this pull request Dec 30, 2018

Closed

Update PSBT test #627

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment