wallet: Setup new autogenerated descriptors on construction
What the developer wrote
Authored by Ava Chow
wallet: Setup new autogenerated descriptors on construction
Instead of having a caller use SetupDescriptorGeneration, just have a
constructor that takes those arguments and sets up the descriptor with
the autogenerated key.
What changed, and why it matters
This is a small internal code cleanup in Bitcoin Core's wallet. It moves the setup of newly generated single-signature descriptors into a dedicated factory-style constructor and tightens some safety checks. There is no obvious user-facing bug or direct exploit here, but the change removes a redundant encryption step and changes when a 'decryption checked' flag is set, which could have subtle wallet-initialization implications.
Treat as a normal code-review item. Verify that removing the explicit CheckDecryptionKey/Encrypt call in CWallet::SetupDescriptorScriptPubKeyMan does not leave an edge case where an encrypted wallet creates an unencrypted descriptor scriptpubkeyman. Confirm tests cover encrypted descriptor wallet creation and keypool top-up. No urgent action indicated absent further evidence.
Security signals we found
Refactor changes encryption/decryption handling path for new descriptor wallets
Assertion added that descriptor must not already exist during generation
m_decryption_thoroughly_checked flag now set during descriptor setup for encrypted wallets
Removed explicit CheckDecryptionKey/Encrypt call in CWallet::SetupDescriptorScriptPubKeyMan
No explicit security claim or CVE referenced in commit message
Evidence from the diff
The commit refactors DescriptorScriptPubKeyMan construction. It introduces GenerateNewSingleSig(), makes the plain constructor protected, moves SetupDescriptorGeneration from public to private, changes its return type from bool to void, replaces a runtime ‘if descriptor exists return false’ with an Assert(!m_wallet_descriptor.descriptor), and moves the m_decryption_thoroughly_checked assignment inside SetupDescriptorGeneration. The caller CWallet::SetupDescriptorScriptPubKeyMan no longer explicitly calls CheckDecryptionKey/Encrypt before descriptor generation; instead it only checks IsLocked() and relies on the new path. This is a behavior-shifting refactor rather than a pure no-op.
Changed components
src/wallet/scriptpubkeyman.cppsrc/wallet/scriptpubkeyman.hsrc/wallet/wallet.cppDescriptorScriptPubKeyManCWallet::SetupDescriptorScriptPubKeyManInspect captured patch +27 / −25
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 98bf5542..d0f899fe 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -853,6 +853,13 @@ std::unique_ptr<DescriptorScriptPubKeyMan> DescriptorScriptPubKeyMan::LoadFromSt
return std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(storage, descriptor, keypool_size, keys, ckeys));
}
+std::unique_ptr<DescriptorScriptPubKeyMan> DescriptorScriptPubKeyMan::GenerateNewSingleSig(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, OutputType addr_type, bool internal)
+{
+ auto spkm = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(storage, keypool_size));
+ spkm->SetupDescriptorGeneration(batch, master_key, addr_type, internal);
+ return spkm;
+}
+
util::Result<CTxDestination> DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type)
{
// Returns true if this descriptor supports getting new addresses. Conditions where we may be unable to fetch them (e.g. locked) are caught later
@@ -1164,15 +1171,11 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const
}
}
-bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal)
+void DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal)
{
LOCK(cs_desc_man);
- assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
-
- // Ignore when there is already a descriptor
- if (m_wallet_descriptor.descriptor) {
- return false;
- }
+ Assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
+ Assert(!m_wallet_descriptor.descriptor);
m_wallet_descriptor = GenerateWalletDescriptor(master_key.Neuter(), addr_type, internal);
@@ -1184,11 +1187,15 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch& batch, co
throw std::runtime_error(std::string(__func__) + ": writing descriptor failed");
}
+ // Set m_decryption_thoroughly_checked for encrypted wallets
+ if (m_storage.HasEncryptionKeys()) {
+ m_decryption_thoroughly_checked = true;
+ }
+
// TopUp
TopUpWithDB(batch);
m_storage.UnsetBlankWalletFlag(batch);
- return true;
}
bool DescriptorScriptPubKeyMan::IsHDEnabled() const
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
index 238888fc..621630c9 100644
--- a/src/wallet/scriptpubkeyman.h
+++ b/src/wallet/scriptpubkeyman.h
@@ -326,24 +326,28 @@ private:
void AddDescriptorKey(const CKey& key, const CPubKey &pubkey);
void UpdateWithSigningProvider(WalletBatch& batch, const FlatSigningProvider& signing_provider) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
+ //! Setup descriptors based on the given CExtKey
+ void SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal);
+
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, int64_t keypool_size)
+ : ScriptPubKeyMan(storage),
+ m_keypool_size(keypool_size)
+ {}
+
WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man);
//! Same as 'TopUp' but designed for use within a batch transaction context
bool TopUpWithDB(WalletBatch& batch, unsigned int size = 0);
public:
- 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);
+ static std::unique_ptr<DescriptorScriptPubKeyMan> GenerateNewSingleSig(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, OutputType addr_type, bool internal);
mutable RecursiveMutex cs_desc_man;
@@ -366,9 +370,6 @@ public:
bool IsHDEnabled() const override;
- //! Setup descriptors based on the given CExtkey
- bool SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal);
-
bool HavePrivateKeys() const override;
bool HasPrivKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
//! Retrieve the particular key if it is available. Returns nullopt if the key is not in the wallet, or if the wallet is locked.
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 5a5af893..8a8755d7 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3553,16 +3553,10 @@ void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc,
DescriptorScriptPubKeyMan& CWallet::SetupDescriptorScriptPubKeyMan(WalletBatch& batch, const CExtKey& master_key, const OutputType& output_type, bool internal)
{
AssertLockHeld(cs_wallet);
- auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, m_keypool_size));
- if (HasEncryptionKeys()) {
- if (IsLocked()) {
- throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors");
- }
- if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, &batch)) {
- throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors");
- }
+ if (IsLocked()) {
+ throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors");
}
- spk_manager->SetupDescriptorGeneration(batch, master_key, output_type, internal);
+ auto spk_manager = DescriptorScriptPubKeyMan::GenerateNewSingleSig(*this, batch, m_keypool_size, master_key, output_type, internal);
DescriptorScriptPubKeyMan* out = spk_manager.get();
uint256 id = spk_manager->GetID();
AddScriptPubKeyMan(id, std::move(spk_manager));
Why this scored 21/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.