Skip to content

Commit 3d36d5d

Browse files
achow101achow101
committed
wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC
If the locked coin needs to be persisted to the wallet database, insteead of having the RPC figure out when to create a WalletBatch and having LockCoin's behavior depend on it, have LockCoin take whether to persist as a parameter so it makes the batch. Since unlocking a persisted locked coin requires a database write as well, we need to track whether the locked coin was persisted to the wallet database so that it can erase the locked coin when necessary. Keeping track of whether a locked coin was persisted is also useful information for future PRs.
1 parent 87ec923 commit 3d36d5d

File tree

8 files changed

+49
-47
lines changed

8 files changed

+49
-47
lines changed

‎src/wallet/interfaces.cpp

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -249,14 +249,12 @@ class WalletImpl : public Wallet
249249
bool lockCoin(const COutPoint& output, const bool write_to_db) override
250250
{
251251
LOCK(m_wallet->cs_wallet);
252-
std::unique_ptr<WalletBatch> batch = write_to_db ? std::make_unique<WalletBatch>(m_wallet->GetDatabase()) : nullptr;
253-
return m_wallet->LockCoin(output, batch.get());
252+
return m_wallet->LockCoin(output, write_to_db);
254253
}
255254
bool unlockCoin(const COutPoint& output) override
256255
{
257256
LOCK(m_wallet->cs_wallet);
258-
std::unique_ptr<WalletBatch> batch = std::make_unique<WalletBatch>(m_wallet->GetDatabase());
259-
return m_wallet->UnlockCoin(output, batch.get());
257+
return m_wallet->UnlockCoin(output);
260258
}
261259
bool isLockedCoin(const COutPoint& output) override
262260
{

‎src/wallet/rpc/coins.cpp

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -356,16 +356,12 @@ RPCHelpMan lockunspent()
356356
outputs.push_back(outpt);
357357
}
358358

359-
std::unique_ptr<WalletBatch> batch = nullptr;
360-
// Unlock is always persistent
361-
if (fUnlock || persistent) batch = std::make_unique<WalletBatch>(pwallet->GetDatabase());
362-
363359
// Atomically set (un)locked status for the outputs.
364360
for (const COutPoint& outpt : outputs) {
365361
if (fUnlock) {
366-
if (!pwallet->UnlockCoin(outpt, batch.get())) throw JSONRPCError(RPC_WALLET_ERROR, "Unlocking coin failed");
362+
if (!pwallet->UnlockCoin(outpt)) throw JSONRPCError(RPC_WALLET_ERROR, "Unlocking coin failed");
367363
} else {
368-
if (!pwallet->LockCoin(outpt, batch.get())) throw JSONRPCError(RPC_WALLET_ERROR, "Locking coin failed");
364+
if (!pwallet->LockCoin(outpt, persistent)) throw JSONRPCError(RPC_WALLET_ERROR, "Locking coin failed");
369365
}
370366
}
371367

‎src/wallet/rpc/spend.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1579,7 +1579,7 @@ RPCHelpMan sendall()
15791579
const bool lock_unspents{options.exists("lock_unspents") ? options["lock_unspents"].get_bool() : false};
15801580
if (lock_unspents) {
15811581
for (const CTxIn& txin : rawTx.vin) {
1582-
pwallet->LockCoin(txin.prevout);
1582+
pwallet->LockCoin(txin.prevout, /*persist=*/false);
15831583
}
15841584
}
15851585

‎src/wallet/spend.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1467,7 +1467,7 @@ util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CM
14671467

14681468
if (lockUnspents) {
14691469
for (const CTxIn& txin : res->tx->vin) {
1470-
wallet.LockCoin(txin.prevout);
1470+
wallet.LockCoin(txin.prevout, /*persist=*/false);
14711471
}
14721472
}
14731473

‎src/wallet/test/wallet_tests.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -458,7 +458,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup)
458458
for (const auto& group : list) {
459459
for (const auto& coin : group.second) {
460460
LOCK(wallet->cs_wallet);
461-
wallet->LockCoin(coin.outpoint);
461+
wallet->LockCoin(coin.outpoint, /*persist=*/false);
462462
}
463463
}
464464
{
@@ -486,7 +486,7 @@ void TestCoinsResult(ListCoinsTest& context, OutputType out_type, CAmount amount
486486
filter.skip_locked = false;
487487
CoinsResult available_coins = AvailableCoins(*context.wallet, nullptr, std::nullopt, filter);
488488
// Lock outputs so they are not spent in follow-up transactions
489-
for (uint32_t i = 0; i < wtx.tx->vout.size(); i++) context.wallet->LockCoin({wtx.GetHash(), i});
489+
for (uint32_t i = 0; i < wtx.tx->vout.size(); i++) context.wallet->LockCoin({wtx.GetHash(), i}, /*persist=*/false);
490490
for (const auto& [type, size] : expected_coins_sizes) BOOST_CHECK_EQUAL(size, available_coins.coins[type].size());
491491
}
492492

‎src/wallet/wallet.cpp

Lines changed: 31 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -785,30 +785,25 @@ bool CWallet::IsSpent(const COutPoint& outpoint) const
785785
return false;
786786
}
787787

788-
void CWallet::AddToSpends(const COutPoint& outpoint, const Txid& txid, WalletBatch* batch)
788+
void CWallet::AddToSpends(const COutPoint& outpoint, const Txid& txid)
789789
{
790790
mapTxSpends.insert(std::make_pair(outpoint, txid));
791791

792-
if (batch) {
793-
UnlockCoin(outpoint, batch);
794-
} else {
795-
WalletBatch temp_batch(GetDatabase());
796-
UnlockCoin(outpoint, &temp_batch);
797-
}
792+
UnlockCoin(outpoint);
798793

799794
std::pair<TxSpends::iterator, TxSpends::iterator> range;
800795
range = mapTxSpends.equal_range(outpoint);
801796
SyncMetaData(range);
802797
}
803798

804799

805-
void CWallet::AddToSpends(const CWalletTx& wtx, WalletBatch* batch)
800+
void CWallet::AddToSpends(const CWalletTx& wtx)
806801
{
807802
if (wtx.IsCoinBase()) // Coinbases don't spend anything!
808803
return;
809804

810805
for (const CTxIn& txin : wtx.tx->vin)
811-
AddToSpends(txin.prevout, wtx.GetHash(), batch);
806+
AddToSpends(txin.prevout, wtx.GetHash());
812807
}
813808

814809
bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
@@ -1058,7 +1053,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
10581053
wtx.nOrderPos = IncOrderPosNext(&batch);
10591054
wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
10601055
wtx.nTimeSmart = ComputeTimeSmart(wtx, rescanning_old_block);
1061-
AddToSpends(wtx, &batch);
1056+
AddToSpends(wtx);
10621057

10631058
// Update birth time when tx time is older than it.
10641059
MaybeUpdateBirthTime(wtx.GetTxTime());
@@ -2622,22 +2617,34 @@ util::Result<void> CWallet::DisplayAddress(const CTxDestination& dest)
26222617
return util::Error{_("There is no ScriptPubKeyManager for this address")};
26232618
}
26242619

2625-
bool CWallet::LockCoin(const COutPoint& output, WalletBatch* batch)
2620+
void CWallet::LoadLockedCoin(const COutPoint& coin, bool persistent)
26262621
{
26272622
AssertLockHeld(cs_wallet);
2628-
setLockedCoins.insert(output);
2629-
if (batch) {
2630-
return batch->WriteLockedUTXO(output);
2623+
m_locked_coins.emplace(coin, persistent);
2624+
}
2625+
2626+
bool CWallet::LockCoin(const COutPoint& output, bool persist)
2627+
{
2628+
AssertLockHeld(cs_wallet);
2629+
LoadLockedCoin(output, persist);
2630+
if (persist) {
2631+
WalletBatch batch(GetDatabase());
2632+
return batch.WriteLockedUTXO(output);
26312633
}
26322634
return true;
26332635
}
26342636

2635-
bool CWallet::UnlockCoin(const COutPoint& output, WalletBatch* batch)
2637+
bool CWallet::UnlockCoin(const COutPoint& output)
26362638
{
26372639
AssertLockHeld(cs_wallet);
2638-
bool was_locked = setLockedCoins.erase(output);
2639-
if (batch && was_locked) {
2640-
return batch->EraseLockedUTXO(output);
2640+
auto locked_coin_it = m_locked_coins.find(output);
2641+
if (locked_coin_it != m_locked_coins.end()) {
2642+
bool persisted = locked_coin_it->second;
2643+
m_locked_coins.erase(locked_coin_it);
2644+
if (persisted) {
2645+
WalletBatch batch(GetDatabase());
2646+
return batch.EraseLockedUTXO(output);
2647+
}
26412648
}
26422649
return true;
26432650
}
@@ -2647,26 +2654,24 @@ bool CWallet::UnlockAllCoins()
26472654
AssertLockHeld(cs_wallet);
26482655
bool success = true;
26492656
WalletBatch batch(GetDatabase());
2650-
for (auto it = setLockedCoins.begin(); it != setLockedCoins.end(); ++it) {
2651-
success &= batch.EraseLockedUTXO(*it);
2657+
for (const auto& [coin, _] : m_locked_coins) {
2658+
success &= batch.EraseLockedUTXO(coin);
26522659
}
2653-
setLockedCoins.clear();
2660+
m_locked_coins.clear();
26542661
return success;
26552662
}
26562663

26572664
bool CWallet::IsLockedCoin(const COutPoint& output) const
26582665
{
26592666
AssertLockHeld(cs_wallet);
2660-
return setLockedCoins.count(output) > 0;
2667+
return m_locked_coins.count(output) > 0;
26612668
}
26622669

26632670
void CWallet::ListLockedCoins(std::vector<COutPoint>& vOutpts) const
26642671
{
26652672
AssertLockHeld(cs_wallet);
2666-
for (std::set<COutPoint>::iterator it = setLockedCoins.begin();
2667-
it != setLockedCoins.end(); it++) {
2668-
COutPoint outpt = (*it);
2669-
vOutpts.push_back(outpt);
2673+
for (const auto& [coin, _] : m_locked_coins) {
2674+
vOutpts.push_back(coin);
26702675
}
26712676
}
26722677

‎src/wallet/wallet.h

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -333,8 +333,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
333333
*/
334334
typedef std::unordered_multimap<COutPoint, Txid, SaltedOutpointHasher> TxSpends;
335335
TxSpends mapTxSpends GUARDED_BY(cs_wallet);
336-
void AddToSpends(const COutPoint& outpoint, const Txid& txid, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
337-
void AddToSpends(const CWalletTx& wtx, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
336+
void AddToSpends(const COutPoint& outpoint, const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
337+
void AddToSpends(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
338338

339339
/**
340340
* Add a transaction to the wallet, or update it. confirm.block_* should
@@ -497,8 +497,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
497497
/** Set of Coins owned by this wallet that we won't try to spend from. A
498498
* Coin may be locked if it has already been used to fund a transaction
499499
* that hasn't confirmed yet. We wouldn't consider the Coin spent already,
500-
* but also shouldn't try to use it again. */
501-
std::set<COutPoint> setLockedCoins GUARDED_BY(cs_wallet);
500+
* but also shouldn't try to use it again.
501+
* bool to track whether this locked coin is persisted to disk.
502+
*/
503+
std::map<COutPoint, bool> m_locked_coins GUARDED_BY(cs_wallet);
502504

503505
/** Registered interfaces::Chain::Notifications handler. */
504506
std::unique_ptr<interfaces::Handler> m_chain_notifications_handler;
@@ -546,8 +548,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
546548
util::Result<void> DisplayAddress(const CTxDestination& dest) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
547549

548550
bool IsLockedCoin(const COutPoint& output) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
549-
bool LockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
550-
bool UnlockCoin(const COutPoint& output, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
551+
void LoadLockedCoin(const COutPoint& coin, bool persistent) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
552+
bool LockCoin(const COutPoint& output, bool persist) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
553+
bool UnlockCoin(const COutPoint& output) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
551554
bool UnlockAllCoins() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
552555
void ListLockedCoins(std::vector<COutPoint>& vOutpts) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
553556

‎src/wallet/walletdb.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1072,7 +1072,7 @@ static DBErrors LoadTxRecords(CWallet* pwallet, DatabaseBatch& batch, std::vecto
10721072
uint32_t n;
10731073
key >> hash;
10741074
key >> n;
1075-
pwallet->LockCoin(COutPoint(hash, n));
1075+
pwallet->LoadLockedCoin(COutPoint(hash, n), /*persistent=*/true);
10761076
return DBErrors::LOAD_OK;
10771077
});
10781078
result = std::max(result, locked_utxo_res.m_result);

0 commit comments

Comments
 (0)