wallet: LearnRelatedScripts only if KeepDestination #17237

pull promag wants to merge 2 commits into bitcoin:master from promag:2019-10-wallet-reservedestination changing 2 files +15 −14
  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: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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. promag cross-referenced this on Nov 7, 2019 from issue Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101
  18. ryanofsky approved
  19. ryanofsky commented at 1:08 PM on November 15, 2019: contributor

    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.

  20. achow101 commented at 5:34 PM on November 15, 2019: member

    Re-ACK 3958295bc8a3787b66b0269190218a3764088f79

  21. ryanofsky commented at 7:47 PM on November 19, 2019: contributor

    @Sjors do you want to review this? I'm not sure who else would be familiar

  22. ryanofsky cross-referenced this on Nov 19, 2019 from issue wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet by achow101
  23. 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.

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

    utACK 3958295bc8a3787b66b0269190218a3764088f79

  25. meshcollider referenced this in commit 7127c31020 on Nov 22, 2019
  26. meshcollider merged this on Nov 22, 2019
  27. meshcollider closed this on Nov 22, 2019
  28. promag deleted the branch on Nov 22, 2019
  29. sidhujag referenced this in commit 2c89dc9c98 on Nov 22, 2019
  30. MarkLTZ cross-referenced this on Apr 4, 2020 from issue Bitcoin PR tracking by MarkLTZ
  31. deadalnix referenced this in commit c6b38e8b94 on Sep 30, 2020
  32. deadalnix referenced this in commit 61d4ae5b75 on Oct 1, 2020
  33. sidhujag referenced this in commit fae7ed590d on Nov 10, 2020
  34. bitcoin locked this on Dec 16, 2021
Contributors
promagDrahtBotlaanwjachow101ryanofskySjorsmeshcollider
Labels
Wallet
Linked (view graph)
#12134 Build previous releases and run functional tests #17261 Make ScriptPubKeyMan an actual interface and the wallet to have multiple#17373 wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:54 UTC