From 7e3dbe4180cbeb65e59b53d9fa98509e9189549d Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Tue, 20 Aug 2024 01:23:10 +0200 Subject: [PATCH 1/4] wallet: Write best block to disk before backup This ensures that the best block is included in the backup which leads to a more consistent behavior when loading the backup. --- src/wallet/wallet.cpp | 8 ++++++++ test/functional/wallet_assumeutxo.py | 19 +++++++++++++++---- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 83e96adf078e8..ff3f45b6d0c4a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3410,6 +3410,14 @@ 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); + } + } return GetDatabase().Backup(strDest); } diff --git a/test/functional/wallet_assumeutxo.py b/test/functional/wallet_assumeutxo.py index a5025a64e7db7..a55102dec9aa9 100755 --- a/test/functional/wallet_assumeutxo.py +++ b/test/functional/wallet_assumeutxo.py @@ -62,9 +62,15 @@ def run_test(self): for n in self.nodes: n.setmocktime(n.getblockheader(n.getbestblockhash())['time']) + # Create a wallet that we will create a backup for later (at snapshot height) n0.createwallet('w') w = n0.get_wallet_rpc("w") + # Create another wallet and backup now (before snapshot height) + n0.createwallet('w2') + w2 = n0.get_wallet_rpc("w2") + w2.backupwallet("backup_w2.dat") + # Generate a series of blocks that `n0` will have in the snapshot, # but that n1 doesn't yet see. In order for the snapshot to activate, # though, we have to ferry over the new headers to n1 so that it @@ -84,6 +90,8 @@ def run_test(self): assert_equal(n.getblockchaininfo()[ "headers"], SNAPSHOT_BASE_HEIGHT) + # This backup is created at the snapshot height, so it's + # not part of the background sync anymore w.backupwallet("backup_w.dat") self.log.info("-- Testing assumeutxo") @@ -126,8 +134,11 @@ def run_test(self): assert_equal(n1.getblockchaininfo()["blocks"], SNAPSHOT_BASE_HEIGHT) - self.log.info("Backup can't be loaded during background sync") - assert_raises_rpc_error(-4, "Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height 299", n1.restorewallet, "w", "backup_w.dat") + self.log.info("Backup from the snapshot height can be loaded during background sync") + n1.restorewallet("w", "backup_w.dat") + + self.log.info("Backup from before the snapshot height can't be loaded during background sync") + assert_raises_rpc_error(-4, "Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height 299", n1.restorewallet, "w2", "backup_w2.dat") PAUSE_HEIGHT = FINAL_HEIGHT - 40 @@ -159,8 +170,8 @@ def run_test(self): self.log.info("Ensuring background validation completes") self.wait_until(lambda: len(n1.getchainstates()['chainstates']) == 1) - self.log.info("Ensuring wallet can be restored from backup") - n1.restorewallet("w", "backup_w.dat") + self.log.info("Ensuring wallet can be restored from a backup that was created before the snapshot height") + n1.restorewallet("w2", "backup_w2.dat") if __name__ == '__main__': From 31c0df038909e40fe9618a4595254907ed1de907 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 26 Aug 2024 10:35:53 -0300 Subject: [PATCH 2/4] wallet: migration, write best locator before unloading wallet --- src/wallet/wallet.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ff3f45b6d0c4a..b921bc5e491e8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4398,6 +4398,11 @@ 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")}; } From 037b101e808ccf9e717751619e04f6e87d614efd Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Wed, 18 Sep 2024 23:43:03 +0200 Subject: [PATCH 3/4] test: Add coverage for best block locator write in wallet_backup --- test/functional/wallet_backup.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index a639c343772cf..83267f77e17d9 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -140,6 +140,25 @@ def restore_wallet_existent_name(self): assert_raises_rpc_error(-36, error_message, node.restorewallet, wallet_name, backup_file) assert wallet_file.exists() + def test_pruned_wallet_backup(self): + self.log.info("Test loading backup on a pruned node when the backup was created close to the prune height of the restoring node") + node = self.nodes[3] + self.restart_node(3, ["-prune=1", "-fastprune=1"]) + # Ensure the chain tip is at height 214, because this test assume it is. + assert_equal(node.getchaintips()[0]["height"], 214) + # We need a few more blocks so we can actually get above an realistic + # minimal prune height + self.generate(node, 50, sync_fun=self.no_op) + # Backup created at block height 264 + node.backupwallet(node.datadir_path / 'wallet_pruned.bak') + # Generate more blocks so we can actually prune the older blocks + self.generate(node, 300, sync_fun=self.no_op) + # This gives us an actual prune height roughly in the range of 220 - 240 + node.pruneblockchain(250) + # The backup should be updated with the latest height (locator) for + # the backup to load successfully this close to the prune height + node.restorewallet(f'pruned', node.datadir_path / 'wallet_pruned.bak') + def run_test(self): self.log.info("Generating initial blockchain") self.generate(self.nodes[0], 1) @@ -242,6 +261,8 @@ def run_test(self): for sourcePath in sourcePaths: assert_raises_rpc_error(-4, "backup failed", self.nodes[0].backupwallet, sourcePath) + self.test_pruned_wallet_backup() + if __name__ == '__main__': WalletBackupTest(__file__).main() From f20fe33e94c6752e5d2ed92511c0bf51a10716ee Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Fri, 20 Sep 2024 18:09:10 +0200 Subject: [PATCH 4/4] test: Add basic balance coverage to wallet_assumeutxo.py --- test/functional/wallet_assumeutxo.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/test/functional/wallet_assumeutxo.py b/test/functional/wallet_assumeutxo.py index a55102dec9aa9..76cd2097a3a19 100755 --- a/test/functional/wallet_assumeutxo.py +++ b/test/functional/wallet_assumeutxo.py @@ -11,7 +11,9 @@ - TODO: test loading a wallet (backup) on a pruned node """ +from test_framework.address import address_to_scriptpubkey from test_framework.test_framework import BitcoinTestFramework +from test_framework.messages import COIN from test_framework.util import ( assert_equal, assert_raises_rpc_error, @@ -65,10 +67,12 @@ def run_test(self): # Create a wallet that we will create a backup for later (at snapshot height) n0.createwallet('w') w = n0.get_wallet_rpc("w") + w_address = w.getnewaddress() # Create another wallet and backup now (before snapshot height) n0.createwallet('w2') w2 = n0.get_wallet_rpc("w2") + w2_address = w2.getnewaddress() w2.backupwallet("backup_w2.dat") # Generate a series of blocks that `n0` will have in the snapshot, @@ -111,7 +115,13 @@ def run_test(self): # Mine more blocks on top of the snapshot that n1 hasn't yet seen. This # will allow us to test n1's sync-to-tip on top of a snapshot. - self.generate(n0, nblocks=100, sync_fun=self.no_op) + w_skp = address_to_scriptpubkey(w_address) + w2_skp = address_to_scriptpubkey(w2_address) + for i in range(100): + if i % 3 == 0: + self.mini_wallet.send_to(from_node=n0, scriptPubKey=w_skp, amount=1 * COIN) + self.mini_wallet.send_to(from_node=n0, scriptPubKey=w2_skp, amount=10 * COIN) + self.generate(n0, nblocks=1, sync_fun=self.no_op) assert_equal(n0.getblockcount(), FINAL_HEIGHT) assert_equal(n1.getblockcount(), START_HEIGHT) @@ -136,6 +146,8 @@ def run_test(self): self.log.info("Backup from the snapshot height can be loaded during background sync") n1.restorewallet("w", "backup_w.dat") + # Balance of w wallet is still still 0 because n1 has not synced yet + assert_equal(n1.getbalance(), 0) self.log.info("Backup from before the snapshot height can't be loaded during background sync") assert_raises_rpc_error(-4, "Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height 299", n1.restorewallet, "w2", "backup_w2.dat") @@ -172,6 +184,13 @@ def run_test(self): self.log.info("Ensuring wallet can be restored from a backup that was created before the snapshot height") n1.restorewallet("w2", "backup_w2.dat") + # Check balance of w2 wallet + assert_equal(n1.getbalance(), 340) + + # Check balance of w wallet after node is synced + n1.loadwallet("w") + w = n1.get_wallet_rpc("w") + assert_equal(w.getbalance(), 34) if __name__ == '__main__':