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

achow101 · 2019-11-20T17:55:06Z

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.

Conflicts

No conflicts as of last run.

achow101 · 2019-11-20T20:42:03Z

* 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.

ryanofsky

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.

meshcollider · 2019-11-22T20:19:37Z

Code review ACK a805c97b83d99ec3e0f48272131cfec92aaf82f5

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.

…let 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.

ryanofsky

Code review ACK 778158576dcfe3412001e1c6d48cb9f00711bfbf. No changes since last review, just simple rebase

instagibbs

utACK contingent on clarification of comment commit

instagibbs · 2019-12-05T16:07:04Z

src/wallet/scriptpubkeyman.h

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.

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.

fair enough.

instagibbs · 2019-12-05T16:56:11Z

utACK 6e77a7b only change is slight elaboration on comment

ryanofsky

Code review ACK 6e77a7b. Only the comment changed since my previous review.

promag

Code review 6e77a7b. 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.

…TopUp()s 6e77a7b keypool: Add comment about TopUp and when to use it (Andrew Chow) ea50e34 keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow) bb2c8ce keypool: Remove superfluous topup from CWallet::GetNewChangeDestination (Andrew Chow) Pull request description: * 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 (comment) ACKs for top commit: instagibbs: utACK 6e77a7b only change is slight elaboration on comment ryanofsky: Code review ACK 6e77a7b. Only the comment changed since my previous review. Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f

…fluous TopUp()s 6e77a7b keypool: Add comment about TopUp and when to use it (Andrew Chow) ea50e34 keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow) bb2c8ce keypool: Remove superfluous topup from CWallet::GetNewChangeDestination (Andrew Chow) Pull request description: * 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: bitcoin#17373 (comment) ACKs for top commit: instagibbs: utACK bitcoin@6e77a7b only change is slight elaboration on comment ryanofsky: Code review ACK 6e77a7b. Only the comment changed since my previous review. Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f

…us TopUp()s Summary: 6e77a7b65cda1b46ce42f0c99ca91562255aeb28 keypool: Add comment about TopUp and when to use it (Andrew Chow) ea50e34b287e0da0806c1116bb55ade730e8ff6c keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow) bb2c8ce23c9d7ba8d0e5538243e07218443c85b4 keypool: Remove superfluous topup from CWallet::GetNewChangeDestination (Andrew Chow) Pull request description: * 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: bitcoin/bitcoin#17373 (comment) --- Backport of Core [[bitcoin/bitcoin#17537 | PR17537]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7722

…fluous TopUp()s 6e77a7b keypool: Add comment about TopUp and when to use it (Andrew Chow) ea50e34 keypool: Move opportunistic TopUps from LegacyScriptPubKeyMan to CWallet and ReserveDestination (Andrew Chow) bb2c8ce keypool: Remove superfluous topup from CWallet::GetNewChangeDestination (Andrew Chow) Pull request description: * 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: bitcoin#17373 (comment) ACKs for top commit: instagibbs: utACK bitcoin@6e77a7b only change is slight elaboration on comment ryanofsky: Code review ACK 6e77a7b. Only the comment changed since my previous review. Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f

achow101 mentioned this pull request Nov 20, 2019

wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet #17373

Merged

fanquake added the Wallet label Nov 20, 2019

ryanofsky reviewed Nov 20, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the cleanup-topups branch from 5921040 to 55b0e9f Compare November 20, 2019 20:02

ryanofsky approved these changes Nov 21, 2019

View reviewed changes

DrahtBot added the Needs rebase label Nov 22, 2019

achow101 added 2 commits November 22, 2019 23:45

achow101 force-pushed the cleanup-topups branch from a805c97 to 7781585 Compare November 23, 2019 04:45

DrahtBot removed the Needs rebase label Nov 23, 2019

ryanofsky approved these changes Nov 26, 2019

View reviewed changes

Sjors mentioned this pull request Nov 27, 2019

Make ScriptPubKeyMan an actual interface and the wallet to have multiple #17261

Merged

instagibbs suggested changes Dec 5, 2019

View reviewed changes

keypool: Add comment about TopUp and when to use it

6e77a7b

achow101 force-pushed the cleanup-topups branch from 7781585 to 6e77a7b Compare December 5, 2019 16:51

ryanofsky approved these changes Dec 13, 2019

View reviewed changes

promag reviewed Dec 17, 2019

View reviewed changes

fanquake merged commit 6e77a7b into bitcoin:master Dec 17, 2019

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

achow101 commented Nov 20, 2019

Uh oh!

Uh oh!

ryanofsky commented Nov 20, 2019

Uh oh!

DrahtBot commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

achow101 commented Nov 20, 2019

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Nov 22, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

achow101 Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Dec 5, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Nov 20, 2019 •

edited

Loading

ryanofsky left a comment •

edited

Loading