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

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

Merged
merged 1 commit into from Feb 2, 2019

Conversation

Projects
None yet
4 participants
@sipa
Copy link
Member

commented Jan 25, 2019

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.

@sipa sipa force-pushed the sipa:201901_flatprovider_pkh branch from ff65689 to 11e0fd8 Jan 25, 2019

@sipa

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

I believe the existing tests are sufficient here.

@fanquake fanquake added the Wallet label Jan 26, 2019

@meshcollider

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

utACK 11e0fd8

Nice, thanks :)

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

Nit: the top comment of DescriptorImpl currently says: (size 1 for PK, PKH, WPKH; any size of Multisig). Add Combo?

Concept ACK, I think. Just checking if I understand the rationale correctly, in the proposed descriptor importmulti :
schermafbeelding 2019-01-30 om 13 34 38

This currently doesn't import the public keys, only the scripts. That can be solved by adding outkeys to pubkey_map, but this would also add all public keys in a multisig descriptor.

Later on in the import process pubkey_map is used to add each pub key as watch-only to the wallet (if it's not already present):

schermafbeelding 2019-01-30 om 13 47 28

This in turn is relevant for AddToWalletIfInvolvingMe which for each new transaction in a block checks if it needs to be added to a wallet. It does that when IsMine() is true for any output (which can mean ISMINE_WATCH_ONLY, ISMINE_SPENDABLE or both). That in turn depends on txout.scriptPubKey, which is handled through IsMineInner(...., IsMineSigVersion::TOP).

That's where my brain gives up, but I'm guessing that IsMineInner will return >0 if an output pays to a public key (hash). That would be bad if that public key belongs to a different member of the multisig group. Conversely, I'm assuming that if we don't take the additional step - on top of adding the script - of adding a public key, then IsMine won't recognize the transaction as belonging to the user.

@sipa

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

This currently doesn't import the public keys, only the scripts. That can be solved by adding outkeys to pubkey_map, but this would also add all public keys in a multisig descriptor.

That's what the code originally was, before I complained that that would make outputs to multisig participant keys marked as IsMIne.

That's where my brain gives up

I agree this is hard to reason about... replacing all this logic with descriptor based logic will hopefully make it more transparent and with fewer gotchas.

Conversely, I'm assuming that if we don't take the additional step - on top of adding the script - of adding a public key, then IsMine won't recognize the transaction as belonging to the user.

Right, the problem is that our only way of importing a public key does two things that ought to be independent:

  • It lets the signing logic know about public keys that occur in scripts (pkh and wpkh) by hash, allowing it to sign (this isn't necessary for things like multisig, as the actual pubkeys occur in the script there, so they don't need to be available in the wallet).
  • It marks PK/PKH outputs as IsMine.

If it was just the first, it would be harmless; teaching how to sign something never hurts if you don't accidentally also make unrelated things treated as IsMine.

What this change does is make descriptor expansions for multisig no longer fill up the pubkeys that occur in the script. This is safe because they weren't actually needed for the first effect, and has the added benefit that when combined with the importmulti code will cause these multisig individual keys to not become watched.

However, it does keep the effect that when you import say a P2WPKH descriptor, the pubkey will be expanded, and then imported, causing P2PKH outputs for the same key to also become watched. Thankfully, that is "policy compatible" - anyone able to spend a P2WPKH output would also be able to spend the P2PKH version. It's unfortunate, but not a disaster. The full solution will be descriptor based IsMine logic, where we can exactly control what is treated as ours.

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

the pubkey will be expanded, and then imported, causing P2PKH outputs for the same key to also become watched.

Is this import also what causes getnewaddress "" legacy to work even if you imported a P2WPKH descriptor? Actually that won't happen until #14075. I find that pretty annoying, because it can break compatibility with the source you imported from. It's even potentially dangerous if only that source has the private keys, and doesn't know how to sign the other types. However, it's not new behavior and it's something we can properly deal with in descriptor based wallets.

Anyway, utACK 11e0fd8

@sipa

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2019

@Sjors Similar but different effect.

For private keys we rely on the "ImplicitlyLearnRelatedScripts" function to know how to spend P2PKH/P2SH/P2WSH (in memory) for every private key in the wallet.

For public keys, the reason is that we only have one record (watch only keys) in the keystore for both watching and solving public key related scripts.

@meshcollider meshcollider merged commit 11e0fd8 into bitcoin:master Feb 2, 2019

2 checks passed

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

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

meshcollider added a commit that referenced this pull request Apr 9, 2019

Merge #15749: Fix: importmulti only imports origin info for PKH outputs
b5d3987 Take non-importing keys into account for spendability warning in descriptor import (Pieter Wuille)
6e59700 Import all origin info in importmulti; even for non-importing pubkeys (Pieter Wuille)
9a93c91 Keep full pubkeys in FlatSigningProvider::origins (Pieter Wuille)

Pull request description:

  This fixes #15743 and #15742.

  Since #15263, pubkeys are no longer imported for non-PKH (or WPKH, or any wrapped form of those) outputs, as that would incorrectly mark outputs to single-key versions of multisig policies as watched.

  As a side effect, this change also caused origin info not to be imported anymore for multisig policies.

  Fix this by plumbing through the full pubkey information for origins in FlatSigningProvider, and then importing all origin info we have in `importmulti` (knowing more never hurts, and additional origin information has no negative consequences like importing the pubkeys themselves).

ACKs for commit b5d398:
  MeshCollider:
    utACK b5d3987

Tree-SHA512: 37caa2be8d01b8baa12f70a58eaa7c583f5f0afbe012e02936dd8790dc5dc852f880b77258b34ddb68cae30c029585f2d1c4f5d00015380557a1e8b471e500f3
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.