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

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: https://github.com/bitcoin/bitcoin/pull/17373#discussion_r348598174

ACKs for top commit:
  instagibbs:
    utACK https://github.com/bitcoin/bitcoin/pull/17537/commits/6e77a7b65cda1b46ce42f0c99ca91562255aeb28 only change is slight elaboration on comment
  ryanofsky:
    Code review ACK 6e77a7b65cda1b46ce42f0c99ca91562255aeb28. Only the comment changed since my previous review.

Tree-SHA512: bdfc8d303842c3fb7c3d40af7abfa6d9dac4ef71a24922bb92229674ee89bfe3113ebb46d3903ac48ef99f0a7d6eaac33282495844f2b31f91b8df55084c421f

1 files changed, 3 insertions, 2 deletions

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 41a816312a..3954f66267 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3105,6 +3105,7 @@ bool CWallet::GetNewDestination(const OutputType type, const std::string label,
     bool result = false;
     auto spk_man = m_spk_man.get();
     if (spk_man) {
+        spk_man->TopUp();
         result = spk_man->GetNewDestination(type, dest, error);
     }
     if (result) {
@@ -3118,8 +3119,6 @@ bool CWallet::GetNewChangeDestination(const OutputType type, CTxDestination& des
 {
     error.clear();
 
-    m_spk_man->TopUp();
-
     ReserveDestination reservedest(this, type);
     if (!reservedest.GetReservedDestination(dest, true)) {
         error = "Error: Keypool ran out, please call keypoolrefill first";
@@ -3297,6 +3296,8 @@ bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool inter
 
     if (nIndex == -1)
     {
+        m_spk_man->TopUp();
+
         CKeyPool keypool;
         if (!m_spk_man->GetReservedDestination(type, internal, address, nIndex, keypool)) {
             return false;