migrate: Handle HD chains that have identical seeds but different IDs
What the developer wrote
Authored by Ava Chow
migrate: Handle HD chains that have identical seeds but different IDs
The seed ID is calculated from a pubkey produced by treating the seed as
a private key. This calculation includes a pubkey compression parameter,
even thought that compression is completely irrelevant for the usage of
the seed as a BIP 32 seed. Thus migration should detect if a seed has
been used multiple times by checking if the computed master key was
already processed.
The spkm_migration fuzzer needs to have it's added descriptors
accounting to be updated for this fix.
What changed, and why it matters
This commit fixes a wallet migration bug in Bitcoin Core. When converting old-style wallets to modern descriptor wallets, the migration code could process the same BIP 32 seed more than once because it identified seeds by a 'seed ID' that changes depending on a technical detail (public-key compression) that does not actually affect the seed's value. The fix now detects duplicate seeds by comparing the derived master extended public key, preventing redundant or conflicting descriptors from being created during migration. There is no direct evidence this is exploitable as an attack; it appears to be a correctness and robustness fix.
Treat as a bug-fix patch with low-to-moderate security relevance. Users relying on legacy wallet migration should upgrade and re-test migration of wallets with multiple HD chains or imported seeds. No immediate emergency response is indicated by the diff alone.
Security signals we found
Migration correctness fix for duplicate HD seed handling
Potential creation of redundant/conflicting wallet descriptors during legacy-to-descriptor migration
Change in seed-duplicate detection from seed_id hash to derived master xpub comparison
Fuzz test updated to reflect new duplicate-chain semantics
Evidence from the diff
LegacyDataSPKM::MigrateToDescriptor() iterates over HD chains and previously used chain.seed_id (a hash of the seed-derived pubkey, with compression flag) to decide whether to migrate a chain. Because seed_id can differ for the same seed when different compression settings are used, the same BIP32 master could be processed twice, producing duplicate/conflicting descriptors. The patch computes the master CExtKey/CExtPubKey from the seed and tracks seen master_xpubs in a std::set, skipping chains whose master xpub was already processed. The fuzz target is updated to detect duplicate chains by comparing raw seed keys rather than seed_id.
Changed components
src/wallet/scriptpubkeyman.cppsrc/wallet/test/fuzz/scriptpubkeyman.cppLegacyDataSPKM::MigrateToDescriptor()spkm_migration fuzz targetInspect captured patch +20 / −11
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index dad8aef3..0759e2d5 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -628,19 +628,28 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
bool can_support_hd_split_feature = m_hd_chain.nVersion >= CHDChain::VERSION_HD_CHAIN_SPLIT;
+ std::set<CExtPubKey> master_xpubs;
for (const CHDChain& chain : chains) {
+ if (chain.seed_id.IsNull()) continue;
+
+ // Get the master xprv
+ CKey seed_key;
+ if (!GetKey(chain.seed_id, seed_key)) {
+ assert(false);
+ }
+ CExtKey master_key;
+ master_key.SetSeed(seed_key);
+
+ // Get the xpub and verify that we haven't already seen this xpub before
+ CExtPubKey master_xpub = master_key.Neuter();
+ const auto& [_, inserted] = master_xpubs.insert(master_xpub);
+ if (!inserted) continue;
+
for (int i = 0; i < 2; ++i) {
// Skip if doing internal chain and split chain is not supported
- if (chain.seed_id.IsNull() || (i == 1 && !can_support_hd_split_feature)) {
+ if (i == 1 && !can_support_hd_split_feature) {
continue;
}
- // Get the master xprv
- CKey seed_key;
- if (!GetKey(chain.seed_id, seed_key)) {
- assert(false);
- }
- CExtKey master_key;
- master_key.SetSeed(seed_key);
// Make the combo descriptor
std::string xpub = EncodeExtPubKey(master_key.Neuter());
diff --git a/src/wallet/test/fuzz/scriptpubkeyman.cpp b/src/wallet/test/fuzz/scriptpubkeyman.cpp
index b6b3c5d5..bb848d1c 100644
--- a/src/wallet/test/fuzz/scriptpubkeyman.cpp
+++ b/src/wallet/test/fuzz/scriptpubkeyman.cpp
@@ -257,10 +257,10 @@ FUZZ_TARGET(spkm_migration, .init = initialize_spkm_migration)
bool add_inactive_hd_chain{fuzzed_data_provider.ConsumeBool() && !keys.empty()};
if (add_inactive_hd_chain) {
- hd_key = PickValue(fuzzed_data_provider, keys);
+ CKey inactive_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();
+ bool dup_chain = hd_key.IsValid() && std::equal(hd_key.begin(), hd_key.end(), inactive_hd_key.begin());
+ hd_chain.seed_id = inactive_hd_key.GetPubKey().GetID();
legacy_data.AddInactiveHDChain(hd_chain);
if (!dup_chain) added_chains++;
}
Why this scored 41/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.