mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-20 14:05:23 +01:00
Code Issues Projects Releases Wiki Activity

Merge bitcoin/bitcoin#22492: wallet: Reorder locks in dumpwallet to avoid lock order assertion

Browse source 
9b85a5e2f7 tests: Test for dumpwallet lock order issue (Andrew Chow)
25d99e6511 Reorder dumpwallet so that cs_main functions go first (Andrew Chow)

Pull request description:

  When a wallet is loaded which has an unconfirmed transaction in the mempool, it will end up establishing the lock order of cs_wallet -> cs_main -> cs_KeyStore. If `dumpwallet` is used on this wallet, then a lock order of cs_wallet -> cs_KeyStore -> cs_main will be used, which causes a lock order assertion. This PR fixes this by reordering `dumpwallet` and `GetKeyBirthTimes` (only used by `dumpwallet`). Specifically, in both functions, the function calls which lock cs_main are done prior to locking cs_KeyStore. This avoids the lock order issue.

  Additionally, I have added a test case to `wallet_dump.py`. Of course testing this requires `--enable-debug`.

  Fixes #22489

ACKs for top commit:
  MarcoFalke:
    review ACK 9b85a5e2f7 🎰
  ryanofsky:
    Code review ACK 9b85a5e2f7. Nice to reduce lock scope, and good test!
  prayank23:
    tACK 9b85a5e2f7
  lsilva01:
    Tested ACK 9b85a5e2f7 under the same conditions reported in issue #22489 and the `dumpwallet` command completed successfully.

Tree-SHA512: d370a8f415ad64ee6a538ff419155837bcdbb167e3831b06572562289239028c6b46d80b23d227286afe875d9351f3377574ed831549ea426fb926af0e19c755
This commit is contained in:
MarcoFalke 2021-07-20 15:04:01 +02:00
commit 539023ab41
No known key found for this signature in database
GPG key ID: CE2B75697E69A548
3 changed files with 53 additions and 35 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

13
src/wallet/rpcdump.cpp
View file

@ -740,7 +740,7 @@ RPCHelpMan dumpwallet()
// the user could have gotten from another RPC command prior to now
wallet.BlockUntilSyncedToCurrentChain();
LOCK2(wallet.cs_wallet, spk_man.cs_KeyStore);
LOCK(wallet.cs_wallet);
EnsureWalletIsUnlocked(wallet);
@ -762,9 +762,16 @@ RPCHelpMan dumpwallet()
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file");
std::map<CKeyID, int64_t> mapKeyBirth;
const std::map<CKeyID, int64_t>& mapKeyPool = spk_man.GetAllReserveKeys();
wallet.GetKeyBirthTimes(mapKeyBirth);
int64_t block_time = 0;
CHECK_NONFATAL(wallet.chain().findBlock(wallet.GetLastBlockHash(), FoundBlock().time(block_time)));
// Note: To avoid a lock order issue, access to cs_main must be locked before cs_KeyStore.
// So we do the two things in this function that lock cs_main first: GetKeyBirthTimes, and findBlock.
LOCK(spk_man.cs_KeyStore);
const std::map<CKeyID, int64_t>& mapKeyPool = spk_man.GetAllReserveKeys();
std::set<CScriptID> scripts = spk_man.GetCScripts();
// sort time/key pairs
@ -779,8 +786,6 @@ RPCHelpMan dumpwallet()
file << strprintf("# Wallet dump created by Bitcoin %s\n", CLIENT_BUILD);
file << strprintf("# * Created on %s\n", FormatISO8601DateTime(GetTime()));
file << strprintf("# * Best block at time of backup was %i (%s),\n", wallet.GetLastBlockHeight(), wallet.GetLastBlockHash().ToString());
int64_t block_time = 0;
CHECK_NONFATAL(wallet.chain().findBlock(wallet.GetLastBlockHash(), FoundBlock().time(block_time)));
file << strprintf("# mined on %s\n", FormatISO8601DateTime(block_time));
file << "\n";

66
src/wallet/wallet.cpp
View file

@ -2298,44 +2298,48 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t>& mapKeyBirth) const {
AssertLockHeld(cs_wallet);
mapKeyBirth.clear();
LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan();
assert(spk_man != nullptr);
LOCK(spk_man->cs_KeyStore);
// get birth times for keys with metadata
for (const auto& entry : spk_man->mapKeyMetadata) {
if (entry.second.nCreateTime) {
mapKeyBirth[entry.first] = entry.second.nCreateTime;
}
}
// map in which we'll infer heights of other keys
std::map<CKeyID, const CWalletTx::Confirmation*> mapKeyFirstBlock;
CWalletTx::Confirmation max_confirm;
max_confirm.block_height = GetLastBlockHeight() > 144 ? GetLastBlockHeight() - 144 : 0; // the tip can be reorganized; use a 144-block safety margin
CHECK_NONFATAL(chain().findAncestorByHeight(GetLastBlockHash(), max_confirm.block_height, FoundBlock().hash(max_confirm.hashBlock)));
for (const CKeyID &keyid : spk_man->GetKeys()) {
if (mapKeyBirth.count(keyid) == 0)
mapKeyFirstBlock[keyid] = &max_confirm;
}
// if there are no such keys, we're done
if (mapKeyFirstBlock.empty())
return;
{
LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan();
assert(spk_man != nullptr);
LOCK(spk_man->cs_KeyStore);
// find first block that affects those keys, if there are any left
for (const auto& entry : mapWallet) {
// iterate over all wallet transactions...
const CWalletTx &wtx = entry.second;
if (wtx.m_confirm.status == CWalletTx::CONFIRMED) {
// ... which are already in a block
for (const CTxOut &txout : wtx.tx->vout) {
// iterate over all their outputs
for (const auto &keyid : GetAffectedKeys(txout.scriptPubKey, *spk_man)) {
// ... and all their affected keys
auto rit = mapKeyFirstBlock.find(keyid);
if (rit != mapKeyFirstBlock.end() && wtx.m_confirm.block_height < rit->second->block_height) {
rit->second = &wtx.m_confirm;
// get birth times for keys with metadata
for (const auto& entry : spk_man->mapKeyMetadata) {
if (entry.second.nCreateTime) {
mapKeyBirth[entry.first] = entry.second.nCreateTime;
}
}
// Prepare to infer birth heights for keys without metadata
for (const CKeyID &keyid : spk_man->GetKeys()) {
if (mapKeyBirth.count(keyid) == 0)
mapKeyFirstBlock[keyid] = &max_confirm;
}
// if there are no such keys, we're done
if (mapKeyFirstBlock.empty())
return;
// find first block that affects those keys, if there are any left
for (const auto& entry : mapWallet) {
// iterate over all wallet transactions...
const CWalletTx &wtx = entry.second;
if (wtx.m_confirm.status == CWalletTx::CONFIRMED) {
// ... which are already in a block
for (const CTxOut &txout : wtx.tx->vout) {
// iterate over all their outputs
for (const auto &keyid : GetAffectedKeys(txout.scriptPubKey, *spk_man)) {
// ... and all their affected keys
auto rit = mapKeyFirstBlock.find(keyid);
if (rit != mapKeyFirstBlock.end() && wtx.m_confirm.block_height < rit->second->block_height) {
rit->second = &wtx.m_confirm;
}
}
}
}

9
test/functional/wallet_dump.py
View file

@ -209,6 +209,15 @@ class WalletDumpTest(BitcoinTestFramework):
with self.nodes[0].assert_debug_log(['Flushing wallet.dat'], timeout=20):
self.nodes[0].getnewaddress()
# Make sure that dumpwallet doesn't have a lock order issue when there is an unconfirmed tx and it is reloaded
# See https://github.com/bitcoin/bitcoin/issues/22489
self.nodes[0].createwallet("w3")
w3 = self.nodes[0].get_wallet_rpc("w3")
w3.importprivkey(privkey=self.nodes[0].get_deterministic_priv_key().key, label="coinbase_import")
w3.sendtoaddress(w3.getnewaddress(), 10)
w3.unloadwallet()
self.nodes[0].loadwallet("w3")
w3.dumpwallet(os.path.join(self.nodes[0].datadir, "w3.dump"))
if __name__ == '__main__':
WalletDumpTest().main()