using bech32 address after importmulti of p2sh-p2wpkh descriptor is broken #18248

importmulti doesn't import all known permutations of the address into setWatchOnly but the -cli and -qt allow the user to request the bech32 variant. IsMine logic only goes the other direction: p2sh -> p2wpkh.

If funds are sent to the bech32 address the wallet doesn't pick up any of it.

Two ideas to fix:

1. Don't and deploy descriptor wallets and importdescriptor instead since this is just for extreme power users and everything legacy IsMine is horrible.
2. Have importmulti descriptor import the bech32 version of the address as well into setWatchOnly

OR

Maybe the p2sh -> p2wpkh direction working is the bug and that should be disabled?

Why is this a bug? If you want payments to p2wpkh, you should import a p2wpkh descriptor.

Someone can remove the "bug" label if they like, it just works the "other way" and that was surprising to me it didn't work this way.

Or open up a new issue and state the currently "working" half is a bug :)

There are lots of problems with the current way of working, where often importing a key or a script results in certain related addresses/scripts to become IsMine as well. This includes the fact that it's harder/expensive to construct a list of all scriptPubKeys/addresses that belong to a wallet, and unclarity about the exact semantics.

Descriptors were designed exactly to solve this problem, so we can have concise descriptions about exactly what is in a wallet, rather than having it implicitly defined based on a set of known private keys, public keys, scripts, and watch-only scripts. importmulti with descriptors only approximates this, as it's using the old IsMine logic, which means that inevitably it's going to treat some things as IsMine that weren't requested. I don't think there is anything we can do about that, except perhaps adding a clarification that importing a descriptor may automatically also import a few more things (though only things with the same semantics; e.g. importing a multisig address should never make payments to the individual pubkeys be treated as yours).

With descriptor wallets, it'll finally be possible to be exact, and importing a particular descriptor will only ever treat that precise descriptor's addresses as IsMine.

It's a "bug" because it used to work so this is technically a regression. Of course, since it isn't necessarily intended behavior, maybe it's not a bug. This was only noticed because HWI was inadvertently relying on this behavior in its tests.

I think that we should just call this "unsupported behavior" and wait for descriptor wallets.

It sounds like @sipa is in favor of option 1.

"Let's do it the right way and stop staring into LegacyScriptPubKeyMan::IsMine madness" sounds correct

It's a "bug" because it used to work so this is technically a regression.

Do you mean the exact same RPC calls resulted in a different set of IsMine addresses in a previous release? Or do you mean abstractly if you were doing this using different RPCs (non-descriptor importmulti?) you'd get different behavior?

Do you mean the exact same RPC calls resulted in a different set of IsMine addresses in a previous release?

This. HWI had a specific test case which happened to rely on this behavior. It was working previously, but no longer works.

Oh, in that case it's less clear what should be done; we could treat it as "anything that importmulti descriptor makes IsMine beyond the descriptor's generated addresses is implementation-defined behavior and subject to change", or we could treat it as "once the RPC is there we won't change the behavior without deprecation cycles".

I'm not completely sure that's true. The specific issue at hand seems to have existed since importmulti supported descriptors. What specifically changed later was how IsMine effected various things like exposing details about that address. I'm on phone but will point to the PR that changed it unless @achow101 beats me to it.

On Mon, Mar 2, 2020, 6:16 PM Pieter Wuille notifications@github.com wrote:

Oh, in that case it's less clear what should be done; we could treat it as "anything that importmulti descriptor makes IsMine beyond the descriptor's generated addresses is implementation-defined behavior and subject to change", or we could treat it as "once the RPC is there we won't change the behavior without deprecation cycles".

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/issues/18248?email_source=notifications&email_token=ABMAFU4RRBN3V5UZNSLDL63RFQ44XA5CNFSM4K73NHWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENRLVJA#issuecomment-593672868, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFU3VO4VERCKPOGCFCYLRFQ44XANCNFSM4K73NHWA .

We have descriptor wallets now, so let's just call this unsupported behavior in a type of wallet that is being deprecated.