Merge #6316: backport: merge bitcoin#23289, #23365, #23777, #23782, #23737, #24039, #24192, #24117, #24789, #25192, #25294, #27280 (auxiliary backports: part 19)

f09752cac1 merge bitcoin#27280: Fix TypeError (expected str instance, bytes found) in wait_for_debug_log (Kittywhiskers Van Gogh)
ecb16808a6 merge bitcoin#25294: Fix wait_for_debug_log UnicodeDecodeError (Kittywhiskers Van Gogh)
445047db63 merge bitcoin#25192: add coverage for unknown value to -blockfilterindex (Kittywhiskers Van Gogh)
f319163815 merge bitcoin#24789: disallow indexes when running reindex-chainstate (Kittywhiskers Van Gogh)
51bc29ee59 merge bitcoin#24117: make indices robust against init aborts (Kittywhiskers Van Gogh)
6645cde0e7 merge bitcoin#24192: Fix feature_init intermittent issues (Kittywhiskers Van Gogh)
a6062445be merge bitcoin#24039: prevent UnicodeDecodeError when opening log file in feature_init.py (Kittywhiskers Van Gogh)
d35af87936 merge bitcoin#23737: make feature_init more robust (Kittywhiskers Van Gogh)
e17c619ca3 merge bitcoin#23782: include two more interruptions points (Kittywhiskers Van Gogh)
577da313df merge bitcoin#23777: follow-ups from bitcoin#23365 (Kittywhiskers Van Gogh)
a681750798 merge bitcoin#23365: Fix backwards search for bestblock (Kittywhiskers Van Gogh)
2e22fd0ba9 merge bitcoin#23289: add stress tests for initialization (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * When backporting [bitcoin#24789](https://github.com/bitcoin/bitcoin/pull/24789), `-txindex=0` had to be appended to the arguments passed in `feature_reindex.py` as unlike Bitcoin ([source](dac44fc06f/src/validation.h (L83))), Dash enables the transaction index by default ([source](74e54b8a12/src/validation.h (L94))).

    As having the index enabled when using `-reindex-chainstate` is now prohibited, without this change, the test will crash.

  ## Breaking Changes

  None expected.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK f09752cac1
  PastaPastaPasta:
    utACK f09752cac1

Tree-SHA512: 72a983e5d5de9b2efd7509beeedbe2e3f32774b1ee20a546e246cca66e1100fa82e7731bce08a5ff620757a6fbaea8a640fb3bc3e7afe20734b79bd92d74f0fd

src/index/base.cpp

@@ -94,11 +94,14 @@ 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;
                 }
+                // block->pprev must exist at this point, since block_to_test is part of the chain
+                // and thus must be encountered when going backwards from the tip
+                assert(block->pprev);
                 block = block->pprev;
             }
         }

src/init.cpp

@@ -554,8 +554,8 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-version", "Print version and exit", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 
     argsman.AddArg("-addressindex", strprintf("Maintain a full address index, used to query for the balance, txids and unspent outputs for addresses (default: %u)", DEFAULT_ADDRESSINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
-    argsman.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
+    argsman.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk. This will also rebuild active optional indexes.", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
-    argsman.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
+    argsman.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead. Deactivate all optional indexes before running this.", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
     argsman.AddArg("-spentindex", strprintf("Maintain a full spent index, used to query the spending txid and input index for an outpoint (default: %u)", DEFAULT_SPENTINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
     argsman.AddArg("-timestampindex", strprintf("Maintain a timestamp index for block hashes, used to query blocks hashes by a range of timestamps (default: %u)", DEFAULT_TIMESTAMPINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
     argsman.AddArg("-txindex", strprintf("Maintain a full transaction index, used by the getrawtransaction rpc call (default: %u)", DEFAULT_TXINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
@@ -1343,6 +1343,19 @@ bool AppInitParameterInteraction(const ArgsManager& args)
         return InitError(_("No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>."));
     }
 
+    if (args.GetBoolArg("-reindex-chainstate", false)) {
+        // indexes that must be deactivated to prevent index corruption, see #24630
+        if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX)) {
+            return InitError(_("-reindex-chainstate option is not compatible with -coinstatsindex. Please temporarily disable coinstatsindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes."));
+        }
+        if (g_enabled_filter_types.count(BlockFilterType::BASIC_FILTER)) {
+            return InitError(_("-reindex-chainstate option is not compatible with -blockfilterindex. Please temporarily disable blockfilterindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes."));
+        }
+        if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
+            return InitError(_("-reindex-chainstate option is not compatible with -txindex. Please temporarily disable txindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes."));
+        }
+    }
+
     try {
         const bool fRecoveryEnabled{llmq::QuorumDataRecoveryEnabled()};
         const bool fQuorumVvecRequestsEnabled{llmq::GetEnabledQuorumVvecSyncEntries().size() > 0};

test/functional/feature_blockfilterindex_prune.py

@@ -15,7 +15,7 @@ from test_framework.governance import EXPECTED_STDERR_NO_GOV_PRUNE
 class FeatureBlockfilterindexPruneTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
-        self.extra_args = [["-fastprune", "-prune=1", "-blockfilterindex=1"]]
+        self.extra_args = [["-fastprune", "-prune=1", "-blockfilterindex=1", "-testactivationheight=v20@2000"]]
 
     def sync_index(self, height):
         expected = {'basic block filter index': {'synced': True, 'best_block_height': height}}
@@ -25,13 +25,13 @@ class FeatureBlockfilterindexPruneTest(BitcoinTestFramework):
         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], 500)
-        self.generate(self.nodes[0], 250)
-        self.generate(self.nodes[0], 250)
         self.sync_index(height=700)
 
         self.log.info("prune some blocks")
         pruneheight = self.nodes[0].pruneblockchain(400)
+        # the prune heights used here and below are magic numbers that are determined by the
+        # thresholds at which block files wrap, so they depend on disk serialization and default block file size.
         assert_equal(pruneheight, 366)
 
         self.log.info("check if we can access the tips blockfilter when we have pruned some blocks")
@@ -40,16 +40,29 @@ class FeatureBlockfilterindexPruneTest(BitcoinTestFramework):
         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", '-testactivationheight=v20@2000'], expected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE)
+        self.restart_node(0, extra_args=["-fastprune", "-prune=1", "-testactivationheight=v20@2000"], expected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE)
 
         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, 932)
+        self.restart_node(0, extra_args=["-fastprune", "-prune=1", "-blockfilterindex=1", "-testactivationheight=v20@2000"], expected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE)
+        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)
+        self.restart_node(0, extra_args=["-fastprune", "-prune=1", "-testactivationheight=v20@2000"], expected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE)
-        assert_greater_than(pruneheight_new, pruneheight)
+        self.generate(self.nodes[0], 1000)
+        pruneheight_3 = self.nodes[0].pruneblockchain(2000)
+        assert_greater_than(pruneheight_3, pruneheight_2)
         self.stop_node(0, expected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE)
 
         self.log.info("make sure we get an init error when starting the node again with block filters")

test/functional/feature_coinstatsindex.py

@@ -245,6 +245,20 @@ class CoinStatsIndexTest(BitcoinTestFramework):
         res10 = index_node.gettxoutsetinfo('muhash')
         assert(res8['txouts'] < res10['txouts'])
 
+        self.log.info("Test that the index works with -reindex")
+
+        self.restart_node(1, extra_args=["-coinstatsindex", "-reindex"])
+        res11 = index_node.gettxoutsetinfo('muhash')
+        assert_equal(res11, res10)
+
+        self.log.info("Test that -reindex-chainstate is disallowed with coinstatsindex")
+
+        self.nodes[1].assert_start_raises_init_error(
+            expected_msg='Error: -reindex-chainstate option is not compatible with -coinstatsindex. '
+            'Please temporarily disable coinstatsindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes.',
+            extra_args=['-coinstatsindex', '-reindex-chainstate'],
+        )
+
     def _test_use_index_option(self):
         self.log.info("Test use_index option for nodes running the index")
 

test/functional/feature_init.py

@@ -0,0 +1,129 @@
+#!/usr/bin/env python3
+# Copyright (c) 2021 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Stress tests related to node initialization."""
+import os
+from pathlib import Path
+
+from test_framework.test_framework import BitcoinTestFramework, SkipTest
+from test_framework.test_node import ErrorMatch
+from test_framework.util import assert_equal
+
+
+class InitStressTest(BitcoinTestFramework):
+    """
+    Ensure that initialization can be interrupted at a number of points and not impair
+    subsequent starts.
+    """
+
+    def set_test_params(self):
+        self.setup_clean_chain = False
+        self.num_nodes = 1
+
+    def run_test(self):
+        """
+        - test terminating initialization after seeing a certain log line.
+        - test removing certain essential files to test startup error paths.
+        """
+        # TODO: skip Windows for now since it isn't clear how to SIGTERM.
+        #
+        # Windows doesn't support `process.terminate()`.
+        # and other approaches (like below) don't work:
+        #
+        #   os.kill(node.process.pid, signal.CTRL_C_EVENT)
+        if os.name == 'nt':
+            raise SkipTest("can't SIGTERM on Windows")
+
+        self.stop_node(0)
+        node = self.nodes[0]
+
+        def sigterm_node():
+            node.process.terminate()
+            node.process.wait()
+
+        def check_clean_start():
+            """Ensure that node restarts successfully after various interrupts."""
+            node.start()
+            node.wait_for_rpc_connection()
+            assert_equal(200, node.getblockcount())
+
+        lines_to_terminate_after = [
+            b'Validating signatures for all blocks',
+            b'scheduler thread start',
+            b'Starting HTTP server',
+            b'Loading P2P addresses',
+            b'Loading banlist',
+            b'Loading block index',
+            b'Switching active chainstate',
+            b'Checking all blk files are present',
+            b'Loaded best chain:',
+            b'init message: Verifying blocks',
+            b'init message: Starting network threads',
+            b'net thread start',
+            b'addcon thread start',
+            b'loadblk thread start',
+            b'txindex thread start',
+            b'block filter index thread start',
+            b'coinstatsindex thread start',
+            b'msghand thread start',
+            b'net thread start',
+            b'addcon thread start',
+        ]
+        if self.is_wallet_compiled():
+            lines_to_terminate_after.append(b'Verifying wallet')
+
+        for terminate_line in lines_to_terminate_after:
+            self.log.info(f"Starting node and will exit after line {terminate_line}")
+            with node.wait_for_debug_log([terminate_line]):
+                node.start(extra_args=['-txindex=1', '-blockfilterindex=1', '-coinstatsindex=1'])
+            self.log.debug("Terminating node after terminate line was found")
+            sigterm_node()
+
+        check_clean_start()
+        self.stop_node(0)
+
+        self.log.info("Test startup errors after removing certain essential files")
+
+        files_to_disturb = {
+            'blocks/index/*.ldb': 'Error opening block database.',
+            'chainstate/*.ldb': 'Error opening block database.',
+            'blocks/blk*.dat': 'Error loading block database.',
+        }
+
+        for file_patt, err_fragment in files_to_disturb.items():
+            target_files = list(node.chain_path.glob(file_patt))
+
+            for target_file in target_files:
+                self.log.info(f"Tweaking file to ensure failure {target_file}")
+                bak_path = str(target_file) + ".bak"
+                target_file.rename(bak_path)
+
+            # TODO: at some point, we should test perturbing the files instead of removing
+            # them, e.g.
+            #
+            # contents = target_file.read_bytes()
+            # tweaked_contents = bytearray(contents)
+            # tweaked_contents[50:250] = b'1' * 200
+            # target_file.write_bytes(bytes(tweaked_contents))
+            #
+            # At the moment I can't get this to work (bitcoind loads successfully?) so
+            # investigate doing this later.
+
+            node.assert_start_raises_init_error(
+                extra_args=['-txindex=1', '-blockfilterindex=1', '-coinstatsindex=1'],
+                expected_msg=err_fragment,
+                match=ErrorMatch.PARTIAL_REGEX,
+            )
+
+            for target_file in target_files:
+                bak_path = str(target_file) + ".bak"
+                self.log.debug(f"Restoring file from {bak_path} and restarting")
+                Path(bak_path).rename(target_file)
+
+            check_clean_start()
+            self.stop_node(0)
+
+
+if __name__ == '__main__':
+    InitStressTest().main()

test/functional/feature_reindex.py

@@ -22,7 +22,7 @@ class ReindexTest(BitcoinTestFramework):
         self.generatetoaddress(self.nodes[0], 3, self.nodes[0].get_deterministic_priv_key().address)
         blockcount = self.nodes[0].getblockcount()
         self.stop_nodes()
-        extra_args = [["-reindex-chainstate" if justchainstate else "-reindex"]]
+        extra_args = [["-reindex-chainstate" if justchainstate else "-reindex", "-txindex=0"]]
         self.start_nodes(extra_args)
         assert_equal(self.nodes[0].getblockcount(), blockcount)  # start_node is blocking on reindex
         self.log.info("Success")

test/functional/p2p_blockfilters.py

@@ -251,6 +251,17 @@ class CompactFiltersTest(BitcoinTestFramework):
         msg = "Error: Cannot set -peerblockfilters without -blockfilterindex."
         self.nodes[0].assert_start_raises_init_error(expected_msg=msg)
 
+        self.log.info("Test unknown value to -blockfilterindex raises an error")
+        self.nodes[0].extra_args = ["-blockfilterindex=abc"]
+        msg = "Error: Unknown -blockfilterindex value abc."
+        self.nodes[0].assert_start_raises_init_error(expected_msg=msg)
+
+        self.log.info("Test -blockfilterindex with -reindex-chainstate raises an error")
+        self.nodes[0].assert_start_raises_init_error(
+            expected_msg='Error: -reindex-chainstate option is not compatible with -blockfilterindex. '
+            'Please temporarily disable blockfilterindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes.',
+            extra_args=['-blockfilterindex', '-reindex-chainstate'],
+        )
 
 def compute_last_header(prev_header, hashes):
     """Compute the last filter header from a starting header and a sequence of filter hashes."""

test/functional/test_framework/test_node.py

@@ -20,6 +20,7 @@ import urllib.parse
 import shlex
 import sys
 import collections
+from pathlib import Path
 
 from .authproxy import JSONRPCException
 from .descriptors import descsum_create
@@ -400,22 +401,31 @@ class TestNode():
     def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT):
         wait_until_helper(self.is_node_stopped, timeout=timeout, timeout_factor=self.timeout_factor)
 
+    @property
+    def chain_path(self) -> Path:
+        return Path(self.datadir) / get_chain_folder(self.datadir, self.chain)
+
+    @property
+    def debug_log_path(self) -> Path:
+        return self.chain_path / 'debug.log'
+
+    def debug_log_bytes(self) -> int:
+        with open(self.debug_log_path, encoding='utf-8') as dl:
+            dl.seek(0, 2)
+            return dl.tell()
+
     @contextlib.contextmanager
     def assert_debug_log(self, expected_msgs, unexpected_msgs=None, timeout=2):
         if unexpected_msgs is None:
             unexpected_msgs = []
         time_end = time.time() + timeout * self.timeout_factor
-        chain = get_chain_folder(self.datadir, self.chain)
+        prev_size = self.debug_log_bytes()
-        debug_log = os.path.join(self.datadir, chain, 'debug.log')
-        with open(debug_log, encoding='utf-8') as dl:
-            dl.seek(0, 2)
-            prev_size = dl.tell()
 
         yield
 
         while True:
             found = True
-            with open(debug_log, encoding='utf-8') as dl:
+            with open(self.debug_log_path, encoding='utf-8') as dl:
                 dl.seek(prev_size)
                 log = dl.read()
             print_log = " - " + "\n - ".join(log.splitlines())
@@ -432,6 +442,42 @@ class TestNode():
             time.sleep(0.05)
         self._raise_assertion_error('Expected messages "{}" does not partially match log:\n\n{}\n\n'.format(str(expected_msgs), print_log))
 
+    @contextlib.contextmanager
+    def wait_for_debug_log(self, expected_msgs, timeout=60):
+        """
+        Block until we see a particular debug log message fragment or until we exceed the timeout.
+        Return:
+            the number of log lines we encountered when matching
+        """
+        time_end = time.time() + timeout * self.timeout_factor
+        prev_size = self.debug_log_bytes()
+
+        yield
+
+        while True:
+            found = True
+            with open(self.debug_log_path, "rb") as dl:
+                dl.seek(prev_size)
+                log = dl.read()
+
+            for expected_msg in expected_msgs:
+                if expected_msg not in log:
+                    found = False
+
+            if found:
+                return
+
+            if time.time() >= time_end:
+                print_log = " - " + "\n - ".join(log.decode("utf8", errors="replace").splitlines())
+                break
+
+            # No sleep here because we want to detect the message fragment as fast as
+            # possible.
+
+        self._raise_assertion_error(
+            'Expected messages "{}" does not partially match log:\n\n{}\n\n'.format(
+                str(expected_msgs), print_log))
+
     @contextlib.contextmanager
     def profile_with_perf(self, profile_name: str):
         """

test/functional/test_runner.py

@@ -327,6 +327,7 @@ BASE_SCRIPTS = [
     'rpc_wipewallettxes.py',
     'feature_dip0020_activation.py',
     'feature_uacomment.py',
+    'feature_init.py',
     'wallet_coinbase_category.py --legacy-wallet',
     'wallet_coinbase_category.py --descriptors',
     'feature_filelock.py',