wallet: Cleanup and move opportunistic and superfluous TopUp()s #17537

pull achow101 wants to merge 3 commits into bitcoin:master from achow101:cleanup-topups changing 3 files +7 −5
  1. achow101 commented at 5:55 pm on November 20, 2019: member
    • The TopUp() in CWallet::GetNewChangeDestination is unnecessary as currently m_spk_man calls TopUp further down the call stack inside LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination). This also lets us prepare for future changes with multiple ScriptPubKeyMans in the wallet.
    • An opportunistic TopUp() is moved from LegacyScriptPubKeyMan::GetNewDestination to CWallet::GetNewDestination.
    • Another opportunistic TopUp() is moved from LegacyScriptPubKeyMan::ReserveKeyFromKeyPool

    Moving opportunistic TopUps ensures that ScriptPubKeyMans will always be topped up before requesting Destinations from them as we cannot always rely on future ScriptPubKeyMan implementaions topping up internally.

    See also: #17373 (review)

  2. fanquake added the label Wallet on Nov 20, 2019
  3. in src/wallet/wallet.cpp:3112 in 5921040b61 outdated 
    3082@@ -3083,6 +3083,7 @@ bool CWallet::GetNewDestination(const OutputType type, const std::string label,
3083     bool result = false;
3084     auto spk_man = m_spk_man.get();
3085     if (spk_man) {
3086+        spk_man->TopUp();
    ryanofsky commented at 6:01 pm on November 20, 2019:

    In commit “keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination” (5921040b61bb5bdceddfbb94e58d51852f403232)

    I think you forgot to remove corresponding topup from LegacyScriptPubKeyMan::GetNewDestination

    achow101 commented at 8:02 pm on November 20, 2019:
    Oops. Fixed.
  4. ryanofsky commented at 6:34 pm on November 20, 2019: member

    To round this out I think it could also be good:

    • To move TopUp call up the stack from LegacyScriptPubKeyMan::MarkUnusedAddresses by having that method return whether it removed any keypool entries.
    • To add a note to ScriptPubKeyMan::TopUp header declaration saying that after initialization, individual ScriptPubKeyMan implementations shouldn’t need to call this internally. External wallet code is responsible for topping up so key pools get managed in a uniform way across implementations.
  5. DrahtBot commented at 7:18 pm on November 20, 2019: member

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

    Conflicts

    No conflicts as of last run.

  6. achow101 force-pushed on Nov 20, 2019
  7. achow101 commented at 8:42 pm on November 20, 2019: member 
    * To move TopUp call up the stack from `LegacyScriptPubKeyMan::MarkUnusedAddresses` by having that method return whether it removed any keypool entries.

    That would be a behavior change. Since the TopUp is currently done as each key is checked, it would be possible that the next key to be checked is added by the TopUp from the previous key.

    * To add a note to `ScriptPubKeyMan::TopUp` header declaration saying that after initialization, individual ScriptPubKeyMan implementations shouldn't need to call this internally. External wallet code is responsible for topping up so key pools get managed in a uniform way across implementations.

    I’ve added a comment to say it should be used sparingly and that the wallet handles calling TopUp for fetching new addresses.

  8. ryanofsky approved
  9. ryanofsky commented at 3:06 pm on November 21, 2019: member

    Code review ACK a805c97b83d99ec3e0f48272131cfec92aaf82f5. Looks great! Simple change that provides more clarity now and should lead to more consistency in the future.

    • To move TopUp call up the stack from LegacyScriptPubKeyMan::MarkUnusedAddresses by having that method return whether it removed any keypool entries.

    That would be a behavior change. Since the TopUp is currently done as each key is checked, it would be possible that the next key to be checked is added by the TopUp from the previous key.

    Oops, sorry for the bad suggestion. It seems like the right way to handle this without changing behavior would be to move GetAffectedKeys and MarkUnusedAddresses code out of the legacy keyman and back into the wallet, and have wallet code just call a new keyman MarkKeysAsUsed(CKeyID) method followed by a topup call. This would simplify the legacy keyman and remove more duplicate functionality and reduce chances for bugs and inconsistencies between keyman implementations.

  10. ryanofsky added this to the "PRs" column in a project
  11. meshcollider commented at 8:19 pm on November 22, 2019: contributor
    Code review ACK a805c97b83d99ec3e0f48272131cfec92aaf82f5
  12. DrahtBot added the label Needs rebase on Nov 22, 2019
  13. keypool: Remove superfluous topup from CWallet::GetNewChangeDestination 
    This does not change behavior. This TopUp() is unnecessary as currently
m_spk_man calls TopUp further down the call stack inside
LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination)

By removing this here, we also prepare for future changes where CWallet
has multiple ScriptPubKeyMans instead of m_spk_man.
    bb2c8ce23c
  14. keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination 
    An opportunistic TopUp is moved from LegacyScriptPubKeyMan::GetNewDestination
to CWallet::GetNewDestination. Another opportunistic TopUp is moved from
LegacyScriptPubKeyMan::ReserveKeyFromKeyPool (called by LegacyScriptPubKeyMan::GetReservedDestination)
to ReserveDestination::GetReservedDestination.

Moving opportunistic TopUps ensures that ScriptPubKeyMans will always
be topped up before requesting Destinations from them as we cannot
always rely on future ScriptPubKeyMan implementaions topping up internally.
As such, it is also unnecessary to keep the TopUp calls in the
LegacyScriptPubKeyMan functions so they are moved.

This does not change behavior as TopUp calls are moved up the call stack.
    ea50e34b28
  15. achow101 force-pushed on Nov 23, 2019
  16. DrahtBot removed the label Needs rebase on Nov 23, 2019
  17. ryanofsky approved
  18. ryanofsky commented at 1:11 am on November 26, 2019: member
    Code review ACK 778158576dcfe3412001e1c6d48cb9f00711bfbf. No changes since last review, just simple rebase
  19. in src/wallet/scriptpubkeyman.h:157 in 778158576d outdated 
    153@@ -154,6 +154,9 @@ class ScriptPubKeyMan
154     virtual void KeepDestination(int64_t index) {}
155     virtual void ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey) {}
156 
157+    /** Fills internal address pool. Should be used sparingly within ScriptPubKeyMan implementations.
    instagibbs commented at 4:07 pm on December 5, 2019:

    This is a vaguely unactionable comment.

    Can we give a positive statement about when it’s to be used? The last two uses appear to be when the old keypool is being blown away.

    achow101 commented at 4:54 pm on December 5, 2019:

    Expanded the comment a bit more.

    It’s kind of hard to say when it should and should not be used. I’m just trying to avoid the previous behavior of sprinkling TopUps everywhere.

    instagibbs commented at 4:55 pm on December 5, 2019:
    fair enough.
  20. instagibbs changes_requested
  21. instagibbs commented at 4:07 pm on December 5, 2019: member
    utACK contingent on clarification of comment commit
  22. keypool: Add comment about TopUp and when to use it 6e77a7b65c
  23. achow101 force-pushed on Dec 5, 2019
  24. instagibbs commented at 4:56 pm on December 5, 2019: member
    utACK https://github.com/bitcoin/bitcoin/pull/17537/commits/6e77a7b65cda1b46ce42f0c99ca91562255aeb28 only change is slight elaboration on comment
  25. fanquake added this to the "Blockers" column in a project
  26. ryanofsky approved
  27. ryanofsky commented at 9:33 pm on December 13, 2019: member
    Code review ACK 6e77a7b65cda1b46ce42f0c99ca91562255aeb28. Only the comment changed since my previous review.
  28. promag commented at 0:39 am on December 17, 2019: member

    Code review 6e77a7b65cda1b46ce42f0c99ca91562255aeb28. Verified that

    • first commit removes unnecessary calls as they are already made later
    • second commit moves the calls near to where the address pool is changed.
  29. fanquake referenced this in commit e6acd9f72c on Dec 17, 2019
  30. fanquake merged this on Dec 17, 2019
  31. fanquake closed this on Dec 17, 2019
  32. fanquake removed this from the "Blockers" column in a project
  33. sidhujag referenced this in commit eff0f71f2b on Dec 17, 2019
  34. fanquake moved this from the "PRs" to the "Done" column in a project
  35. jasonbcox referenced this in commit 34b1f1fca4 on Oct 1, 2020
  36. sidhujag referenced this in commit 3b26837621 on Nov 10, 2020
  37. DrahtBot locked this on Feb 15, 2022


achow101 ryanofsky DrahtBot meshcollider instagibbs promag

Labels
Wallet

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: 2024-09-29 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me