diff options
| -rw-r--r-- | src/wallet/rpcdump.cpp | 13 | ||||
| -rw-r--r-- | src/wallet/wallet.cpp | 66 | ||||
| -rwxr-xr-x | test/functional/wallet_dump.py | 9 |
3 files changed, 53 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<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"; 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<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); + + // 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; + } } } } diff --git a/test/functional/wallet_dump.py b/test/functional/wallet_dump.py index eb54da99f5..91d6121679 100755 --- a/test/functional/wallet_dump.py +++ b/test/functional/wallet_dump.py @@ -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() |