From 2a6c53371be770f7e945316481b231dcbed01dd9 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 2 Jun 2026 14:16:25 -0700 Subject: [PATCH] wallet, spkm: Treat Descriptor ID as an opaque SPKM ID Instead of treating the descriptor ID as something which has a meaning which can be verified, treat the ID read from disk as some opaque blob used solely to identify and tie together specific records from disk. This removes the usage of the ID for duplication checks or comparison, and removes the check that the read ID matches a computed ID. When writing new descriptors to disk, the ID is still calculated from the old Descriptor ID method for backwards compatibility. But this fact is opaque to all further usages of the ID. --- src/wallet/export.cpp | 6 +++--- .../external_signer_scriptpubkeyman.cpp | 4 ++-- src/wallet/external_signer_scriptpubkeyman.h | 2 +- src/wallet/rpc/wallet.cpp | 3 +-- src/wallet/scriptpubkeyman.cpp | 13 +++++++------ src/wallet/scriptpubkeyman.h | 7 +++++-- src/wallet/test/walletload_tests.cpp | 19 +++++-------------- src/wallet/wallet.cpp | 15 +++++++-------- src/wallet/walletdb.cpp | 5 ----- src/wallet/walletutil.h | 3 --- 10 files changed, 31 insertions(+), 46 deletions(-) diff --git a/src/wallet/export.cpp b/src/wallet/export.cpp index fb6bdf5a307..91cbc8a9864 100644 --- a/src/wallet/export.cpp +++ b/src/wallet/export.cpp @@ -107,13 +107,13 @@ util::Result ExportWatchOnlyWallet(const CWallet& wallet, const fs: WalletDescriptor w_desc(std::move(descs.at(0)), desc_info.creation_time, range_start, range_end, desc_info.next_index); // For descriptors that cannot self expand (i.e. needs private keys or cache), set the cache - uint256 desc_id = w_desc.id; if (!w_desc.descriptor->CanSelfExpand()) { w_desc.cache = desc_info.cache; } // Add to the watchonly wallet - if (auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, dummy_keys, /*label=*/"", /*internal=*/false); !spkm_res) { + auto spkm_res = watchonly_wallet->AddWalletDescriptor(w_desc, dummy_keys, /*label=*/"", /*internal=*/false); + if (!spkm_res) { return util::Error{util::ErrorString(spkm_res)}; } @@ -125,7 +125,7 @@ util::Result ExportWatchOnlyWallet(const CWallet& wallet, const fs: if (desc_info.internal) { internal = *desc_info.internal; } - watchonly_wallet->AddActiveScriptPubKeyMan(desc_id, *Assert(w_desc.descriptor->GetOutputType()), internal); + watchonly_wallet->AddActiveScriptPubKeyMan(spkm_res->get().GetID(), *Assert(w_desc.descriptor->GetOutputType()), internal); } } diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index 1020ef6f667..96f1ee6cfcc 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -21,9 +21,9 @@ using common::PSBTError; namespace wallet { -std::unique_ptr ExternalSignerScriptPubKeyMan::LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) +std::unique_ptr ExternalSignerScriptPubKeyMan::LoadFromStorage(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) { - return std::unique_ptr(new ExternalSignerScriptPubKeyMan(storage, descriptor, keypool_size, keys, ckeys)); + return std::unique_ptr(new ExternalSignerScriptPubKeyMan(storage, id, descriptor, keypool_size, keys, ckeys)); } std::unique_ptr ExternalSignerScriptPubKeyMan::CreateNew(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, std::unique_ptr desc) diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index ea17ca1a063..51c7362d983 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -19,7 +19,7 @@ private: using DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan; public: - static std::unique_ptr LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); + static std::unique_ptr LoadFromStorage(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); static std::unique_ptr CreateNew(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, std::unique_ptr desc); static util::Result GetExternalSigner(); diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 054ab6b7631..2763f1826f6 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -812,8 +812,7 @@ static RPCMethod createwalletdescriptor() WalletBatch batch{pwallet->GetDatabase()}; for (bool internal : internals) { WalletDescriptor w_desc = GenerateWalletDescriptor(xpub, *output_type, internal); - uint256 w_id = DescriptorID(*w_desc.descriptor); - if (!pwallet->GetScriptPubKeyMan(w_id)) { + if (!pwallet->GetDescriptorScriptPubKeyMan(w_desc)) { spkms.emplace_back(pwallet->SetupDescriptorScriptPubKeyMan(batch, active_hdkey, *output_type, internal)); } } diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 481d23f7dd3..5a1fe006c9c 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -846,11 +846,12 @@ std::unique_ptr DescriptorScriptPubKeyMan::CreateFrom return spkm; } -DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) +DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) : ScriptPubKeyMan(storage), m_map_keys(keys), m_map_crypted_keys(ckeys), m_keypool_size(keypool_size), + m_id(id), m_wallet_descriptor(descriptor) { if (!keys.empty() && !ckeys.empty()) { @@ -859,9 +860,9 @@ DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletStorage& storage, Wal Load(); } -std::unique_ptr DescriptorScriptPubKeyMan::LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) +std::unique_ptr DescriptorScriptPubKeyMan::LoadFromStorage(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) { - return std::unique_ptr(new DescriptorScriptPubKeyMan(storage, descriptor, keypool_size, keys, ckeys)); + return std::unique_ptr(new DescriptorScriptPubKeyMan(storage, id, descriptor, keypool_size, keys, ckeys)); } std::unique_ptr DescriptorScriptPubKeyMan::GenerateNewSingleSig(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, OutputType addr_type, bool internal) @@ -1498,8 +1499,7 @@ std::unique_ptr DescriptorScriptPubKeyMan::GetMetadata(const CTxDe uint256 DescriptorScriptPubKeyMan::GetID() const { - LOCK(cs_desc_man); - return m_wallet_descriptor.id; + return m_id; } void DescriptorScriptPubKeyMan::Load() @@ -1538,7 +1538,8 @@ void DescriptorScriptPubKeyMan::Load() bool DescriptorScriptPubKeyMan::HasWalletDescriptor(const WalletDescriptor& desc) const { LOCK(cs_desc_man); - return !m_wallet_descriptor.id.IsNull() && !desc.id.IsNull() && m_wallet_descriptor.id == desc.id; + // Compare by using the canonical string to make the hardened indicators consistent for comparison + return m_wallet_descriptor.descriptor->ToCanonicalString() == desc.descriptor->ToCanonicalString(); } void DescriptorScriptPubKeyMan::WriteDescriptor() diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 0c3dcc6c4d5..0c94862d880 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -301,6 +301,8 @@ private: */ mutable std::map m_musig2_secnonces; + const uint256 m_id; + bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); KeyMap GetKeys() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); @@ -319,12 +321,13 @@ private: protected: //! Create a DescriptorScriptPubKeyMan from existing data (i.e. during loading) - DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); + DescriptorScriptPubKeyMan(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); //! Create a new DescriptorScriptPubKeyMan from a descriptor (e.g. from an import, newly generated) DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size) : ScriptPubKeyMan(storage), m_keypool_size(keypool_size), + m_id(DescriptorID(*descriptor.descriptor)), m_wallet_descriptor(descriptor) {} @@ -337,7 +340,7 @@ protected: bool TopUpWithDB(WalletBatch& batch, unsigned int size = 0); public: - static std::unique_ptr LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); + static std::unique_ptr LoadFromStorage(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); static std::unique_ptr CreateFromImport(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider); static std::unique_ptr CreateFromMigration(WalletStorage& storage, WalletBatch& batch, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider); static std::unique_ptr GenerateNewSingleSig(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, OutputType addr_type, bool internal); diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp index abda2bf4cb1..a9710eea90a 100644 --- a/src/wallet/test/walletload_tests.cpp +++ b/src/wallet/test/walletload_tests.cpp @@ -25,7 +25,7 @@ public: std::string ToCanonicalString() const override { return desc; } std::optional GetOutputType() const override { return OutputType::UNKNOWN; } - bool IsRange() const override { return false; } + bool IsRange() const override { return true; } bool IsSolvable() const override { return false; } bool IsSingleType() const override { return true; } bool HavePrivateKeys(const SigningProvider&) const override { return false; } @@ -66,19 +66,11 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup) } // Test 2 - // Now write a valid descriptor with an invalid ID. - // As the software produces another ID for the descriptor, the loading process must be aborted. + // Now write a valid descriptor with a different ID which must be accepted database = CreateMockableWalletDatabase(); - // Verify the error - bool found = false; - DebugLogHelper logHelper("The descriptor ID calculated by the wallet differs from the one in DB", [&](const std::string* s) { - found = true; - return false; - }); - { - // Write valid descriptor with invalid ID + // Write valid descriptor with arbitrary ID WalletBatch batch(*database); std::string desc = "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/*)#cjjspncu"; WalletDescriptor wallet_descriptor(std::make_shared(desc), 0, 0, 0, 0); @@ -86,10 +78,9 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_descriptors, TestingSetup) } { - // Now try to load the wallet and verify the error. + // Now try to load the wallet and verify the result. const std::shared_ptr wallet(new CWallet(m_node.chain.get(), "", std::move(database))); - BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(_error, _warnings), DBErrors::CORRUPT); - BOOST_CHECK(found); // The error must be logged + BOOST_CHECK_EQUAL(wallet->PopulateWalletFromDB(_error, _warnings), DBErrors::LOAD_OK); } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 310e9ea07a1..f81deca52bd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3603,9 +3603,9 @@ void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc, { std::unique_ptr spk_manager; if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { - spk_manager = ExternalSignerScriptPubKeyMan::LoadFromStorage(*this, desc, m_keypool_size, keys, ckeys); + spk_manager = ExternalSignerScriptPubKeyMan::LoadFromStorage(*this, id, desc, m_keypool_size, keys, ckeys); } else { - spk_manager = DescriptorScriptPubKeyMan::LoadFromStorage(*this, desc, m_keypool_size, keys, ckeys); + spk_manager = DescriptorScriptPubKeyMan::LoadFromStorage(*this, id, desc, m_keypool_size, keys, ckeys); } AddScriptPubKeyMan(id, std::move(spk_manager)); } @@ -3765,14 +3765,13 @@ void CWallet::DeactivateScriptPubKeyMan(uint256 id, OutputType type, bool intern DescriptorScriptPubKeyMan* CWallet::GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const { - auto spk_man_pair = m_spk_managers.find(desc.id); + auto spk_man_pair = std::find_if(m_spk_managers.begin(), m_spk_managers.end(), [&desc](const auto& item) { + DescriptorScriptPubKeyMan* spk_manager = dynamic_cast(item.second.get()); + return spk_manager != nullptr && spk_manager->HasWalletDescriptor(desc); + }); if (spk_man_pair != m_spk_managers.end()) { - // Try to downcast to DescriptorScriptPubKeyMan then check if the descriptors match - DescriptorScriptPubKeyMan* spk_manager = dynamic_cast(spk_man_pair->second.get()); - if (spk_manager != nullptr && spk_manager->HasWalletDescriptor(desc)) { - return spk_manager; - } + return dynamic_cast(spk_man_pair->second.get()); } return nullptr; diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 1a9ebaa3549..93575ef43ff 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -777,11 +777,6 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat return DBErrors::UNKNOWN_DESCRIPTOR; } - if (id != desc.id) { - strErr = "The descriptor ID calculated by the wallet differs from the one in DB"; - return DBErrors::CORRUPT; - } - DescriptorCache cache; // Get key cache for this descriptor diff --git a/src/wallet/walletutil.h b/src/wallet/walletutil.h index 6e50c283dd4..60e136a7e15 100644 --- a/src/wallet/walletutil.h +++ b/src/wallet/walletutil.h @@ -68,7 +68,6 @@ private: int32_t range_end = 0; // Item after the last; end of range, exclusive, i.e. [range_start, range_end). This will increment with each TopUp() public: std::shared_ptr descriptor; - uint256 id; // Descriptor ID (calculated once at descriptor initialization/deserialization) uint64_t creation_time = 0; DescriptorCache cache; @@ -109,7 +108,6 @@ public: throw std::ios_base::failure("Can't load a multipath descriptor from databases"); } descriptor = std::move(descs.at(0)); - id = DescriptorID(*descriptor); } SERIALIZE_METHODS(WalletDescriptor, obj) @@ -126,7 +124,6 @@ public: next_index(next_index), range_end(descriptor->IsRange() ? range_end : 1), descriptor(descriptor), - id(DescriptorID(*descriptor)), creation_time(creation_time) {} void UpdateFrom(const WalletDescriptor& other);