← Watch feed
wallet: include keys when constructing DescriptorSPKM during import
Public commit record
65/100 · Adequate
What the developer wrote
Authored by Ava Chow
wallet: include keys when constructing DescriptorSPKM during import
When importing a descriptor, all of the descriptor data should be
provided at the same time in the constructor.
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides an explanatory body
What changed, and why it matters
This Bitcoin Core wallet patch changes how private keys are added when a user imports or migrates to a descriptor-based wallet. Previously, keys were added to the descriptor manager in a separate step after creation. The patch bundles the keys into the manager's constructor or update call so all descriptor data is provided at once. The commit message frames this as a code-quality fix, not a security fix. There is no direct evidence in the diff or commit message that this closes an exploitable vulnerability, but it does reduce a window where the wallet state could be inconsistent.
Recommended action
Treat as a defensive hardening/refactoring change. Reviewers should verify that CreateFromImport, CreateFromMigration, and UpdateWithSigningProvider correctly preserve previous error handling and database transaction behavior, and that no new exceptions escape in ways that could leave the wallet in an inconsistent state. No urgent security deployment is indicated by the supplied materials.
Security signals we found
01
Refactor moves private-key injection into constructor/update path under the same lock
02
Reduces temporal window between descriptor creation and key/top-up operations
03
No explicit vulnerability, CVE, or security bug is stated in the commit or title
04
Change is localized to wallet descriptor import/migration code paths
Evidence from the diff
The refactor introduces two factory methods, DescriptorScriptPubKeyMan::CreateFromImport and ::CreateFromMigration, and a helper UpdateWithSigningProvider, so that private keys from a FlatSigningProvider are written to the wallet database and the keypool is topped up while the descriptor manager’s lock is held during construction/update. Previously, callers constructed an empty DescriptorScriptPubKeyMan and then separately called AddDescriptorKey / TopUp. The change ensures atomicity between descriptor, keys, and generated scriptPubKeys during import and wallet migration. No explicit bug or exploit is described in the commit materials.
Changed components
src/wallet/scriptpubkeyman.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/wallet.cppBitcoin Core wallet descriptor import and migrationInspect captured patch +63 / −36
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 5ec51a73..98bf5542 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -596,16 +596,15 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
// Construct the combo descriptor
std::string desc_str = "combo(" + origin_str + HexStr(key.GetPubKey()) + ")";
- FlatSigningProvider keys;
+ FlatSigningProvider provider;
std::string error;
- std::vector<std::unique_ptr<Descriptor>> descs = Parse(desc_str, keys, error, false);
+ std::vector<std::unique_ptr<Descriptor>> descs = Parse(desc_str, provider, error, false);
CHECK_NONFATAL(descs.size() == 1); // It shouldn't be possible to have an invalid or multipath descriptor
WalletDescriptor w_desc(std::move(descs.at(0)), creation_time, 0, 0, 0);
// Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
- auto desc_spk_man = std::make_unique<DescriptorScriptPubKeyMan>(m_storage, w_desc, /*keypool_size=*/0);
- WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey()));
- desc_spk_man->TopUpWithDB(batch);
+ provider.keys.emplace(key.GetPubKey().GetID(), key);
+ auto desc_spk_man = DescriptorScriptPubKeyMan::CreateFromMigration(m_storage, batch, w_desc, /*keypool_size=*/0, provider);
auto desc_spks = desc_spk_man->GetScriptPubKeys();
// Remove the scriptPubKeys from our current set
@@ -644,17 +643,16 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
// Make the combo descriptor
std::string xpub = EncodeExtPubKey(master_key.Neuter());
std::string desc_str = "combo(" + xpub + "/0h/" + ToString(i) + "h/*h)";
- FlatSigningProvider keys;
+ FlatSigningProvider provider;
std::string error;
- std::vector<std::unique_ptr<Descriptor>> descs = Parse(desc_str, keys, error, false);
+ std::vector<std::unique_ptr<Descriptor>> descs = Parse(desc_str, provider, error, false);
CHECK_NONFATAL(descs.size() == 1); // It shouldn't be possible to have an invalid or multipath descriptor
uint32_t chain_counter = std::max((i == 1 ? chain.nInternalChainCounter : chain.nExternalChainCounter), (uint32_t)0);
WalletDescriptor w_desc(std::move(descs.at(0)), 0, 0, chain_counter, 0);
// Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
- auto desc_spk_man = std::make_unique<DescriptorScriptPubKeyMan>(m_storage, w_desc, /*keypool_size=*/0);
- WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, master_key.key, master_key.key.GetPubKey()));
- desc_spk_man->TopUpWithDB(batch);
+ provider.keys.emplace(master_key.key.GetPubKey().GetID(), master_key.key);
+ auto desc_spk_man = DescriptorScriptPubKeyMan::CreateFromMigration(m_storage, batch, w_desc, /*keypool_size=*/0, provider);
auto desc_spks = desc_spk_man->GetScriptPubKeys();
// Remove the scriptPubKeys from our current set
@@ -727,16 +725,15 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
desc->Expand(0, provider, desc_spks, provider);
} else {
// Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
- WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
- auto desc_spk_man = std::make_unique<DescriptorScriptPubKeyMan>(m_storage, w_desc, /*keypool_size=*/0);
for (const auto& keyid : privkeyids) {
CKey key;
if (!GetKey(keyid, key)) {
continue;
}
- WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey()));
+ keys.keys.emplace(key.GetPubKey().GetID(), key);
}
- desc_spk_man->TopUpWithDB(batch);
+ WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
+ auto desc_spk_man = DescriptorScriptPubKeyMan::CreateFromMigration(m_storage, batch, w_desc, /*keypool_size=*/0, keys);
auto desc_spks_set = desc_spk_man->GetScriptPubKeys();
desc_spks.insert(desc_spks.end(), desc_spks_set.begin(), desc_spks_set.end());
@@ -821,6 +818,23 @@ bool LegacyDataSPKM::DeleteRecordsWithDB(WalletBatch& batch)
return batch.EraseRecords(DBKeys::LEGACY_TYPES);
}
+std::unique_ptr<DescriptorScriptPubKeyMan> DescriptorScriptPubKeyMan::CreateFromImport(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider)
+{
+ auto spkm = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(storage, descriptor, keypool_size));
+ LOCK(spkm->cs_desc_man);
+ WalletBatch batch(storage.GetDatabase());
+ spkm->UpdateWithSigningProvider(batch, provider);
+ return spkm;
+}
+
+std::unique_ptr<DescriptorScriptPubKeyMan> DescriptorScriptPubKeyMan::CreateFromMigration(WalletStorage& storage, WalletBatch& batch, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider)
+{
+ auto spkm = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(storage, descriptor, keypool_size));
+ LOCK(spkm->cs_desc_man);
+ spkm->UpdateWithSigningProvider(batch, provider);
+ return spkm;
+}
+
DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys)
: ScriptPubKeyMan(storage),
m_map_keys(keys),
@@ -1565,7 +1579,7 @@ void DescriptorScriptPubKeyMan::UpgradeDescriptorCache()
}
}
-util::Result<void> DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descriptor)
+util::Result<void> DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descriptor, const FlatSigningProvider& provider)
{
LOCK(cs_desc_man);
std::string error;
@@ -1578,10 +1592,29 @@ util::Result<void> DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescr
m_max_cached_index = -1;
m_wallet_descriptor = descriptor;
+ WalletBatch batch(m_storage.GetDatabase());
+ UpdateWithSigningProvider(batch, provider);
NotifyFirstKeyTimeChanged(this, m_wallet_descriptor.creation_time);
return {};
}
+void DescriptorScriptPubKeyMan::UpdateWithSigningProvider(WalletBatch& batch, const FlatSigningProvider& signing_provider)
+{
+ AssertLockHeld(cs_desc_man);
+ // Add the private keys to the descriptor
+ for (const auto& entry : signing_provider.keys) {
+ const CKey& key = entry.second;
+ if (!AddDescriptorKeyWithDB(batch, key, key.GetPubKey())) {
+ throw std::runtime_error(std::string(__func__) + ": writing descriptor private key failed");
+ }
+ }
+
+ // Top up key pool, to generate scriptPubKeys
+ if (!TopUpWithDB(batch)) {
+ throw std::runtime_error("Could not top up scriptPubKeys");
+ }
+}
+
bool DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error)
{
LOCK(cs_desc_man);
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index 2762ebc2..238888fc 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -303,6 +303,13 @@ private:
*/
mutable std::map<uint256, MuSig2SecNonce> m_musig2_secnonces;
+ //! Create a new DescriptorScriptPubKeyMan from an existing descriptor (i.e. from an import)
+ DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size)
+ : ScriptPubKeyMan(storage),
+ m_keypool_size(keypool_size),
+ m_wallet_descriptor(descriptor)
+ {}
+
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);
@@ -316,6 +323,9 @@ private:
void Load();
+ void AddDescriptorKey(const CKey& key, const CPubKey &pubkey);
+ void UpdateWithSigningProvider(WalletBatch& batch, const FlatSigningProvider& signing_provider) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
+
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);
@@ -326,18 +336,14 @@ protected:
bool TopUpWithDB(WalletBatch& batch, unsigned int size = 0);
public:
- //! Create a new DescriptorScriptPubKeyMan from an existing descriptor (i.e. from an import)
- DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size)
- : ScriptPubKeyMan(storage),
- m_keypool_size(keypool_size),
- m_wallet_descriptor(descriptor)
- {}
DescriptorScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size)
: ScriptPubKeyMan(storage),
m_keypool_size(keypool_size)
{}
static std::unique_ptr<DescriptorScriptPubKeyMan> LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys);
+ static std::unique_ptr<DescriptorScriptPubKeyMan> CreateFromImport(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider);
+ static std::unique_ptr<DescriptorScriptPubKeyMan> CreateFromMigration(WalletStorage& storage, WalletBatch& batch, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider);
mutable RecursiveMutex cs_desc_man;
@@ -391,9 +397,8 @@ public:
uint256 GetID() const override;
bool HasWalletDescriptor(const WalletDescriptor& desc) const;
- util::Result<void> UpdateWalletDescriptor(WalletDescriptor& descriptor);
+ util::Result<void> UpdateWalletDescriptor(WalletDescriptor& descriptor, const FlatSigningProvider& provider);
bool CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error);
- void AddDescriptorKey(const CKey& key, const CPubKey &pubkey);
void WriteDescriptor();
WalletDescriptor GetWalletDescriptor() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 7b94b659..7f40fa70 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3753,11 +3753,11 @@ util::Result<std::reference_wrapper<DescriptorScriptPubKeyMan>> CWallet::AddWall
auto spk_man = GetDescriptorScriptPubKeyMan(desc);
if (spk_man) {
WalletLogPrintf("Update existing descriptor: %s\n", desc.descriptor->ToString());
- if (auto spkm_res = spk_man->UpdateWalletDescriptor(desc); !spkm_res) {
+ if (auto spkm_res = spk_man->UpdateWalletDescriptor(desc, signing_provider); !spkm_res) {
return util::Error{util::ErrorString(spkm_res)};
}
} else {
- auto new_spk_man = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size));
+ auto new_spk_man = DescriptorScriptPubKeyMan::CreateFromImport(*this, desc, m_keypool_size, signing_provider);
spk_man = new_spk_man.get();
// Save the descriptor to memory
@@ -3765,17 +3765,6 @@ util::Result<std::reference_wrapper<DescriptorScriptPubKeyMan>> CWallet::AddWall
AddScriptPubKeyMan(id, std::move(new_spk_man));
}
- // Add the private keys to the descriptor
- for (const auto& entry : signing_provider.keys) {
- const CKey& key = entry.second;
- spk_man->AddDescriptorKey(key, key.GetPubKey());
- }
-
- // Top up key pool, the manager will generate new scriptPubKeys internally
- if (!spk_man->TopUp()) {
- return util::Error{_("Could not top up scriptPubKeys")};
- }
-
// Apply the label if necessary
// Note: we disable labels for ranged descriptors
if (!desc.descriptor->IsRange()) {
Risk score
Our methodology →Why this scored 29/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.