From 599ff5adfc7e1227c6d97d861d0715aee57611dd Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 30 Jun 2022 20:51:40 -0300 Subject: [PATCH 1/3] wallet: avoid double TopUp() calls on descriptor wallets 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->TopUp() 2) Calls spk_man->GetNewDestination() --> which, after the basic script checks, calls TopUp() again. B) CWallet::GetReservedDestination: 1) Calls spk_man->TopUp() 2) Calls spk_man->GetReservedDestination() --> which calls to GetNewDestination (which calls to TopUp again). --- src/wallet/scriptpubkeyman.cpp | 6 ++++++ src/wallet/wallet.cpp | 6 +----- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 7ff017775e..496dfd81a7 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -28,6 +28,9 @@ util::Result 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 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 index de1078e646..117a747c97 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2346,7 +2346,6 @@ util::Result 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 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; From 76b982a4a5328c1357dbc5361317f682db160876 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 4 Jun 2022 14:03:25 -0300 Subject: [PATCH 2/3] wallet: remove unused `nAccountingEntryNumber` field --- src/wallet/wallet.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index cf33ea21f2..b6bcca4015 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -398,7 +398,6 @@ public: TxItems wtxOrdered; int64_t nOrderPosNext GUARDED_BY(cs_wallet) = 0; - uint64_t nAccountingEntryNumber = 0; std::map m_address_book GUARDED_BY(cs_wallet); const CAddressBookData* FindAddressBookEntry(const CTxDestination&, bool allow_change = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From bfb9b94ebefdb95ac7656836975b3d5afc428744 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 1 Jul 2022 16:56:51 -0300 Subject: [PATCH 3/3] wallet: remove duplicate descriptor type check in GetNewDestination --- src/wallet/scriptpubkeyman.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 496dfd81a7..daf122e0e1 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1676,7 +1676,7 @@ util::Result DescriptorScriptPubKeyMan::GetNewDestination(const std::optional desc_addr_type = m_wallet_descriptor.descriptor->GetOutputType(); assert(desc_addr_type); if (type != *desc_addr_type) { - throw std::runtime_error(std::string(__func__) + ": Types are inconsistent"); + throw std::runtime_error(std::string(__func__) + ": Types are inconsistent. Stored type does not match type of newly generated address"); } TopUp(); @@ -1694,11 +1694,8 @@ util::Result DescriptorScriptPubKeyMan::GetNewDestination(const } CTxDestination dest; - std::optional out_script_type = m_wallet_descriptor.descriptor->GetOutputType(); - if (out_script_type && out_script_type == type) { - ExtractDestination(scripts_temp[0], dest); - } else { - throw std::runtime_error(std::string(__func__) + ": Types are inconsistent. Stored type does not match type of newly generated address"); + if (!ExtractDestination(scripts_temp[0], dest)) { + return util::Error{_("Error: Cannot extract destination from the generated scriptpubkey")}; // shouldn't happen } m_wallet_descriptor.next_index++; WalletBatch(m_storage.GetDatabase()).WriteDescriptor(GetID(), m_wallet_descriptor);