From 7bacabb204b6c34f9545f0b37e2c66296ad2c0de Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 26 Apr 2024 17:12:52 -0400 Subject: [PATCH 1/7] wallet: Update best block record after block dis/connect When a block is connected, if the new block had anything relevant to the wallet, update the best block record on disk. If not, also sync the best block record to disk every 144 blocks. Also reuse the new WriteBestBlock method in BackupWallet. --- src/wallet/test/wallet_tests.cpp | 8 ++-- src/wallet/wallet.cpp | 62 ++++++++++++++++++------- src/wallet/wallet.h | 16 +++---- test/functional/wallet_reorgsrestore.py | 8 ++-- 4 files changed, 62 insertions(+), 32 deletions(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 650e62fa877..3425eb62ddc 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -106,7 +106,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) LOCK(wallet.cs_wallet); LOCK(Assert(m_node.chainman)->GetMutex()); wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); - wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash()); + wallet.SetLastBlockProcessed(newTip->nHeight, newTip->GetBlockHash()); } AddKey(wallet, coinbaseKey); WalletRescanReserver reserver(wallet); @@ -116,8 +116,8 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) { CBlockLocator locator; - BOOST_CHECK(!WalletBatch{wallet.GetDatabase()}.ReadBestBlock(locator)); - BOOST_CHECK(locator.IsNull()); + BOOST_CHECK(WalletBatch{wallet.GetDatabase()}.ReadBestBlock(locator)); + BOOST_CHECK(!locator.IsNull() && locator.vHave.front() == newTip->GetBlockHash()); } CWallet::ScanResult result = wallet.ScanForWalletTransactions(/*start_block=*/oldTip->GetBlockHash(), /*start_height=*/oldTip->nHeight, /*max_height=*/{}, reserver, /*fUpdate=*/false, /*save_progress=*/true); @@ -130,7 +130,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) { CBlockLocator locator; BOOST_CHECK(WalletBatch{wallet.GetDatabase()}.ReadBestBlock(locator)); - BOOST_CHECK(!locator.IsNull()); + BOOST_CHECK(!locator.IsNull() && locator.vHave.front() == newTip->GetBlockHash()); } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3c5df177679..e0cdd0187c6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -663,6 +663,22 @@ void CWallet::chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) batch.WriteBestBlock(loc); } +void CWallet::SetLastBlockProcessedInMem(int block_height, uint256 block_hash) +{ + AssertLockHeld(cs_wallet); + + m_last_block_processed = block_hash; + m_last_block_processed_height = block_height; +} + +void CWallet::SetLastBlockProcessed(int block_height, uint256 block_hash) +{ + AssertLockHeld(cs_wallet); + + SetLastBlockProcessedInMem(block_height, block_hash); + WriteBestBlock(); +} + void CWallet::SetMinVersion(enum WalletFeature nVersion, WalletBatch* batch_in) { LOCK(cs_wallet); @@ -1378,15 +1394,16 @@ void CWallet::RecursiveUpdateTxState(WalletBatch* batch, const uint256& tx_hash, } } -void CWallet::SyncTransaction(const CTransactionRef& ptx, const SyncTxState& state, bool update_tx, bool rescanning_old_block) +bool CWallet::SyncTransaction(const CTransactionRef& ptx, const SyncTxState& state, bool update_tx, bool rescanning_old_block) { if (!AddToWalletIfInvolvingMe(ptx, state, update_tx, rescanning_old_block)) - return; // Not one of ours + return false; // Not one of ours // If a transaction changes 'conflicted' state, that changes the balance // available of the outputs it spends. So force those to be // recomputed, also: MarkInputsDirty(ptx); + return true; } void CWallet::transactionAddedToMempool(const CTransactionRef& tx) { @@ -1473,18 +1490,25 @@ void CWallet::blockConnected(ChainstateRole role, const interfaces::BlockInfo& b assert(block.data); LOCK(cs_wallet); - m_last_block_processed_height = block.height; - m_last_block_processed = block.hash; + // Update the best block in memory first. This will set the best block's height, which is + // needed by MarkConflicted. + SetLastBlockProcessedInMem(block.height, block.hash); // No need to scan block if it was created before the wallet birthday. // Uses chain max time and twice the grace period to adjust time for block time variability. if (block.chain_time_max < m_birth_time.load() - (TIMESTAMP_WINDOW * 2)) return; // Scan block + bool wallet_updated = false; for (size_t index = 0; index < block.data->vtx.size(); index++) { - SyncTransaction(block.data->vtx[index], TxStateConfirmed{block.hash, block.height, static_cast(index)}); + wallet_updated |= SyncTransaction(block.data->vtx[index], TxStateConfirmed{block.hash, block.height, static_cast(index)}); transactionRemovedFromMempool(block.data->vtx[index], MemPoolRemovalReason::BLOCK); } + + // Update on disk if this block resulted in us updating a tx, or periodically every 144 blocks (~1 day) + if (wallet_updated || block.height % 144 == 0) { + WriteBestBlock(); + } } void CWallet::blockDisconnected(const interfaces::BlockInfo& block) @@ -1496,9 +1520,6 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block) // be unconfirmed, whether or not the transaction is added back to the mempool. // User may have to call abandontransaction again. It may be addressed in the // future with a stickier abandoned state or even removing abandontransaction call. - m_last_block_processed_height = block.height - 1; - m_last_block_processed = *Assert(block.prev_hash); - int disconnect_height = block.height; for (size_t index = 0; index < block.data->vtx.size(); index++) { @@ -1532,6 +1553,9 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block) } } } + + // Update the best block + SetLastBlockProcessed(block.height - 1, *Assert(block.prev_hash)); } void CWallet::updatedBlockTip() @@ -3264,14 +3288,7 @@ void CWallet::postInitProcess() bool CWallet::BackupWallet(const std::string& strDest) const { - if (m_chain) { - CBlockLocator loc; - WITH_LOCK(cs_wallet, chain().findBlock(m_last_block_processed, FoundBlock().locator(loc))); - if (!loc.IsNull()) { - WalletBatch batch(GetDatabase()); - batch.WriteBestBlock(loc); - } - } + WITH_LOCK(cs_wallet, WriteBestBlock()); return GetDatabase().Backup(strDest); } @@ -4453,4 +4470,17 @@ std::optional CWallet::GetKey(const CKeyID& keyid) const } return std::nullopt; } + +void CWallet::WriteBestBlock() const +{ + AssertLockHeld(cs_wallet); + + if (!m_last_block_processed.IsNull()) { + CBlockLocator loc; + chain().findBlock(m_last_block_processed, FoundBlock().locator(loc)); + + WalletBatch batch(GetDatabase()); + batch.WriteBestBlock(loc); + } +} } // namespace wallet diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index e32b8c7272b..9334aa0485e 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -370,7 +370,7 @@ private: void SyncMetaData(std::pair) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - void SyncTransaction(const CTransactionRef& tx, const SyncTxState& state, bool update_tx = true, bool rescanning_old_block = false) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool SyncTransaction(const CTransactionRef& tx, const SyncTxState& state, bool update_tx = true, bool rescanning_old_block = false) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** WalletFlags set on this wallet. */ std::atomic m_wallet_flags{0}; @@ -437,6 +437,9 @@ private: static NodeClock::time_point GetDefaultNextResend(); + // Update last block processed in memory only + void SetLastBlockProcessedInMem(int block_height, uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + public: /** * Main wallet lock. @@ -974,13 +977,10 @@ public: assert(m_last_block_processed_height >= 0); return m_last_block_processed; } - /** Set last block processed height, currently only use in unit test */ - void SetLastBlockProcessed(int block_height, uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) - { - AssertLockHeld(cs_wallet); - m_last_block_processed_height = block_height; - m_last_block_processed = block_hash; - }; + /** Set last block processed height, and write to database */ + void SetLastBlockProcessed(int block_height, uint256 block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Write the current best block to database */ + void WriteBestBlock() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Connect the signals from ScriptPubKeyMans to the signals in CWallet void ConnectScriptPubKeyManNotifiers(); diff --git a/test/functional/wallet_reorgsrestore.py b/test/functional/wallet_reorgsrestore.py index 4025741f65b..73ca173f237 100755 --- a/test/functional/wallet_reorgsrestore.py +++ b/test/functional/wallet_reorgsrestore.py @@ -119,11 +119,11 @@ class ReorgsRestoreTest(BitcoinTestFramework): self.start_node(0) assert_equal(node.getbestblockhash(), tip) - # Due to an existing bug, the wallet incorrectly keeps the transaction in an abandoned state, even though that's - # no longer the case (after the unclean shutdown, the node's chain returned to the pre-invalidation tip). - # This issue blocks any future spending and results in an incorrect balance display. + # After disconnecting the block, the wallet should record the new best block. + # Upon reload after the crash, since the chainstate was not flushed, the tip contains the previously abandoned + # coinbase. This should be rescanned and now un-abandoned. wallet = node.get_wallet_rpc("reorg_crash") - assert_equal(wallet.getwalletinfo()['immature_balance'], 0) # FIXME: #31824. + assert_equal(wallet.gettransaction(coinbase_tx_id)['details'][0]['abandoned'], False) # Previously, a bug caused the node to crash if two block disconnection events occurred consecutively. # Ensure this is no longer the case by simulating a new reorg. From 6d3a8b195a826448c021dd189255ca41ba70cc5a Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 3 Jun 2024 16:24:13 -0400 Subject: [PATCH 2/7] wallet: Replace chainStateFlushed in loading with SetLastBlockProcessed The only reason to call chainStateFlushed during wallet loading is to ensure that the best block is written. Do these writes explicitly to prepare for removing chainStateFlushed, while also ensuring that the wallet's in memory state tracking is written to disk. Additionally, after rescanning on wallet loading, instead of writing the locator for the current chain tip, write the locator for the last block that the rescan had scanned. This ensures that the stored best block record matches the wallet's current state. Any blocks dis/connected during the rescan are processed after the rescan and the last block processed will be updated accordingly. --- src/wallet/wallet.cpp | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e0cdd0187c6..a90c4de54c3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2912,6 +2912,8 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri !walletInstance->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET); if (fFirstRun) { + LOCK(walletInstance->cs_wallet); + // ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key walletInstance->SetMinVersion(FEATURE_LATEST); @@ -2921,7 +2923,6 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri assert(walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) || !(wallet_creation_flags & (WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET))) { - LOCK(walletInstance->cs_wallet); if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { walletInstance->SetupDescriptorScriptPubKeyMans(); // SetupDescriptorScriptPubKeyMans already calls SetupGeneration for us so we don't need to call SetupGeneration separately @@ -2937,7 +2938,10 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } if (chain) { - walletInstance->chainStateFlushed(ChainstateRole::NORMAL, chain->getTipLocator()); + std::optional tip_height = chain->getHeight(); + if (tip_height) { + walletInstance->SetLastBlockProcessed(*tip_height, chain->getBlockHash(*tip_height)); + } } } else if (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) { // Make it impossible to disable private keys after creation @@ -3220,13 +3224,21 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf { WalletRescanReserver reserver(*walletInstance); - if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(chain.getBlockHash(rescan_height), rescan_height, /*max_height=*/{}, reserver, /*fUpdate=*/true, /*save_progress=*/true).status)) { + if (!reserver.reserve()) { + error = _("Failed to acquire rescan reserver during wallet initialization"); + return false; + } + ScanResult scan_res = walletInstance->ScanForWalletTransactions(chain.getBlockHash(rescan_height), rescan_height, /*max_height=*/{}, reserver, /*fUpdate=*/true, /*save_progress=*/true); + if (ScanResult::SUCCESS != scan_res.status) { error = _("Failed to rescan the wallet during initialization"); return false; } + walletInstance->m_attaching_chain = false; + // Set and update the best block record + // Set last block scanned as the last block processed as it may be different in case the case of a reorg. + // Also save the best block locator because rescanning only updates it intermittently. + walletInstance->SetLastBlockProcessed(*scan_res.last_scanned_height, scan_res.last_scanned_block); } - walletInstance->m_attaching_chain = false; - walletInstance->chainStateFlushed(ChainstateRole::NORMAL, chain.getTipLocator()); } walletInstance->m_attaching_chain = false; From 7fd3e1cf0c88553e0722048ce488f265883558e7 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 23 Apr 2025 15:02:35 -0700 Subject: [PATCH 3/7] wallet, bench: Write a bestblock record in WalletMigration Migrating a wallet requires having a bestblock record. This is always the case in normal operation as such a record is always written on wallet loading if it didn't already exist. However, within the unit tests and benchmarks, this is not guaranteed. Since migration requires the record, WalletMigration needs to also add this record before the benchmark. --- src/bench/wallet_migration.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/bench/wallet_migration.cpp b/src/bench/wallet_migration.cpp index b5e512f3693..a172ec44838 100644 --- a/src/bench/wallet_migration.cpp +++ b/src/bench/wallet_migration.cpp @@ -32,6 +32,10 @@ static void WalletMigration(benchmark::Bench& bench) LegacyDataSPKM* legacy_spkm = wallet->GetOrCreateLegacyDataSPKM(); WalletBatch batch{wallet->GetDatabase()}; + // Write a best block record as migration expects one to exist + CBlockLocator loc; + batch.WriteBestBlock(loc); + // Add watch-only addresses std::vector scripts_watch_only; for (int w = 0; w < NUM_WATCH_ONLY_ADDR; ++w) { From 98a1a5275c8c395fe47ff7f10109d75b06bc391d Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 3 Jun 2024 16:25:33 -0400 Subject: [PATCH 4/7] wallet: Remove chainStateFlushed chainStateFlushed is no longer needed since the best block is updated after a block is scanned. Since the chainstate being flushed does not necessarily coincide with the wallet having processed said block, it does not entirely make sense for the wallet to be recording that block as its best block, and this can cause race conditions where some blocks are not processed. Thus, remove this notification. --- src/bench/wallet_migration.cpp | 1 - src/wallet/wallet.cpp | 23 ----------------------- src/wallet/wallet.h | 2 -- 3 files changed, 26 deletions(-) diff --git a/src/bench/wallet_migration.cpp b/src/bench/wallet_migration.cpp index a172ec44838..13146fe2897 100644 --- a/src/bench/wallet_migration.cpp +++ b/src/bench/wallet_migration.cpp @@ -28,7 +28,6 @@ static void WalletMigration(benchmark::Bench& bench) // Setup legacy wallet std::unique_ptr wallet = std::make_unique(test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase()); - wallet->chainStateFlushed(ChainstateRole::NORMAL, CBlockLocator{}); LegacyDataSPKM* legacy_spkm = wallet->GetOrCreateLegacyDataSPKM(); WalletBatch batch{wallet->GetDatabase()}; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a90c4de54c3..5ff3d1433d6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -652,17 +652,6 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, return false; } -void CWallet::chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) -{ - // Don't update the best block until the chain is attached so that in case of a shutdown, - // the rescan will be restarted at next startup. - if (m_attaching_chain || role == ChainstateRole::BACKGROUND) { - return; - } - WalletBatch batch(GetDatabase()); - batch.WriteBestBlock(loc); -} - void CWallet::SetLastBlockProcessedInMem(int block_height, uint256 block_hash) { AssertLockHeld(cs_wallet); @@ -3143,11 +3132,6 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf // be pending on the validation-side until lock release. Blocks that are connected while the // rescan is ongoing will not be processed in the rescan but with the block connected notifications, // so the wallet will only be completeley synced after the notifications delivery. - // chainStateFlushed notifications are ignored until the rescan is finished - // so that in case of a shutdown event, the rescan will be repeated at the next start. - // This is temporary until rescan and notifications delivery are unified under same - // interface. - walletInstance->m_attaching_chain = true; //ignores chainStateFlushed notifications walletInstance->m_chain_notifications_handler = walletInstance->chain().handleNotifications(walletInstance); // If rescan_required = true, rescan_height remains equal to 0 @@ -3233,14 +3217,12 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf error = _("Failed to rescan the wallet during initialization"); return false; } - walletInstance->m_attaching_chain = false; // Set and update the best block record // Set last block scanned as the last block processed as it may be different in case the case of a reorg. // Also save the best block locator because rescanning only updates it intermittently. walletInstance->SetLastBlockProcessed(*scan_res.last_scanned_height, scan_res.last_scanned_block); } } - walletInstance->m_attaching_chain = false; return true; } @@ -4229,11 +4211,6 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle return util::Error{_("Error: This wallet is already a descriptor wallet")}; } - // Flush chain state before unloading wallet - CBlockLocator locator; - WITH_LOCK(wallet->cs_wallet, context.chain->findBlock(wallet->GetLastBlockHash(), FoundBlock().locator(locator))); - if (!locator.IsNull()) wallet->chainStateFlushed(ChainstateRole::NORMAL, locator); - if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) { return util::Error{_("Unable to unload the wallet before migrating")}; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 9334aa0485e..04363e607d3 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -305,7 +305,6 @@ private: std::atomic fAbortRescan{false}; std::atomic fScanningWallet{false}; // controlled by WalletRescanReserver - std::atomic m_attaching_chain{false}; std::atomic m_scanning_with_passphrase{false}; std::atomic m_scanning_start{SteadyClock::time_point{}}; std::atomic m_scanning_progress{0}; @@ -787,7 +786,6 @@ public: /** should probably be renamed to IsRelevantToMe */ bool IsFromMe(const CTransaction& tx) const; CAmount GetDebit(const CTransaction& tx, const isminefilter& filter) const; - void chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) override; DBErrors LoadWallet(); From 876a2585a8b69e12ac171a0d9ff5aab864067c42 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 6 Mar 2025 11:49:43 -0800 Subject: [PATCH 5/7] wallet: Remove unnecessary database Close step on shutdown StopWallets, which was being called prior to UnloadWallets, performs an unnecessary database closing step. This causes issues in UnloadWallets which does additional database cleanups. Since the database closing step is unnecessary, StopWallets is removed, and UnloadWallets is now called from WalletLoaderImpl::stop. --- src/wallet/interfaces.cpp | 4 ++-- src/wallet/load.cpp | 7 ------- src/wallet/load.h | 4 ---- 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 5308b4418d7..a093dab7e70 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -553,7 +553,7 @@ public: m_context.chain = &chain; m_context.args = &args; } - ~WalletLoaderImpl() override { UnloadWallets(m_context); } + ~WalletLoaderImpl() override { stop(); } //! ChainClient methods void registerRpcs() override @@ -574,7 +574,7 @@ public: m_context.scheduler = &scheduler; return StartWallets(m_context); } - void stop() override { return StopWallets(m_context); } + void stop() override { return UnloadWallets(m_context); } void setMockTime(int64_t time) override { return SetMockTime(time); } void schedulerMockForward(std::chrono::seconds delta) override { Assert(m_context.scheduler)->MockForward(delta); } diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index 3fdad7d6fb1..379f606f088 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -162,13 +162,6 @@ void StartWallets(WalletContext& context) context.scheduler->scheduleEvery([&context] { MaybeResendWalletTxs(context); }, 1min); } -void StopWallets(WalletContext& context) -{ - for (const std::shared_ptr& pwallet : GetWallets(context)) { - pwallet->Close(); - } -} - void UnloadWallets(WalletContext& context) { auto wallets = GetWallets(context); diff --git a/src/wallet/load.h b/src/wallet/load.h index e7224c27ee9..20859d5c4d8 100644 --- a/src/wallet/load.h +++ b/src/wallet/load.h @@ -28,10 +28,6 @@ bool LoadWallets(WalletContext& context); //! Complete startup of wallets. void StartWallets(WalletContext& context); -//! Stop all wallets. Wallets will be flushed first. -void StopWallets(WalletContext& context); - -//! Close all wallets. void UnloadWallets(WalletContext& context); } // namespace wallet From b44b7c03fef01e0b5db704e50762b3d16b3da69e Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 5 Mar 2025 17:50:52 -0800 Subject: [PATCH 6/7] wallet: Write best block record on unload --- src/wallet/wallet.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5ff3d1433d6..8d379da4e0e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -164,6 +164,7 @@ bool RemoveWallet(WalletContext& context, const std::shared_ptr& wallet interfaces::Chain& chain = wallet->chain(); std::string name = wallet->GetName(); + WITH_LOCK(wallet->cs_wallet, wallet->WriteBestBlock()); // Unregister with the validation interface which also drops shared pointers. wallet->m_chain_notifications_handler.reset(); From 30a94b1ab9ae850d55cb9eb606a06890437bc75e Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 11 Apr 2025 15:01:02 -0700 Subject: [PATCH 7/7] test, wallet: Remove concurrent writes test Since CWallet::chainStateFlushed is now no-op, this test no longer tests the concurrent writes scenario. There are no other cases where multiple DatabaseBatches are open at the same time. --- test/functional/wallet_descriptor.py | 40 ---------------------------- 1 file changed, 40 deletions(-) diff --git a/test/functional/wallet_descriptor.py b/test/functional/wallet_descriptor.py index 4b2f76c6dc9..48eb5763d9a 100755 --- a/test/functional/wallet_descriptor.py +++ b/test/functional/wallet_descriptor.py @@ -9,10 +9,7 @@ try: except ImportError: pass -import concurrent.futures - from test_framework.blocktools import COINBASE_MATURITY -from test_framework.descriptors import descsum_create from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_not_equal, @@ -33,41 +30,6 @@ class WalletDescriptorTest(BitcoinTestFramework): self.skip_if_no_wallet() self.skip_if_no_py_sqlite3() - def test_concurrent_writes(self): - self.log.info("Test sqlite concurrent writes are in the correct order") - self.restart_node(0, extra_args=["-unsafesqlitesync=0"]) - self.nodes[0].createwallet(wallet_name="concurrency", blank=True) - wallet = self.nodes[0].get_wallet_rpc("concurrency") - # First import a descriptor that uses hardened dervation so that topping up - # Will require writing a ton to db - wallet.importdescriptors([{"desc":descsum_create("wpkh(tprv8ZgxMBicQKsPeuVhWwi6wuMQGfPKi9Li5GtX35jVNknACgqe3CY4g5xgkfDDJcmtF7o1QnxWDRYw4H5P26PXq7sbcUkEqeR4fg3Kxp2tigg/0h/0h/*h)"), "timestamp": "now", "active": True}]) - with concurrent.futures.ThreadPoolExecutor(max_workers=1) as thread: - topup = thread.submit(wallet.keypoolrefill, newsize=1000) - - # Then while the topup is running, we need to do something that will call - # ChainStateFlushed which will trigger a write to the db, hopefully at the - # same time that the topup still has an open db transaction. - self.nodes[0].cli.gettxoutsetinfo() - assert_equal(topup.result(), None) - - wallet.unloadwallet() - - # Check that everything was written - wallet_db = self.nodes[0].wallets_path / "concurrency" / self.wallet_data_filename - conn = sqlite3.connect(wallet_db) - with conn: - # Retrieve the bestblock_nomerkle record - bestblock_rec = conn.execute("SELECT value FROM main WHERE hex(key) = '1262657374626C6F636B5F6E6F6D65726B6C65'").fetchone()[0] - # Retrieve the number of descriptor cache records - # Since we store binary data, sqlite's comparison operators don't work everywhere - # so just retrieve all records and process them ourselves. - db_keys = conn.execute("SELECT key FROM main").fetchall() - cache_records = len([k[0] for k in db_keys if b"walletdescriptorcache" in k[0]]) - conn.close() - - assert_equal(bestblock_rec[5:37][::-1].hex(), self.nodes[0].getbestblockhash()) - assert_equal(cache_records, 1000) - def run_test(self): # Make a descriptor wallet self.log.info("Making a descriptor wallet") @@ -254,8 +216,6 @@ class WalletDescriptorTest(BitcoinTestFramework): conn.close() assert_raises_rpc_error(-4, "Unexpected legacy entry in descriptor wallet found.", self.nodes[0].loadwallet, "crashme") - self.test_concurrent_writes() - if __name__ == '__main__': WalletDescriptorTest(__file__).main()