wallet: avoid double TopUp() calls on descriptor wallets

furszy · furszy · commit 599ff5adfc7e · 2022-08-12T12:36:04.000-03:00
Move TopUp() responsibility from the wallet class to each scriptpubkeyman.
So each spkm can decide to call it or not after perform the basic checks
for the new destination request.

Reason:

We were calling it twice in the following flows for descriptor wallets:

A) CWallet::GetNewDestination:
   1) Calls spk_man-&gt;TopUp()
   2) Calls spk_man-&gt;GetNewDestination() --&gt; which, after the basic script checks, calls TopUp() again.

B) CWallet::GetReservedDestination:
   1) Calls spk_man-&gt;TopUp()
   2) Calls spk_man-&gt;GetReservedDestination() --&gt; which calls to GetNewDestination (which calls to TopUp again).
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
@@ -28,6 +28,9 @@ util::Result<CTxDestination> LegacyScriptPubKeyMan::GetNewDestination(const Outp
     }
     assert(type != OutputType::BECH32M);
 
+    // Fill-up keypool if needed
+    TopUp();
+
     LOCK(cs_KeyStore);
 
     // Generate a new key that is added to wallet
@@ -304,6 +307,9 @@ util::Result<CTxDestination> LegacyScriptPubKeyMan::GetReservedDestination(const
         return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")};
     }
 
+    // Fill-up keypool if needed
+    TopUp();
+
     if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
         return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")};
     }
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -2346,7 +2346,6 @@ util::Result<CTxDestination> CWallet::GetNewDestination(const OutputType type, c
         return util::Error{strprintf(_("Error: No %s addresses available."), FormatOutputType(type))};
     }
 
-    spk_man->TopUp();
     auto op_dest = spk_man->GetNewDestination(type);
     if (op_dest) {
         SetAddressBook(*op_dest, label, "receive");
@@ -2440,10 +2439,7 @@ util::Result<CTxDestination> ReserveDestination::GetReservedDestination(bool int
         return util::Error{strprintf(_("Error: No %s addresses available."), FormatOutputType(type))};
     }
 
-    if (nIndex == -1)
-    {
-        m_spk_man->TopUp();
-
+    if (nIndex == -1) {
         CKeyPool keypool;
         auto op_address = m_spk_man->GetReservedDestination(type, internal, nIndex, keypool);
         if (!op_address) return op_address;

Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,9 @@ util::Result<CTxDestination> LegacyScriptPubKeyMan::GetNewDestination(const Outp`
`28`	`28`	`}`
`29`	`29`	`assert(type != OutputType::BECH32M);`
`30`	`30`
	`31`	`+ // Fill-up keypool if needed`
	`32`	`+ TopUp();`
	`33`	`+`
`31`	`34`	`LOCK(cs_KeyStore);`
`32`	`35`
`33`	`36`	`// Generate a new key that is added to wallet`
`@@ -304,6 +307,9 @@ util::Result<CTxDestination> LegacyScriptPubKeyMan::GetReservedDestination(const`
`304`	`307`	`return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")};`
`305`	`308`	`}`
`306`	`309`
	`310`	`+ // Fill-up keypool if needed`
	`311`	`+ TopUp();`
	`312`	`+`
`307`	`313`	`if (!ReserveKeyFromKeyPool(index, keypool, internal)) {`
`308`	`314`	`return util::Error{_("Error: Keypool ran out, please call keypoolrefill first")};`
`309`	`315`	`}`