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

Allow descriptor imports with importmulti #14491

Merged
merged 6 commits into from Feb 7, 2019

Conversation

@meshcollider
Copy link
Member

commented Oct 16, 2018

Based on #14454 #14565, last two commits only are for review.

Best reviewed with ?w=1

Allows a descriptor to be imported into the wallet using importmulti RPC. Start and end of range can be specified for ranged descriptors. The descriptor is implicitly converted to old structures on import.

Also adds a simple test of a P2SH-P2WPKH address being imported as a descriptor. More tests to come, as well as release notes.

src/wallet/rpcdump.cpp Outdated
if (::IsMine(*pwallet, raw_pubkey_script) == ISMINE_SPENDABLE) {
throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this public key");
}
if (!pwallet->AddWatchOnly(raw_pubkey_script, timestamp)) {

This comment has been minimized.

Copy link
@sipa

sipa Oct 16, 2018

Member

For the same reason as pointed out in the other PR, you can't do this; you're going to mark payments to individual multisig pubkeys as incoming payments.

You'll need a way to restrict this to P2PK, P2WPKH, and P2SH/P2WSH wrapped versions of those.

EDIT: I realize that when it's about private keys, the same effect applies too, and there it can't be avoided. Perhaps this stuff is just to scary, and we should wait until there's a way to actually specify what to treat as ours explicitly...

This comment has been minimized.

Copy link
@instagibbs

instagibbs Oct 16, 2018

Member

Restricting it from multisig(to avoid being tricked as you mention) would make this even more confusing to a user.

Unfortunate.

This comment has been minimized.

Copy link
@meshcollider

meshcollider Oct 16, 2018

Author Member

Mentioned on IRC, but what if we just never import public keys at all, and only either A) add the scriptPubKey as watch only or B) import the private key. If we have the private key then IMO it's less scary, because it's still "ours" and we can access the funds

This comment has been minimized.

Copy link
@sipa

sipa Oct 16, 2018

Member

You're right; the same concern doesn't exist for private keys as you're obviously able to spend those coins anyway.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Oct 16, 2018

Member

Would not importing the pubkeys affect the wallet's ability to construct PSBT inputs?

This comment has been minimized.

Copy link
@sipa

sipa Oct 16, 2018

Member

Only for things that have PKH/WPKH construction in them (GetPubKey is needed for those, which finds a pubkey based on pubkeyhash).

@meshcollider To be clear, we do have to import the pubkey for those; otherwise the result is not solvable.

@promag
Copy link
Member

left a comment

Just some comments while reading the code. Will test.

src/wallet/rpcdump.cpp Outdated

success = true;
}
if (data.exists("scriptPubKey")) {

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Member
if (data.exists("scriptPubKey") && data.exists("descriptor")) {
    // throw error because these should be exclusive?
}

and add test.

throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor is invalid");
}
if (!parsed_desc->IsRange() && data.exists("range")) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Range should not be specified for an un-ranged descriptor");

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Member

Missing test.

src/wallet/rpcdump.cpp Outdated
if (!parsed_desc->IsRange() && data.exists("range")) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Range should not be specified for an un-ranged descriptor");
} else if (parsed_desc->IsRange() && !data.exists("range")) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Descriptor is ranged, please specify the range");

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Member

Missing test.

Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
} else if (data.exists("descriptor")) {
ProcessImportDesc(pwallet, data, timestamp);
} else {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Either a descriptor or scriptPubKey must be provided.");

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Member

Missing test.

This comment has been minimized.

Copy link
@meshcollider

meshcollider Oct 16, 2018

Author Member

I said more tests were coming 😅

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 7, 2019

Member

Still coming?

This comment has been minimized.

Copy link
@instagibbs
Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
Show resolved Hide resolved doc/release-notes-14454.md Outdated
Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

After discussion with sipa, closing for now, will come back to this after #14454 is merged

@achow101

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Since #14454 has been merged, this can be reopened?

@sipa

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

I believe a simpler approach is possible on top of #14565.

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2018

Yep I'll reopen this when rebased on 14565

@meshcollider meshcollider reopened this Nov 3, 2018

@meshcollider meshcollider force-pushed the meshcollider:201810_importmulti_desc_2 branch Nov 3, 2018

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Nov 3, 2018

Rebased on #14565

Still planning on adding more tests + release notes, please don't nitpick the lack of tests yet

@meshcollider meshcollider force-pushed the meshcollider:201810_importmulti_desc_2 branch 7 times, most recently to 6894bf5 Nov 3, 2018

@Sjors
Copy link
Member

left a comment

Concept ACK, lightly tested 6894bf5 and it's able to import a test watch-only wallet I used previously.

I didn't use the watchonly while importing a descriptor with an xpub, should that be allowed? Or should watchonly argument not be allowed when using a descriptor?

Much smaller code change than I would have thought, nice!

It would be nice if range had a default of [0, DEFAULT_KEYPOOL_SIZE] or even using the -keypool argument.

Can you add a usage example with a descriptor?

When I cherry-pick #14477 on top of this and inspect a used address with getaddressinfo, the origin info in the descriptor seems both wrong and incomplete: "desc": "wpkh([224885f8]026...)", where 224885f8 is not the master fingerprint I used, and derivation info is missing. There might be some reusable stuff in #14021.

If I use watchonly: true, then getaddressinfo doesn't show a descriptor and it says solvable: false, which seems wrong (the latter also happens without cherry-pick).

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2018

@Sjors thanks for the feedback :)

If I use watchonly: true, then getaddressinfo doesn't show a descriptor and it says solvable: false, which seems wrong (the latter also happens without cherry-pick).

It should only watch for the scriptPubKey and import no other information, watch-only is a different requirement than being solvable without private keys.

the origin info in the descriptor seems both wrong and incomplete: "desc": "wpkh([224885f8]026...)", where 224885f8 is not the master fingerprint I used, and derivation info is missing.

You're right, good point. I'll take a look at andrews PR

I didn't use the watchonly while importing a descriptor with an xpub, should that be allowed? Or should watchonly argument not be allowed when using a descriptor?

I think it should be allowed, because of the above reason. Using an xpub without watch only allows it to be solvable without being spendable

@sipa

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

@meshcollider @Sjors I was imagining that "watchonly" would be implicit when using descriptors (addr() and raw() would be watchonly; anything else would result in a solvable result).

The reason for "watchonly" is so that users need to be explicit about the fact they don't want solvability (generally, you should always want solvability, but if you can't, you can tell importmulti that it's fine without).

@Sjors

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

@sipa I like your suggestion. In that case we should disallow usage of the watchonly param when combined with a descriptor.

Regarding getting origin information from the descriptors, @achow101 just rebased #14021 on top of this PR. It's a non trivial change. Perhaps for this PR it's best to not store origin information. Just make sure that if you do walletcreatefundedpsbt with bip32 flag set to true, then the result doesn't crash decodepsbt.

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2018

I don't see the problem if you want to add a ranged descriptor as watch only to import all the scriptPubKeys but not retain any more info than that?

@Sjors

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

IIUC you would indicate that in the descriptor by wrapping the result in addr(...). Mandating that removes ambiguity from how a descriptor ends up in a wallet.

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2018

@Sjors It was my understanding that addr(...) cannot contain another type of descriptor or key (e.g. a ranged BIP32 key), only the base58/bech32 encoded address formats. If I'm wrong then I'm happy to change this PR, @sipa could weigh in here

@instagibbs

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

xpub byte prefix mismatch results in very generic error; would be nice to give something more meaningful

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 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:

  • #15032 (Nit fixes for PR 14565 by MeshCollider)
  • #14918 (RPCHelpMan: Check default values are given at compile-time by MarcoFalke)

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.

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

I think you've now changed it to never importing public keys.

If I import "pkh(036f1aef8329e88a8e7dca56a4e8f908697555478b26709f7513eba54db4acea21)", and subsequently call getaddressinfo 1H2JaLY37d3PeqGgMV6yDStzHqenoRcCYF, it says "solvable: False". All information for solvability is available, however.

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

The following tests reproduces what @sipa found:

        # Test importing of a P2PKH address via descriptor
        key = get_key(self.nodes[0])
        self.log.info("Should import a p2pkh address from descriptor")
        self.test_importmulti({"desc": "pkh(" + key.pubkey + ")",
                               "timestamp": "now",
                               "label": "Descriptor import test"},
                              True,
                              warnings=["Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag."])
        test_address(self.nodes[1],
                     key.p2pkh_addr,
                     solvable=True,
                     ismine=False,
                     label="Descriptor import test")
@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

More IRC discussion: http://www.erisian.com.au/bitcoin-core-dev/log-2019-01-24.html#l-742

Beyond the immediate bug that's been found, I'm confused about the pubkey issues being discussed:
#14491 (comment), #14491 (comment). Just at a really high level it's not clear to me if there is:

  1. One clearly correct solution to implement in this PR.
  2. One solution to implement in this PR that is clearly better than alternatives, but with known limitations that can be addressed later.
  3. Disagreement about multiple solutions with different tradeoffs.

@sipa / @meshcollider if you could clarify whether the situation is (1), (2), or (3), it would help the discussion make more sense...

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

@ryanofsky I suspect this is subjective, but this is my view:

  • Due to the inherent quirks in the current IsMine logic, we can't guarantee that just the exact scriptPubKey implied by the provided descriptor will match. If that was possible, it would be the obvious solution - but it's a choice we make by implementing this as a layer on top of the existing IsMine logic.
  • A strong requirement in my view is that all the things that are imported are "policy compatible" with the descriptor. By that I mean that all scriptPubKeys made watched by importing a descriptor must be spendable by the same group of keys that can spend the sPK corresponding to the descriptor. If this is violated, you risk accepting a payment to something you can't spend.
  • A weaker requirement is that whenever all information for solvability is present in the descriptor, the output is considered solvable.

I believe the above is all possible with the current IsMine logic, so I think it's the most obvious solution. It requires only importing the public keys which occur inside P2PKH/P2WPKH, but not those inside multisig.

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

@meshcollider See #15263, that should make it easier.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

Tested a bit and works as expected (limited tests).
A nitpick would be why the range is a JSON object rather then an array with two items.

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

@jonasschnelli I had the array initially, but the RPCHelpMan doesn't support that kind of help output so I just changed it

@kallewoof
Copy link
Member

left a comment

utACK with some nits

std::copy(scripts_temp.begin(), scripts_temp.end(), std::inserter(script_pub_keys, script_pub_keys.end()));
}

for (auto const& x : out_keys.scripts) {

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jan 30, 2019

Member

Nit: const auto for consistency

// This does not take into account threshold multisigs which could be spendable without all keys.
// Thus, threshold multisigs without all keys will be considered not spendable here, even if they are,
// perhaps triggering a false warning message. This is consistent with the current wallet IsMine check.
bool spendable = std::all_of(out_keys.pubkeys.begin(), out_keys.pubkeys.end(), [&](const std::pair<CKeyID, CPubKey>& used_key){ return privkey_map.count(used_key.first) > 0; });

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jan 30, 2019

Member

I think this line deserves to be 3 lines

bool spendable = std::all_of(out_keys.pubkeys.begin(), out_keys.pubkeys.end(), [&](const std::pair<CKeyID, CPubKey>& used_key) {
   return privkey_map.count(used_key.first) > 0;
});

I would even argue it could do with 4

bool spendable = std::all_of(out_keys.pubkeys.begin(), out_keys.pubkeys.end(),
   [&](const std::pair<CKeyID, CPubKey>& used_key) {
      return privkey_map.count(used_key.first) > 0;
   });
UniValue result(UniValue::VOBJ);

try {
const bool internal = data.exists("internal") ? data["internal"].get_bool() : false;

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jan 30, 2019

Member

Could just do

= data.exists("internal") && data["internal"].get_bool()
bool have_solving_data;

if (data.exists("scriptPubKey") && data.exists("desc")) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Both a descriptor and a scriptPubKey should not be provided.");

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jan 30, 2019

Member

This sentence looks weird. "Descriptors and scriptPubKeys cannot be used at the same time." maybe?

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 2, 2019

I took a stab at rebasing this, but it was absolute hell :-) Maybe @achow101 has more luck...

meshcollider added a commit that referenced this pull request Feb 2, 2019

Merge #15263: Descriptor expansions only need pubkey entries for PKH/…
…WPKH

11e0fd8 Descriptor expansions only need pubkey entries for PKH/WPKH (Pieter Wuille)

Pull request description:

  Currently, calling `Expand` on a `Descriptor` object will populate the output FlatSigningProvider with all public keys involved in the descriptor. This is overkill, as pubkey entries are only needed when the lookup of a public key based on its hash is desired (which is the case for `pkh`, `wpkh`, and `combo` descriptors).

  Fix this by pushing the population of pubkey entries down into the individual descriptor implementation's `MakeScript` function, instead of doing it generically.

  This should make it easier to implement #14491 without importing P2PKH outputs for the individual public keys listed inside a multisig.

Tree-SHA512: 5bc7e9bd29f1b3bc63514803e9489b3bf126bfc177d46313aa9eeb98770ec61a97b55bd8ad4e2384154799f24b1bc4183bfdb4708b2ffa6e37ed2601a451cabc
@achow101

This comment has been minimized.

Copy link
Member

commented Feb 3, 2019

https://github.com/achow101/bitcoin/tree/rebase-14491 is a rebase of this which handles the merge conflicts caused by #15235. It does not account for #15263 (even though that is in it's history).

jnewbery and others added some commits Jan 11, 2019

[wallet] Refactor ProcessImport()
This commit is move-only and doesn't make any functional changes. It
simply moves code around within ProcessImport() in preparation for
refactors in the next commits.
[wallet] Add ProcessImportLegacy()
This commit adds a ProcessImportLegacy() function which
currently does nothing. It also unindents a block of
code for a future move-only change.

Reviewer hint: review with -w to ignore whitespace changes.
[wallet] Refactor ProcessImport() to call ProcessImportLegacy()
This is almost entirely a move-only commit.

Reviewer hint: use --color-moved=zebra for review.

@meshcollider meshcollider force-pushed the meshcollider:201810_importmulti_desc_2 branch from 02f1a89 to b985e9c Feb 5, 2019

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

I've rebased and squashed as well as added some more tests for descriptor imports and fixed the public key issue now #15263 has gone in

@DrahtBot DrahtBot removed the Needs rebase label Feb 5, 2019

@Sjors

Sjors approved these changes Feb 5, 2019

Copy link
Member

left a comment

utACK b985e9c

FlatSigningProvider out_keys;

// Expand all descriptors to get public keys and scripts.
// TODO: get private keys from descriptors too

This comment has been minimized.

Copy link
@Sjors

Sjors Feb 5, 2019

Member

Nit: add (TODO to add) expandRange method to Descriptor instance

This comment has been minimized.

Copy link
@meshcollider

meshcollider Feb 5, 2019

Author Member

@Sjors this TODO is addressed in #15024 :)

@achow101

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

utACK b985e9c

@ryanofsky
Copy link
Contributor

left a comment

utACK b985e9c. Changes since last review: rebase, more tests, changes to comments, documentation, and error checking.

// Check if this private key corresponds to a public key from the descriptor
if (!pubkey_map.count(id)) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Unused private key provided");
warnings.push_back("Ignoring irrelevant private key.");

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 5, 2019

Contributor

re: #14491 (comment)

These comments don't appear to be resolved, but I think that's ok.

// This does not take into account threshold multisigs which could be spendable without all keys.
// Thus, threshold multisigs without all keys will be considered not spendable here, even if they are,
// perhaps triggering a false warning message. This is consistent with the current wallet IsMine check.
bool spendable = std::all_of(pubkey_map.begin(), pubkey_map.end(),

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 5, 2019

Contributor

In commit "[wallet] Allow descriptor imports with importmulti" (9f48053)

It would be really nice to move this duplicated watchonly checking code out of ProcessLegacy and ProcessImportDescriptor either up into ProcessImport or down to into a common helper function. The watchonly checks are weird enough to deal with once, much less twice in two functions with slightly different variable names and comments.

// This does not take into account threshold multisigs which could be spendable without all keys
bool spendable = std::all_of(pubkey_map.begin(), pubkey_map.end(), [&](const std::pair<CKeyID, CPubKey>& used_key){ return privkey_map.count(used_key.first) > 0; });
if (!watch_only && !spendable) {
warnings.push_back("Some private keys are missing, outputs will be considered watchonly. If this is intentional, specify the watchonly flag.");

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 5, 2019

Contributor

re: #14491 (comment)

s/specify the watchonly flag/specify the watchonly flag to suppress this warning/

Note: implementing this suggestion would make the warning text vary between ProcessImportLegacy and ProcessImportDescriptor.

// Generate the script and destination for the scriptPubKey provided
CScript script;
CTxDestination dest;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 5, 2019

Contributor

In commit "[wallet] Allow descriptor imports with importmulti" (9f48053)

Note: Changes here more are directly related to previous commit "[wallet] Refactor ProcessImport()" (a1b25e1) than this one.

import_data.import_scripts.emplace(x.second);
}

std::copy(out_keys.pubkeys.begin(), out_keys.pubkeys.end(), std::inserter(pubkey_map, pubkey_map.end()));

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Feb 5, 2019

Contributor

re: #14491 (comment)

For future reference, this was fixed by removing out.pubkeys.emplace in #15263.

I think it would be helpful to have a comment here noting that this imports the public keys which occur inside P2PKH and P2WPKH descriptors, but not those inside multisig descriptors. It would also be great to include or link to sipa's rationale for this in #14491 (comment)

@laanwj laanwj merged commit b985e9c into bitcoin:master Feb 7, 2019

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 Feb 7, 2019

Merge #14491: Allow descriptor imports with importmulti
b985e9c Add release notes for importmulti descriptor support (MeshCollider)
fbb5e93 Add test for importing via descriptor (MeshCollider)
9f48053 [wallet] Allow descriptor imports with importmulti (MeshCollider)
d2b381c [wallet] Refactor ProcessImport() to call ProcessImportLegacy() (John Newbery)
4cac0dd [wallet] Add ProcessImportLegacy() (John Newbery)
a1b25e1 [wallet] Refactor ProcessImport() (John Newbery)

Pull request description:

  ~~Based on #14454 #14565, last two commits only are for review.~~

  Best reviewed with `?w=1`

  Allows a descriptor to be imported into the wallet using `importmulti` RPC. Start and end of range can be specified for ranged descriptors. The descriptor is implicitly converted to old structures on import.

  Also adds a simple test of a P2SH-P2WPKH address being imported as a descriptor. More tests to come, as well as release notes.

Tree-SHA512: 160eb6fd574c4ae5b70e0109f7e5ccc95d9309138603408a1114ceb3c558065409c0d7afb66926bc8e1743c365a3b300c5f944ff18b2451acc0514fbeca1f2b3

@meshcollider meshcollider deleted the meshcollider:201810_importmulti_desc_2 branch Feb 8, 2019

@fanquake fanquake removed this from Blockers in High-priority for review Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.