Descriptor expansions only need pubkey entries for PKH/WPKH

sipa commented at 6:37 pm on January 25, 2019: member

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.

Descriptor expansions only need pubkey entries for PKH/WPKH 11e0fd8d66

sipa force-pushed on Jan 25, 2019

sipa commented at 6:40 pm on January 25, 2019: member

I believe the existing tests are sufficient here.

fanquake added the label Wallet on Jan 26, 2019

meshcollider commented at 7:35 pm on January 26, 2019: contributor

utACK https://github.com/bitcoin/bitcoin/pull/15263/commits/11e0fd8d66677a92b6d12a39d0a26f1b39d9a46e

Nice, thanks :)

Sjors commented at 1:05 pm on January 30, 2019: member

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 :

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):

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 commented at 3:08 am on January 31, 2019: member

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 commented at 9:50 am on January 31, 2019: member

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 commented at 5:41 pm on January 31, 2019: member

@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 merged this on Feb 2, 2019

meshcollider closed this on Feb 2, 2019

meshcollider referenced this in commit 6e6b859f85 on Feb 2, 2019

meshcollider referenced this in commit 54798c3a31 on Apr 9, 2019

deadalnix referenced this in commit 94317263b4 on May 21, 2020

kittywhiskers referenced this in commit c1ae9093e2 on Oct 28, 2021

pravblockc referenced this in commit 993d38accd on Nov 18, 2021

MarcoFalke locked this on Dec 16, 2021

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