Skip to content

Commit fadc08a

Browse files
achow101achow101
committed
Locking: Lock cs_KeyStore instead of cs_wallet in legacy keyman
This commit only affects locking behavior and doesn't have other changes.
1 parent f5be479 commit fadc08a

File tree

13 files changed

+112
-116
lines changed

13 files changed

+112
-116
lines changed

‎src/qt/test/wallettests.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,8 +145,7 @@ void TestGUI(interfaces::Node& node)
145145
{
146146
auto spk_man = wallet->GetLegacyScriptPubKeyMan();
147147
auto locked_chain = wallet->chain().lock();
148-
LOCK(wallet->cs_wallet);
149-
AssertLockHeld(spk_man->cs_wallet);
148+
LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore);
150149
wallet->SetAddressBook(GetDestinationForKey(test.coinbaseKey.GetPubKey(), wallet->m_default_address_type), "", "receive");
151150
spk_man->AddKeyPubKey(test.coinbaseKey, test.coinbaseKey.GetPubKey());
152151
wallet->SetLastBlockProcessed(105, ::ChainActive().Tip()->GetBlockHash());

‎src/test/util/wallet.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,7 @@ std::string getnewaddress(CWallet& w)
2727
void importaddress(CWallet& wallet, const std::string& address)
2828
{
2929
auto spk_man = wallet.GetLegacyScriptPubKeyMan();
30-
LOCK(wallet.cs_wallet);
31-
AssertLockHeld(spk_man->cs_wallet);
30+
LOCK2(wallet.cs_wallet, spk_man->cs_KeyStore);
3231
const auto dest = DecodeDestination(address);
3332
assert(IsValidDestination(dest));
3433
const auto script = GetScriptForDestination(dest);

‎src/wallet/rpcdump.cpp‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -700,7 +700,7 @@ UniValue dumpprivkey(const JSONRPCRequest& request)
700700
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet);
701701

702702
auto locked_chain = pwallet->chain().lock();
703-
LOCK(pwallet->cs_wallet);
703+
LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore);
704704

705705
EnsureWalletIsUnlocked(pwallet);
706706

@@ -751,8 +751,7 @@ UniValue dumpwallet(const JSONRPCRequest& request)
751751
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet);
752752

753753
auto locked_chain = pwallet->chain().lock();
754-
LOCK(pwallet->cs_wallet);
755-
AssertLockHeld(spk_man.cs_wallet);
754+
LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore);
756755

757756
EnsureWalletIsUnlocked(pwallet);
758757

‎src/wallet/rpcwallet.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -983,7 +983,7 @@ static UniValue addmultisigaddress(const JSONRPCRequest& request)
983983
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet);
984984

985985
auto locked_chain = pwallet->chain().lock();
986-
LOCK(pwallet->cs_wallet);
986+
LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore);
987987

988988
std::string label;
989989
if (!request.params[2].isNull())
@@ -4014,7 +4014,7 @@ UniValue sethdseed(const JSONRPCRequest& request)
40144014
}
40154015

40164016
auto locked_chain = pwallet->chain().lock();
4017-
LOCK(pwallet->cs_wallet);
4017+
LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore);
40184018

40194019
// Do not do anything to non-HD wallets
40204020
if (!pwallet->CanSupportFeature(FEATURE_HD)) {

‎src/wallet/scriptpubkeyman.cpp‎

Lines changed: 34 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
1515
{
16+
LOCK(cs_KeyStore);
1617
error.clear();
1718

1819
// Generate a new key that is added to wallet
@@ -238,7 +239,6 @@ bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key
238239

239240
bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch)
240241
{
241-
AssertLockHeld(cs_wallet);
242242
LOCK(cs_KeyStore);
243243
encrypted_batch = batch;
244244
if (!mapCryptedKeys.empty()) {
@@ -269,6 +269,7 @@ bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBat
269269

270270
bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool)
271271
{
272+
LOCK(cs_KeyStore);
272273
if (!CanGetAddresses(internal)) {
273274
return false;
274275
}
@@ -282,7 +283,7 @@ bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool i
282283

283284
void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
284285
{
285-
AssertLockHeld(cs_wallet);
286+
LOCK(cs_KeyStore);
286287
// extract addresses and check if they match with an unused keypool key
287288
for (const auto& keyid : GetAffectedKeys(script, *this)) {
288289
std::map<CKeyID, int64_t>::const_iterator mi = m_pool_key_to_index.find(keyid);
@@ -299,7 +300,7 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
299300

300301
void LegacyScriptPubKeyMan::UpgradeKeyMetadata()
301302
{
302-
AssertLockHeld(cs_wallet);
303+
LOCK(cs_KeyStore);
303304
if (m_storage.IsLocked() || m_storage.IsWalletFlagSet(WALLET_FLAG_KEY_ORIGIN_METADATA)) {
304305
return;
305306
}
@@ -352,7 +353,7 @@ bool LegacyScriptPubKeyMan::IsHDEnabled() const
352353

353354
bool LegacyScriptPubKeyMan::CanGetAddresses(bool internal)
354355
{
355-
LOCK(cs_wallet);
356+
LOCK(cs_KeyStore);
356357
// Check if the keypool has keys
357358
bool keypool_has_keys;
358359
if (internal && m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) {
@@ -369,7 +370,7 @@ bool LegacyScriptPubKeyMan::CanGetAddresses(bool internal)
369370

370371
bool LegacyScriptPubKeyMan::Upgrade(int prev_version, std::string& error)
371372
{
372-
AssertLockHeld(cs_wallet);
373+
LOCK(cs_KeyStore);
373374
error = "";
374375
bool hd_upgrade = false;
375376
bool split_upgrade = false;
@@ -410,7 +411,7 @@ bool LegacyScriptPubKeyMan::HavePrivateKeys() const
410411

411412
void LegacyScriptPubKeyMan::RewriteDB()
412413
{
413-
AssertLockHeld(cs_wallet);
414+
LOCK(cs_KeyStore);
414415
setInternalKeyPool.clear();
415416
setExternalKeyPool.clear();
416417
m_pool_key_to_index.clear();
@@ -435,7 +436,7 @@ static int64_t GetOldestKeyTimeInPool(const std::set<int64_t>& setKeyPool, Walle
435436

436437
int64_t LegacyScriptPubKeyMan::GetOldestKeyPoolTime()
437438
{
438-
LOCK(cs_wallet);
439+
LOCK(cs_KeyStore);
439440

440441
WalletBatch batch(m_storage.GetDatabase());
441442

@@ -453,25 +454,25 @@ int64_t LegacyScriptPubKeyMan::GetOldestKeyPoolTime()
453454

454455
size_t LegacyScriptPubKeyMan::KeypoolCountExternalKeys()
455456
{
456-
AssertLockHeld(cs_wallet);
457+
LOCK(cs_KeyStore);
457458
return setExternalKeyPool.size() + set_pre_split_keypool.size();
458459
}
459460

460461
unsigned int LegacyScriptPubKeyMan::GetKeyPoolSize() const
461462
{
462-
AssertLockHeld(cs_wallet);
463+
LOCK(cs_KeyStore);
463464
return setInternalKeyPool.size() + setExternalKeyPool.size() + set_pre_split_keypool.size();
464465
}
465466

466467
int64_t LegacyScriptPubKeyMan::GetTimeFirstKey() const
467468
{
468-
AssertLockHeld(cs_wallet);
469+
LOCK(cs_KeyStore);
469470
return nTimeFirstKey;
470471
}
471472

472473
const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(const CTxDestination& dest) const
473474
{
474-
AssertLockHeld(cs_wallet);
475+
LOCK(cs_KeyStore);
475476

476477
CKeyID key_id = GetKeyForDestination(*this, dest);
477478
if (!key_id.IsNull()) {
@@ -496,7 +497,7 @@ const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(const CTxDestination& des
496497
*/
497498
void LegacyScriptPubKeyMan::UpdateTimeFirstKey(int64_t nCreateTime)
498499
{
499-
AssertLockHeld(cs_wallet);
500+
AssertLockHeld(cs_KeyStore);
500501
if (nCreateTime <= 1) {
501502
// Cannot determine birthday information, so set the wallet birthday to
502503
// the beginning of time.
@@ -513,13 +514,14 @@ bool LegacyScriptPubKeyMan::LoadKey(const CKey& key, const CPubKey &pubkey)
513514

514515
bool LegacyScriptPubKeyMan::AddKeyPubKey(const CKey& secret, const CPubKey &pubkey)
515516
{
517+
LOCK(cs_KeyStore);
516518
WalletBatch batch(m_storage.GetDatabase());
517519
return LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(batch, secret, pubkey);
518520
}
519521

520522
bool LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& secret, const CPubKey& pubkey)
521523
{
522-
AssertLockHeld(cs_wallet);
524+
AssertLockHeld(cs_KeyStore);
523525

524526
// Make sure we aren't adding private keys to private key disabled wallets
525527
assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
@@ -574,14 +576,14 @@ bool LegacyScriptPubKeyMan::LoadCScript(const CScript& redeemScript)
574576

575577
void LegacyScriptPubKeyMan::LoadKeyMetadata(const CKeyID& keyID, const CKeyMetadata& meta)
576578
{
577-
AssertLockHeld(cs_wallet);
579+
LOCK(cs_KeyStore);
578580
UpdateTimeFirstKey(meta.nCreateTime);
579581
mapKeyMetadata[keyID] = meta;
580582
}
581583

582584
void LegacyScriptPubKeyMan::LoadScriptMetadata(const CScriptID& script_id, const CKeyMetadata& meta)
583585
{
584-
AssertLockHeld(cs_wallet);
586+
LOCK(cs_KeyStore);
585587
UpdateTimeFirstKey(meta.nCreateTime);
586588
m_script_metadata[script_id] = meta;
587589
}
@@ -630,7 +632,7 @@ bool LegacyScriptPubKeyMan::AddCryptedKey(const CPubKey &vchPubKey,
630632
if (!AddCryptedKeyInner(vchPubKey, vchCryptedSecret))
631633
return false;
632634
{
633-
LOCK(cs_wallet);
635+
LOCK(cs_KeyStore);
634636
if (encrypted_batch)
635637
return encrypted_batch->WriteCryptedKey(vchPubKey,
636638
vchCryptedSecret,
@@ -663,7 +665,6 @@ static bool ExtractPubKey(const CScript &dest, CPubKey& pubKeyOut)
663665

664666
bool LegacyScriptPubKeyMan::RemoveWatchOnly(const CScript &dest)
665667
{
666-
AssertLockHeld(cs_wallet);
667668
{
668669
LOCK(cs_KeyStore);
669670
setWatchOnly.erase(dest);
@@ -734,7 +735,7 @@ bool LegacyScriptPubKeyMan::AddWatchOnly(const CScript& dest, int64_t nCreateTim
734735

735736
void LegacyScriptPubKeyMan::SetHDChain(const CHDChain& chain, bool memonly)
736737
{
737-
LOCK(cs_wallet);
738+
LOCK(cs_KeyStore);
738739
if (!memonly && !WalletBatch(m_storage.GetDatabase()).WriteHDChain(chain))
739740
throw std::runtime_error(std::string(__func__) + ": writing chain failed");
740741

@@ -771,7 +772,7 @@ bool LegacyScriptPubKeyMan::GetKeyOrigin(const CKeyID& keyID, KeyOriginInfo& inf
771772
{
772773
CKeyMetadata meta;
773774
{
774-
LOCK(cs_wallet);
775+
LOCK(cs_KeyStore);
775776
auto it = mapKeyMetadata.find(keyID);
776777
if (it != mapKeyMetadata.end()) {
777778
meta = it->second;
@@ -821,7 +822,7 @@ CPubKey LegacyScriptPubKeyMan::GenerateNewKey(WalletBatch &batch, bool internal)
821822
{
822823
assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
823824
assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET));
824-
AssertLockHeld(cs_wallet);
825+
AssertLockHeld(cs_KeyStore);
825826
bool fCompressed = m_storage.CanSupportFeature(FEATURE_COMPRPUBKEY); // default to compressed public keys if we want 0.6.0 wallets
826827

827828
CKey secret;
@@ -913,7 +914,7 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata&
913914

914915
void LegacyScriptPubKeyMan::LoadKeyPool(int64_t nIndex, const CKeyPool &keypool)
915916
{
916-
AssertLockHeld(cs_wallet);
917+
LOCK(cs_KeyStore);
917918
if (keypool.m_pre_split) {
918919
set_pre_split_keypool.insert(nIndex);
919920
} else if (keypool.fInternal) {
@@ -935,7 +936,7 @@ void LegacyScriptPubKeyMan::LoadKeyPool(int64_t nIndex, const CKeyPool &keypool)
935936
bool LegacyScriptPubKeyMan::CanGenerateKeys()
936937
{
937938
// A wallet can generate keys if it has an HD seed (IsHDEnabled) or it is a non-HD wallet (pre FEATURE_HD)
938-
LOCK(cs_wallet);
939+
LOCK(cs_KeyStore);
939940
return IsHDEnabled() || !m_storage.CanSupportFeature(FEATURE_HD);
940941
}
941942

@@ -962,7 +963,7 @@ CPubKey LegacyScriptPubKeyMan::DeriveNewSeed(const CKey& key)
962963
metadata.hd_seed_id = seed.GetID();
963964

964965
{
965-
LOCK(cs_wallet);
966+
LOCK(cs_KeyStore);
966967

967968
// mem store the metadata
968969
mapKeyMetadata[seed.GetID()] = metadata;
@@ -977,7 +978,7 @@ CPubKey LegacyScriptPubKeyMan::DeriveNewSeed(const CKey& key)
977978

978979
void LegacyScriptPubKeyMan::SetHDSeed(const CPubKey& seed)
979980
{
980-
LOCK(cs_wallet);
981+
LOCK(cs_KeyStore);
981982
// store the keyid (hash160) together with
982983
// the child index counter in the database
983984
// as a hdchain object
@@ -1000,7 +1001,7 @@ bool LegacyScriptPubKeyMan::NewKeyPool()
10001001
return false;
10011002
}
10021003
{
1003-
LOCK(cs_wallet);
1004+
LOCK(cs_KeyStore);
10041005
WalletBatch batch(m_storage.GetDatabase());
10051006

10061007
for (const int64_t nIndex : setInternalKeyPool) {
@@ -1034,7 +1035,7 @@ bool LegacyScriptPubKeyMan::TopUp(unsigned int kpSize)
10341035
return false;
10351036
}
10361037
{
1037-
LOCK(cs_wallet);
1038+
LOCK(cs_KeyStore);
10381039

10391040
if (m_storage.IsLocked()) return false;
10401041

@@ -1076,7 +1077,7 @@ bool LegacyScriptPubKeyMan::TopUp(unsigned int kpSize)
10761077

10771078
void LegacyScriptPubKeyMan::AddKeypoolPubkeyWithDB(const CPubKey& pubkey, const bool internal, WalletBatch& batch)
10781079
{
1079-
LOCK(cs_wallet);
1080+
LOCK(cs_KeyStore);
10801081
assert(m_max_keypool_index < std::numeric_limits<int64_t>::max()); // How in the hell did you use so many keys?
10811082
int64_t index = ++m_max_keypool_index;
10821083
if (!batch.WritePool(index, CKeyPool(pubkey, internal))) {
@@ -1107,7 +1108,7 @@ void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, co
11071108
{
11081109
// Return to key pool
11091110
{
1110-
LOCK(cs_wallet);
1111+
LOCK(cs_KeyStore);
11111112
if (fInternal) {
11121113
setInternalKeyPool.insert(nIndex);
11131114
} else if (!set_pre_split_keypool.empty()) {
@@ -1131,7 +1132,7 @@ bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType typ
11311132

11321133
CKeyPool keypool;
11331134
{
1134-
LOCK(cs_wallet);
1135+
LOCK(cs_KeyStore);
11351136
int64_t nIndex;
11361137
if (!ReserveKeyFromKeyPool(nIndex, keypool, internal) && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
11371138
if (m_storage.IsLocked()) return false;
@@ -1150,7 +1151,7 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key
11501151
nIndex = -1;
11511152
keypool.vchPubKey = CPubKey();
11521153
{
1153-
LOCK(cs_wallet);
1154+
LOCK(cs_KeyStore);
11541155

11551156
bool fReturningInternal = fRequestedInternal;
11561157
fReturningInternal &= (IsHDEnabled() && m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) || m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
@@ -1210,7 +1211,7 @@ void LegacyScriptPubKeyMan::LearnAllRelatedScripts(const CPubKey& key)
12101211

12111212
void LegacyScriptPubKeyMan::MarkReserveKeysAsUsed(int64_t keypool_id)
12121213
{
1213-
AssertLockHeld(cs_wallet);
1214+
AssertLockHeld(cs_KeyStore);
12141215
bool internal = setInternalKeyPool.count(keypool_id);
12151216
if (!internal) assert(setExternalKeyPool.count(keypool_id) || set_pre_split_keypool.count(keypool_id));
12161217
std::set<int64_t> *setKeyPool = internal ? &setInternalKeyPool : (set_pre_split_keypool.empty() ? &setExternalKeyPool : &set_pre_split_keypool);
@@ -1281,7 +1282,7 @@ bool LegacyScriptPubKeyMan::AddCScriptWithDB(WalletBatch& batch, const CScript&
12811282

12821283
bool LegacyScriptPubKeyMan::AddKeyOriginWithDB(WalletBatch& batch, const CPubKey& pubkey, const KeyOriginInfo& info)
12831284
{
1284-
LOCK(cs_wallet);
1285+
LOCK(cs_KeyStore);
12851286
std::copy(info.fingerprint, info.fingerprint + 4, mapKeyMetadata[pubkey.GetID()].key_origin.fingerprint);
12861287
mapKeyMetadata[pubkey.GetID()].key_origin.path = info.path;
12871288
mapKeyMetadata[pubkey.GetID()].has_key_origin = true;
@@ -1397,8 +1398,7 @@ std::set<CKeyID> LegacyScriptPubKeyMan::GetKeys() const
13971398
// Temporary CWallet accessors and aliases.
13981399
LegacyScriptPubKeyMan::LegacyScriptPubKeyMan(CWallet& wallet)
13991400
: ScriptPubKeyMan(wallet),
1400-
m_wallet(wallet),
1401-
cs_wallet(wallet.cs_wallet) {}
1401+
m_wallet(wallet) {}
14021402

14031403
void LegacyScriptPubKeyMan::NotifyWatchonlyChanged(bool fHaveWatchOnly) const { return m_wallet.NotifyWatchonlyChanged(fHaveWatchOnly); }
14041404
void LegacyScriptPubKeyMan::NotifyCanGetAddressesChanged() const { return m_wallet.NotifyCanGetAddressesChanged(); }

0 commit comments

Comments
 (0)