Skip to content

Commit 0192bd0

Browse files
laanwjlaanwj
committed
Merge #17369: Refactor: Move encryption code between KeyMan and Wallet
7cecf10 Replace LegacyScriptPubKeyMan::IsCrypted with LegacyScriptPubKeyMan::HasEncryptionKeys (Andrew Chow) bf64171 Remove SetCrypted() and fUseCrypto; Change IsCrypted()'s implementation (Andrew Chow) 77a7771 Rename EncryptKeys to Encrypt and pass in the encrypted batch to use (Andrew Chow) 35f962f Clear mapKeys before encrypting (Andrew Chow) 14b5efd Move fDecryptionThoroughlyChecked from CWallet to LegacyScriptPubKeyMan (Andrew Chow) 97c0374 Move Unlock implementation to LegacyScriptPubKeyMan (Andrew Chow) e576b13 Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey() (Andrew Chow) fd9d6ee Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage (Andrew Chow) Pull request description: Let wallet class handle locked/unlocked status and master key, and let keyman handle encrypting its data and determining whether there is encrypted data. There should be no change in behavior, but state is tracked differently. The fUseCrypto atomic bool is eliminated and replaced with equivalent HasEncryptionKeys checks. Split from #17261 ACKs for top commit: laanwj: ACK 7cecf10 Tree-SHA512: 95a997c366ca539abba0c0a7a0015f39d27b55220683d8d86344ff2d926db4724da67700d2c8ec2d82ed75d07404318c6cb81544af8aadeefab312167257e673
2 parents 3914e87 + 7cecf10 commit 0192bd0

File tree

4 files changed

+75
-64
lines changed

4 files changed

+75
-64
lines changed

‎src/wallet/scriptpubkeyman.cpp

Lines changed: 29 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -202,12 +202,11 @@ isminetype LegacyScriptPubKeyMan::IsMine(const CScript& script) const
202202
assert(false);
203203
}
204204

205-
bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn, bool accept_no_keys)
205+
bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys)
206206
{
207207
{
208208
LOCK(cs_KeyStore);
209-
if (!SetCrypted())
210-
return false;
209+
assert(mapKeys.empty());
211210

212211
bool keyPass = mapCryptedKeys.empty(); // Always pass when there are no encrypted keys
213212
bool keyFail = false;
@@ -217,7 +216,7 @@ bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn, bool accept_no_keys)
217216
const CPubKey &vchPubKey = (*mi).second.first;
218217
const std::vector<unsigned char> &vchCryptedSecret = (*mi).second.second;
219218
CKey key;
220-
if (!DecryptKey(vMasterKeyIn, vchCryptedSecret, vchPubKey, key))
219+
if (!DecryptKey(master_key, vchCryptedSecret, vchPubKey, key))
221220
{
222221
keyFail = true;
223222
break;
@@ -233,32 +232,39 @@ bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn, bool accept_no_keys)
233232
}
234233
if (keyFail || (!keyPass && !accept_no_keys))
235234
return false;
236-
vMasterKey = vMasterKeyIn;
237235
fDecryptionThoroughlyChecked = true;
238236
}
239-
NotifyStatusChanged(this);
240237
return true;
241238
}
242239

243-
bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn)
240+
bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch)
244241
{
242+
AssertLockHeld(cs_wallet);
245243
LOCK(cs_KeyStore);
246-
if (!mapCryptedKeys.empty() || IsCrypted())
244+
encrypted_batch = batch;
245+
if (!mapCryptedKeys.empty()) {
246+
encrypted_batch = nullptr;
247247
return false;
248+
}
248249

249-
fUseCrypto = true;
250-
for (const KeyMap::value_type& mKey : mapKeys)
250+
KeyMap keys_to_encrypt;
251+
keys_to_encrypt.swap(mapKeys); // Clear mapKeys so AddCryptedKeyInner will succeed.
252+
for (const KeyMap::value_type& mKey : keys_to_encrypt)
251253
{
252254
const CKey &key = mKey.second;
253255
CPubKey vchPubKey = key.GetPubKey();
254256
CKeyingMaterial vchSecret(key.begin(), key.end());
255257
std::vector<unsigned char> vchCryptedSecret;
256-
if (!EncryptSecret(vMasterKeyIn, vchSecret, vchPubKey.GetHash(), vchCryptedSecret))
258+
if (!EncryptSecret(master_key, vchSecret, vchPubKey.GetHash(), vchCryptedSecret)) {
259+
encrypted_batch = nullptr;
257260
return false;
258-
if (!AddCryptedKey(vchPubKey, vchCryptedSecret))
261+
}
262+
if (!AddCryptedKey(vchPubKey, vchCryptedSecret)) {
263+
encrypted_batch = nullptr;
259264
return false;
265+
}
260266
}
261-
mapKeys.clear();
267+
encrypted_batch = nullptr;
262268
return true;
263269
}
264270

@@ -543,7 +549,7 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& s
543549
RemoveWatchOnly(script);
544550
}
545551

546-
if (!IsCrypted()) {
552+
if (!m_storage.HasEncryptionKeys()) {
547553
return batch.WriteKey(pubkey,
548554
secret.GetPrivKey(),
549555
mapKeyMetadata[pubkey.GetID()]);
@@ -584,7 +590,7 @@ void LegacyScriptPubKeyMan::LoadScriptMetadata(const CScriptID& script_id, const
584590
bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey)
585591
{
586592
LOCK(cs_KeyStore);
587-
if (!IsCrypted()) {
593+
if (!m_storage.HasEncryptionKeys()) {
588594
return FillableSigningProvider::AddKeyPubKey(key, pubkey);
589595
}
590596

@@ -594,7 +600,7 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyInner(const CKey& key, const CPubKey &pu
594600

595601
std::vector<unsigned char> vchCryptedSecret;
596602
CKeyingMaterial vchSecret(key.begin(), key.end());
597-
if (!EncryptSecret(vMasterKey, vchSecret, pubkey.GetHash(), vchCryptedSecret)) {
603+
if (!EncryptSecret(m_storage.GetEncryptionKey(), vchSecret, pubkey.GetHash(), vchCryptedSecret)) {
598604
return false;
599605
}
600606

@@ -612,9 +618,7 @@ bool LegacyScriptPubKeyMan::LoadCryptedKey(const CPubKey &vchPubKey, const std::
612618
bool LegacyScriptPubKeyMan::AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret)
613619
{
614620
LOCK(cs_KeyStore);
615-
if (!SetCrypted()) {
616-
return false;
617-
}
621+
assert(mapKeys.empty());
618622

619623
mapCryptedKeys[vchPubKey.GetID()] = make_pair(vchPubKey, vchCryptedSecret);
620624
ImplicitlyLearnRelatedKeyScripts(vchPubKey);
@@ -741,7 +745,7 @@ void LegacyScriptPubKeyMan::SetHDChain(const CHDChain& chain, bool memonly)
741745
bool LegacyScriptPubKeyMan::HaveKey(const CKeyID &address) const
742746
{
743747
LOCK(cs_KeyStore);
744-
if (!IsCrypted()) {
748+
if (!m_storage.HasEncryptionKeys()) {
745749
return FillableSigningProvider::HaveKey(address);
746750
}
747751
return mapCryptedKeys.count(address) > 0;
@@ -750,7 +754,7 @@ bool LegacyScriptPubKeyMan::HaveKey(const CKeyID &address) const
750754
bool LegacyScriptPubKeyMan::GetKey(const CKeyID &address, CKey& keyOut) const
751755
{
752756
LOCK(cs_KeyStore);
753-
if (!IsCrypted()) {
757+
if (!m_storage.HasEncryptionKeys()) {
754758
return FillableSigningProvider::GetKey(address, keyOut);
755759
}
756760

@@ -759,7 +763,7 @@ bool LegacyScriptPubKeyMan::GetKey(const CKeyID &address, CKey& keyOut) const
759763
{
760764
const CPubKey &vchPubKey = (*mi).second.first;
761765
const std::vector<unsigned char> &vchCryptedSecret = (*mi).second.second;
762-
return DecryptKey(vMasterKey, vchCryptedSecret, vchPubKey, keyOut);
766+
return DecryptKey(m_storage.GetEncryptionKey(), vchCryptedSecret, vchPubKey, keyOut);
763767
}
764768
return false;
765769
}
@@ -797,7 +801,7 @@ bool LegacyScriptPubKeyMan::GetWatchPubKey(const CKeyID &address, CPubKey &pubke
797801
bool LegacyScriptPubKeyMan::GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const
798802
{
799803
LOCK(cs_KeyStore);
800-
if (!IsCrypted()) {
804+
if (!m_storage.HasEncryptionKeys()) {
801805
if (!FillableSigningProvider::GetPubKey(address, vchPubKeyOut)) {
802806
return GetWatchPubKey(address, vchPubKeyOut);
803807
}
@@ -1383,7 +1387,7 @@ bool LegacyScriptPubKeyMan::ImportScriptPubKeys(const std::set<CScript>& script_
13831387
std::set<CKeyID> LegacyScriptPubKeyMan::GetKeys() const
13841388
{
13851389
LOCK(cs_KeyStore);
1386-
if (!IsCrypted()) {
1390+
if (!m_storage.HasEncryptionKeys()) {
13871391
return FillableSigningProvider::GetKeys();
13881392
}
13891393
std::set<CKeyID> set_address;
@@ -1397,13 +1401,8 @@ std::set<CKeyID> LegacyScriptPubKeyMan::GetKeys() const
13971401
LegacyScriptPubKeyMan::LegacyScriptPubKeyMan(CWallet& wallet)
13981402
: ScriptPubKeyMan(wallet),
13991403
m_wallet(wallet),
1400-
cs_wallet(wallet.cs_wallet),
1401-
vMasterKey(wallet.vMasterKey),
1402-
fUseCrypto(wallet.fUseCrypto),
1403-
fDecryptionThoroughlyChecked(wallet.fDecryptionThoroughlyChecked) {}
1404+
cs_wallet(wallet.cs_wallet) {}
14041405

1405-
bool LegacyScriptPubKeyMan::SetCrypted() { return m_wallet.SetCrypted(); }
1406-
bool LegacyScriptPubKeyMan::IsCrypted() const { return m_wallet.IsCrypted(); }
14071406
void LegacyScriptPubKeyMan::NotifyWatchonlyChanged(bool fHaveWatchOnly) const { return m_wallet.NotifyWatchonlyChanged(fHaveWatchOnly); }
14081407
void LegacyScriptPubKeyMan::NotifyCanGetAddressesChanged() const { return m_wallet.NotifyCanGetAddressesChanged(); }
14091408
template<typename... Params> void LegacyScriptPubKeyMan::WalletLogPrintf(const std::string& fmt, const Params&... parameters) const { return m_wallet.WalletLogPrintf(fmt, parameters...); }

‎src/wallet/scriptpubkeyman.h

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ class WalletStorage
3131
virtual void UnsetBlankWalletFlag(WalletBatch&) = 0;
3232
virtual bool CanSupportFeature(enum WalletFeature) const = 0;
3333
virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr, bool = false) = 0;
34+
virtual const CKeyingMaterial& GetEncryptionKey() const = 0;
35+
virtual bool HasEncryptionKeys() const = 0;
3436
virtual bool IsLocked() const = 0;
3537
};
3638

@@ -150,6 +152,10 @@ class ScriptPubKeyMan
150152
virtual bool GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error) { return false; }
151153
virtual isminetype IsMine(const CScript& script) const { return ISMINE_NO; }
152154

155+
//! Check that the given decryption key is valid for this ScriptPubKeyMan, i.e. it decrypts all of the keys handled by it.
156+
virtual bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) { return false; }
157+
virtual bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) { return false; }
158+
153159
virtual bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) { return false; }
154160
virtual void KeepDestination(int64_t index, const OutputType& type) {}
155161
virtual void ReturnDestination(int64_t index, bool internal, const CTxDestination& addr) {}
@@ -193,6 +199,9 @@ class ScriptPubKeyMan
193199
class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider
194200
{
195201
private:
202+
//! keeps track of whether Unlock has run a thorough check before
203+
bool fDecryptionThoroughlyChecked = false;
204+
196205
using WatchOnlySet = std::set<CScript>;
197206
using WatchKeyMap = std::map<CKeyID, CPubKey>;
198207

@@ -272,8 +281,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
272281
bool GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error) override;
273282
isminetype IsMine(const CScript& script) const override;
274283

275-
//! will encrypt previously unencrypted keys
276-
bool EncryptKeys(CKeyingMaterial& vMasterKeyIn);
284+
bool CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys = false) override;
285+
bool Encrypt(const CKeyingMaterial& master_key, WalletBatch* batch) override;
277286

278287
bool GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool) override;
279288
void KeepDestination(int64_t index, const OutputType& type) override;
@@ -403,16 +412,11 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
403412
friend class CWallet;
404413
friend class ReserveDestination;
405414
LegacyScriptPubKeyMan(CWallet& wallet);
406-
bool SetCrypted();
407-
bool IsCrypted() const;
408415
void NotifyWatchonlyChanged(bool fHaveWatchOnly) const;
409416
void NotifyCanGetAddressesChanged() const;
410417
template<typename... Params> void WalletLogPrintf(const std::string& fmt, const Params&... parameters) const;
411418
CWallet& m_wallet;
412419
CCriticalSection& cs_wallet;
413-
CKeyingMaterial& vMasterKey GUARDED_BY(cs_KeyStore);
414-
std::atomic<bool>& fUseCrypto;
415-
bool& fDecryptionThoroughlyChecked;
416420
};
417421

418422
#endif // BITCOIN_WALLET_SCRIPTPUBKEYMAN_H

‎src/wallet/wallet.cpp

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -532,8 +532,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
532532
{
533533
LOCK(cs_wallet);
534534
mapMasterKeys[++nMasterKeyMaxID] = kMasterKey;
535-
assert(!encrypted_batch);
536-
encrypted_batch = new WalletBatch(*database);
535+
WalletBatch* encrypted_batch = new WalletBatch(*database);
537536
if (!encrypted_batch->TxnBegin()) {
538537
delete encrypted_batch;
539538
encrypted_batch = nullptr;
@@ -542,7 +541,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
542541
encrypted_batch->WriteMasterKey(nMasterKeyMaxID, kMasterKey);
543542

544543
if (auto spk_man = m_spk_man.get()) {
545-
if (!spk_man->EncryptKeys(_vMasterKey)) {
544+
if (!spk_man->Encrypt(_vMasterKey, encrypted_batch)) {
546545
encrypted_batch->TxnAbort();
547546
delete encrypted_batch;
548547
encrypted_batch = nullptr;
@@ -4003,15 +4002,9 @@ std::vector<OutputGroup> CWallet::GroupOutputs(const std::vector<COutput>& outpu
40034002
return groups;
40044003
}
40054004

4006-
bool CWallet::SetCrypted()
4005+
bool CWallet::IsCrypted() const
40074006
{
4008-
LOCK(cs_KeyStore);
4009-
if (fUseCrypto)
4010-
return true;
4011-
if (!mapKeys.empty())
4012-
return false;
4013-
fUseCrypto = true;
4014-
return true;
4007+
return HasEncryptionKeys();
40154008
}
40164009

40174010
bool CWallet::IsLocked() const
@@ -4025,7 +4018,7 @@ bool CWallet::IsLocked() const
40254018

40264019
bool CWallet::Lock()
40274020
{
4028-
if (!SetCrypted())
4021+
if (!IsCrypted())
40294022
return false;
40304023

40314024
{
@@ -4037,6 +4030,21 @@ bool CWallet::Lock()
40374030
return true;
40384031
}
40394032

4033+
bool CWallet::Unlock(const CKeyingMaterial& vMasterKeyIn, bool accept_no_keys)
4034+
{
4035+
{
4036+
LOCK(cs_KeyStore);
4037+
if (m_spk_man) {
4038+
if (!m_spk_man->CheckDecryptionKey(vMasterKeyIn, accept_no_keys)) {
4039+
return false;
4040+
}
4041+
}
4042+
vMasterKey = vMasterKeyIn;
4043+
}
4044+
NotifyStatusChanged(this);
4045+
return true;
4046+
}
4047+
40404048
ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const CScript& script) const
40414049
{
40424050
return m_spk_man.get();
@@ -4056,3 +4064,13 @@ LegacyScriptPubKeyMan* CWallet::GetLegacyScriptPubKeyMan() const
40564064
{
40574065
return m_spk_man.get();
40584066
}
4067+
4068+
const CKeyingMaterial& CWallet::GetEncryptionKey() const
4069+
{
4070+
return vMasterKey;
4071+
}
4072+
4073+
bool CWallet::HasEncryptionKeys() const
4074+
{
4075+
return !mapMasterKeys.empty();
4076+
}

‎src/wallet/wallet.h

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -597,14 +597,7 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
597597
private:
598598
CKeyingMaterial vMasterKey GUARDED_BY(cs_KeyStore);
599599

600-
//! if fUseCrypto is true, mapKeys must be empty
601-
//! if fUseCrypto is false, vMasterKey must be empty
602-
std::atomic<bool> fUseCrypto;
603600

604-
//! keeps track of whether Unlock has run a thorough check before
605-
bool fDecryptionThoroughlyChecked;
606-
607-
bool SetCrypted();
608601
bool Unlock(const CKeyingMaterial& vMasterKeyIn, bool accept_no_keys = false);
609602

610603
std::atomic<bool> fAbortRescan{false};
@@ -734,9 +727,7 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
734727

735728
/** Construct wallet with specified name and database implementation. */
736729
CWallet(interfaces::Chain* chain, const WalletLocation& location, std::unique_ptr<WalletDatabase> database)
737-
: fUseCrypto(false),
738-
fDecryptionThoroughlyChecked(false),
739-
m_chain(chain),
730+
: m_chain(chain),
740731
m_location(location),
741732
database(std::move(database))
742733
{
@@ -746,11 +737,9 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
746737
{
747738
// Should not have slots connected at this point.
748739
assert(NotifyUnload.empty());
749-
delete encrypted_batch;
750-
encrypted_batch = nullptr;
751740
}
752741

753-
bool IsCrypted() const { return fUseCrypto; }
742+
bool IsCrypted() const;
754743
bool IsLocked() const override;
755744
bool Lock();
756745

@@ -1136,6 +1125,9 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
11361125

11371126
LegacyScriptPubKeyMan* GetLegacyScriptPubKeyMan() const;
11381127

1128+
const CKeyingMaterial& GetEncryptionKey() const override;
1129+
bool HasEncryptionKeys() const override;
1130+
11391131
// Temporary LegacyScriptPubKeyMan accessors and aliases.
11401132
friend class LegacyScriptPubKeyMan;
11411133
std::unique_ptr<LegacyScriptPubKeyMan> m_spk_man = MakeUnique<LegacyScriptPubKeyMan>(*this);
@@ -1145,8 +1137,6 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
11451137
LegacyScriptPubKeyMan::CryptedKeyMap& mapCryptedKeys GUARDED_BY(cs_KeyStore) = m_spk_man->mapCryptedKeys;
11461138
LegacyScriptPubKeyMan::WatchOnlySet& setWatchOnly GUARDED_BY(cs_KeyStore) = m_spk_man->setWatchOnly;
11471139
LegacyScriptPubKeyMan::WatchKeyMap& mapWatchKeys GUARDED_BY(cs_KeyStore) = m_spk_man->mapWatchKeys;
1148-
WalletBatch*& encrypted_batch GUARDED_BY(cs_wallet) = m_spk_man->encrypted_batch;
1149-
using CryptedKeyMap = LegacyScriptPubKeyMan::CryptedKeyMap;
11501140

11511141
/** Get last block processed height */
11521142
int GetLastBlockHeight() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)

0 commit comments

Comments
 (0)