Skip to content

Commit 5a95c51

Browse files
fanquakefanquake
committed
Merge bitcoin/bitcoin#20191: wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag
1811810 refactor: remove m_internal from DescriptorSPKman (S3RK) Pull request description: Rationale: improve consistency between `CWallet` and `DescriptorScriptPubKeyMan`; simplify `ScriptPubKeyMan` interface. Descriptor in itself is neither internal or external. It's responsibility of a wallet to assign and manage descriptors for a specific purpose. Duplicating information about internalness of a descriptor could lead to inconsistencies and unexpected behaviour (for example misreporting keypool size). ACKs for top commit: instagibbs: reACK bitcoin/bitcoin@1811810 achow101: reACK 1811810 Tree-SHA512: d5613b7f6795b290bfa0fd8cb0536de1714d0cf72cba402266bd06d550758ebad690b54fc0a336a1c7414b5814aa4a37c90a6ae89926474a97d30956d7e034ff
2 parents 045bb06 + 1811810 commit 5a95c51

File tree

4 files changed

+19
-40
lines changed

4 files changed

+19
-40
lines changed

‎src/wallet/external_signer_scriptpubkeyman.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan
1515
ExternalSignerScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor)
1616
: DescriptorScriptPubKeyMan(storage, descriptor)
1717
{}
18-
ExternalSignerScriptPubKeyMan(WalletStorage& storage, bool internal)
19-
: DescriptorScriptPubKeyMan(storage, internal)
18+
ExternalSignerScriptPubKeyMan(WalletStorage& storage)
19+
: DescriptorScriptPubKeyMan(storage)
2020
{}
2121

2222
/** Provide a descriptor at setup time

‎src/wallet/scriptpubkeyman.cpp

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1613,12 +1613,10 @@ std::set<CKeyID> LegacyScriptPubKeyMan::GetKeys() const
16131613
return set_address;
16141614
}
16151615

1616-
void LegacyScriptPubKeyMan::SetInternal(bool internal) {}
1617-
16181616
bool DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
16191617
{
16201618
// Returns true if this descriptor supports getting new addresses. Conditions where we may be unable to fetch them (e.g. locked) are caught later
1621-
if (!CanGetAddresses(m_internal)) {
1619+
if (!CanGetAddresses()) {
16221620
error = "No addresses available";
16231621
return false;
16241622
}
@@ -1876,7 +1874,7 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const
18761874
}
18771875
}
18781876

1879-
bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type)
1877+
bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type, bool internal)
18801878
{
18811879
if (addr_type == OutputType::BECH32M) {
18821880
// Don't allow setting up taproot descriptors yet
@@ -1924,7 +1922,7 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(const CExtKey& master_
19241922
desc_prefix += "/0'";
19251923
}
19261924

1927-
std::string internal_path = m_internal ? "/1" : "/0";
1925+
std::string internal_path = internal ? "/1" : "/0";
19281926
std::string desc_str = desc_prefix + "/0'" + internal_path + desc_suffix;
19291927

19301928
// Make the descriptor
@@ -1979,13 +1977,6 @@ int64_t DescriptorScriptPubKeyMan::GetOldestKeyPoolTime() const
19791977
return 0;
19801978
}
19811979

1982-
size_t DescriptorScriptPubKeyMan::KeypoolCountExternalKeys() const
1983-
{
1984-
if (m_internal) {
1985-
return 0;
1986-
}
1987-
return GetKeyPoolSize();
1988-
}
19891980

19901981
unsigned int DescriptorScriptPubKeyMan::GetKeyPoolSize() const
19911982
{
@@ -2187,11 +2178,6 @@ uint256 DescriptorScriptPubKeyMan::GetID() const
21872178
return id;
21882179
}
21892180

2190-
void DescriptorScriptPubKeyMan::SetInternal(bool internal)
2191-
{
2192-
this->m_internal = internal;
2193-
}
2194-
21952181
void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache)
21962182
{
21972183
LOCK(cs_desc_man);

‎src/wallet/scriptpubkeyman.h

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,6 @@ class ScriptPubKeyMan
216216

217217
virtual int64_t GetOldestKeyPoolTime() const { return GetTime(); }
218218

219-
virtual size_t KeypoolCountExternalKeys() const { return 0; }
220219
virtual unsigned int GetKeyPoolSize() const { return 0; }
221220

222221
virtual int64_t GetTimeFirstKey() const { return 0; }
@@ -239,8 +238,6 @@ class ScriptPubKeyMan
239238

240239
virtual uint256 GetID() const { return uint256(); }
241240

242-
virtual void SetInternal(bool internal) {}
243-
244241
/** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */
245242
template<typename... Params>
246243
void WalletLogPrintf(std::string fmt, Params... parameters) const {
@@ -386,7 +383,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
386383
void RewriteDB() override;
387384

388385
int64_t GetOldestKeyPoolTime() const override;
389-
size_t KeypoolCountExternalKeys() const override;
386+
size_t KeypoolCountExternalKeys() const;
390387
unsigned int GetKeyPoolSize() const override;
391388

392389
int64_t GetTimeFirstKey() const override;
@@ -405,8 +402,6 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
405402

406403
uint256 GetID() const override;
407404

408-
void SetInternal(bool internal) override;
409-
410405
// Map from Key ID to key metadata.
411406
std::map<CKeyID, CKeyMetadata> mapKeyMetadata GUARDED_BY(cs_KeyStore);
412407

@@ -533,8 +528,6 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
533528
PubKeyMap m_map_pubkeys GUARDED_BY(cs_desc_man);
534529
int32_t m_max_cached_index = -1;
535530

536-
bool m_internal = false;
537-
538531
KeyMap m_map_keys GUARDED_BY(cs_desc_man);
539532
CryptedKeyMap m_map_crypted_keys GUARDED_BY(cs_desc_man);
540533

@@ -560,9 +553,8 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
560553
: ScriptPubKeyMan(storage),
561554
m_wallet_descriptor(descriptor)
562555
{}
563-
DescriptorScriptPubKeyMan(WalletStorage& storage, bool internal)
564-
: ScriptPubKeyMan(storage),
565-
m_internal(internal)
556+
DescriptorScriptPubKeyMan(WalletStorage& storage)
557+
: ScriptPubKeyMan(storage)
566558
{}
567559

568560
mutable RecursiveMutex cs_desc_man;
@@ -587,7 +579,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
587579
bool IsHDEnabled() const override;
588580

589581
//! Setup descriptors based on the given CExtkey
590-
bool SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type);
582+
bool SetupDescriptorGeneration(const CExtKey& master_key, OutputType addr_type, bool internal);
591583

592584
/** Provide a descriptor at setup time
593585
* Returns false if already setup or setup fails, true if setup is successful
@@ -597,7 +589,6 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
597589
bool HavePrivateKeys() const override;
598590

599591
int64_t GetOldestKeyPoolTime() const override;
600-
size_t KeypoolCountExternalKeys() const override;
601592
unsigned int GetKeyPoolSize() const override;
602593

603594
int64_t GetTimeFirstKey() const override;
@@ -616,8 +607,6 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
616607

617608
uint256 GetID() const override;
618609

619-
void SetInternal(bool internal) override;
620-
621610
void SetCache(const DescriptorCache& cache);
622611

623612
bool AddKey(const CKeyID& key_id, const CKey& key);

‎src/wallet/wallet.cpp

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2094,9 +2094,14 @@ size_t CWallet::KeypoolCountExternalKeys() const
20942094
{
20952095
AssertLockHeld(cs_wallet);
20962096

2097+
auto legacy_spk_man = GetLegacyScriptPubKeyMan();
2098+
if (legacy_spk_man) {
2099+
return legacy_spk_man->KeypoolCountExternalKeys();
2100+
}
2101+
20972102
unsigned int count = 0;
2098-
for (auto spk_man : GetActiveScriptPubKeyMans()) {
2099-
count += spk_man->KeypoolCountExternalKeys();
2103+
for (auto spk_man : m_external_spk_managers) {
2104+
count += spk_man.second->GetKeyPoolSize();
21002105
}
21012106

21022107
return count;
@@ -3112,7 +3117,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
31123117
// TODO: Setup taproot (bech32m) descriptors by default
31133118
continue;
31143119
}
3115-
auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, internal));
3120+
auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this));
31163121
if (IsCrypted()) {
31173122
if (IsLocked()) {
31183123
throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors");
@@ -3121,7 +3126,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
31213126
throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors");
31223127
}
31233128
}
3124-
spk_manager->SetupDescriptorGeneration(master_key, t);
3129+
spk_manager->SetupDescriptorGeneration(master_key, t, internal);
31253130
uint256 id = spk_manager->GetID();
31263131
m_spk_managers[id] = std::move(spk_manager);
31273132
AddActiveScriptPubKeyMan(id, t, internal);
@@ -3147,7 +3152,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
31473152
continue;
31483153
}
31493154
OutputType t = *desc->GetOutputType();
3150-
auto spk_manager = std::unique_ptr<ExternalSignerScriptPubKeyMan>(new ExternalSignerScriptPubKeyMan(*this, internal));
3155+
auto spk_manager = std::unique_ptr<ExternalSignerScriptPubKeyMan>(new ExternalSignerScriptPubKeyMan(*this));
31513156
spk_manager->SetupDescriptor(std::move(desc));
31523157
uint256 id = spk_manager->GetID();
31533158
m_spk_managers[id] = std::move(spk_manager);
@@ -3176,7 +3181,6 @@ void CWallet::LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool intern
31763181
auto& spk_mans = internal ? m_internal_spk_managers : m_external_spk_managers;
31773182
auto& spk_mans_other = internal ? m_external_spk_managers : m_internal_spk_managers;
31783183
auto spk_man = m_spk_managers.at(id).get();
3179-
spk_man->SetInternal(internal);
31803184
spk_mans[type] = spk_man;
31813185

31823186
if (spk_mans_other[type] == spk_man) {

0 commit comments

Comments
 (0)