Skip to content

Commit 80d4231

Browse files
fanquakefanquake
committed
Merge bitcoin#19980: refactor: Some wallet cleanups
9b74461 refactor: Assert before dereference in CWallet::GetDatabase (João Barbosa) 021feb3 refactor: Drop redudant CWallet::GetDBHandle (João Barbosa) Pull request description: ACKs for top commit: achow101: Code Review ACK 9b74461 meshcollider: utACK 9b74461 ryanofsky: Code review ACK 9b74461. Changes since last review: rebasing due to conflict, dropping wallet path commit c6a5cd7 as suggested in discussion, making GetDatabase() const in the earlier commit. Giving more descriptive title like Tree-SHA512: 68cf3b5e9fe0acb3a5cd081086629989f213f1904cc344e5775767b56759a7d905b1e1c303afbe40f172ff81bf07f3719b59d8f6ec2de3fdd53cd0e2d220fb25
2 parents f17e8ba + 9b74461 commit 80d4231

File tree

3 files changed

+34
-37
lines changed

3 files changed

+34
-37
lines changed

‎src/wallet/wallet.cpp

Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -419,7 +419,7 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
419419
return false;
420420
if (!crypter.Encrypt(_vMasterKey, pMasterKey.second.vchCryptedKey))
421421
return false;
422-
WalletBatch(*database).WriteMasterKey(pMasterKey.first, pMasterKey.second);
422+
WalletBatch(GetDatabase()).WriteMasterKey(pMasterKey.first, pMasterKey.second);
423423
if (fWasLocked)
424424
Lock();
425425
return true;
@@ -432,7 +432,7 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
432432

433433
void CWallet::chainStateFlushed(const CBlockLocator& loc)
434434
{
435-
WalletBatch batch(*database);
435+
WalletBatch batch(GetDatabase());
436436
batch.WriteBestBlock(loc);
437437
}
438438

@@ -444,7 +444,7 @@ void CWallet::SetMinVersion(enum WalletFeature nVersion, WalletBatch* batch_in)
444444
nWalletVersion = nVersion;
445445

446446
{
447-
WalletBatch* batch = batch_in ? batch_in : new WalletBatch(*database);
447+
WalletBatch* batch = batch_in ? batch_in : new WalletBatch(GetDatabase());
448448
if (nWalletVersion > 40000)
449449
batch->WriteMinVersion(nWalletVersion);
450450
if (!batch_in)
@@ -484,12 +484,12 @@ bool CWallet::HasWalletSpend(const uint256& txid) const
484484

485485
void CWallet::Flush()
486486
{
487-
database->Flush();
487+
GetDatabase().Flush();
488488
}
489489

490490
void CWallet::Close()
491491
{
492-
database->Close();
492+
GetDatabase().Close();
493493
}
494494

495495
void CWallet::SyncMetaData(std::pair<TxSpends::iterator, TxSpends::iterator> range)
@@ -615,7 +615,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
615615
{
616616
LOCK(cs_wallet);
617617
mapMasterKeys[++nMasterKeyMaxID] = kMasterKey;
618-
WalletBatch* encrypted_batch = new WalletBatch(*database);
618+
WalletBatch* encrypted_batch = new WalletBatch(GetDatabase());
619619
if (!encrypted_batch->TxnBegin()) {
620620
delete encrypted_batch;
621621
encrypted_batch = nullptr;
@@ -667,12 +667,12 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
667667

668668
// Need to completely rewrite the wallet file; if we don't, bdb might keep
669669
// bits of the unencrypted private key in slack space in the database file.
670-
database->Rewrite();
670+
GetDatabase().Rewrite();
671671

672672
// BDB seems to have a bad habit of writing old data into
673673
// slack space in .dat files; that is bad if the old data is
674674
// unencrypted private keys. So:
675-
database->ReloadDbEnv();
675+
GetDatabase().ReloadDbEnv();
676676

677677
}
678678
NotifyStatusChanged(this);
@@ -683,7 +683,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
683683
DBErrors CWallet::ReorderTransactions()
684684
{
685685
LOCK(cs_wallet);
686-
WalletBatch batch(*database);
686+
WalletBatch batch(GetDatabase());
687687

688688
// Old wallets didn't have any defined order for transactions
689689
// Probably a bad idea to change the output of this
@@ -744,7 +744,7 @@ int64_t CWallet::IncOrderPosNext(WalletBatch* batch)
744744
if (batch) {
745745
batch->WriteOrderPosNext(nOrderPosNext);
746746
} else {
747-
WalletBatch(*database).WriteOrderPosNext(nOrderPosNext);
747+
WalletBatch(GetDatabase()).WriteOrderPosNext(nOrderPosNext);
748748
}
749749
return nRet;
750750
}
@@ -774,7 +774,7 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash)
774774

775775
wtx.mapValue["replaced_by_txid"] = newHash.ToString();
776776

777-
WalletBatch batch(*database);
777+
WalletBatch batch(GetDatabase());
778778

779779
bool success = true;
780780
if (!batch.WriteTx(wtx)) {
@@ -846,7 +846,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio
846846
{
847847
LOCK(cs_wallet);
848848

849-
WalletBatch batch(*database, fFlushOnClose);
849+
WalletBatch batch(GetDatabase(), fFlushOnClose);
850850

851851
uint256 hash = tx->GetHash();
852852

@@ -1045,7 +1045,7 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
10451045
{
10461046
LOCK(cs_wallet);
10471047

1048-
WalletBatch batch(*database);
1048+
WalletBatch batch(GetDatabase());
10491049

10501050
std::set<uint256> todo;
10511051
std::set<uint256> done;
@@ -1108,7 +1108,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c
11081108
return;
11091109

11101110
// Do not flush the wallet here for performance reasons
1111-
WalletBatch batch(*database, false);
1111+
WalletBatch batch(GetDatabase(), false);
11121112

11131113
std::set<uint256> todo;
11141114
std::set<uint256> done;
@@ -1446,13 +1446,13 @@ void CWallet::SetWalletFlag(uint64_t flags)
14461446
{
14471447
LOCK(cs_wallet);
14481448
m_wallet_flags |= flags;
1449-
if (!WalletBatch(*database).WriteWalletFlags(m_wallet_flags))
1449+
if (!WalletBatch(GetDatabase()).WriteWalletFlags(m_wallet_flags))
14501450
throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed");
14511451
}
14521452

14531453
void CWallet::UnsetWalletFlag(uint64_t flag)
14541454
{
1455-
WalletBatch batch(*database);
1455+
WalletBatch batch(GetDatabase());
14561456
UnsetWalletFlagWithDB(batch, flag);
14571457
}
14581458

@@ -1491,7 +1491,7 @@ bool CWallet::AddWalletFlags(uint64_t flags)
14911491
LOCK(cs_wallet);
14921492
// We should never be writing unknown non-tolerable wallet flags
14931493
assert(((flags & KNOWN_WALLET_FLAGS) >> 32) == (flags >> 32));
1494-
if (!WalletBatch(*database).WriteWalletFlags(flags)) {
1494+
if (!WalletBatch(GetDatabase()).WriteWalletFlags(flags)) {
14951495
throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed");
14961496
}
14971497

@@ -1582,7 +1582,7 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
15821582
return false;
15831583
}
15841584
if (apply_label) {
1585-
WalletBatch batch(*database);
1585+
WalletBatch batch(GetDatabase());
15861586
for (const CScript& script : script_pub_keys) {
15871587
CTxDestination dest;
15881588
ExtractDestination(script, dest);
@@ -3177,10 +3177,10 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
31773177
LOCK(cs_wallet);
31783178

31793179
fFirstRunRet = false;
3180-
DBErrors nLoadWalletRet = WalletBatch(*database).LoadWallet(this);
3180+
DBErrors nLoadWalletRet = WalletBatch(GetDatabase()).LoadWallet(this);
31813181
if (nLoadWalletRet == DBErrors::NEED_REWRITE)
31823182
{
3183-
if (database->Rewrite("\x04pool"))
3183+
if (GetDatabase().Rewrite("\x04pool"))
31843184
{
31853185
for (const auto& spk_man_pair : m_spk_managers) {
31863186
spk_man_pair.second->RewriteDB();
@@ -3204,7 +3204,7 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
32043204
DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut)
32053205
{
32063206
AssertLockHeld(cs_wallet);
3207-
DBErrors nZapSelectTxRet = WalletBatch(*database).ZapSelectTx(vHashIn, vHashOut);
3207+
DBErrors nZapSelectTxRet = WalletBatch(GetDatabase()).ZapSelectTx(vHashIn, vHashOut);
32083208
for (const uint256& hash : vHashOut) {
32093209
const auto& it = mapWallet.find(hash);
32103210
wtxOrdered.erase(it->second.m_it_wtxOrdered);
@@ -3216,7 +3216,7 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256
32163216

32173217
if (nZapSelectTxRet == DBErrors::NEED_REWRITE)
32183218
{
3219-
if (database->Rewrite("\x04pool"))
3219+
if (GetDatabase().Rewrite("\x04pool"))
32203220
{
32213221
for (const auto& spk_man_pair : m_spk_managers) {
32223222
spk_man_pair.second->RewriteDB();
@@ -3254,14 +3254,14 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add
32543254

32553255
bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& strName, const std::string& strPurpose)
32563256
{
3257-
WalletBatch batch(*database);
3257+
WalletBatch batch(GetDatabase());
32583258
return SetAddressBookWithDB(batch, address, strName, strPurpose);
32593259
}
32603260

32613261
bool CWallet::DelAddressBook(const CTxDestination& address)
32623262
{
32633263
bool is_mine;
3264-
WalletBatch batch(*database);
3264+
WalletBatch batch(GetDatabase());
32653265
{
32663266
LOCK(cs_wallet);
32673267
// If we want to delete receiving addresses, we need to take care that DestData "used" (and possibly newer DestData) gets preserved (and the "deleted" address transformed into a change entry instead of actually being deleted)
@@ -4008,7 +4008,7 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::st
40084008
int rescan_height = 0;
40094009
if (!gArgs.GetBoolArg("-rescan", false))
40104010
{
4011-
WalletBatch batch(*walletInstance->database);
4011+
WalletBatch batch(walletInstance->GetDatabase());
40124012
CBlockLocator locator;
40134013
if (batch.ReadBestBlock(locator)) {
40144014
if (const Optional<int> fork_height = chain.findLocatorFork(locator)) {
@@ -4071,7 +4071,7 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, const std::st
40714071
}
40724072
}
40734073
walletInstance->chainStateFlushed(chain.getTipLocator());
4074-
walletInstance->database->IncrementUpdateCounter();
4074+
walletInstance->GetDatabase().IncrementUpdateCounter();
40754075
}
40764076

40774077
{
@@ -4149,7 +4149,7 @@ void CWallet::postInitProcess()
41494149

41504150
bool CWallet::BackupWallet(const std::string& strDest) const
41514151
{
4152-
return database->Backup(strDest);
4152+
return GetDatabase().Backup(strDest);
41534153
}
41544154

41554155
CKeyPool::CKeyPool()
@@ -4452,7 +4452,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
44524452

44534453
void CWallet::AddActiveScriptPubKeyMan(uint256 id, OutputType type, bool internal)
44544454
{
4455-
WalletBatch batch(*database);
4455+
WalletBatch batch(GetDatabase());
44564456
if (!batch.WriteActiveScriptPubKeyMan(static_cast<uint8_t>(type), id, internal)) {
44574457
throw std::runtime_error(std::string(__func__) + ": writing active ScriptPubKeyMan id failed");
44584458
}

‎src/wallet/wallet.h

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -695,7 +695,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
695695
std::string m_name;
696696

697697
/** Internal database handle. */
698-
std::unique_ptr<WalletDatabase> database;
698+
std::unique_ptr<WalletDatabase> const m_database;
699699

700700
/**
701701
* The following is used to keep track of how far behind the wallet is
@@ -729,14 +729,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
729729
*/
730730
mutable RecursiveMutex cs_wallet;
731731

732-
/** Get database handle used by this wallet. Ideally this function would
733-
* not be necessary.
734-
*/
735-
WalletDatabase& GetDBHandle()
732+
WalletDatabase& GetDatabase() const override
736733
{
737-
return *database;
734+
assert(static_cast<bool>(m_database));
735+
return *m_database;
738736
}
739-
WalletDatabase& GetDatabase() const override { return *database; }
740737

741738
/**
742739
* Select a set of coins such that nValueRet >= nTargetValue and at least
@@ -758,7 +755,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
758755
CWallet(interfaces::Chain* chain, const std::string& name, std::unique_ptr<WalletDatabase> database)
759756
: m_chain(chain),
760757
m_name(name),
761-
database(std::move(database))
758+
m_database(std::move(database))
762759
{
763760
}
764761

‎src/wallet/walletdb.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -945,7 +945,7 @@ void MaybeCompactWalletDB()
945945
}
946946

947947
for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) {
948-
WalletDatabase& dbh = pwallet->GetDBHandle();
948+
WalletDatabase& dbh = pwallet->GetDatabase();
949949

950950
unsigned int nUpdateCounter = dbh.nUpdateCounter;
951951

0 commit comments

Comments
 (0)