From 7efa6a4fb43154044a147ddfc0df0e180ea66c4d Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 26 Apr 2024 17:12:52 -0400 Subject: [PATCH 1/9] 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. --- src/wallet/test/wallet_tests.cpp | 8 ++--- src/wallet/wallet.cpp | 45 ++++++++++++++++++++----- src/wallet/wallet.h | 14 ++++---- test/functional/wallet_reorgsrestore.py | 8 ++--- 4 files changed, 51 insertions(+), 24 deletions(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index bb76e4219ca..e539f1408f5 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -109,7 +109,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); @@ -119,8 +119,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); @@ -133,7 +133,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 deffaef75da..bfafb9be729 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -659,6 +659,27 @@ 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); + + CBlockLocator loc; + chain().findBlock(m_last_block_processed, FoundBlock().locator(loc)); + + WalletBatch batch(GetDatabase()); + batch.WriteBestBlock(loc); +} + void CWallet::SetMinVersion(enum WalletFeature nVersion, WalletBatch* batch_in) { LOCK(cs_wallet); @@ -1425,15 +1446,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) { @@ -1520,18 +1542,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) { + SetLastBlockProcessed(block.height, block.hash); + } } void CWallet::blockDisconnected(const interfaces::BlockInfo& block) @@ -1543,9 +1572,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++) { @@ -1579,6 +1605,9 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block) } } } + + // Update the best block + SetLastBlockProcessed(block.height - 1, *Assert(block.prev_hash)); } void CWallet::updatedBlockTip() diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 659e3854e71..e6a60a082cb 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. @@ -989,13 +992,8 @@ 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); //! 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 dbadf50e85d..1a49b886421 100755 --- a/test/functional/wallet_reorgsrestore.py +++ b/test/functional/wallet_reorgsrestore.py @@ -122,11 +122,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 recorded 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 8b180358fb4c0585f2d9b8c4239d99a98c1fd663 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 3 Jun 2024 16:24:13 -0400 Subject: [PATCH 2/9] wallet: Replace chainStateFlushed in loading with WriteBestBlock 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. --- src/wallet/wallet.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index bfafb9be729..8bcb05ebf7c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3120,7 +3120,8 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } if (chain) { - walletInstance->chainStateFlushed(ChainstateRole::NORMAL, chain->getTipLocator()); + WalletBatch batch(walletInstance->GetDatabase()); + batch.WriteBestBlock(chain->getTipLocator()); } } else if (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) { // Make it impossible to disable private keys after creation @@ -3409,7 +3410,8 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf } } walletInstance->m_attaching_chain = false; - walletInstance->chainStateFlushed(ChainstateRole::NORMAL, chain.getTipLocator()); + WalletBatch batch(walletInstance->GetDatabase()); + batch.WriteBestBlock(chain.getTipLocator()); walletInstance->GetDatabase().IncrementUpdateCounter(); } walletInstance->m_attaching_chain = false; From 2150a268c288eaf79f9e28a18372e5ff3b3694a4 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 23 Apr 2025 15:02:35 -0700 Subject: [PATCH 3/9] 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 afb79d98afb..adef24524c8 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 35b636469983fdcd5f8e62448f4733effe596e41 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 3 Jun 2024 16:25:33 -0400 Subject: [PATCH 4/9] wallet: Remove WriteBestBlock from chainStateFlushed --- src/wallet/wallet.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8bcb05ebf7c..75e52c82e36 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -655,8 +655,6 @@ void CWallet::chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) if (m_attaching_chain || role == ChainstateRole::BACKGROUND) { return; } - WalletBatch batch(GetDatabase()); - batch.WriteBestBlock(loc); } void CWallet::SetLastBlockProcessedInMem(int block_height, uint256 block_hash) From 08668272030bfcffc78efd77b642d6e50563d5dc Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 11 Apr 2025 12:08:34 -0700 Subject: [PATCH 5/9] wallet: Write the last block scanned instead of tip after init rescan 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 | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 75e52c82e36..5b880cfb0ca 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3402,14 +3402,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 + // Although ScanForWalletTransactions will have stopped at the best block that was set prior to the rescan, + // we still need to make sure that the best block on disk is set correctly as rescanning may overwrite it. + walletInstance->SetLastBlockProcessed(*scan_res.last_scanned_height, scan_res.last_scanned_block); } - walletInstance->m_attaching_chain = false; - WalletBatch batch(walletInstance->GetDatabase()); - batch.WriteBestBlock(chain.getTipLocator()); walletInstance->GetDatabase().IncrementUpdateCounter(); } walletInstance->m_attaching_chain = false; From 9113e60f550833fbb3f19c511b4fe28f909e1fd5 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 6 Mar 2025 11:49:43 -0800 Subject: [PATCH 6/9] 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 | 5 +---- 3 files changed, 3 insertions(+), 13 deletions(-) diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 21e8a0b3bd2..f4b42296b02 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -572,7 +572,7 @@ public: m_context.chain = &chain; m_context.args = &args; } - ~WalletLoaderImpl() override { UnloadWallets(m_context); } + ~WalletLoaderImpl() override { stop(); } //! ChainClient methods void registerRpcs() override @@ -594,7 +594,7 @@ public: return StartWallets(m_context); } void flush() override { return FlushWallets(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 e77999b1115..c6b96cd3b4f 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -173,13 +173,6 @@ void FlushWallets(WalletContext& context) } } -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 c079cad955b..b95829a3826 100644 --- a/src/wallet/load.h +++ b/src/wallet/load.h @@ -31,10 +31,7 @@ void StartWallets(WalletContext& context); //! Flush all wallets in preparation for shutdown. void FlushWallets(WalletContext& context); -//! Stop all wallets. Wallets will be flushed first. -void StopWallets(WalletContext& context); - -//! Close all wallets. +//! Flush and unload all wallets. void UnloadWallets(WalletContext& context); } // namespace wallet From eceb848494ea24d3686a9ff9a2d25b657cce9d6e Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 5 Mar 2025 17:50:52 -0800 Subject: [PATCH 7/9] wallet: Write best block record on unload --- src/wallet/wallet.cpp | 14 ++++++++++++++ src/wallet/wallet.h | 2 ++ 2 files changed, 16 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5b880cfb0ca..0f513dc71e7 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(); + wallet->WriteBestBlock(); // Unregister with the validation interface which also drops shared pointers. wallet->m_chain_notifications_handler.reset(); @@ -4721,4 +4722,17 @@ std::optional CWallet::GetKey(const CKeyID& keyid) const } return std::nullopt; } + +void CWallet::WriteBestBlock() const +{ + LOCK(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 e6a60a082cb..cb4d351a2d0 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -994,6 +994,8 @@ public: } /** 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; //! Connect the signals from ScriptPubKeyMans to the signals in CWallet void ConnectScriptPubKeyManNotifiers(); From 2d72a8894cfb609ee616b098e165c722183ba325 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 29 Apr 2024 15:54:38 -0400 Subject: [PATCH 8/9] 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 | 21 --------------------- src/wallet/wallet.h | 2 -- 3 files changed, 24 deletions(-) diff --git a/src/bench/wallet_migration.cpp b/src/bench/wallet_migration.cpp index adef24524c8..04baea34ddb 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 0f513dc71e7..917ffb38393 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -649,15 +649,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; - } -} - void CWallet::SetLastBlockProcessedInMem(int block_height, uint256 block_hash) { AssertLockHeld(cs_wallet); @@ -3322,11 +3313,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 @@ -3412,7 +3398,6 @@ 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 // Although ScanForWalletTransactions will have stopped at the best block that was set prior to the rescan, // we still need to make sure that the best block on disk is set correctly as rescanning may overwrite it. @@ -3420,7 +3405,6 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf } walletInstance->GetDatabase().IncrementUpdateCounter(); } - walletInstance->m_attaching_chain = false; return true; } @@ -4469,11 +4453,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 cb4d351a2d0..201c81482be 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}; @@ -791,7 +790,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 de7b5eda7ef04e875850fe594de2a89992b7bd46 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 11 Apr 2025 15:01:02 -0700 Subject: [PATCH 9/9] 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 31ffd934f41..4c98c3e31fa 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, @@ -36,41 +33,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): if self.is_bdb_compiled(): # Make a legacy wallet and check it is BDB @@ -278,8 +240,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()