IsMine: Set state to WATCH_ONLY if we can get the pubkey #17374

1. 
   achow101 commented at 11:23 pm on November 4, 2019: member

   This fixes a logic bug in watch only detection.

   Split from #17261

2. IsMine: Set state to WATCH_ONLY if we can get the pubkey

   This fixes a logic bug in watch only detection.

   9972b2836f
3. fanquake added the label Wallet on Nov 4, 2019
4. ryanofsky added this to the "PRs" column in a project

   ---

5. 
   Sjors commented at 5:54 pm on November 5, 2019: member

   ACK 9972b28, trusting my previous self here, I remember understanding what this did :-) #16341 (review)
6. 
   MarcoFalke commented at 5:57 pm on November 5, 2019: member

   When was this bug introduced? Does it need backport? Would a regression test be appropriate?
7. 
   achow101 commented at 9:48 pm on November 5, 2019: member

   Looking at this much more closely, I’m not entirely sure that this is really a bug fix or whether it is needed.

   The reason this was included in the ScriptPubKeyMan PR was that because IsMine is relied upon more heavily in the ScriptPubKeyMan model, wallet_importmulti.py was failing. It failed in the specific case where a legacy address was fetched, and from its information, a wpkh() descriptor (so an output type that does not match the address) was imported into another wallet. Then getaddressinfo was called using the original legacy address but on the imported wallet and it failed to find any information. This change was introduced to make the test pass, but perhaps it would have been better to change the test to be consistent with the address types.

   Furthermore, the wallet with the import does not recognize the legacy address as being watch only. getaddressinfo reports it as not watch only and coins sent to that address are not detected by the wallet. This PR would change that, furthering the “any pubkey we have” behavior of the ismine, which I don’t think is actually desirable. But, at the same time, the current behavior is that we are able to get metadata about the legacy address (we return the pubkey and key origin info). After the rest of the ScriptPubKeyMan refactor, this would no longer be true; getaddressinfo will not return any information about the legacy address and treat it as any other third party address unrelated to the wallet even though we have the pubkey.

   And lastly, the current behavior is that we do have this weird state where the legacy address is not watch only but is solvable. So we could add key origin information to a PSBT which has inputs spending from this address but the wallet wouldn’t be tracking that address. ScriptPubKeyMan does change that behavior too (due to checking IsMine).

8. 
   instagibbs commented at 10:09 pm on November 5, 2019: member

   this might not be worth fixing. AFAIK it was only introduced with descriptor import, but the behavior was never a regression as such.
9. achow101 closed this on Nov 5, 2019

   ---

10. 
    Sjors commented at 7:33 am on November 6, 2019: member

    Paging @sipa.
11. ryanofsky moved this from the "PRs" to the "Done" column in a project

    ---

12. meshcollider referenced this in commit 4effd67bf4 on Nov 22, 2019
13. sidhujag referenced this in commit b609c9ed8a on Nov 22, 2019
14. sidhujag referenced this in commit 49ef95dc89 on Nov 10, 2020
15. DrahtBot locked this on Dec 16, 2021

Contributors
achow101 Sjors MarcoFalke instagibbs

Labels
Wallet