Some cleanup; fix spurious failure in chainstate bootstrap tests

Author: ImplOfAnImpl

CHANGELOG.md

@@ -84,6 +84,8 @@ The format is loosely based on [Keep a Changelog](https://keepachangelog.com/en/
     - Importing a bootstrap file will no longer fail if some of the blocks already exist in the
       chainstate.
 
+    - Bootstrapping can now be interrupted via Ctrl-C.
+
     - The speed of the import was improved.
 
   - General

Cargo.lock

@@ -54,6 +54,7 @@ struct BootstrapFileHeader {
     pub blocks_count: u64,
 }
 
+/// Size of the encoded BootstrapFileHeader.
 const FILE_HEADER_SIZE: usize = 24;
 
 // In format v0, blocks go directly after the header, each block preceded by its length
@@ -134,20 +135,20 @@ pub fn import_bootstrap_stream<P, S: std::io::Read>(
 where
     P: FnMut(WithId<Block>) -> Result<bool, BootstrapError>,
 {
-    let mut buffer_queue = Vec::<u8>::with_capacity(1024 * 1024);
+    let mut buffer = Vec::<u8>::with_capacity(1024 * 1024);
 
     let header = {
-        fill_buffer(&mut buffer_queue, file_reader, FILE_HEADER_SIZE)?;
+        fill_buffer(&mut buffer, file_reader, FILE_HEADER_SIZE)?;
         ensure!(
-            buffer_queue.len() == FILE_HEADER_SIZE,
+            buffer.len() == FILE_HEADER_SIZE,
             BootstrapError::FileTooSmall
         );
-        check_for_legacy_format(&buffer_queue)?;
+        check_for_legacy_format(&buffer)?;
 
-        BootstrapFileHeader::decode_all(&mut buffer_queue.as_slice())?
+        BootstrapFileHeader::decode_all(&mut buffer.as_slice())?
     };
 
-    buffer_queue.clear();
+    buffer.clear();
 
     ensure!(
         &header.file_magic_bytes == FILE_MAGIC_BYTES,
@@ -163,33 +164,27 @@ where
     );
 
     for _ in 0..header.blocks_count {
-        fill_buffer(&mut buffer_queue, file_reader, size_of::<BlockSizeType>())?;
+        fill_buffer(&mut buffer, file_reader, size_of::<BlockSizeType>())?;
         ensure!(
-            buffer_queue.len() == size_of::<BlockSizeType>(),
+            buffer.len() == size_of::<BlockSizeType>(),
             BootstrapError::BadFileFormat
         );
         let block_size = BlockSizeType::from_le_bytes(
-            buffer_queue
-                .as_slice()
-                .try_into()
-                .expect("Buffer is known to have the correct size"),
+            buffer.as_slice().try_into().expect("Buffer is known to have the correct size"),
         )
         .try_into()?;
         ensure!(
             block_size <= MAX_BLOCK_SIZE,
             BootstrapError::BlockSizeTooBig(block_size)
         );
-        buffer_queue.clear();
+        buffer.clear();
 
-        fill_buffer(&mut buffer_queue, file_reader, block_size)?;
-        ensure!(
-            buffer_queue.len() == block_size,
-            BootstrapError::BadFileFormat
-        );
+        fill_buffer(&mut buffer, file_reader, block_size)?;
+        ensure!(buffer.len() == block_size, BootstrapError::BadFileFormat);
 
-        let block = Block::decode_all(&mut buffer_queue.as_slice())?;
+        let block = Block::decode_all(&mut buffer.as_slice())?;
         let should_continue = process_block_func(block.into())?;
-        buffer_queue.clear();
+        buffer.clear();
 
         if !should_continue {
             break;
@@ -210,19 +205,19 @@ fn check_for_legacy_format(header_bytes: &[u8]) -> Result<(), BootstrapError> {
 }
 
 fn fill_buffer<S: std::io::Read>(
-    buffer_queue: &mut Vec<u8>,
+    buffer: &mut Vec<u8>,
     reader: &mut std::io::BufReader<S>,
     max_buffer_size: usize,
 ) -> Result<(), BootstrapError> {
-    while buffer_queue.len() < max_buffer_size {
+    while buffer.len() < max_buffer_size {
         let data = reader.fill_buf()?;
         if data.is_empty() {
             break;
         }
 
-        let remaining_len = max_buffer_size - buffer_queue.len();
+        let remaining_len = max_buffer_size - buffer.len();
         let len_to_consume = std::cmp::min(remaining_len, data.len());
-        buffer_queue.extend_from_slice(&data[..len_to_consume]);
+        buffer.extend_from_slice(&data[..len_to_consume]);
         reader.consume(len_to_consume);
     }
 

chainstate/src/detail/bootstrap.rs

@@ -903,6 +903,8 @@ impl<S: BlockchainStorage, V: TransactionVerificationStrategy> Chainstate<S, V>
         let chain_config = Arc::clone(&self.chain_config);
         let mut block_processor = |block: WithId<Block>| -> Result<_, BootstrapError> {
             // If chainstate is being shutdown, stop immediately.
+            // Note that we return an error instead of Ok(false) to avoid an interrupted import
+            // being treated as successful.
             if self.shutdown_initiated_rx.as_ref().is_some_and(|rx| rx.borrow().test()) {
                 return Err(BootstrapError::Interrupted);
             }
@@ -995,6 +997,8 @@ where
 ///
 /// The main purpose is to make sure the reckless mode stays enabled during bootstrapping for
 /// the entire duration (i.e. for fresh blocks too).
+///
+/// Note: it is assumed that inc/dec are only called during bootstrapping or ibd.
 struct DbRecklessModeCounter {
     counter: u32,
 }

chainstate/src/detail/mod.rs

@@ -30,10 +30,10 @@ use common::{
     primitives::{BlockHeight, Id},
     time_getter::TimeGetter,
 };
-use detail::Chainstate;
-use interface::chainstate_interface_impl;
 use utils::set_flag::SetFlag;
 
+use crate::{detail::Chainstate, interface::chainstate_interface_impl};
+
 pub use crate::{
     config::{ChainstateConfig, MaxTipAge},
     detail::{

chainstate/src/lib.rs

@@ -47,6 +47,16 @@ fn check_height_order(blocks: &[Id<Block>], tf: &TestFramework) {
     }
 }
 
+fn make_chain_config(chain_type: ChainType) -> ChainConfig {
+    chain::config::Builder::new(chain_type)
+        .consensus_upgrades(NetUpgrades::unit_tests())
+        .data_in_no_signature_witness_allowed(true)
+        .genesis_unittest(Destination::AnyoneCanSpend)
+        // Force empty checkpoints because a custom genesis is used.
+        .checkpoints(BTreeMap::new())
+        .build()
+}
+
 const EXPECTED_MAGIC_BYTES: &str = "MLBTSTRP";
 
 fn make_header_data(chain_config: &ChainConfig, version: u32, blocks_count: u64) -> Vec<u8> {
@@ -69,16 +79,20 @@ fn gen_blocks(
     blocks_count: usize,
     mut rng: impl Rng + CryptoRng,
 ) -> Vec<Block> {
-    let mut tf = TestFramework::builder(&mut rng).with_chain_config(chain_config).build();
-    let genesis_id = tf.genesis().get_id();
-    tf.create_chain(&genesis_id.into(), blocks_count, &mut rng).unwrap();
-
-    tf.chainstate
-        .get_block_id_tree_as_list()
-        .unwrap()
-        .iter()
-        .map(|block_id| tf.chainstate.get_block(block_id).unwrap().unwrap())
-        .collect_vec()
+    if blocks_count > 0 {
+        let mut tf = TestFramework::builder(&mut rng).with_chain_config(chain_config).build();
+        let genesis_id = tf.genesis().get_id();
+        tf.create_chain(&genesis_id.into(), blocks_count, &mut rng).unwrap();
+
+        tf.chainstate
+            .get_block_id_tree_as_list()
+            .unwrap()
+            .iter()
+            .map(|block_id| tf.chainstate.get_block(block_id).unwrap().unwrap())
+            .collect_vec()
+    } else {
+        Vec::new()
+    }
 }
 
 fn export_to_vec(tf: &TestFramework, with_stale_blocks: bool) -> Vec<u8> {
@@ -528,16 +542,6 @@ fn unsupported_version(#[case] seed: Seed) {
     });
 }
 
-fn make_chain_config(chain_type: ChainType) -> ChainConfig {
-    chain::config::Builder::new(chain_type)
-        .consensus_upgrades(NetUpgrades::unit_tests())
-        .data_in_no_signature_witness_allowed(true)
-        .genesis_unittest(Destination::AnyoneCanSpend)
-        // Force empty checkpoints because a custom genesis is used.
-        .checkpoints(BTreeMap::new())
-        .build()
-}
-
 // The data starts with wrong format magic bytes.
 #[rstest]
 #[trace]
@@ -707,6 +711,8 @@ fn block_size_too_big(#[case] seed: Seed) {
     });
 }
 
+// If chainstate reckless mode is enabled, it should be switched on when bootstrapping starts
+// and off when it ends, including the case when it ends due to an error.
 #[rstest]
 #[trace]
 #[case(Seed::from_entropy())]

chainstate/test-suite/src/tests/bootstrap.rs

@@ -9,7 +9,6 @@ authors = ["Samer Afach <samer.afach@mintlayer.org>", "Ben Marsh <benjamin.marsh
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-chainstate = { path = "../chainstate" }
 logging = { path = "../logging" }
 node-lib = { path = "../node-lib/" }
 utils = { path = "../utils" }

node-daemon/Cargo.toml

@@ -52,10 +52,10 @@ pub async fn run_node_daemon() -> anyhow::Result<ExitCode> {
                     );
                 }
                 Err(err) => {
-                    // Note: we don't return it as a generic error, because bootstrapping will
-                    // likely fail due to user's mistake rather than node's malfunction, so we
-                    // don't want for e.g. the stack trace to be printed (which would happen with
-                    // a "normal" error when backtrace is enabled).
+                    // Note: we don't return an error here, because bootstrapping will likely fail
+                    // due to a user mistake rather than node malfunction, so we don't want for
+                    // e.g. the stack trace to be printed in this case (anyhow::Error does this
+                    // when backtrace is enabled).
                     logging::log::error!("Node bootstrapping failed: {err}");
                     return Ok(ExitCode(1));
                 }

node-lib/src/node_daemon_runner.rs

@@ -107,14 +107,6 @@ impl<T> FailingImpl<T> {
             transaction_failures: 0,
         }
     }
-
-    pub fn inner(&self) -> &T {
-        &self.inner
-    }
-
-    pub fn inner_mut(&mut self) -> &mut T {
-        &mut self.inner
-    }
 }
 
 impl<T: Clone> Clone for FailingImpl<T> {

storage/failing/src/backend.rs

@@ -71,10 +71,6 @@ impl<B: Backend, Sch: Schema> Storage<B, Sch> {
     pub fn backend_impl(&self) -> &B::Impl {
         &self.backend
     }
-
-    pub fn backend_impl_mut(&mut self) -> &mut B::Impl {
-        &mut self.backend
-    }
 }
 
 impl<B: Backend, Sch: Schema> Storage<B, Sch> {