From 25d99e6511d8c43b2025a89bcd8295de755346a7 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Sun, 18 Jul 2021 22:53:26 -0400 Subject: [PATCH] Reorder dumpwallet so that cs_main functions go first DEBUG_LOCKORDER expects cs_wallet, cs_main, and cs_KeyStore to be acquired in that order. However dumpwallet would take these in the order cs_wallet, cs_KeyStore, cs_main. So when configured with `--enable-debug`, it is possible to hit the lock order assertion when using dumpwallet. To fix this, cs_wallet and cs_KeyStore are no longer locked at the same time. Instead cs_wallet will be locked first. Then the functions which lock cs_main will be run. Lastly cs_KeyStore will be locked afterwards. This avoids the lock order issue. Furthermore, since GetKeyBirthTimes (only used by dumpwallet) also uses a function that locks cs_main, and itself also locks cs_KeyStore, the same reordering is done here. --- src/wallet/rpcdump.cpp | 13 ++++++--- src/wallet/wallet.cpp | 66 ++++++++++++++++++++++-------------------- 2 files changed, 44 insertions(+), 35 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index ea97b339cf..ac60504419 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -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 mapKeyBirth; - const std::map& 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& mapKeyPool = spk_man.GetAllReserveKeys(); std::set 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"; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 27565aefc9..9a61ca698d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2298,44 +2298,48 @@ void CWallet::GetKeyBirthTimes(std::map& 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 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); + + // 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; + } - // 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; + // 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; + } } } }