Skip to content

Commit 6a72f26

Browse files
author
Antoine Riard
committed
[wallet] Remove locked_chain from CWallet, its RPCs and tests
This change is intended to make the bitcoin node and its rpc, network and gui interfaces more responsive while the wallet is in use. Currently because the node's cs_main mutex is always locked before the wallet's cs_wallet mutex (to prevent deadlocks), cs_main currently stays locked while the wallet does relatively slow things like creating and listing transactions. This commit only remmove chain lock tacking in wallet code, and invert lock order from cs_main, cs_wallet to cs_wallet, cs_main. must happen at once to avoid any deadlock. Previous commit were only removing Chain::Lock methods to Chain interface and enforcing they take cs_main. Remove LockChain method from CWallet and Chain::Lock interface.
1 parent 8411788 commit 6a72f26

File tree

11 files changed

+106
-201
lines changed

11 files changed

+106
-201
lines changed

‎src/interfaces/chain.cpp

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -53,11 +53,6 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<Rec
5353
return true;
5454
}
5555

56-
class LockImpl : public Chain::Lock, public UniqueLock<RecursiveMutex>
57-
{
58-
using UniqueLock::UniqueLock;
59-
};
60-
6156
class NotificationsProxy : public CValidationInterface
6257
{
6358
public:
@@ -150,13 +145,6 @@ class ChainImpl : public Chain
150145
{
151146
public:
152147
explicit ChainImpl(NodeContext& node) : m_node(node) {}
153-
std::unique_ptr<Chain::Lock> lock(bool try_lock) override
154-
{
155-
auto lock = MakeUnique<LockImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock);
156-
if (try_lock && lock && !*lock) return {};
157-
std::unique_ptr<Chain::Lock> result = std::move(lock); // Temporary to avoid CWG 1579
158-
return result;
159-
}
160148
Optional<int> getHeight() override
161149
{
162150
LOCK(::cs_main);

‎src/interfaces/chain.h

Lines changed: 9 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -59,39 +59,27 @@ class FoundBlock
5959
//! internal workings of the bitcoin node, and not being very convenient to use.
6060
//! Chain methods should be cleaned up and simplified over time. Examples:
6161
//!
62-
//! * The Chain::lock() method, which lets clients delay chain tip updates
63-
//! should be removed when clients are able to respond to updates
64-
//! asynchronously
65-
//! (https://github.com/bitcoin/bitcoin/pull/10973#issuecomment-380101269).
66-
//!
67-
//! * The initMessage() and showProgress() methods which the wallet uses to send
62+
//! * The initMessages() and showProgress() methods which the wallet uses to send
6863
//! notifications to the GUI should go away when GUI and wallet can directly
6964
//! communicate with each other without going through the node
7065
//! (https://github.com/bitcoin/bitcoin/pull/15288#discussion_r253321096).
7166
//!
7267
//! * The handleRpc, registerRpcs, rpcEnableDeprecated methods and other RPC
7368
//! methods can go away if wallets listen for HTTP requests on their own
7469
//! ports instead of registering to handle requests on the node HTTP port.
70+
//!
71+
//! * Move fee estimation queries to an asynchronous interface and let the
72+
//! wallet cache it, fee estimation being driven by node mempool, wallet
73+
//! should be the consumer.
74+
//!
75+
//! * The `guessVerificationProgress`, `getBlockHeight`, `getBlockHash`, etc
76+
//! methods can go away if rescan logic is moved on the node side, and wallet
77+
//! only register rescan request.
7578
class Chain
7679
{
7780
public:
7881
virtual ~Chain() {}
7982

80-
//! Interface for querying locked chain state, used by legacy code that
81-
//! assumes state won't change between calls. New code should avoid using
82-
//! the Lock interface and instead call higher-level Chain methods
83-
//! that return more information so the chain doesn't need to stay locked
84-
//! between calls.
85-
class Lock
86-
{
87-
public:
88-
virtual ~Lock() {}
89-
};
90-
91-
//! Return Lock interface. Chain is locked when this is called, and
92-
//! unlocked when the returned interface is freed.
93-
virtual std::unique_ptr<Lock> lock(bool try_lock = false) = 0;
94-
9583
//! Get current chain height, not including genesis block (returns 0 if
9684
//! chain only contains genesis block, nullopt if chain does not contain
9785
//! any blocks)

‎src/interfaces/wallet.cpp

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -196,25 +196,21 @@ class WalletImpl : public Wallet
196196
}
197197
void lockCoin(const COutPoint& output) override
198198
{
199-
auto locked_chain = m_wallet->chain().lock();
200199
LOCK(m_wallet->cs_wallet);
201200
return m_wallet->LockCoin(output);
202201
}
203202
void unlockCoin(const COutPoint& output) override
204203
{
205-
auto locked_chain = m_wallet->chain().lock();
206204
LOCK(m_wallet->cs_wallet);
207205
return m_wallet->UnlockCoin(output);
208206
}
209207
bool isLockedCoin(const COutPoint& output) override
210208
{
211-
auto locked_chain = m_wallet->chain().lock();
212209
LOCK(m_wallet->cs_wallet);
213210
return m_wallet->IsLockedCoin(output.hash, output.n);
214211
}
215212
void listLockedCoins(std::vector<COutPoint>& outputs) override
216213
{
217-
auto locked_chain = m_wallet->chain().lock();
218214
LOCK(m_wallet->cs_wallet);
219215
return m_wallet->ListLockedCoins(outputs);
220216
}
@@ -225,7 +221,6 @@ class WalletImpl : public Wallet
225221
CAmount& fee,
226222
std::string& fail_reason) override
227223
{
228-
auto locked_chain = m_wallet->chain().lock();
229224
LOCK(m_wallet->cs_wallet);
230225
CTransactionRef tx;
231226
if (!m_wallet->CreateTransaction(recipients, tx, fee, change_pos,
@@ -238,14 +233,12 @@ class WalletImpl : public Wallet
238233
WalletValueMap value_map,
239234
WalletOrderForm order_form) override
240235
{
241-
auto locked_chain = m_wallet->chain().lock();
242236
LOCK(m_wallet->cs_wallet);
243237
m_wallet->CommitTransaction(std::move(tx), std::move(value_map), std::move(order_form));
244238
}
245239
bool transactionCanBeAbandoned(const uint256& txid) override { return m_wallet->TransactionCanBeAbandoned(txid); }
246240
bool abandonTransaction(const uint256& txid) override
247241
{
248-
auto locked_chain = m_wallet->chain().lock();
249242
LOCK(m_wallet->cs_wallet);
250243
return m_wallet->AbandonTransaction(txid);
251244
}
@@ -273,7 +266,6 @@ class WalletImpl : public Wallet
273266
}
274267
CTransactionRef getTx(const uint256& txid) override
275268
{
276-
auto locked_chain = m_wallet->chain().lock();
277269
LOCK(m_wallet->cs_wallet);
278270
auto mi = m_wallet->mapWallet.find(txid);
279271
if (mi != m_wallet->mapWallet.end()) {
@@ -283,7 +275,6 @@ class WalletImpl : public Wallet
283275
}
284276
WalletTx getWalletTx(const uint256& txid) override
285277
{
286-
auto locked_chain = m_wallet->chain().lock();
287278
LOCK(m_wallet->cs_wallet);
288279
auto mi = m_wallet->mapWallet.find(txid);
289280
if (mi != m_wallet->mapWallet.end()) {
@@ -293,7 +284,6 @@ class WalletImpl : public Wallet
293284
}
294285
std::vector<WalletTx> getWalletTxs() override
295286
{
296-
auto locked_chain = m_wallet->chain().lock();
297287
LOCK(m_wallet->cs_wallet);
298288
std::vector<WalletTx> result;
299289
result.reserve(m_wallet->mapWallet.size());
@@ -307,10 +297,6 @@ class WalletImpl : public Wallet
307297
int& num_blocks,
308298
int64_t& block_time) override
309299
{
310-
auto locked_chain = m_wallet->chain().lock(true /* try_lock */);
311-
if (!locked_chain) {
312-
return false;
313-
}
314300
TRY_LOCK(m_wallet->cs_wallet, locked_wallet);
315301
if (!locked_wallet) {
316302
return false;
@@ -331,7 +317,6 @@ class WalletImpl : public Wallet
331317
bool& in_mempool,
332318
int& num_blocks) override
333319
{
334-
auto locked_chain = m_wallet->chain().lock();
335320
LOCK(m_wallet->cs_wallet);
336321
auto mi = m_wallet->mapWallet.find(txid);
337322
if (mi != m_wallet->mapWallet.end()) {
@@ -368,8 +353,6 @@ class WalletImpl : public Wallet
368353
}
369354
bool tryGetBalances(WalletBalances& balances, int& num_blocks, bool force, int cached_num_blocks) override
370355
{
371-
auto locked_chain = m_wallet->chain().lock(true /* try_lock */);
372-
if (!locked_chain) return false;
373356
TRY_LOCK(m_wallet->cs_wallet, locked_wallet);
374357
if (!locked_wallet) {
375358
return false;
@@ -386,31 +369,26 @@ class WalletImpl : public Wallet
386369
}
387370
isminetype txinIsMine(const CTxIn& txin) override
388371
{
389-
auto locked_chain = m_wallet->chain().lock();
390372
LOCK(m_wallet->cs_wallet);
391373
return m_wallet->IsMine(txin);
392374
}
393375
isminetype txoutIsMine(const CTxOut& txout) override
394376
{
395-
auto locked_chain = m_wallet->chain().lock();
396377
LOCK(m_wallet->cs_wallet);
397378
return m_wallet->IsMine(txout);
398379
}
399380
CAmount getDebit(const CTxIn& txin, isminefilter filter) override
400381
{
401-
auto locked_chain = m_wallet->chain().lock();
402382
LOCK(m_wallet->cs_wallet);
403383
return m_wallet->GetDebit(txin, filter);
404384
}
405385
CAmount getCredit(const CTxOut& txout, isminefilter filter) override
406386
{
407-
auto locked_chain = m_wallet->chain().lock();
408387
LOCK(m_wallet->cs_wallet);
409388
return m_wallet->GetCredit(txout, filter);
410389
}
411390
CoinsList listCoins() override
412391
{
413-
auto locked_chain = m_wallet->chain().lock();
414392
LOCK(m_wallet->cs_wallet);
415393
CoinsList result;
416394
for (const auto& entry : m_wallet->ListCoins()) {
@@ -424,7 +402,6 @@ class WalletImpl : public Wallet
424402
}
425403
std::vector<WalletTxOut> getCoins(const std::vector<COutPoint>& outputs) override
426404
{
427-
auto locked_chain = m_wallet->chain().lock();
428405
LOCK(m_wallet->cs_wallet);
429406
std::vector<WalletTxOut> result;
430407
result.reserve(outputs.size());
@@ -496,6 +473,7 @@ class WalletImpl : public Wallet
496473
{
497474
return MakeHandler(m_wallet->NotifyCanGetAddressesChanged.connect(fn));
498475
}
476+
CWallet* wallet() override { return m_wallet.get(); }
499477

500478
std::shared_ptr<CWallet> m_wallet;
501479
};

‎src/interfaces/wallet.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,9 @@ class Wallet
300300
//! Register handler for keypool changed messages.
301301
using CanGetAddressesChangedFn = std::function<void()>;
302302
virtual std::unique_ptr<Handler> handleCanGetAddressesChanged(CanGetAddressesChangedFn fn) = 0;
303+
304+
//! Return pointer to internal wallet class, useful for testing.
305+
virtual CWallet* wallet() { return nullptr; }
303306
};
304307

305308
//! Information about one wallet address.

‎src/qt/test/wallettests.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,6 @@ void TestGUI(interfaces::Node& node)
145145
wallet->LoadWallet(firstRun);
146146
{
147147
auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan();
148-
auto locked_chain = wallet->chain().lock();
149148
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());

‎src/wallet/feebumper.cpp

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,6 @@ namespace feebumper {
140140

141141
bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid)
142142
{
143-
auto locked_chain = wallet.chain().lock();
144143
LOCK(wallet.cs_wallet);
145144
const CWalletTx* wtx = wallet.GetWalletTx(txid);
146145
if (wtx == nullptr) return false;
@@ -156,7 +155,6 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
156155
// We are going to modify coin control later, copy to re-use
157156
CCoinControl new_coin_control(coin_control);
158157

159-
auto locked_chain = wallet.chain().lock();
160158
LOCK(wallet.cs_wallet);
161159
errors.clear();
162160
auto it = wallet.mapWallet.find(txid);
@@ -240,14 +238,12 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
240238
}
241239

242240
bool SignTransaction(CWallet& wallet, CMutableTransaction& mtx) {
243-
auto locked_chain = wallet.chain().lock();
244241
LOCK(wallet.cs_wallet);
245242
return wallet.SignTransaction(mtx);
246243
}
247244

248245
Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, uint256& bumped_txid)
249246
{
250-
auto locked_chain = wallet.chain().lock();
251247
LOCK(wallet.cs_wallet);
252248
if (!errors.empty()) {
253249
return Result::MISC_ERROR;

‎src/wallet/rpcdump.cpp

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,6 @@ UniValue importprivkey(const JSONRPCRequest& request)
133133
WalletRescanReserver reserver(*pwallet);
134134
bool fRescan = true;
135135
{
136-
auto locked_chain = pwallet->chain().lock();
137136
LOCK(pwallet->cs_wallet);
138137

139138
EnsureWalletIsUnlocked(pwallet);
@@ -285,7 +284,6 @@ UniValue importaddress(const JSONRPCRequest& request)
285284
fP2SH = request.params[3].get_bool();
286285

287286
{
288-
auto locked_chain = pwallet->chain().lock();
289287
LOCK(pwallet->cs_wallet);
290288

291289
CTxDestination dest = DecodeDestination(request.params[0].get_str());
@@ -317,7 +315,6 @@ UniValue importaddress(const JSONRPCRequest& request)
317315
{
318316
RescanWallet(*pwallet, reserver);
319317
{
320-
auto locked_chain = pwallet->chain().lock();
321318
LOCK(pwallet->cs_wallet);
322319
pwallet->ReacceptWalletTransactions();
323320
}
@@ -361,7 +358,6 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
361358
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Something wrong with merkleblock");
362359
}
363360

364-
auto locked_chain = pwallet->chain().lock();
365361
LOCK(pwallet->cs_wallet);
366362
int height;
367363
if (!pwallet->chain().findAncestorByHash(pwallet->GetLastBlockHash(), merkleBlock.header.GetHash(), FoundBlock().height(height))) {
@@ -407,7 +403,6 @@ UniValue removeprunedfunds(const JSONRPCRequest& request)
407403
},
408404
}.Check(request);
409405

410-
auto locked_chain = pwallet->chain().lock();
411406
LOCK(pwallet->cs_wallet);
412407

413408
uint256 hash(ParseHashV(request.params[0], "txid"));
@@ -487,7 +482,6 @@ UniValue importpubkey(const JSONRPCRequest& request)
487482
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey is not a valid public key");
488483

489484
{
490-
auto locked_chain = pwallet->chain().lock();
491485
LOCK(pwallet->cs_wallet);
492486

493487
std::set<CScript> script_pub_keys;
@@ -505,7 +499,6 @@ UniValue importpubkey(const JSONRPCRequest& request)
505499
{
506500
RescanWallet(*pwallet, reserver);
507501
{
508-
auto locked_chain = pwallet->chain().lock();
509502
LOCK(pwallet->cs_wallet);
510503
pwallet->ReacceptWalletTransactions();
511504
}
@@ -557,7 +550,6 @@ UniValue importwallet(const JSONRPCRequest& request)
557550
int64_t nTimeBegin = 0;
558551
bool fGood = true;
559552
{
560-
auto locked_chain = pwallet->chain().lock();
561553
LOCK(pwallet->cs_wallet);
562554

563555
EnsureWalletIsUnlocked(pwallet);
@@ -700,7 +692,6 @@ UniValue dumpprivkey(const JSONRPCRequest& request)
700692

701693
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet);
702694

703-
auto locked_chain = pwallet->chain().lock();
704695
LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore);
705696

706697
EnsureWalletIsUnlocked(pwallet);
@@ -756,7 +747,6 @@ UniValue dumpwallet(const JSONRPCRequest& request)
756747
// the user could have gotten from another RPC command prior to now
757748
wallet.BlockUntilSyncedToCurrentChain();
758749

759-
auto locked_chain = pwallet->chain().lock();
760750
LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore);
761751

762752
EnsureWalletIsUnlocked(&wallet);
@@ -780,7 +770,7 @@ UniValue dumpwallet(const JSONRPCRequest& request)
780770

781771
std::map<CKeyID, int64_t> mapKeyBirth;
782772
const std::map<CKeyID, int64_t>& mapKeyPool = spk_man.GetAllReserveKeys();
783-
pwallet->GetKeyBirthTimes(*locked_chain, mapKeyBirth);
773+
pwallet->GetKeyBirthTimes(mapKeyBirth);
784774

785775
std::set<CScriptID> scripts = spk_man.GetCScripts();
786776

@@ -1379,7 +1369,6 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
13791369
int64_t nLowestTimestamp = 0;
13801370
UniValue response(UniValue::VARR);
13811371
{
1382-
auto locked_chain = pwallet->chain().lock();
13831372
LOCK(pwallet->cs_wallet);
13841373
EnsureWalletIsUnlocked(pwallet);
13851374

@@ -1414,7 +1403,6 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
14141403
if (fRescan && fRunScan && requests.size()) {
14151404
int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, reserver, true /* update */);
14161405
{
1417-
auto locked_chain = pwallet->chain().lock();
14181406
LOCK(pwallet->cs_wallet);
14191407
pwallet->ReacceptWalletTransactions();
14201408
}
@@ -1676,7 +1664,6 @@ UniValue importdescriptors(const JSONRPCRequest& main_request) {
16761664
bool rescan = false;
16771665
UniValue response(UniValue::VARR);
16781666
{
1679-
auto locked_chain = pwallet->chain().lock();
16801667
LOCK(pwallet->cs_wallet);
16811668
EnsureWalletIsUnlocked(pwallet);
16821669

@@ -1705,7 +1692,6 @@ UniValue importdescriptors(const JSONRPCRequest& main_request) {
17051692
if (rescan) {
17061693
int64_t scanned_time = pwallet->RescanFromTime(lowest_timestamp, reserver, true /* update */);
17071694
{
1708-
auto locked_chain = pwallet->chain().lock();
17091695
LOCK(pwallet->cs_wallet);
17101696
pwallet->ReacceptWalletTransactions();
17111697
}

0 commit comments

Comments
 (0)