wallet migration, fuzz: Migrate hd seed once
What the developer wrote
Authored by Ava Chow
wallet migration, fuzz: Migrate hd seed once
If a wallet has multiple HD chains that have the same seed, we should
only migrate that seed a single time.
This fixes a fuzz crash that occurs once the return value of
AddDescriptorKeyWithDB is checked during descriptor construction.
What changed, and why it matters
This commit fixes a bug in Bitcoin Core's wallet migration logic where the same HD (hierarchical deterministic) wallet seed could be processed more than once if it appeared in both the active and inactive HD chain lists. The duplicate processing could cause a crash during migration, which was discovered through automated fuzz testing. The fix uses a set data structure to ensure each unique seed is only migrated once. There is no direct evidence in the commit that this is exploitable by an attacker to steal funds or compromise the network; it appears to be a reliability/correctness fix for a fuzz-discovered crash.
Treat as a routine bug-fix patch. Reviewers should verify that the new CHDChain comparator is consistent and that no other migration logic relies on duplicate chain entries. No urgent security response is indicated by the available materials, but the fix should be included in normal release testing.
Security signals we found
Fixes a fuzz-test crash in wallet migration code
Prevents duplicate HD seed migration
Adds std::set deduplication based on seed_id
No explicit security impact disclosed by vendor
No CVE, advisory, or researcher attribution in commit
Evidence from the diff
LegacyDataSPKM::MigrateToDescriptor() previously collected HD chains into a std::vector, which could contain duplicate CHDChain entries if m_hd_chain and an entry in m_inactive_hd_chains shared the same seed_id. The patch changes the container to std::set
Changed components
src/wallet/scriptpubkeyman.cppsrc/wallet/walletdb.hsrc/wallet/test/fuzz/scriptpubkeyman.cppInspect captured patch +11 / −4
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 8a1c0a45..a65dee60 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -619,10 +619,10 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
}
// Handle HD keys by using the CHDChains
- std::vector<CHDChain> chains;
- chains.push_back(m_hd_chain);
+ std::set<CHDChain> chains;
+ chains.insert(m_hd_chain);
for (const auto& chain_pair : m_inactive_hd_chains) {
- chains.push_back(chain_pair.second);
+ chains.insert(chain_pair.second);
}
bool can_support_hd_split_feature = m_hd_chain.nVersion >= CHDChain::VERSION_HD_CHAIN_SPLIT;
diff --git a/src/wallet/test/fuzz/scriptpubkeyman.cpp b/src/wallet/test/fuzz/scriptpubkeyman.cpp
index a627c770..341543ff 100644
--- a/src/wallet/test/fuzz/scriptpubkeyman.cpp
+++ b/src/wallet/test/fuzz/scriptpubkeyman.cpp
@@ -229,6 +229,7 @@ FUZZ_TARGET(spkm_migration, .init = initialize_spkm_migration)
if (legacy_data.LoadKey(key, pub_key) && std::find(keys.begin(), keys.end(), key) == keys.end()) keys.push_back(key);
}
+ size_t added_chains = 0;
bool add_hd_chain{fuzzed_data_provider.ConsumeBool() && !keys.empty()};
CHDChain hd_chain;
auto version{fuzzed_data_provider.ConsumeBool() ? CHDChain::VERSION_HD_CHAIN_SPLIT : CHDChain::VERSION_HD_BASE};
@@ -238,14 +239,17 @@ FUZZ_TARGET(spkm_migration, .init = initialize_spkm_migration)
hd_chain.nVersion = version;
hd_chain.seed_id = hd_key.GetPubKey().GetID();
legacy_data.LoadHDChain(hd_chain);
+ added_chains++;
}
bool add_inactive_hd_chain{fuzzed_data_provider.ConsumeBool() && !keys.empty()};
if (add_inactive_hd_chain) {
hd_key = PickValue(fuzzed_data_provider, keys);
hd_chain.nVersion = fuzzed_data_provider.ConsumeBool() ? CHDChain::VERSION_HD_CHAIN_SPLIT : CHDChain::VERSION_HD_BASE;
+ bool dup_chain = hd_chain.seed_id == hd_key.GetPubKey().GetID();
hd_chain.seed_id = hd_key.GetPubKey().GetID();
legacy_data.AddInactiveHDChain(hd_chain);
+ if (!dup_chain) added_chains++;
}
bool watch_only = false;
@@ -323,7 +327,6 @@ FUZZ_TARGET(spkm_migration, .init = initialize_spkm_migration)
auto result{legacy_data.MigrateToDescriptor()};
assert(result);
- size_t added_chains{static_cast<size_t>(add_hd_chain) + static_cast<size_t>(add_inactive_hd_chain)};
if ((add_hd_chain && version >= CHDChain::VERSION_HD_CHAIN_SPLIT) || (!add_hd_chain && add_inactive_hd_chain)) {
added_chains *= 2;
}
diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h
index 455fc745..454435cf 100644
--- a/src/wallet/walletdb.h
+++ b/src/wallet/walletdb.h
@@ -126,6 +126,10 @@ public:
{
return seed_id == chain.seed_id;
}
+ bool operator<(const CHDChain& chain) const
+ {
+ return seed_id < chain.seed_id;
+ }
};
class CKeyMetadata
Why this scored 32/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.