wallet: fix change detection of imported internal descriptors #20266

1. 
   achow101 commented at 9:56 pm on October 29, 2020: member

   Import internal descriptors were having address book entries added which meant they would be detected as non-change. Fix this and add a test for it.
2. Fix change detection of imported internal descriptors bd93fc9945
3. DrahtBot added the label RPC/REST/ZMQ on Oct 29, 2020
4. DrahtBot added the label Wallet on Oct 29, 2020
5. 
   DrahtBot commented at 1:17 am on October 30, 2020: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #20243 (rpc, wallet: Expose wallet id in getwalletinfo RPC output by hebasto)
   • #20205 (wallet: Properly support a wallet id by achow101)
   • #19833 (wallet: Avoid locking cs_wallet recursively by promag)
   • #19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

6. 
   MarcoFalke commented at 10:37 am on October 30, 2020: member

   Is this for 0.21?
7. 
   achow101 commented at 10:50 pm on November 1, 2020: member

   Is this for 0.21?

   Yes, this is a bug fix.

8. MarcoFalke added this to the milestone 0.21.0 on Nov 2, 2020
9. jnewbery renamed this:
   Fix change detection of imported internal descriptors
   wallet: fix change detection of imported internal descriptors
   on Nov 2, 2020
10. 
    meshcollider commented at 3:37 am on November 4, 2020: contributor

    utACK bd93fc9945bfd4be117990c5d861f61ddd451f96

    Good catch

11. 
    fanquake commented at 7:57 am on November 6, 2020: member

    @S3RK you might be interested in reviewing?
12. in src/wallet/wallet.cpp:4570 in bd93fc9945

    4566@@ -4567,7 +4567,7 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
    4567         }
    4568 
    4569         CTxDestination dest;
    4570-        if (ExtractDestination(script_pub_keys.at(0), dest)) {
    4571+        if (!internal && ExtractDestination(script_pub_keys.at(0), dest)) {
    

    ---

    ---

    

    S3RK commented at 11:45 am on November 6, 2020:

    Is it expected that we will set an empty label even when a label is not specified in request at all?

    nit: I think it’s more clear to make all the checks in the parent if (for range, internal and for label emptiness). This also might help to avoid unnecessary log message above.

    ---

    

    achow101 commented at 4:26 pm on November 6, 2020:

    Is it expected that we will set an empty label even when a label is not specified in request at all?

    Yes.

    ---

    

    achow101 commented at 4:41 pm on November 6, 2020:

    Moved it up.

    ---

    

    achow101 commented at 4:46 pm on November 6, 2020:

    On second thought, nevermind. Reverted back to previous.

    Moving it up will make the boolean logic at the outer if a little more confusing, and I think it’s fine (perhaps even good) to be checking that scriptPubKeys were being generated for non-ranged things.

    ---

    

    S3RK commented at 2:13 am on November 7, 2020:

    On second thought, nevermind. Reverted back to previous.

    Moving it up will make the boolean logic at the outer if a little more confusing, and I think it’s fine (perhaps even good) to be checking that scriptPubKeys were being generated for non-ranged things.

    Though the check becomes redundant after #20153 I think I maybe even should drop it in my PR

13. achow101 force-pushed on Nov 6, 2020
14. achow101 force-pushed on Nov 6, 2020
15. 
    promag commented at 0:30 am on November 9, 2020: member

    Code review ACK bd93fc9945bfd4be117990c5d861f61ddd451f96.
16. fanquake requested review from meshcollider on Nov 9, 2020
17. 
    laanwj commented at 2:13 pm on November 9, 2020: member

    Code review ACK bd93fc9945bfd4be117990c5d861f61ddd451f96
18. laanwj merged this on Nov 9, 2020
19. laanwj closed this on Nov 9, 2020

    ---

20. sidhujag referenced this in commit 61b6d76883 on Nov 9, 2020
21. Fabcien referenced this in commit 5b40efcde2 on Dec 22, 2021
22. DrahtBot locked this on Feb 15, 2022

Contributors
achow101 DrahtBot MarcoFalke meshcollider fanquake S3RK promag laanwj

Review Requested
meshcollider

Labels
Wallet RPC/REST/ZMQ

Milestone
0.21.0