Merge bitcoin/bitcoin#32580: wallet, test: best block locator matches scan state follow-ups

1b5c545 wallet, test: best block locator matches scan state follow-ups (rkrux)

Pull request description:

  Few follows-ups from #30221: Use `SetLastBlockProcessedInMem` more in `AttachChain`, add not null locator check in `WriteBestBlock`. Add log and few assertions in `wallet_reorgstore` test.

ACKs for top commit:
  achow101:
    ACK 1b5c545
  pablomartin4btc:
    cr-ACK 1b5c545

Tree-SHA512: 34edde55beef5714cea2e1131c29b57da2dc32ea091cd81878014de503c128f02c3ab88aee1e456541d7937e033dca5a81b03e9e2888cf781d71b62ad9b5ca5c

Author: achow101

src/wallet/wallet.cpp

@@ -3103,11 +3103,9 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
 
     const std::optional<int> tip_height = chain.getHeight();
     if (tip_height) {
-        walletInstance->m_last_block_processed = chain.getBlockHash(*tip_height);
-        walletInstance->m_last_block_processed_height = *tip_height;
+        walletInstance->SetLastBlockProcessedInMem(*tip_height, chain.getBlockHash(*tip_height));
     } else {
-        walletInstance->m_last_block_processed.SetNull();
-        walletInstance->m_last_block_processed_height = -1;
+        walletInstance->SetLastBlockProcessedInMem(-1, uint256());
     }
 
     if (tip_height && *tip_height != rescan_height)
@@ -3172,7 +3170,7 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
                 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.
+            // Set last block scanned as the last block processed as it may be different in 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);
         }
@@ -4458,8 +4456,10 @@ void CWallet::WriteBestBlock() const
         CBlockLocator loc;
         chain().findBlock(m_last_block_processed, FoundBlock().locator(loc));
 
-        WalletBatch batch(GetDatabase());
-        batch.WriteBestBlock(loc);
+        if (!loc.IsNull()) {
+            WalletBatch batch(GetDatabase());
+            batch.WriteBestBlock(loc);
+        }
     }
 }
 

test/functional/wallet_reorgsrestore.py

@@ -90,7 +90,7 @@ def test_coinbase_automatic_abandon_during_startup(self):
         assert_equal(wallet0.gettransaction(descendant_tx_id)['details'][0]['abandoned'], True)
 
     def test_reorg_handling_during_unclean_shutdown(self):
-        self.log.info("Test that wallet doesn't crash due to a duplicate block disconnection event after an unclean shutdown")
+        self.log.info("Test that wallet transactions are un-abandoned in case of temporarily invalidated blocks and wallet doesn't crash due to a duplicate block disconnection event after an unclean shutdown")
         node = self.nodes[0]
         # Receive coinbase reward on a new wallet
         node.createwallet(wallet_name="reorg_crash", load_on_startup=True)
@@ -104,6 +104,7 @@ def test_reorg_handling_during_unclean_shutdown(self):
 
         # Disconnect tip and sync wallet state
         tip = wallet.getbestblockhash()
+        tip_height = wallet.getblockstats(tip)["height"]
         wallet.invalidateblock(tip)
         wallet.syncwithvalidationinterfacequeue()
 
@@ -116,14 +117,17 @@ def test_reorg_handling_during_unclean_shutdown(self):
         node.kill_process()
 
         # Restart the node and confirm that it has not persisted the last chain state changes to disk
-        self.start_node(0)
+        # that leads to a rescan by the wallet
+        with self.nodes[0].assert_debug_log(expected_msgs=[f"Rescanning last 1 blocks (from block {tip_height - 1})...\n"]):
+            self.start_node(0)
         assert_equal(node.getbestblockhash(), tip)
 
         # 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.
+        # coinbase. This was rescanned and now un-abandoned.
         wallet = node.get_wallet_rpc("reorg_crash")
         assert_equal(wallet.gettransaction(coinbase_tx_id)['details'][0]['abandoned'], False)
+        assert_greater_than(wallet.getbalances()["mine"]["immature"], 0)
 
         # 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.