I believe the existing tests are sufficient here.

utACK 11e0fd8

Nice, thanks :)

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.

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.

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

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