mirrors/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-22 15:04:44 +01:00
Code Issues Projects Releases Wiki Activity

Reorder dumpwallet so that cs_main functions go first

Browse source 
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.
This commit is contained in:
Andrew Chow 2021-07-18 22:53:26 -04:00
parent e8f85e0e86
commit 25d99e6511
2 changed files with 44 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";

14
src/wallet/wallet.cpp
View file

@ -2298,6 +2298,13 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t>& mapKeyBirth) const {
AssertLockHeld(cs_wallet);
mapKeyBirth.clear();
// 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)));
{
LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan();
assert(spk_man != nullptr);
LOCK(spk_man->cs_KeyStore);
@ -2309,11 +2316,7 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t>& mapKeyBirth) const {
}
}
// 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)));
// 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;
@ -2341,6 +2344,7 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t>& mapKeyBirth) const {
}
}
}
}
// Extract block timestamps for those keys
for (const auto& entry : mapKeyFirstBlock) {