Skip to content

Commit 608359b

Browse files
author
MarcoFalke
committed
Merge #16426: Reverse cs_main, cs_wallet lock order and reduce cs_main locking
6a72f26 [wallet] Remove locked_chain from CWallet, its RPCs and tests (Antoine Riard) 8411788 [wallet] Move methods from Chain::Lock interface to simple Chain (Antoine Riard) 0a76287 [wallet] Move getBlockHash from Chain::Lock interface to simple Chain (Antoine Riard) de13363 [wallet] Move getBlockHeight from Chain::Lock interface to simple Chain (Antoine Riard) b855592 [wallet] Move getHeight from Chain::Lock interface to simple Chain (Antoine Riard) Pull request description: 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. Switching the lock order so `cs_main` is acquired after `cs_wallet` allows `cs_main` to be only locked intermittently while the wallet is doing slow operations, so the node is not blocked waiting for the wallet. To review the present PR, most of getting right the move is ensuring any `LockAssertion` in `Chain::Lock` method is amended as a `LOCK(cs_main)`. And in final commit, check that any wallet code which was previously locking the chain is now calling a method, enforcing the lock taking job. So far the only exception I found is `handleNotifications`, which should be corrected. ACKs for top commit: MarcoFalke: re-ACK 6a72f26 🔏 fjahr: re-ACK 6a72f26 ryanofsky: Code review ACK 6a72f26. Only difference compared to the rebase I posted is reverting unneeded SetLastBlockProcessed change in wallet_disableprivkeys test Tree-SHA512: 9168b3bf3432d4f8bc4d9fa9246ac057050848e673efc264c8f44345f243ba9697b05c22c809a79d1b51bf0de1c4ed317960e496480f8d71e584468d4dd1b0ad
2 parents e5b9308 + 6a72f26 commit 608359b

File tree

11 files changed

+257
-379
lines changed

11 files changed

+257
-379
lines changed

‎src/interfaces/chain.cpp

Lines changed: 57 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -53,88 +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-
Optional<int> getHeight() override
59-
{
60-
LockAssertion lock(::cs_main);
61-
int height = ::ChainActive().Height();
62-
if (height >= 0) {
63-
return height;
64-
}
65-
return nullopt;
66-
}
67-
Optional<int> getBlockHeight(const uint256& hash) override
68-
{
69-
LockAssertion lock(::cs_main);
70-
CBlockIndex* block = LookupBlockIndex(hash);
71-
if (block && ::ChainActive().Contains(block)) {
72-
return block->nHeight;
73-
}
74-
return nullopt;
75-
}
76-
uint256 getBlockHash(int height) override
77-
{
78-
LockAssertion lock(::cs_main);
79-
CBlockIndex* block = ::ChainActive()[height];
80-
assert(block != nullptr);
81-
return block->GetBlockHash();
82-
}
83-
bool haveBlockOnDisk(int height) override
84-
{
85-
LockAssertion lock(::cs_main);
86-
CBlockIndex* block = ::ChainActive()[height];
87-
return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0;
88-
}
89-
Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) override
90-
{
91-
LockAssertion lock(::cs_main);
92-
CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height);
93-
if (block) {
94-
if (hash) *hash = block->GetBlockHash();
95-
return block->nHeight;
96-
}
97-
return nullopt;
98-
}
99-
Optional<int> findFork(const uint256& hash, Optional<int>* height) override
100-
{
101-
LockAssertion lock(::cs_main);
102-
const CBlockIndex* block = LookupBlockIndex(hash);
103-
const CBlockIndex* fork = block ? ::ChainActive().FindFork(block) : nullptr;
104-
if (height) {
105-
if (block) {
106-
*height = block->nHeight;
107-
} else {
108-
height->reset();
109-
}
110-
}
111-
if (fork) {
112-
return fork->nHeight;
113-
}
114-
return nullopt;
115-
}
116-
CBlockLocator getTipLocator() override
117-
{
118-
LockAssertion lock(::cs_main);
119-
return ::ChainActive().GetLocator();
120-
}
121-
Optional<int> findLocatorFork(const CBlockLocator& locator) override
122-
{
123-
LockAssertion lock(::cs_main);
124-
if (CBlockIndex* fork = FindForkInGlobalIndex(::ChainActive(), locator)) {
125-
return fork->nHeight;
126-
}
127-
return nullopt;
128-
}
129-
bool checkFinalTx(const CTransaction& tx) override
130-
{
131-
LockAssertion lock(::cs_main);
132-
return CheckFinalTx(tx);
133-
}
134-
135-
using UniqueLock::UniqueLock;
136-
};
137-
13856
class NotificationsProxy : public CValidationInterface
13957
{
14058
public:
@@ -227,12 +145,64 @@ class ChainImpl : public Chain
227145
{
228146
public:
229147
explicit ChainImpl(NodeContext& node) : m_node(node) {}
230-
std::unique_ptr<Chain::Lock> lock(bool try_lock) override
148+
Optional<int> getHeight() override
149+
{
150+
LOCK(::cs_main);
151+
int height = ::ChainActive().Height();
152+
if (height >= 0) {
153+
return height;
154+
}
155+
return nullopt;
156+
}
157+
Optional<int> getBlockHeight(const uint256& hash) override
158+
{
159+
LOCK(::cs_main);
160+
CBlockIndex* block = LookupBlockIndex(hash);
161+
if (block && ::ChainActive().Contains(block)) {
162+
return block->nHeight;
163+
}
164+
return nullopt;
165+
}
166+
uint256 getBlockHash(int height) override
167+
{
168+
LOCK(::cs_main);
169+
CBlockIndex* block = ::ChainActive()[height];
170+
assert(block);
171+
return block->GetBlockHash();
172+
}
173+
bool haveBlockOnDisk(int height) override
174+
{
175+
LOCK(cs_main);
176+
CBlockIndex* block = ::ChainActive()[height];
177+
return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0;
178+
}
179+
Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) override
231180
{
232-
auto lock = MakeUnique<LockImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock);
233-
if (try_lock && lock && !*lock) return {};
234-
std::unique_ptr<Chain::Lock> result = std::move(lock); // Temporary to avoid CWG 1579
235-
return result;
181+
LOCK(cs_main);
182+
CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height);
183+
if (block) {
184+
if (hash) *hash = block->GetBlockHash();
185+
return block->nHeight;
186+
}
187+
return nullopt;
188+
}
189+
CBlockLocator getTipLocator() override
190+
{
191+
LOCK(cs_main);
192+
return ::ChainActive().GetLocator();
193+
}
194+
bool checkFinalTx(const CTransaction& tx) override
195+
{
196+
LOCK(cs_main);
197+
return CheckFinalTx(tx);
198+
}
199+
Optional<int> findLocatorFork(const CBlockLocator& locator) override
200+
{
201+
LOCK(cs_main);
202+
if (CBlockIndex* fork = FindForkInGlobalIndex(::ChainActive(), locator)) {
203+
return fork->nHeight;
204+
}
205+
return nullopt;
236206
}
237207
bool findBlock(const uint256& hash, const FoundBlock& block) override
238208
{

‎src/interfaces/chain.h

Lines changed: 42 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -59,81 +59,61 @@ 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-
//! Get current chain height, not including genesis block (returns 0 if
91-
//! chain only contains genesis block, nullopt if chain does not contain
92-
//! any blocks).
93-
virtual Optional<int> getHeight() = 0;
94-
95-
//! Get block height above genesis block. Returns 0 for genesis block,
96-
//! 1 for following block, and so on. Returns nullopt for a block not
97-
//! included in the current chain.
98-
virtual Optional<int> getBlockHeight(const uint256& hash) = 0;
99-
100-
//! Get block hash. Height must be valid or this function will abort.
101-
virtual uint256 getBlockHash(int height) = 0;
102-
103-
//! Check that the block is available on disk (i.e. has not been
104-
//! pruned), and contains transactions.
105-
virtual bool haveBlockOnDisk(int height) = 0;
106-
107-
//! Return height of the first block in the chain with timestamp equal
108-
//! or greater than the given time and height equal or greater than the
109-
//! given height, or nullopt if there is no block with a high enough
110-
//! timestamp and height. Also return the block hash as an optional output parameter
111-
//! (to avoid the cost of a second lookup in case this information is needed.)
112-
virtual Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) = 0;
113-
114-
//! Return height of the specified block if it is on the chain, otherwise
115-
//! return the height of the highest block on chain that's an ancestor
116-
//! of the specified block, or nullopt if there is no common ancestor.
117-
//! Also return the height of the specified block as an optional output
118-
//! parameter (to avoid the cost of a second hash lookup in case this
119-
//! information is desired).
120-
virtual Optional<int> findFork(const uint256& hash, Optional<int>* height) = 0;
121-
122-
//! Get locator for the current chain tip.
123-
virtual CBlockLocator getTipLocator() = 0;
124-
125-
//! Return height of the highest block on chain in common with the locator,
126-
//! which will either be the original block used to create the locator,
127-
//! or one of its ancestors.
128-
virtual Optional<int> findLocatorFork(const CBlockLocator& locator) = 0;
129-
130-
//! Check if transaction will be final given chain height current time.
131-
virtual bool checkFinalTx(const CTransaction& tx) = 0;
132-
};
83+
//! Get current chain height, not including genesis block (returns 0 if
84+
//! chain only contains genesis block, nullopt if chain does not contain
85+
//! any blocks)
86+
virtual Optional<int> getHeight() = 0;
87+
88+
//! Get block height above genesis block. Returns 0 for genesis block,
89+
//! 1 for following block, and so on. Returns nullopt for a block not
90+
//! included in the current chain.
91+
virtual Optional<int> getBlockHeight(const uint256& hash) = 0;
92+
93+
//! Get block hash. Height must be valid or this function will abort.
94+
virtual uint256 getBlockHash(int height) = 0;
95+
96+
//! Check that the block is available on disk (i.e. has not been
97+
//! pruned), and contains transactions.
98+
virtual bool haveBlockOnDisk(int height) = 0;
99+
100+
//! Return height of the first block in the chain with timestamp equal
101+
//! or greater than the given time and height equal or greater than the
102+
//! given height, or nullopt if there is no block with a high enough
103+
//! timestamp and height. Also return the block hash as an optional output parameter
104+
//! (to avoid the cost of a second lookup in case this information is needed.)
105+
virtual Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) = 0;
106+
107+
//! Get locator for the current chain tip.
108+
virtual CBlockLocator getTipLocator() = 0;
109+
110+
//! Return height of the highest block on chain in common with the locator,
111+
//! which will either be the original block used to create the locator,
112+
//! or one of its ancestors.
113+
virtual Optional<int> findLocatorFork(const CBlockLocator& locator) = 0;
133114

134-
//! Return Lock interface. Chain is locked when this is called, and
135-
//! unlocked when the returned interface is freed.
136-
virtual std::unique_ptr<Lock> lock(bool try_lock = false) = 0;
115+
//! Check if transaction will be final given chain height current time.
116+
virtual bool checkFinalTx(const CTransaction& tx) = 0;
137117

138118
//! Return whether node has the block and optionally return block metadata
139119
//! or contents.

0 commit comments

Comments
 (0)