wallet: LearnRelatedScripts only if KeepDestination #17237

1. 
   promag commented at 8:47 am on October 24, 2019: member

   Only mutates the wallet if the reserved key is kept.

   First commit is a refactor that makes the address type a class member.

   The second commit moves LearnRelatedScripts from GetReservedDestination to KeepDestination to avoid an unnecessary call to AddCScript - which in turn prevents multiple entries of the same script in the wallet DB.

2. promag force-pushed on Oct 24, 2019
3. DrahtBot added the label Wallet on Oct 24, 2019
4. 
   DrahtBot commented at 12:15 pm on October 24, 2019: 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:

   • #17537 (wallet: Cleanup and move opportunistic and superfluous TopUp()s by achow101)
   • #17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
   • #17373 (wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet by achow101)
   • #17331 (Use effective values throughout coin selection by achow101)
   • #17219 (wallet: allow transaction without change if keypool is empty by Sjors)

   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.

5. 
   laanwj commented at 6:00 pm on October 24, 2019: member

   Can you elaborate a bit, what the difference is here? Does it fix an issue, or is it just a conceptual refactor?
6. 
   promag commented at 8:43 am on October 25, 2019: member

   This is not a refactor, it prevents a call to CWallet::AddCScript if CWallet::CreateTransaction fails for any reason - avoiding multiple entries of the same script in the wallet db.

   Updated OP.

   I’ll see if I can add a test that fails without this change.

7. 
   achow101 commented at 4:54 pm on October 25, 2019: member

   Code review ACK d4643bfa37f0f9174fe359e128f482a971a00140
8. DrahtBot added the label Needs rebase on Oct 29, 2019
9. promag force-pushed on Nov 1, 2019
10. DrahtBot removed the label Needs rebase on Nov 2, 2019
11. promag force-pushed on Nov 2, 2019
12. DrahtBot added the label Needs rebase on Nov 4, 2019
13. wallet: Lock address type in ReserveDestination 55295fba4c
14. wallet: LearnRelatedScripts only if KeepDestination 3958295bc8
15. promag force-pushed on Nov 4, 2019
16. DrahtBot removed the label Needs rebase on Nov 4, 2019
17. ryanofsky approved
18. 
    ryanofsky commented at 1:08 pm on November 15, 2019: member

    Code review ACK 3958295bc8a3787b66b0269190218a3764088f79. I like this change. The new behavior makes more sense, and the change makes the code clearer, since the current LearnRelatedScripts call is hard to understand and explain. (Personally, I’d like it if this PR were merged before #17373 or that PR was rebased on top of this one so it would be less confusing.)

    re: #17237 (comment)

    I’ll see if I can add a test that fails without this change.

    A test would be nice to have either here or in a followup PR. I think we really need to get out of the habit of making behavior changes in the wallet without corresponding test changes. Not having tests is dangerous and also impedes code review because test changes can make it easier to see what code changes are doing.

19. 
    achow101 commented at 5:34 pm on November 15, 2019: member

    Re-ACK 3958295bc8a3787b66b0269190218a3764088f79
20. 
    ryanofsky commented at 7:47 pm on November 19, 2019: member

    @Sjors do you want to review this? I’m not sure who else would be familiar
21. 
    Sjors commented at 1:05 pm on November 20, 2019: member

    ACK 3958295bc8a3787b66b0269190218a3764088f79

    A test could, I’ll just shamelessly plug #12134 again, create a pre-segwit wallet, open it on master, draft a transaction with a SegWit change address, cancel it, downgrade to pre-segwit bitcoin core wallet, and then check that older wallet version don’t see the key. But I don’t think that’s terribly useful.

22. 
    meshcollider commented at 8:25 pm on November 22, 2019: contributor

    utACK 3958295bc8a3787b66b0269190218a3764088f79
23. meshcollider referenced this in commit 7127c31020 on Nov 22, 2019
24. meshcollider merged this on Nov 22, 2019
25. meshcollider closed this on Nov 22, 2019

    ---

26. promag deleted the branch on Nov 22, 2019
27. sidhujag referenced this in commit 2c89dc9c98 on Nov 22, 2019
28. deadalnix referenced this in commit c6b38e8b94 on Sep 30, 2020
29. deadalnix referenced this in commit 61d4ae5b75 on Oct 1, 2020
30. sidhujag referenced this in commit fae7ed590d on Nov 10, 2020
31. DrahtBot locked this on Dec 16, 2021

Contributors
promag DrahtBot laanwj achow101 ryanofsky Sjors meshcollider

Labels
Wallet