Verify the header in the substrate import queue (paritytech#44)

* Verify header in substrate import queue

* No panic for empty block download batch list

* FMT

crates/sc-consensus-nakamoto/src/lib.rs

@@ -13,6 +13,7 @@ pub use block_import::{
     insert_bitcoin_block_hash_mapping, BitcoinBlockImport, BitcoinBlockImporter, ImportConfig,
     ImportStatus,
 };
+pub use chain_params::ChainParams;
 pub use import_queue::{
     bitcoin_import_queue, BlockImportQueue, ImportBlocks, ImportManyBlocksResult,
 };

crates/sc-consensus-nakamoto/src/verification.rs

@@ -189,7 +189,7 @@ where
     ) -> Result<(), Error> {
         match self.block_verification {
             BlockVerification::Full => {
-                let lock_time_cutoff = self.header_verifier.verify_header(&block.header)?;
+                let lock_time_cutoff = self.header_verifier.verify(&block.header)?;
 
                 if block_number >= self.chain_params.segwit_height
                     && !block.check_witness_commitment()
@@ -205,7 +205,7 @@ where
                 self.verify_transactions(block_number, block, txids, lock_time_cutoff)?;
             }
             BlockVerification::HeaderOnly => {
-                self.header_verifier.verify_header(&block.header)?;
+                self.header_verifier.verify(&block.header)?;
             }
             BlockVerification::None => {}
         }

crates/sc-consensus-nakamoto/src/verification/header_verify.rs

@@ -71,7 +71,7 @@ where
     ///     - The time must be greater than the median time of the last 11 blocks.
     ///
     /// <https://github.com/bitcoin/bitcoin/blob/6f9db1ebcab4064065ccd787161bf2b87e03cc1f/src/validation.cpp#L4146>
-    pub fn verify_header(&self, header: &BitcoinHeader) -> Result<u32, Error> {
+    pub fn verify(&self, header: &BitcoinHeader) -> Result<u32, Error> {
         let prev_block_hash = header.prev_blockhash;
 
         let prev_block_header = self.client.block_header(prev_block_hash).ok_or(

crates/subcoin-network/src/block_downloader/headers_first.rs

@@ -381,7 +381,7 @@ where
                 best_queued_number = self.download_manager.best_queued_number,
                 requested_blocks_count = get_data_msg.len(),
                 downloaded_headers = self.downloaded_headers.len(),
-                "Headers from {start} to {end} downloaded, requesting blocks",
+                "Downloaded headers from {start} to {end}, requesting blocks",
             );
 
             self.download_state =
@@ -397,7 +397,14 @@ where
                 .collect::<VecDeque<HashSet<_>>>();
 
             let total_batches = batches.len();
-            let initial_batch = batches.pop_front().expect("Batch must not be empty; qed");
+
+            let Some(initial_batch) = batches.pop_front() else {
+                tracing::warn!(
+                    ?total_batches,
+                    "Download batches is empty, failed to start new block download"
+                );
+                return SyncAction::None;
+            };
 
             let get_data_msg =
                 prepare_ordered_block_data_request(initial_batch.clone(), &self.downloaded_headers);

crates/subcoin-node/src/cli.rs

@@ -181,7 +181,7 @@ pub fn run() -> sc_cli::Result<()> {
                     task_manager,
                     import_queue,
                     ..
-                } = subcoin_service::new_partial(&config)?;
+                } = subcoin_service::new_partial(&config, bitcoin::Network::Bitcoin)?;
                 Ok((cmd.run(client, import_queue), task_manager))
             })
         }
@@ -192,7 +192,7 @@ pub fn run() -> sc_cli::Result<()> {
                     client,
                     task_manager,
                     ..
-                } = subcoin_service::new_partial(&config)?;
+                } = subcoin_service::new_partial(&config, bitcoin::Network::Bitcoin)?;
                 Ok((cmd.run(client, config.database), task_manager))
             })
         }
@@ -203,7 +203,7 @@ pub fn run() -> sc_cli::Result<()> {
                     client,
                     task_manager,
                     ..
-                } = subcoin_service::new_partial(&config)?;
+                } = subcoin_service::new_partial(&config, bitcoin::Network::Bitcoin)?;
 
                 let run_cmd = async move {
                     tracing::info!("Exporting raw state...");
@@ -235,7 +235,7 @@ pub fn run() -> sc_cli::Result<()> {
                     task_manager,
                     backend,
                     ..
-                } = subcoin_service::new_partial(&config)?;
+                } = subcoin_service::new_partial(&config, bitcoin::Network::Bitcoin)?;
                 Ok((cmd.run(client, backend, None), task_manager))
             })
         }
@@ -251,7 +251,7 @@ pub fn run() -> sc_cli::Result<()> {
                     }
                     BenchmarkCmd::Block(cmd) => {
                         let PartialComponents { client, .. } =
-                            subcoin_service::new_partial(&config)?;
+                            subcoin_service::new_partial(&config, bitcoin::Network::Bitcoin)?;
                         cmd.run(client)
                     }
                     #[cfg(not(feature = "runtime-benchmarks"))]
@@ -263,7 +263,7 @@ pub fn run() -> sc_cli::Result<()> {
                     BenchmarkCmd::Storage(cmd) => {
                         let PartialComponents {
                             client, backend, ..
-                        } = subcoin_service::new_partial(&config)?;
+                        } = subcoin_service::new_partial(&config, bitcoin::Network::Bitcoin)?;
                         let db = backend.expose_db();
                         let storage = backend.expose_storage();
 

crates/subcoin-node/src/commands/run.rs

@@ -97,7 +97,7 @@ impl RunCmd {
         storage_monitor: sc_storage_monitor::StorageMonitorParams,
     ) -> sc_cli::Result<TaskManager> {
         let block_execution_strategy = run.common_params.block_execution_strategy();
-        let network = run.common_params.bitcoin_network();
+        let bitcoin_network = run.common_params.bitcoin_network();
         let import_config = run.common_params.import_config();
         let no_finalizer = run.no_finalizer;
         let major_sync_confirmation_depth = run.major_sync_confirmation_depth;
@@ -107,11 +107,10 @@ impl RunCmd {
             backend,
             mut task_manager,
             block_executor,
-            keystore_container,
             telemetry,
             ..
         } = subcoin_service::new_node(subcoin_service::SubcoinConfiguration {
-            network,
+            network: bitcoin_network,
             config: &config,
             block_execution_strategy,
             no_hardware_benchmarks,
@@ -141,7 +140,7 @@ impl RunCmd {
 
         let (subcoin_networking, subcoin_network_handle) = subcoin_network::Network::new(
             client.clone(),
-            run.subcoin_network_params(network),
+            run.subcoin_network_params(bitcoin_network),
             import_queue,
             spawn_handle.clone(),
             config.prometheus_registry().cloned(),
@@ -174,7 +173,7 @@ impl RunCmd {
                     client.clone(),
                     backend,
                     &mut task_manager,
-                    keystore_container.keystore(),
+                    bitcoin_network,
                     telemetry,
                 )?
             }
@@ -184,7 +183,7 @@ impl RunCmd {
                     client.clone(),
                     backend,
                     &mut task_manager,
-                    keystore_container.keystore(),
+                    bitcoin_network,
                     telemetry,
                 )?
             }

crates/subcoin-service/src/lib.rs

@@ -15,7 +15,7 @@ use genesis_block_builder::GenesisBlockBuilder;
 use sc_client_api::{AuxStore, BlockchainEvents, Finalizer, HeaderBackend};
 use sc_consensus::import_queue::BasicQueue;
 use sc_consensus::{BlockImportParams, Verifier};
-use sc_consensus_nakamoto::{BlockExecutionStrategy, BlockExecutor};
+use sc_consensus_nakamoto::{BlockExecutionStrategy, BlockExecutor, ChainParams, HeaderVerifier};
 use sc_executor::NativeElseWasmExecutor;
 use sc_network_sync::SyncingService;
 use sc_service::config::PrometheusConfig;
@@ -28,7 +28,6 @@ use sc_utils::mpsc::TracingUnboundedSender;
 use sp_consensus::SyncOracle;
 use sp_core::traits::SpawnNamed;
 use sp_core::Encode;
-use sp_keystore::KeystorePtr;
 use sp_runtime::traits::{Block as BlockT, CheckedSub, Header as HeaderT};
 use std::ops::Deref;
 use std::path::Path;
@@ -272,7 +271,7 @@ pub fn start_substrate_network<N>(
     client: Arc<FullClient>,
     _backend: Arc<FullBackend>,
     task_manager: &mut TaskManager,
-    _keystore: KeystorePtr,
+    bitcoin_network: bitcoin::Network,
     mut telemetry: Option<Telemetry>,
 ) -> Result<SubstrateNetworkingParts, ServiceError>
 where
@@ -294,7 +293,7 @@ where
     );
 
     let import_queue = BasicQueue::new(
-        SubstrateImportQueueVerifier,
+        SubstrateImportQueueVerifier::new(client.clone(), bitcoin_network),
         Box::new(client.clone()),
         None,
         &task_manager.spawn_essential_handle(),
@@ -389,15 +388,15 @@ pub async fn finalize_confirmed_blocks<Block, Client, Backend>(
     Backend: sc_client_api::backend::Backend<Block> + 'static,
 {
     // Use `every_import_notification_stream()` so that we can receive the notifications even when
-    // major syncing.
+    // the Substrate network is major syncing.
     let mut block_import_stream = client.every_import_notification_stream();
 
     while let Some(notification) = block_import_stream.next().await {
         let block_number = client
             .number(notification.hash)
             .ok()
             .flatten()
-            .expect("Imported Block must be available; qed");
+            .expect("Imported block must be available; qed");
 
         let Some(confirmed_block_number) = block_number.checked_sub(&confirmation_depth.into())
         else {
@@ -411,14 +410,20 @@ pub async fn finalize_confirmed_blocks<Block, Client, Backend>(
         }
 
         if subcoin_networking_is_major_syncing.load(Ordering::Relaxed) {
-            // During major sync, finalize every `major_sync_confirmation_depth` block to avoid race conditions:
+            // During the major sync of Subcoin networking, we choose to finalize every `major_sync_confirmation_depth`
+            // block to avoid race conditions:
             // >Safety violation: attempted to revert finalized block...
             if confirmed_block_number < finalized_number + major_sync_confirmation_depth.into() {
                 continue;
             }
         }
 
         if let Some(sync_service) = substrate_sync_service.as_ref() {
+            // State sync relies on the finalized block notification to progress
+            // Substrate chain sync component relies on the finalized block notification to
+            // initiate the state sync, do not attempt to finalize the block when the queued blocks
+            // are not empty, so that the state sync can be started when the last finalized block
+            // notification is sent.
             if sync_service.is_major_syncing()
                 && sync_service.num_queued_blocks().await.unwrap_or(0) > 0
             {
@@ -450,6 +455,8 @@ pub async fn finalize_confirmed_blocks<Block, Client, Backend>(
                             || substrate_sync_service
                                 .map(|sync_service| sync_service.is_major_syncing())
                                 .unwrap_or(false);
+
+                        // Only print the log when not major syncing to not clutter the logs.
                         if !is_major_syncing {
                             tracing::info!("✅ Successfully finalized block: {block_to_finalize}");
                         }
@@ -477,7 +484,10 @@ type PartialComponents = sc_service::PartialComponents<
 >;
 
 /// Creates a partial node, for the chain ops commands.
-pub fn new_partial(config: &Configuration) -> Result<PartialComponents, ServiceError> {
+pub fn new_partial(
+    config: &Configuration,
+    bitcoin_network: bitcoin::Network,
+) -> Result<PartialComponents, ServiceError> {
     let telemetry = config
         .telemetry_endpoints
         .clone()
@@ -517,7 +527,7 @@ pub fn new_partial(config: &Configuration) -> Result<PartialComponents, ServiceE
     );
 
     let import_queue = BasicQueue::new(
-        SubstrateImportQueueVerifier,
+        SubstrateImportQueueVerifier::new(client.clone(), bitcoin_network),
         Box::new(client.clone()),
         None,
         &task_manager.spawn_essential_handle(),
@@ -539,23 +549,46 @@ pub fn new_partial(config: &Configuration) -> Result<PartialComponents, ServiceE
 /// Verifier used by the Substrate import queue.
 ///
 /// Verifies the blocks received from the Substrate networking.
-pub struct SubstrateImportQueueVerifier;
+pub struct SubstrateImportQueueVerifier<Block, Client> {
+    btc_header_verifier: HeaderVerifier<Block, Client>,
+}
+
+impl<Block, Client> SubstrateImportQueueVerifier<Block, Client> {
+    /// Constructs a new instance of [`SubstrateImportQueueVerifier`].
+    pub fn new(client: Arc<Client>, network: bitcoin::Network) -> Self {
+        Self {
+            btc_header_verifier: HeaderVerifier::new(client, ChainParams::new(network)),
+        }
+    }
+}
 
 #[async_trait::async_trait]
-impl<Block: BlockT> Verifier<Block> for SubstrateImportQueueVerifier {
+impl<Block, Client> Verifier<Block> for SubstrateImportQueueVerifier<Block, Client>
+where
+    Block: BlockT,
+    Client: HeaderBackend<Block> + AuxStore,
+{
     async fn verify(
         &self,
         mut block_import_params: BlockImportParams<Block>,
     ) -> Result<BlockImportParams<Block>, String> {
-        // TODO: Verify header.
+        let substrate_header = &block_import_params.header;
+
+        let btc_header =
+            subcoin_primitives::extract_bitcoin_block_header::<Block>(substrate_header)
+                .map_err(|err| format!("Failed to extract bitcoin header: {err:?}"))?;
+
+        self.btc_header_verifier
+            .verify(&btc_header)
+            .map_err(|err| format!("Invalid header: {err:?}"))?;
 
         block_import_params.fork_choice = Some(sc_consensus::ForkChoiceStrategy::LongestChain);
 
         let bitcoin_block_hash =
-            subcoin_primitives::extract_bitcoin_block_hash::<Block>(&block_import_params.header)
+            subcoin_primitives::extract_bitcoin_block_hash::<Block>(substrate_header)
                 .map_err(|err| format!("Failed to extract bitcoin block hash: {err:?}"))?;
 
-        let substrate_block_hash = block_import_params.header.hash();
+        let substrate_block_hash = substrate_header.hash();
 
         sc_consensus_nakamoto::insert_bitcoin_block_hash_mapping::<Block>(
             &mut block_import_params,