Merge bitcoin/bitcoin#27850: test: Add unit & functional test coverage for blockstore

de8f9123af test: cover read-only blockstore (Matthew Zipkin)
5c2185b3b6 ci: enable chattr +i capability inside containers (Matthew Zipkin)
e573f24202 unit test: add coverage for BlockManager (Matthew Zipkin)

Pull request description:

  This PR adds unit and functional tests to cover the behavior described in #2039. In particular, that bitcoind will crash on startup if a reindex is requested but the `blk` files are read-only. Eventually this behavior can be updated with https://github.com/bitcoin/bitcoin/pull/27039. This PR just commits the test coverage from #27039 as suggested in https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1584915782

ACKs for top commit:
  jonatack:
    ACK de8f9123af modulo suggestions in https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1319010039, tested on macOS, but not on Linux for the Linux-related change in the last push
  achow101:
    ACK de8f9123af
  MarcoFalke:
    lgtm ACK de8f9123af 📶

Tree-SHA512: b9bd684035dcea11c901b649fc39f397a2155a9a8459f3348e67947e387e45312fddeccb52981aef486f8a31deebb5356a7901c1bb94b78f82c24192a369af73

ci/test/00_setup_env.sh

@@ -67,7 +67,7 @@ export BASE_BUILD_DIR=${BASE_BUILD_DIR:-$BASE_SCRATCH_DIR/build}
 # The folder for previous release binaries.
 # This folder exists only on the ci guest, and on the ci host as a volume.
 export PREVIOUS_RELEASES_DIR=${PREVIOUS_RELEASES_DIR:-$BASE_ROOT_DIR/prev_releases}
-export CI_BASE_PACKAGES=${CI_BASE_PACKAGES:-build-essential libtool autotools-dev automake pkg-config bsdmainutils curl ca-certificates ccache python3 rsync git procps bison}
+export CI_BASE_PACKAGES=${CI_BASE_PACKAGES:-build-essential libtool autotools-dev automake pkg-config bsdmainutils curl ca-certificates ccache python3 rsync git procps bison e2fsprogs}
 export GOAL=${GOAL:-install}
 export DIR_QA_ASSETS=${DIR_QA_ASSETS:-${BASE_SCRATCH_DIR}/qa-assets}
 export CI_RETRY_EXE=${CI_RETRY_EXE:-"retry --"}

ci/test/00_setup_env_i686_centos.sh

@@ -9,7 +9,7 @@ export LC_ALL=C.UTF-8
 export HOST=i686-pc-linux-gnu
 export CONTAINER_NAME=ci_i686_centos
 export CI_IMAGE_NAME_TAG="quay.io/centos/amd64:stream9"
-export CI_BASE_PACKAGES="gcc-c++ glibc-devel.x86_64 libstdc++-devel.x86_64 glibc-devel.i686 libstdc++-devel.i686 ccache libtool make git python3 python3-pip which patch lbzip2 xz procps-ng dash rsync coreutils bison util-linux"
+export CI_BASE_PACKAGES="gcc-c++ glibc-devel.x86_64 libstdc++-devel.x86_64 glibc-devel.i686 libstdc++-devel.i686 ccache libtool make git python3 python3-pip which patch lbzip2 xz procps-ng dash rsync coreutils bison util-linux e2fsprogs"
 export PIP_PACKAGES="pyzmq"
 export GOAL="install"
 export NO_WERROR=1  # Suppress error: #warning _FORTIFY_SOURCE > 2 is treated like 2 on this platform [-Werror=cpp]

ci/test/04_install.sh

@@ -31,7 +31,9 @@ if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then
   fi
 
   # shellcheck disable=SC2086
-  CI_CONTAINER_ID=$(docker run $CI_CONTAINER_CAP --rm --interactive --detach --tty \
+
+
+  CI_CONTAINER_ID=$(docker run --cap-add LINUX_IMMUTABLE $CI_CONTAINER_CAP --rm --interactive --detach --tty \
                   --mount "type=bind,src=$BASE_READ_ONLY_DIR,dst=$BASE_READ_ONLY_DIR,readonly" \
                   --mount "type=volume,src=${CONTAINER_NAME}_ccache,dst=$CCACHE_DIR" \
                   --mount "type=volume,src=${CONTAINER_NAME}_depends,dst=$DEPENDS_DIR" \

src/test/blockmanager_tests.cpp

@@ -8,10 +8,12 @@
 #include <node/context.h>
 #include <node/kernel_notifications.h>
 #include <script/solver.h>
+#include <primitives/block.h>
 #include <util/chaintype.h>
 #include <validation.h>
 
 #include <boost/test/unit_test.hpp>
+#include <test/util/logging.h>
 #include <test/util/setup_common.h>
 
 using node::BLOCK_SERIALIZATION_HEADER_SIZE;
@@ -130,4 +132,73 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
     BOOST_CHECK(!blockman.CheckBlockDataAvailability(tip, *last_pruned_block));
 }
 
+BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file)
+{
+    KernelNotifications notifications{m_node.exit_status};
+    node::BlockManager::Options blockman_opts{
+        .chainparams = Params(),
+        .blocks_dir = m_args.GetBlocksDirPath(),
+        .notifications = notifications,
+    };
+    BlockManager blockman{m_node.kernel->interrupt, blockman_opts};
+
+    // Test blocks with no transactions, not even a coinbase
+    CBlock block1;
+    block1.nVersion = 1;
+    CBlock block2;
+    block2.nVersion = 2;
+    CBlock block3;
+    block3.nVersion = 3;
+
+    // They are 80 bytes header + 1 byte 0x00 for vtx length
+    constexpr int TEST_BLOCK_SIZE{81};
+
+    // Blockstore is empty
+    BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), 0);
+
+    // Write the first block; dbp=nullptr means this block doesn't already have a disk
+    // location, so allocate a free location and write it there.
+    FlatFilePos pos1{blockman.SaveBlockToDisk(block1, /*nHeight=*/1, /*dbp=*/nullptr)};
+
+    // Write second block
+    FlatFilePos pos2{blockman.SaveBlockToDisk(block2, /*nHeight=*/2, /*dbp=*/nullptr)};
+
+    // Two blocks in the file
+    BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), (TEST_BLOCK_SIZE + BLOCK_SERIALIZATION_HEADER_SIZE) * 2);
+
+    // First two blocks are written as expected
+    // Errors are expected because block data is junk, thrown AFTER successful read
+    CBlock read_block;
+    BOOST_CHECK_EQUAL(read_block.nVersion, 0);
+    {
+        ASSERT_DEBUG_LOG("ReadBlockFromDisk: Errors in block header");
+        BOOST_CHECK(!blockman.ReadBlockFromDisk(read_block, pos1));
+        BOOST_CHECK_EQUAL(read_block.nVersion, 1);
+    }
+    {
+        ASSERT_DEBUG_LOG("ReadBlockFromDisk: Errors in block header");
+        BOOST_CHECK(!blockman.ReadBlockFromDisk(read_block, pos2));
+        BOOST_CHECK_EQUAL(read_block.nVersion, 2);
+    }
+
+    // When FlatFilePos* dbp is given, SaveBlockToDisk() will not write or
+    // overwrite anything to the flat file block storage. It will, however,
+    // update the blockfile metadata. This is to facilitate reindexing
+    // when the user has the blocks on disk but the metadata is being rebuilt.
+    // Verify this behavior by attempting (and failing) to write block 3 data
+    // to block 2 location.
+    CBlockFileInfo* block_data = blockman.GetBlockFileInfo(0);
+    BOOST_CHECK_EQUAL(block_data->nBlocks, 2);
+    BOOST_CHECK(blockman.SaveBlockToDisk(block3, /*nHeight=*/3, /*dbp=*/&pos2) == pos2);
+    // Metadata is updated...
+    BOOST_CHECK_EQUAL(block_data->nBlocks, 3);
+    // ...but there are still only two blocks in the file
+    BOOST_CHECK_EQUAL(blockman.CalculateCurrentUsage(), (TEST_BLOCK_SIZE + BLOCK_SERIALIZATION_HEADER_SIZE) * 2);
+
+    // Block 2 was not overwritten:
+    //   SaveBlockToDisk() did not call WriteBlockToDisk() because `FlatFilePos* dbp` was non-null
+    blockman.ReadBlockFromDisk(read_block, pos2);
+    BOOST_CHECK_EQUAL(read_block.nVersion, 2);
+}
+
 BOOST_AUTO_TEST_SUITE_END()

test/functional/feature_reindex_readonly.py

@@ -0,0 +1,64 @@
+#!/usr/bin/env python3
+# Copyright (c) 2023-present The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test running bitcoind with -reindex from a read-only blockstore
+- Start a node, generate blocks, then restart with -reindex after setting blk files to read-only
+"""
+
+import platform
+import stat
+import subprocess
+from test_framework.test_framework import BitcoinTestFramework
+
+
+class BlockstoreReindexTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+        self.extra_args = [["-fastprune"]]
+
+    def reindex_readonly(self):
+        self.log.debug("Generate block big enough to start second block file")
+        fastprune_blockfile_size = 0x10000
+        opreturn = "6a"
+        nulldata = fastprune_blockfile_size * "ff"
+        self.generateblock(self.nodes[0], output=f"raw({opreturn}{nulldata})", transactions=[])
+        self.stop_node(0)
+
+        assert (self.nodes[0].chain_path / "blocks" / "blk00000.dat").exists()
+        assert (self.nodes[0].chain_path / "blocks" / "blk00001.dat").exists()
+
+        self.log.debug("Make the first block file read-only")
+        filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat"
+        filename.chmod(stat.S_IREAD)
+
+        used_chattr = False
+        if platform.system() == "Linux":
+            try:
+                subprocess.run(['chattr', '+i', filename], capture_output=True, check=True)
+                used_chattr = True
+                self.log.info("Made file immutable with chattr")
+            except subprocess.CalledProcessError as e:
+                self.log.warning(str(e))
+                if e.stdout:
+                    self.log.warning(f"stdout: {e.stdout}")
+                if e.stderr:
+                    self.log.warning(f"stderr: {e.stderr}")
+
+        self.log.debug("Attempt to restart and reindex the node with the unwritable block file")
+        with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]):
+            self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'],
+                expected_msg="Error: A fatal internal error occurred, see debug.log for details")
+
+        if used_chattr:
+            subprocess.check_call(['chattr', '-i', filename])
+
+        filename.chmod(0o777)
+
+    def run_test(self):
+        self.reindex_readonly()
+
+
+if __name__ == '__main__':
+    BlockstoreReindexTest().main()

test/functional/test_runner.py

@@ -162,6 +162,7 @@ BASE_SCRIPTS = [
     'wallet_abandonconflict.py --legacy-wallet',
     'wallet_abandonconflict.py --descriptors',
     'feature_reindex.py',
+    'feature_reindex_readonly.py',
     'wallet_labels.py --legacy-wallet',
     'wallet_labels.py --descriptors',
     'p2p_compactblocks.py',