From cba01aa8f9ef4014f660e6aeeb8063cad9765295 Mon Sep 17 00:00:00 2001 From: fanquake Date: Thu, 1 Jul 2021 10:08:33 +0800 Subject: [PATCH] Merge bitcoin/bitcoin#20191: wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag 181181019c5baa3e2d5b675d1843a45aa028781c refactor: remove m_internal from DescriptorSPKman (S3RK) Pull request description: Rationale: improve consistency between `CWallet` and `DescriptorScriptPubKeyMan`; simplify `ScriptPubKeyMan` interface. Descriptor in itself is neither internal or external. It's responsibility of a wallet to assign and manage descriptors for a specific purpose. Duplicating information about internalness of a descriptor could lead to inconsistencies and unexpected behaviour (for example misreporting keypool size). ACKs for top commit: instagibbs: reACK https://github.com/bitcoin/bitcoin/pull/20191/commits/181181019c5baa3e2d5b675d1843a45aa028781c achow101: reACK 181181019c5baa3e2d5b675d1843a45aa028781c Tree-SHA512: d5613b7f6795b290bfa0fd8cb0536de1714d0cf72cba402266bd06d550758ebad690b54fc0a336a1c7414b5814aa4a37c90a6ae89926474a97d30956d7e034ff --- src/wallet/scriptpubkeyman.cpp | 20 +++----------------- src/wallet/scriptpubkeyman.h | 20 ++++---------------- src/wallet/wallet.cpp | 14 +++++++++----- 3 files changed, 16 insertions(+), 38 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 52d07ff7fa..2a0a19a5da 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1779,12 +1779,10 @@ bool LegacyScriptPubKeyMan::GetHDChain(CHDChain& hdChainRet) const return !m_hd_chain.IsNull(); } -void LegacyScriptPubKeyMan::SetInternal(bool internal) {} - bool DescriptorScriptPubKeyMan::GetNewDestination(CTxDestination& dest, bilingual_str& error) { // Returns true if this descriptor supports getting new addresses. Conditions where we may be unable to fetch them (e.g. locked) are caught later - if (!CanGetAddresses(m_internal)) { + if (!CanGetAddresses()) { error = _("No addresses available"); return false; } @@ -2042,7 +2040,7 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const } } -bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key) +bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key, bool internal) { LOCK(cs_desc_man); assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); @@ -2060,7 +2058,7 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_ std::string desc_prefix = strprintf("pkh(%s/44'/%d'", xpub, Params().ExtCoinType()); std::string desc_suffix = "/*)"; - std::string internal_path = m_internal ? "/1" : "/0"; + std::string internal_path = internal ? "/1" : "/0"; std::string desc_str = desc_prefix + "/0'" + internal_path + desc_suffix; // Make the descriptor @@ -2115,13 +2113,6 @@ int64_t DescriptorScriptPubKeyMan::GetOldestKeyPoolTime() const return 0; } -size_t DescriptorScriptPubKeyMan::KeypoolCountExternalKeys() const -{ - if (m_internal) { - return 0; - } - return GetKeyPoolSize(); -} unsigned int DescriptorScriptPubKeyMan::GetKeyPoolSize() const { @@ -2332,11 +2323,6 @@ uint256 DescriptorScriptPubKeyMan::GetID() const return m_wallet_descriptor.id; } -void DescriptorScriptPubKeyMan::SetInternal(bool internal) -{ - this->m_internal = internal; -} - void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache) { LOCK(cs_desc_man); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index e3f1916272..7b248d7736 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -195,7 +195,6 @@ public: virtual int64_t GetOldestKeyPoolTime() const { return GetTime(); } - virtual size_t KeypoolCountExternalKeys() const { return 0; } virtual unsigned int GetKeyPoolSize() const { return 0; } virtual int64_t GetTimeFirstKey() const { return 0; } @@ -219,8 +218,6 @@ public: virtual uint256 GetID() const { return uint256(); } - virtual void SetInternal(bool internal) {} - /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */ template void WalletLogPrintf(std::string fmt, Params... parameters) const { @@ -345,8 +342,7 @@ public: void RewriteDB() override; int64_t GetOldestKeyPoolTime() const override; - size_t KeypoolCountExternalKeys() const override; - + size_t KeypoolCountExternalKeys() const; unsigned int GetKeyPoolSize() const override; int64_t GetTimeFirstKey() const override; @@ -366,8 +362,6 @@ public: uint256 GetID() const override; - void SetInternal(bool internal) override; - // Map from Key ID to key metadata. std::map mapKeyMetadata GUARDED_BY(cs_KeyStore); @@ -520,8 +514,6 @@ private: PubKeyMap m_map_pubkeys GUARDED_BY(cs_desc_man); int32_t m_max_cached_index = -1; - bool m_internal = false; - KeyMap m_map_keys GUARDED_BY(cs_desc_man); CryptedKeyMap m_map_crypted_keys GUARDED_BY(cs_desc_man); @@ -544,9 +536,8 @@ public: : ScriptPubKeyMan(storage), m_wallet_descriptor(descriptor) {} - DescriptorScriptPubKeyMan(WalletStorage& storage, bool internal) - : ScriptPubKeyMan(storage), - m_internal(internal) + DescriptorScriptPubKeyMan(WalletStorage& storage) + : ScriptPubKeyMan(storage) {} mutable RecursiveMutex cs_desc_man; @@ -571,12 +562,11 @@ public: bool IsHDEnabled() const override; //! Setup descriptors based on the given CExtkey - bool SetupDescriptorGeneration(const CExtKey& master_key); + bool SetupDescriptorGeneration(const CExtKey& master_key, bool internal); bool HavePrivateKeys() const override; int64_t GetOldestKeyPoolTime() const override; - size_t KeypoolCountExternalKeys() const override; unsigned int GetKeyPoolSize() const override; int64_t GetTimeFirstKey() const override; @@ -596,8 +586,6 @@ public: uint256 GetID() const override; - void SetInternal(bool internal) override; - void SetCache(const DescriptorCache& cache); bool AddKey(const CKeyID& key_id, const CKey& key); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a5db987bbb..aa70eef716 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4145,9 +4145,14 @@ size_t CWallet::KeypoolCountExternalKeys() const { AssertLockHeld(cs_wallet); + auto legacy_spk_man = GetLegacyScriptPubKeyMan(); + if (legacy_spk_man) { + return legacy_spk_man->KeypoolCountExternalKeys(); + } + unsigned int count = 0; - for (auto spk_man : GetActiveScriptPubKeyMans()) { - count += spk_man->KeypoolCountExternalKeys(); + if (m_external_spk_managers) { + count += m_external_spk_managers->GetKeyPoolSize(); } return count; @@ -5847,7 +5852,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans() for (bool internal : {false, true}) { { // OUTPUT_TYPE is only one: LEGACY - auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, internal)); + auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this)); if (IsCrypted()) { if (IsLocked()) { throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors"); @@ -5856,7 +5861,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans() throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors"); } } - spk_manager->SetupDescriptorGeneration(master_key); + spk_manager->SetupDescriptorGeneration(master_key, internal); uint256 id = spk_manager->GetID(); m_spk_managers[id] = std::move(spk_manager); AddActiveScriptPubKeyMan(id, internal); @@ -5883,7 +5888,6 @@ void CWallet::LoadActiveScriptPubKeyMan(uint256 id, bool internal) auto& spk_mans = internal ? m_internal_spk_managers : m_external_spk_managers; auto& spk_mans_other = internal ? m_external_spk_managers : m_internal_spk_managers; auto spk_man = m_spk_managers.at(id).get(); - spk_man->SetInternal(internal); spk_mans = spk_man; if (spk_mans_other == spk_man) {