importmulti: Don’t add internal addresses to address book #14679

1. 
   instagibbs commented at 4:27 pm on November 7, 2018: member

   Currently anything imported with internal will not be treated as change since checking the address book is a primary test of this.

   Added basic tests of all combinations of arguments and change identification.

   Resolves https://github.com/bitcoin/bitcoin/issues/14662

2. 
   sipa commented at 4:37 pm on November 7, 2018: member

   This seems to be a bug that has existed since importmulti was introduced?

   I guess few people import change addresses.

3. 
   instagibbs commented at 4:40 pm on November 7, 2018: member

   Yes, it’s been here since the introduction I believe.

   With my hww setup I use PSBTs which have no notion of IsChange, so I’ve never run into it. Only realized this by reviewing your other PR.

4. importmulti: Don't add internal addresses to address book 7afddfa8ce
5. in src/wallet/rpcdump.cpp:998 in 92a84695f5 outdated

    993@@ -994,8 +994,8 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
    994             throw JSONRPCError(RPC_WALLET_ERROR, "Error adding scriptPubKey script to wallet");
    995         }
    996 
    997-        // add to address book or update label
    998-        if (IsValidDestination(scriptpubkey_dest)) {
    999+        // if not internal add to address book or update label
   1000+        if (!internal && IsValidDestination(scriptpubkey_dest)) {
   

   ---

   ---

   

   sipa commented at 7:06 pm on November 7, 2018:

   I think this could just be:

   0if (!internal) {
   1    assert(IsValidDestination(scriptpubkey_dest));
   2    ...
   3}
   

   ---

   

   instagibbs commented at 7:43 pm on November 7, 2018:

   done
6. instagibbs force-pushed on Nov 7, 2018
7. 
   DrahtBot commented at 8:27 pm on November 7, 2018: 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:

   • #14565 (Overhaul importmulti logic by sipa)

   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.

8. fanquake added the label Wallet on Nov 7, 2018
9. 
   meshcollider commented at 9:47 pm on November 8, 2018: contributor

   Good catch, utACK https://github.com/bitcoin/bitcoin/pull/14679/commits/7afddfa8cefd01249ad59cf2370e7cec90b34f6f
10. 
    sipa commented at 11:09 pm on November 8, 2018: member

    utACK 7afddfa8cefd01249ad59cf2370e7cec90b34f6f
11. 
    achow101 commented at 7:33 pm on November 13, 2018: member

    utACK 7afddfa8cefd01249ad59cf2370e7cec90b34f6f
12. laanwj merged this on Nov 13, 2018
13. laanwj closed this on Nov 13, 2018

    ---

14. laanwj referenced this in commit f617e05c38 on Nov 13, 2018
15. fanquake added the label Needs backport on Nov 29, 2018
16. 
    MarcoFalke commented at 3:55 pm on November 30, 2018: member

    fanquake added the Needs backport label a day ago @fanquake Backport to what branch?

17. MarcoFalke added this to the milestone 0.17.2 on Dec 5, 2018
18. 
    MarcoFalke commented at 10:48 pm on December 5, 2018: member

    Assigned 0.17.2, because it is not a new bug
19. fanquake referenced this in commit ae1b6756c9 on Dec 9, 2018
20. fanquake removed the label Needs backport on Dec 13, 2018
21. 
    fanquake commented at 1:25 pm on December 13, 2018: member

    Removed “Needs backport”, being done in #14900.
22. meshcollider referenced this in commit a057cc08fd on Dec 24, 2018
23. deadalnix referenced this in commit 937a91eb9e on Feb 14, 2020
24. MarcoFalke locked this on Sep 8, 2021

Contributors
instagibbs sipa DrahtBot meshcollider achow101 MarcoFalke fanquake

Labels
Wallet

Milestone
0.17.2