Merge bitcoin/bitcoin#31556: validation: Send correct notification during snapshot completion

bc43ecaf6d test: add functional test for balance after snapshot completion (Martin Zumsande)
226d03dd61 validation: Send correct notification during snapshot completion (Martin Zumsande)

Pull request description:

  After AssumeUtxo background sync is completed in a `ActivateBestChain()` call, the `GetRole()` function called with `BlockConnected()` returns `ChainstateRole::NORMAL` instead of `ChainstateRole::BACKGROUND` for this chainstate.
  This would make the wallet (which ignores `BlockConnected` notifications for the background chainstate) process it, change `m_last_block_processed_height` to the (ancient) snapshot height, and display an incorrect balance.

  Fix this by caching the chainstate role before calling `ActivateBestChainStep()`.
  Also contains a test for this situation that fails on master.

  Fixes #31546

ACKs for top commit:
  fjahr:
    re-ACK bc43ecaf6d
  achow101:
    ACK bc43ecaf6d
  furszy:
    Code review ACK bc43ecaf6d
  TheCharlatan:
    lgtm ACK bc43ecaf6d

Tree-SHA512: c5db677cf3fbab3a33ec127ec6c27c8812299e8368fd3c986bc34d0e515c4eb256f6104479f27829eefc098197de3af75d64ddca636b6b612900a0e21243e4f2

src/validation.cpp

@@ -3529,6 +3529,10 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
 
                 bool fInvalidFound = false;
                 std::shared_ptr<const CBlock> nullBlockPtr;
+                // BlockConnected signals must be sent for the original role;
+                // in case snapshot validation is completed during ActivateBestChainStep, the
+                // result of GetRole() changes from BACKGROUND to NORMAL.
+               const ChainstateRole chainstate_role{this->GetRole()};
                 if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace)) {
                     // A system error occurred
                     return false;
@@ -3544,7 +3548,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
                 for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
                     assert(trace.pblock && trace.pindex);
                     if (m_chainman.m_options.signals) {
-                        m_chainman.m_options.signals->BlockConnected(this->GetRole(), trace.pblock, trace.pindex);
+                        m_chainman.m_options.signals->BlockConnected(chainstate_role, trace.pblock, trace.pindex);
                     }
                 }
 

test/functional/wallet_assumeutxo.py

@@ -17,6 +17,7 @@ from test_framework.messages import COIN
 from test_framework.util import (
     assert_equal,
     assert_raises_rpc_error,
+    ensure_for,
 )
 from test_framework.wallet import MiniWallet
 
@@ -34,17 +35,18 @@ class AssumeutxoTest(BitcoinTestFramework):
 
     def set_test_params(self):
         """Use the pregenerated, deterministic chain up to height 199."""
-        self.num_nodes = 2
+        self.num_nodes = 3
         self.rpc_timeout = 120
         self.extra_args = [
             [],
             [],
+            [],
         ]
 
     def setup_network(self):
         """Start with the nodes disconnected so that one can generate a snapshot
         including blocks the other hasn't yet seen."""
-        self.add_nodes(2)
+        self.add_nodes(3)
         self.start_nodes(extra_args=self.extra_args)
 
     def run_test(self):
@@ -57,6 +59,7 @@ class AssumeutxoTest(BitcoinTestFramework):
         """
         n0 = self.nodes[0]
         n1 = self.nodes[1]
+        n2 = self.nodes[2]
 
         self.mini_wallet = MiniWallet(n0)
 
@@ -88,6 +91,7 @@ class AssumeutxoTest(BitcoinTestFramework):
 
             # make n1 aware of the new header, but don't give it the block.
             n1.submitheader(newblock)
+            n2.submitheader(newblock)
 
         # Ensure everyone is seeing the same headers.
         for n in self.nodes:
@@ -125,6 +129,7 @@ class AssumeutxoTest(BitcoinTestFramework):
 
         assert_equal(n0.getblockcount(), FINAL_HEIGHT)
         assert_equal(n1.getblockcount(), START_HEIGHT)
+        assert_equal(n2.getblockcount(), START_HEIGHT)
 
         assert_equal(n0.getblockchaininfo()["blocks"], FINAL_HEIGHT)
 
@@ -192,6 +197,13 @@ class AssumeutxoTest(BitcoinTestFramework):
         w = n1.get_wallet_rpc("w")
         assert_equal(w.getbalance(), 34)
 
+        self.log.info("Check balance of a wallet that is active during snapshot completion")
+        n2.restorewallet("w", "backup_w.dat")
+        loaded = n2.loadtxoutset(dump_output['path'])
+        self.connect_nodes(0, 2)
+        self.wait_until(lambda: len(n2.getchainstates()['chainstates']) == 1)
+        ensure_for(duration=1, f=lambda: (n2.getbalance() == 34))
+
 
 if __name__ == '__main__':
     AssumeutxoTest(__file__).main()