From 91f601f0bf1089fa2d19f46cae2a913ae4073807 Mon Sep 17 00:00:00 2001
From: Dmitry Markin <dmitry@markin.tech>
Date: Fri, 1 Mar 2024 13:19:17 +0200
Subject: [PATCH] Fix crash of synced parachain node run with `--sync=warp`
 (#3523)

Fixes https://github.com/paritytech/polkadot-sdk/issues/3496.
---
 substrate/client/network/sync/src/engine.rs   |  2 +
 substrate/client/network/sync/src/strategy.rs | 88 +++++++++++++++++--
 2 files changed, 82 insertions(+), 8 deletions(-)

diff --git a/substrate/client/network/sync/src/engine.rs b/substrate/client/network/sync/src/engine.rs
index c1a7009eeb017..24640fdc45576 100644
--- a/substrate/client/network/sync/src/engine.rs
+++ b/substrate/client/network/sync/src/engine.rs
@@ -656,6 +656,8 @@ where
 					Some(event) => self.process_notification_event(event),
 					None => return,
 				},
+				// TODO: setting of warp sync target block should be moved to the initialization of
+				// `SyncingEngine`, see https://github.com/paritytech/polkadot-sdk/issues/3537.
 				warp_target_block_header = &mut self.warp_sync_target_block_header_rx_fused => {
 					if let Err(_) = self.pass_warp_sync_target_block_header(warp_target_block_header) {
 						error!(
diff --git a/substrate/client/network/sync/src/strategy.rs b/substrate/client/network/sync/src/strategy.rs
index 7d6e6a8d3b8b7..dabcf37ae6321 100644
--- a/substrate/client/network/sync/src/strategy.rs
+++ b/substrate/client/network/sync/src/strategy.rs
@@ -30,7 +30,7 @@ use crate::{
 };
 use chain_sync::{ChainSync, ChainSyncAction, ChainSyncMode};
 use libp2p::PeerId;
-use log::{debug, error, info};
+use log::{debug, error, info, warn};
 use prometheus_endpoint::Registry;
 use sc_client_api::{BlockBackend, ProofProvider};
 use sc_consensus::{BlockImportError, BlockImportStatus, IncomingBlock};
@@ -159,7 +159,7 @@ impl<B: BlockT> From<ChainSyncAction<B>> for SyncingAction<B> {
 
 /// Proxy to specific syncing strategies.
 pub struct SyncingStrategy<B: BlockT, Client> {
-	/// Syncing configuration.
+	/// Initial syncing configuration.
 	config: SyncingConfig,
 	/// Client used by syncing strategies.
 	client: Arc<Client>,
@@ -466,15 +466,27 @@ where
 		&mut self,
 		target_header: B::Header,
 	) -> Result<(), ()> {
-		match self.warp {
-			Some(ref mut warp) => {
-				warp.set_target_block(target_header);
-				Ok(())
+		match self.config.mode {
+			SyncMode::Warp => match self.warp {
+				Some(ref mut warp) => {
+					warp.set_target_block(target_header);
+					Ok(())
+				},
+				None => {
+					// As mode is set to warp sync, but no warp sync strategy is active, this means
+					// that warp sync has already finished / was skipped.
+					warn!(
+						target: LOG_TARGET,
+						"Discarding warp sync target, as warp sync was seemingly skipped due \
+						 to node being (partially) synced.",
+					);
+					Ok(())
+				},
 			},
-			None => {
+			_ => {
 				error!(
 					target: LOG_TARGET,
-					"Cannot set warp sync target block: no warp sync strategy is active."
+					"Cannot set warp sync target block: not in warp sync mode."
 				);
 				debug_assert!(false);
 				Err(())
@@ -587,3 +599,63 @@ where
 		}
 	}
 }
+
+#[cfg(test)]
+mod test {
+	use super::*;
+	use futures::executor::block_on;
+	use sc_block_builder::BlockBuilderBuilder;
+	use substrate_test_runtime_client::{
+		ClientBlockImportExt, ClientExt, DefaultTestClientBuilderExt, TestClientBuilder,
+		TestClientBuilderExt,
+	};
+
+	/// Regression test for crash when starting already synced parachain node with `--sync=warp`.
+	/// We must remove this after setting of warp sync target block is moved to initialization of
+	/// `SyncingEngine` (issue https://github.com/paritytech/polkadot-sdk/issues/3537).
+	#[test]
+	fn set_target_block_finished_warp_sync() {
+		// Populate database with finalized state.
+		let mut client = Arc::new(TestClientBuilder::new().build());
+		let block = BlockBuilderBuilder::new(&*client)
+			.on_parent_block(client.chain_info().best_hash)
+			.with_parent_block_number(client.chain_info().best_number)
+			.build()
+			.unwrap()
+			.build()
+			.unwrap()
+			.block;
+		block_on(client.import(BlockOrigin::Own, block.clone())).unwrap();
+		let just = (*b"TEST", Vec::new());
+		client.finalize_block(block.hash(), Some(just)).unwrap();
+		let target_block = BlockBuilderBuilder::new(&*client)
+			.on_parent_block(client.chain_info().best_hash)
+			.with_parent_block_number(client.chain_info().best_number)
+			.build()
+			.unwrap()
+			.build()
+			.unwrap()
+			.block;
+
+		// Initialize syncing strategy.
+		let config = SyncingConfig {
+			mode: SyncMode::Warp,
+			max_parallel_downloads: 3,
+			max_blocks_per_request: 64,
+			metrics_registry: None,
+		};
+		let mut strategy =
+			SyncingStrategy::new(config, client, Some(WarpSyncConfig::WaitForTarget)).unwrap();
+
+		// Warp sync instantly finishes as we have finalized state in DB.
+		let actions = strategy.actions().unwrap();
+		assert_eq!(actions.len(), 1);
+		assert!(matches!(actions[0], SyncingAction::Finished));
+		assert!(strategy.warp.is_none());
+
+		// Try setting the target block. We mustn't crash.
+		strategy
+			.set_warp_sync_target_block_header(target_block.header().clone())
+			.unwrap();
+	}
+}