Merge bitcoin/bitcoin#30678: wallet: Write best block to disk before backup

f20fe33e94 test: Add basic balance coverage to wallet_assumeutxo.py (Fabian Jahr)
037b101e80 test: Add coverage for best block locator write in wallet_backup (Fabian Jahr)
31c0df0389 wallet: migration, write best locator before unloading wallet (furszy)
7e3dbe4180 wallet: Write best block to disk before backup (Fabian Jahr)

Pull request description:

  I discovered that we don't write the best block to disk when trying to explain the behavior described here: https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719951882

  In the context of that test, the behavior is confusing and I think it also shows that one of the already existing tests in `wallet_assumeutxo.py` doesn't actually test what it says. It only fails because the best block isn't written and actually, the height of the backup that is loaded is at the snapshot height during backup. So it really shouldn't fail since it's past the background sync blocks already.

  I'm not sure if this is super relevant in practice though so I am first looking for concept ACKs on the `BackupWallet` code change. Either way, I think this behavior should be documented better if it is left as is and the test should be changed.

ACKs for top commit:
  achow101:
    ACK f20fe33e94
  furszy:
    ACK f20fe33

Tree-SHA512: bb384a940df5c942fffe2eb06314ade4fc5d9b924012bfef3b1c456c4182a30825d1e137d8ae561d93d3a8a2f4d1c1ffe568132d20fa7d04844f1e289ab4a28b

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);
 }
 
@@ -4390,6 +4398,11 @@ util::Result<MigrationResult> 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")};
         }

test/functional/wallet_assumeutxo.py

@@ -11,7 +11,9 @@ See feature_assumeutxo.py for background.
 - 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,
@@ -62,8 +64,16 @@ class AssumeutxoTest(BitcoinTestFramework):
         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")
+        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,
         # but that n1 doesn't yet see. In order for the snapshot to activate,
@@ -84,6 +94,8 @@ class AssumeutxoTest(BitcoinTestFramework):
             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")
@@ -103,7 +115,13 @@ class AssumeutxoTest(BitcoinTestFramework):
 
         # 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)
@@ -126,8 +144,13 @@ class AssumeutxoTest(BitcoinTestFramework):
 
         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")
+        # 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")
 
         PAUSE_HEIGHT = FINAL_HEIGHT - 40
 
@@ -159,8 +182,15 @@ class AssumeutxoTest(BitcoinTestFramework):
         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")
+        # 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__':

test/functional/wallet_backup.py

@@ -140,6 +140,25 @@ class WalletBackupTest(BitcoinTestFramework):
         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 @@ class WalletBackupTest(BitcoinTestFramework):
         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()