Merge bitcoin/bitcoin#23365: index: Fix backwards search for bestblock

9600ea0 test: Add edge case of pruning up to index height (Martin Zumsande)
698c524 index: Fix backwards search for bestblock (Martin Zumsande)

Pull request description:

  This PR attempts to fix an intermittent Init issue encountered during the stress testing of #23289, which relates to the pruning-compatible filter reconstruction logic introduced in #15946.

  The problem would occur when the node starts with `-txindex=1` but `ThreadSync` is interrupted after it sets `m_best_block_index` to Genesis, and before it gets do any further work.
  In that case, during the next restart of the node, an Init error would be thrown because  `BaseIndex::Init()` tries to backtrack from the tip to the last block which has been successfully indexed (here: Genesis), but the backtracking logic didn't work properly in this case:
  The loop
  `while (block_to_test && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))` checks if a predecessor exists **before** performing the check `block_to_test == block` and then possbily setting `prune_violation = false`
  If `block_to_test` and `block` are the Genesis block this check will not be reached because `block->pprev` does not exist.

  To reproduce this bug on regtest:
  1) start a node with a fresh datadir using `-txindex=1` (or any other index)
  2) stop and restart without any index
  3) mine a block
  3) stop and restart again with the index enabled
  ->InitError `Error: txindex best block of the index goes beyond pruned data. (...)`

  Fix this by requiring that we have the data for the block of the current iteration `block` (instead of requiring it for the predecessor `block->pprev`)
  That way, the check for `block_to_test == block` is also reached when `block_to_test` is the Genesis block.
  No longer requiring the data of `block->pprev` also means that we can now prune up to `m_best_block_index` height without requiring a reindex (one block more than before). I added this edge case to `feature_blockfilterindex_prune.py`, the new version should fail on master.

ACKs for top commit:
  ryanofsky:
    Partial code review ACK 9600ea0 for the code change, not the test changes. (Test changes are indirect and little over my head.) It seems obvious that previous code `prune_violation = true, while (block->pprev)` would incorrectly detect a prune violation at the genesis block, and the fix here make sense and looks correct.

Tree-SHA512: c717f372cee8fd49718b1b18bfe237aa6ba3ff4468588c10e1272d7a2ef3981d10af4e57de51dec295e2ca72d441bc6c2812f7990011a94d7f818775e3ff1a38

Author: MarcoFalke

src/index/base.cpp

@@ -91,11 +91,12 @@ bool BaseIndex::Init()
             const CBlockIndex* block = active_chain.Tip();
             prune_violation = true;
             // check backwards from the tip if we have all block data until we reach the indexes bestblock
-            while (block_to_test && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
+            while (block_to_test && block && (block->nStatus & BLOCK_HAVE_DATA)) {
                 if (block_to_test == block) {
                     prune_violation = false;
                     break;
                 }
+                assert(block->pprev);
                 block = block->pprev;
             }
         }

test/functional/feature_blockfilterindex_prune.py

@@ -24,9 +24,7 @@ def run_test(self):
         self.log.info("check if we can access a blockfilter when pruning is enabled but no blocks are actually pruned")
         self.sync_index(height=200)
         assert_greater_than(len(self.nodes[0].getblockfilter(self.nodes[0].getbestblockhash())['filter']), 0)
-        # Mine two batches of blocks to avoid hitting NODE_NETWORK_LIMITED_MIN_BLOCKS disconnection
-        self.generate(self.nodes[0], 250)
-        self.generate(self.nodes[0], 250)
+        self.generate(self.nodes[0], 500)
         self.sync_index(height=700)
 
         self.log.info("prune some blocks")
@@ -39,16 +37,29 @@ def run_test(self):
         self.log.info("check if we can access the blockfilter of a pruned block")
         assert_greater_than(len(self.nodes[0].getblockfilter(self.nodes[0].getblockhash(2))['filter']), 0)
 
+        # mine and sync index up to a height that will later be the pruneheight
+        self.generate(self.nodes[0], 298)
+        self.sync_index(height=998)
+
         self.log.info("start node without blockfilterindex")
         self.restart_node(0, extra_args=["-fastprune", "-prune=1"])
 
         self.log.info("make sure accessing the blockfilters throws an error")
         assert_raises_rpc_error(-1, "Index is not enabled for filtertype basic", self.nodes[0].getblockfilter, self.nodes[0].getblockhash(2))
-        self.generate(self.nodes[0], 1000)
+        self.generate(self.nodes[0], 502)
+
+        self.log.info("prune exactly up to the blockfilterindexes best block while blockfilters are disabled")
+        pruneheight_2 = self.nodes[0].pruneblockchain(1000)
+        assert_equal(pruneheight_2, 998)
+        self.restart_node(0, extra_args=["-fastprune", "-prune=1", "-blockfilterindex=1"])
+        self.log.info("make sure that we can continue with the partially synced index after having pruned up to the index height")
+        self.sync_index(height=1500)
 
         self.log.info("prune below the blockfilterindexes best block while blockfilters are disabled")
-        pruneheight_new = self.nodes[0].pruneblockchain(1000)
-        assert_greater_than(pruneheight_new, pruneheight)
+        self.restart_node(0, extra_args=["-fastprune", "-prune=1"])
+        self.generate(self.nodes[0], 1000)
+        pruneheight_3 = self.nodes[0].pruneblockchain(2000)
+        assert_greater_than(pruneheight_3, pruneheight_2)
         self.stop_node(0)
 
         self.log.info("make sure we get an init error when starting the node again with block filters")