Skip to content

Commit

Permalink
[wallet] Remove locked_chain from CWallet, its RPCs and tests
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
Antoine Riard committed Apr 30, 2020
1 parent 8411788 commit 6a72f26
Show file tree
Hide file tree
Showing 11 changed files with 106 additions and 201 deletions.
12 changes: 0 additions & 12 deletions src/interfaces/chain.cpp
View file
Expand Up @@ -53,11 +53,6 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<Rec
return true;
}

class LockImpl : public Chain::Lock, public UniqueLock<RecursiveMutex>
{
using UniqueLock::UniqueLock;
};

class NotificationsProxy : public CValidationInterface
{
public:
Expand Down Expand Up @@ -150,13 +145,6 @@ class ChainImpl : public Chain
{
public:
explicit ChainImpl(NodeContext& node) : m_node(node) {}
std::unique_ptr<Chain::Lock> lock(bool try_lock) 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;
}
Optional<int> getHeight() override
{
LOCK(::cs_main);
Expand Down
30 changes: 9 additions & 21 deletions src/interfaces/chain.h
View file
Expand Up @@ -59,39 +59,27 @@ 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() {}
};

//! 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;

//! Get current chain height, not including genesis block (returns 0 if
//! chain only contains genesis block, nullopt if chain does not contain
//! any blocks)
Expand Down
24 changes: 1 addition & 23 deletions src/interfaces/wallet.cpp
View file
Expand Up @@ -196,25 +196,21 @@ class WalletImpl : public Wallet
}
void lockCoin(const COutPoint& output) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
return m_wallet->LockCoin(output);
}
void unlockCoin(const COutPoint& output) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
return m_wallet->UnlockCoin(output);
}
bool isLockedCoin(const COutPoint& output) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
return m_wallet->IsLockedCoin(output.hash, output.n);
}
void listLockedCoins(std::vector<COutPoint>& outputs) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
return m_wallet->ListLockedCoins(outputs);
}
Expand All @@ -225,7 +221,6 @@ class WalletImpl : public Wallet
CAmount& fee,
std::string& fail_reason) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
CTransactionRef tx;
if (!m_wallet->CreateTransaction(recipients, tx, fee, change_pos,
Expand All @@ -238,14 +233,12 @@ class WalletImpl : public Wallet
WalletValueMap value_map,
WalletOrderForm order_form) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
m_wallet->CommitTransaction(std::move(tx), std::move(value_map), std::move(order_form));
}
bool transactionCanBeAbandoned(const uint256& txid) override { return m_wallet->TransactionCanBeAbandoned(txid); }
bool abandonTransaction(const uint256& txid) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
return m_wallet->AbandonTransaction(txid);
}
Expand Down Expand Up @@ -273,7 +266,6 @@ class WalletImpl : public Wallet
}
CTransactionRef getTx(const uint256& txid) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
auto mi = m_wallet->mapWallet.find(txid);
if (mi != m_wallet->mapWallet.end()) {
Expand All @@ -283,7 +275,6 @@ class WalletImpl : public Wallet
}
WalletTx getWalletTx(const uint256& txid) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
auto mi = m_wallet->mapWallet.find(txid);
if (mi != m_wallet->mapWallet.end()) {
Expand All @@ -293,7 +284,6 @@ class WalletImpl : public Wallet
}
std::vector<WalletTx> getWalletTxs() override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
std::vector<WalletTx> result;
result.reserve(m_wallet->mapWallet.size());
Expand All @@ -307,10 +297,6 @@ class WalletImpl : public Wallet
int& num_blocks,
int64_t& block_time) override
{
auto locked_chain = m_wallet->chain().lock(true /* try_lock */);
if (!locked_chain) {
return false;
}
TRY_LOCK(m_wallet->cs_wallet, locked_wallet);
if (!locked_wallet) {
return false;
Expand All @@ -331,7 +317,6 @@ class WalletImpl : public Wallet
bool& in_mempool,
int& num_blocks) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
auto mi = m_wallet->mapWallet.find(txid);
if (mi != m_wallet->mapWallet.end()) {
Expand Down Expand Up @@ -368,8 +353,6 @@ class WalletImpl : public Wallet
}
bool tryGetBalances(WalletBalances& balances, int& num_blocks, bool force, int cached_num_blocks) override
{
auto locked_chain = m_wallet->chain().lock(true /* try_lock */);
if (!locked_chain) return false;
TRY_LOCK(m_wallet->cs_wallet, locked_wallet);
if (!locked_wallet) {
return false;
Expand All @@ -386,31 +369,26 @@ class WalletImpl : public Wallet
}
isminetype txinIsMine(const CTxIn& txin) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
return m_wallet->IsMine(txin);
}
isminetype txoutIsMine(const CTxOut& txout) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
return m_wallet->IsMine(txout);
}
CAmount getDebit(const CTxIn& txin, isminefilter filter) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
return m_wallet->GetDebit(txin, filter);
}
CAmount getCredit(const CTxOut& txout, isminefilter filter) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
return m_wallet->GetCredit(txout, filter);
}
CoinsList listCoins() override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
CoinsList result;
for (const auto& entry : m_wallet->ListCoins()) {
Expand All @@ -424,7 +402,6 @@ class WalletImpl : public Wallet
}
std::vector<WalletTxOut> getCoins(const std::vector<COutPoint>& outputs) override
{
auto locked_chain = m_wallet->chain().lock();
LOCK(m_wallet->cs_wallet);
std::vector<WalletTxOut> result;
result.reserve(outputs.size());
Expand Down Expand Up @@ -496,6 +473,7 @@ class WalletImpl : public Wallet
{
return MakeHandler(m_wallet->NotifyCanGetAddressesChanged.connect(fn));
}
CWallet* wallet() override { return m_wallet.get(); }

std::shared_ptr<CWallet> m_wallet;
};
Expand Down
3 changes: 3 additions & 0 deletions src/interfaces/wallet.h
View file
Expand Up @@ -300,6 +300,9 @@ class Wallet
//! Register handler for keypool changed messages.
using CanGetAddressesChangedFn = std::function<void()>;
virtual std::unique_ptr<Handler> handleCanGetAddressesChanged(CanGetAddressesChangedFn fn) = 0;

//! Return pointer to internal wallet class, useful for testing.
virtual CWallet* wallet() { return nullptr; }
};

//! Information about one wallet address.
Expand Down
1 change: 0 additions & 1 deletion src/qt/test/wallettests.cpp
View file
Expand Up @@ -145,7 +145,6 @@ void TestGUI(interfaces::Node& node)
wallet->LoadWallet(firstRun);
{
auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan();
auto locked_chain = wallet->chain().lock();
LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore);
wallet->SetAddressBook(GetDestinationForKey(test.coinbaseKey.GetPubKey(), wallet->m_default_address_type), "", "receive");
spk_man->AddKeyPubKey(test.coinbaseKey, test.coinbaseKey.GetPubKey());
Expand Down
4 changes: 0 additions & 4 deletions src/wallet/feebumper.cpp
View file
Expand Up @@ -140,7 +140,6 @@ namespace feebumper {

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

auto locked_chain = wallet.chain().lock();
LOCK(wallet.cs_wallet);
errors.clear();
auto it = wallet.mapWallet.find(txid);
Expand Down Expand Up @@ -240,14 +238,12 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
}

bool SignTransaction(CWallet& wallet, CMutableTransaction& mtx) {
auto locked_chain = wallet.chain().lock();
LOCK(wallet.cs_wallet);
return wallet.SignTransaction(mtx);
}

Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, uint256& bumped_txid)
{
auto locked_chain = wallet.chain().lock();
LOCK(wallet.cs_wallet);
if (!errors.empty()) {
return Result::MISC_ERROR;
Expand Down
16 changes: 1 addition & 15 deletions src/wallet/rpcdump.cpp
View file
Expand Up @@ -133,7 +133,6 @@ UniValue importprivkey(const JSONRPCRequest& request)
WalletRescanReserver reserver(*pwallet);
bool fRescan = true;
{
auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);

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

{
auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);

CTxDestination dest = DecodeDestination(request.params[0].get_str());
Expand Down Expand Up @@ -317,7 +315,6 @@ UniValue importaddress(const JSONRPCRequest& request)
{
RescanWallet(*pwallet, reserver);
{
auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);
pwallet->ReacceptWalletTransactions();
}
Expand Down Expand Up @@ -361,7 +358,6 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Something wrong with merkleblock");
}

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

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

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

{
auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);

std::set<CScript> script_pub_keys;
Expand All @@ -505,7 +499,6 @@ UniValue importpubkey(const JSONRPCRequest& request)
{
RescanWallet(*pwallet, reserver);
{
auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);
pwallet->ReacceptWalletTransactions();
}
Expand Down Expand Up @@ -557,7 +550,6 @@ UniValue importwallet(const JSONRPCRequest& request)
int64_t nTimeBegin = 0;
bool fGood = true;
{
auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);

EnsureWalletIsUnlocked(pwallet);
Expand Down Expand Up @@ -700,7 +692,6 @@ UniValue dumpprivkey(const JSONRPCRequest& request)

LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet);

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

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

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

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

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

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

Expand Down Expand Up @@ -1379,7 +1369,6 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
int64_t nLowestTimestamp = 0;
UniValue response(UniValue::VARR);
{
auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);
EnsureWalletIsUnlocked(pwallet);

Expand Down Expand Up @@ -1414,7 +1403,6 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
if (fRescan && fRunScan && requests.size()) {
int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, reserver, true /* update */);
{
auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);
pwallet->ReacceptWalletTransactions();
}
Expand Down Expand Up @@ -1676,7 +1664,6 @@ UniValue importdescriptors(const JSONRPCRequest& main_request) {
bool rescan = false;
UniValue response(UniValue::VARR);
{
auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);
EnsureWalletIsUnlocked(pwallet);

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

0 comments on commit 6a72f26

Please sign in to comment.