diff --git a/src/bench/wallet_migration.cpp b/src/bench/wallet_migration.cpp index 49591f5fc25..52da05367f2 100644 --- a/src/bench/wallet_migration.cpp +++ b/src/bench/wallet_migration.cpp @@ -28,10 +28,13 @@ 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()}; + // 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) { diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 84755897feb..d96ad8ae68d 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 diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 072935ee897..96d475c2823 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 fc1c31ad27c..ba1abc365cf 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(); @@ -652,15 +653,20 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, return false; } -void CWallet::chainStateFlushed(ChainstateRole role, const CBlockLocator& loc) +void CWallet::SetLastBlockProcessedInMem(int block_height, uint256 block_hash) { - // 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); + 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) @@ -1377,15 +1383,16 @@ void CWallet::RecursiveUpdateTxState(WalletBatch* batch, const Txid& tx_hash, co } } -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) { @@ -1472,18 +1479,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) @@ -1495,9 +1509,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++) { @@ -1531,6 +1542,9 @@ void CWallet::blockDisconnected(const interfaces::BlockInfo& block) } } } + + // Update the best block + SetLastBlockProcessed(block.height - 1, *Assert(block.prev_hash)); } void CWallet::updatedBlockTip() @@ -2885,6 +2899,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); @@ -2894,7 +2910,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 @@ -2910,7 +2925,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 @@ -3112,11 +3130,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 @@ -3193,15 +3206,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; } + // 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; return true; } @@ -3261,14 +3280,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); } @@ -4197,11 +4209,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")}; } @@ -4450,4 +4457,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 54e5c94200f..c27124a52aa 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -306,7 +306,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}; @@ -371,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}; @@ -438,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. @@ -780,7 +782,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(); @@ -970,13 +971,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_descriptor.py b/test/functional/wallet_descriptor.py index ecf3ef5ec0a..356b1ac47e3 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() 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.