Skip to content

Commit

Permalink
Merge bitcoin#16426: Reverse cs_main, cs_wallet lock order and reduce…
Browse files Browse the repository at this point in the history 
… 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
  • Loading branch information
MarcoFalke committed May 1, 2020
2 parents e5b9308 + 6a72f26 commit 608359b
Show file tree
Hide file tree
Showing 11 changed files with 257 additions and 379 deletions.
144 changes: 57 additions & 87 deletions src/interfaces/chain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,88 +53,6 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<Rec
return true;
}

class LockImpl : public Chain::Lock, public UniqueLock<RecursiveMutex>
{
Optional<int> getHeight() override
{
LockAssertion lock(::cs_main);
int height = ::ChainActive().Height();
if (height >= 0) {
return height;
}
return nullopt;
}
Optional<int> getBlockHeight(const uint256& hash) override
{
LockAssertion lock(::cs_main);
CBlockIndex* block = LookupBlockIndex(hash);
if (block && ::ChainActive().Contains(block)) {
return block->nHeight;
}
return nullopt;
}
uint256 getBlockHash(int height) override
{
LockAssertion lock(::cs_main);
CBlockIndex* block = ::ChainActive()[height];
assert(block != nullptr);
return block->GetBlockHash();
}
bool haveBlockOnDisk(int height) override
{
LockAssertion lock(::cs_main);
CBlockIndex* block = ::ChainActive()[height];
return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0;
}
Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) override
{
LockAssertion lock(::cs_main);
CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height);
if (block) {
if (hash) *hash = block->GetBlockHash();
return block->nHeight;
}
return nullopt;
}
Optional<int> findFork(const uint256& hash, Optional<int>* height) override
{
LockAssertion lock(::cs_main);
const CBlockIndex* block = LookupBlockIndex(hash);
const CBlockIndex* fork = block ? ::ChainActive().FindFork(block) : nullptr;
if (height) {
if (block) {
*height = block->nHeight;
} else {
height->reset();
}
}
if (fork) {
return fork->nHeight;
}
return nullopt;
}
CBlockLocator getTipLocator() override
{
LockAssertion lock(::cs_main);
return ::ChainActive().GetLocator();
}
Optional<int> findLocatorFork(const CBlockLocator& locator) override
{
LockAssertion lock(::cs_main);
if (CBlockIndex* fork = FindForkInGlobalIndex(::ChainActive(), locator)) {
return fork->nHeight;
}
return nullopt;
}
bool checkFinalTx(const CTransaction& tx) override
{
LockAssertion lock(::cs_main);
return CheckFinalTx(tx);
}

using UniqueLock::UniqueLock;
};

class NotificationsProxy : public CValidationInterface
{
public:
Expand Down Expand Up @@ -227,12 +145,64 @@ class ChainImpl : public Chain
{
public:
explicit ChainImpl(NodeContext& node) : m_node(node) {}
std::unique_ptr<Chain::Lock> lock(bool try_lock) override
Optional<int> getHeight() override
{
LOCK(::cs_main);
int height = ::ChainActive().Height();
if (height >= 0) {
return height;
}
return nullopt;
}
Optional<int> getBlockHeight(const uint256& hash) override
{
LOCK(::cs_main);
CBlockIndex* block = LookupBlockIndex(hash);
if (block && ::ChainActive().Contains(block)) {
return block->nHeight;
}
return nullopt;
}
uint256 getBlockHash(int height) override
{
LOCK(::cs_main);
CBlockIndex* block = ::ChainActive()[height];
assert(block);
return block->GetBlockHash();
}
bool haveBlockOnDisk(int height) override
{
LOCK(cs_main);
CBlockIndex* block = ::ChainActive()[height];
return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0;
}
Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) override
{
auto lock = MakeUnique<LockImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock);
if (try_lock && lock && !*lock) return {};
std::unique_ptr<Chain::Lock> result = std::move(lock); // Temporary to avoid CWG 1579
return result;
LOCK(cs_main);
CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height);
if (block) {
if (hash) *hash = block->GetBlockHash();
return block->nHeight;
}
return nullopt;
}
CBlockLocator getTipLocator() override
{
LOCK(cs_main);
return ::ChainActive().GetLocator();
}
bool checkFinalTx(const CTransaction& tx) override
{
LOCK(cs_main);
return CheckFinalTx(tx);
}
Optional<int> findLocatorFork(const CBlockLocator& locator) override
{
LOCK(cs_main);
if (CBlockIndex* fork = FindForkInGlobalIndex(::ChainActive(), locator)) {
return fork->nHeight;
}
return nullopt;
}
bool findBlock(const uint256& hash, const FoundBlock& block) override
{
Expand Down
104 changes: 42 additions & 62 deletions src/interfaces/chain.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,81 +59,61 @@ class FoundBlock
//! internal workings of the bitcoin node, and not being very convenient to use.
//! Chain methods should be cleaned up and simplified over time. Examples:
//!
//! * The Chain::lock() method, which lets clients delay chain tip updates
//! should be removed when clients are able to respond to updates
//! asynchronously
//! (https://github.com/bitcoin/bitcoin/pull/10973#issuecomment-380101269).
//!
//! * The initMessage() and showProgress() methods which the wallet uses to send
//! * The initMessages() and showProgress() methods which the wallet uses to send
//! notifications to the GUI should go away when GUI and wallet can directly
//! communicate with each other without going through the node
//! (https://github.com/bitcoin/bitcoin/pull/15288#discussion_r253321096).
//!
//! * The handleRpc, registerRpcs, rpcEnableDeprecated methods and other RPC
//! methods can go away if wallets listen for HTTP requests on their own
//! ports instead of registering to handle requests on the node HTTP port.
//!
//! * Move fee estimation queries to an asynchronous interface and let the
//! wallet cache it, fee estimation being driven by node mempool, wallet
//! should be the consumer.
//!
//! * The `guessVerificationProgress`, `getBlockHeight`, `getBlockHash`, etc
//! methods can go away if rescan logic is moved on the node side, and wallet
//! only register rescan request.
class Chain
{
public:
virtual ~Chain() {}

//! Interface for querying locked chain state, used by legacy code that
//! assumes state won't change between calls. New code should avoid using
//! the Lock interface and instead call higher-level Chain methods
//! that return more information so the chain doesn't need to stay locked
//! between calls.
class Lock
{
public:
virtual ~Lock() {}

//! Get current chain height, not including genesis block (returns 0 if
//! chain only contains genesis block, nullopt if chain does not contain
//! any blocks).
virtual Optional<int> getHeight() = 0;

//! Get block height above genesis block. Returns 0 for genesis block,
//! 1 for following block, and so on. Returns nullopt for a block not
//! included in the current chain.
virtual Optional<int> getBlockHeight(const uint256& hash) = 0;

//! Get block hash. Height must be valid or this function will abort.
virtual uint256 getBlockHash(int height) = 0;

//! Check that the block is available on disk (i.e. has not been
//! pruned), and contains transactions.
virtual bool haveBlockOnDisk(int height) = 0;

//! Return height of the first block in the chain with timestamp equal
//! or greater than the given time and height equal or greater than the
//! given height, or nullopt if there is no block with a high enough
//! timestamp and height. Also return the block hash as an optional output parameter
//! (to avoid the cost of a second lookup in case this information is needed.)
virtual Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) = 0;

//! Return height of the specified block if it is on the chain, otherwise
//! return the height of the highest block on chain that's an ancestor
//! of the specified block, or nullopt if there is no common ancestor.
//! Also return the height of the specified block as an optional output
//! parameter (to avoid the cost of a second hash lookup in case this
//! information is desired).
virtual Optional<int> findFork(const uint256& hash, Optional<int>* height) = 0;

//! Get locator for the current chain tip.
virtual CBlockLocator getTipLocator() = 0;

//! Return height of the highest block on chain in common with the locator,
//! which will either be the original block used to create the locator,
//! or one of its ancestors.
virtual Optional<int> findLocatorFork(const CBlockLocator& locator) = 0;

//! Check if transaction will be final given chain height current time.
virtual bool checkFinalTx(const CTransaction& tx) = 0;
};
//! Get current chain height, not including genesis block (returns 0 if
//! chain only contains genesis block, nullopt if chain does not contain
//! any blocks)
virtual Optional<int> getHeight() = 0;

//! Get block height above genesis block. Returns 0 for genesis block,
//! 1 for following block, and so on. Returns nullopt for a block not
//! included in the current chain.
virtual Optional<int> getBlockHeight(const uint256& hash) = 0;

//! Get block hash. Height must be valid or this function will abort.
virtual uint256 getBlockHash(int height) = 0;

//! Check that the block is available on disk (i.e. has not been
//! pruned), and contains transactions.
virtual bool haveBlockOnDisk(int height) = 0;

//! Return height of the first block in the chain with timestamp equal
//! or greater than the given time and height equal or greater than the
//! given height, or nullopt if there is no block with a high enough
//! timestamp and height. Also return the block hash as an optional output parameter
//! (to avoid the cost of a second lookup in case this information is needed.)
virtual Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) = 0;

//! Get locator for the current chain tip.
virtual CBlockLocator getTipLocator() = 0;

//! Return height of the highest block on chain in common with the locator,
//! which will either be the original block used to create the locator,
//! or one of its ancestors.
virtual Optional<int> findLocatorFork(const CBlockLocator& locator) = 0;

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

//! Return whether node has the block and optionally return block metadata
//! or contents.
Expand Down
Loading

0 comments on commit 608359b

Please sign in to comment.