From 5b56fd51e28f1c9edb2baf0189605ff98234ddee Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Tue, 24 Oct 2023 11:24:44 +0200 Subject: [PATCH 1/3] block import: update fork choice is option in cumulus-client-consensus-common --- cumulus/client/consensus/common/src/lib.rs | 38 ++++++++++++++------ cumulus/client/consensus/common/src/tests.rs | 6 ++-- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/cumulus/client/consensus/common/src/lib.rs b/cumulus/client/consensus/common/src/lib.rs index 08bceabb2bd4a..6972d079cd33b 100644 --- a/cumulus/client/consensus/common/src/lib.rs +++ b/cumulus/client/consensus/common/src/lib.rs @@ -111,12 +111,14 @@ impl ParachainConsensus for Box + Send + /// Parachain specific block import. /// -/// This is used to set `block_import_params.fork_choice` to `false` as long as the block origin is -/// not `NetworkInitialSync`. The best block for parachains is determined by the relay chain. -/// Meaning we will update the best block, as it is included by the relay-chain. +/// If `update_fork_choice` is `true` this is used to set `block_import_params.fork_choice` to +/// `false` as long as the block origin is not `NetworkInitialSync`. The best block for parachains +/// is determined by the relay chain. Meaning we will update the best block, as it is included by +/// the relay-chain. pub struct ParachainBlockImport { inner: BI, monitor: Option>>, + update_fork_choice: bool, } impl> ParachainBlockImport { @@ -141,13 +143,27 @@ impl> ParachainBlockImport let monitor = level_limit.map(|level_limit| SharedData::new(LevelMonitor::new(level_limit, backend))); - Self { inner, monitor } + Self { inner, monitor, update_fork_choice: false } + } + + /// Create a new instance which allows to update `params.fork_choice`. + /// + /// The number of leaves per level limit is set to `LevelLimit::Default`. + pub fn new_with_update_fork_choice_enabled(inner: BI, backend: Arc) -> Self { + Self { + update_fork_choice: true, + ..Self::new_with_limit(inner, backend, LevelLimit::Default) + } } } impl Clone for ParachainBlockImport { fn clone(&self) -> Self { - ParachainBlockImport { inner: self.inner.clone(), monitor: self.monitor.clone() } + ParachainBlockImport { + inner: self.inner.clone(), + monitor: self.monitor.clone(), + update_fork_choice: self.update_fork_choice, + } } } @@ -182,11 +198,13 @@ where params.finalized = true; } - // Best block is determined by the relay chain, or if we are doing the initial sync - // we import all blocks as new best. - params.fork_choice = Some(sc_consensus::ForkChoiceStrategy::Custom( - params.origin == sp_consensus::BlockOrigin::NetworkInitialSync, - )); + if self.update_fork_choice { + // Best block is determined by the relay chain, or if we are doing the initial sync + // we import all blocks as new best. + params.fork_choice = Some(sc_consensus::ForkChoiceStrategy::Custom( + params.origin == sp_consensus::BlockOrigin::NetworkInitialSync, + )); + } let maybe_lock = self.monitor.as_ref().map(|monitor_lock| { let mut monitor = monitor_lock.shared_data_locked(); diff --git a/cumulus/client/consensus/common/src/tests.rs b/cumulus/client/consensus/common/src/tests.rs index 9658a0add790d..3e5409af3b800 100644 --- a/cumulus/client/consensus/common/src/tests.rs +++ b/cumulus/client/consensus/common/src/tests.rs @@ -1124,7 +1124,8 @@ fn find_potential_parents_aligned_with_pending() { let backend = Arc::new(Backend::new_test(1000, 1)); let client = Arc::new(TestClientBuilder::with_backend(backend.clone()).build()); - let mut para_import = ParachainBlockImport::new(client.clone(), backend.clone()); + let mut para_import = + ParachainBlockImport::new_with_update_fork_choice_enabled(client.clone(), backend.clone()); let relay_parent = relay_hash_from_block_num(10); // Choose different relay parent for alternative chain to get new hashes. @@ -1279,7 +1280,8 @@ fn find_potential_parents_aligned_no_pending() { let backend = Arc::new(Backend::new_test(1000, 1)); let client = Arc::new(TestClientBuilder::with_backend(backend.clone()).build()); - let mut para_import = ParachainBlockImport::new(client.clone(), backend.clone()); + let mut para_import = + ParachainBlockImport::new_with_update_fork_choice_enabled(client.clone(), backend.clone()); let relay_parent = relay_hash_from_block_num(10); // Choose different relay parent for alternative chain to get new hashes. From 6ae495cab4b44ff13ce95297f474c0722fb52b36 Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Tue, 21 Nov 2023 13:48:01 +0100 Subject: [PATCH 2/3] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- cumulus/client/consensus/common/src/lib.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/cumulus/client/consensus/common/src/lib.rs b/cumulus/client/consensus/common/src/lib.rs index 6972d079cd33b..9de73d33aa587 100644 --- a/cumulus/client/consensus/common/src/lib.rs +++ b/cumulus/client/consensus/common/src/lib.rs @@ -111,14 +111,11 @@ impl ParachainConsensus for Box + Send + /// Parachain specific block import. /// -/// If `update_fork_choice` is `true` this is used to set `block_import_params.fork_choice` to -/// `false` as long as the block origin is not `NetworkInitialSync`. The best block for parachains -/// is determined by the relay chain. Meaning we will update the best block, as it is included by -/// the relay-chain. +/// Specialized block import for parachains. It supports to delay setting the best block until the relay chain has included a candidate in its best block. By default the delayed best block setting is disabled. The block import also monitors the imported blocks and prunes by default if there are too many blocks at the same height. Too many blocks at the same height can for example happen if the relay chain is rejecting the parachain blocks in the validation. pub struct ParachainBlockImport { inner: BI, monitor: Option>>, - update_fork_choice: bool, + delayed_best_block: bool, } impl> ParachainBlockImport { @@ -146,10 +143,10 @@ impl> ParachainBlockImport Self { inner, monitor, update_fork_choice: false } } - /// Create a new instance which allows to update `params.fork_choice`. + /// Create a new instance which delays setting the best block. /// /// The number of leaves per level limit is set to `LevelLimit::Default`. - pub fn new_with_update_fork_choice_enabled(inner: BI, backend: Arc) -> Self { + pub fn new_with_delayed_best_block(inner: BI, backend: Arc) -> Self { Self { update_fork_choice: true, ..Self::new_with_limit(inner, backend, LevelLimit::Default) From b7962f994e4656182470524aec3ffbb46ba2b923 Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Tue, 21 Nov 2023 14:41:29 +0100 Subject: [PATCH 3/3] fixes --- cumulus/client/consensus/common/src/lib.rs | 14 +++++++++----- cumulus/client/consensus/common/src/tests.rs | 4 ++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/cumulus/client/consensus/common/src/lib.rs b/cumulus/client/consensus/common/src/lib.rs index 9de73d33aa587..cebe34e7ea588 100644 --- a/cumulus/client/consensus/common/src/lib.rs +++ b/cumulus/client/consensus/common/src/lib.rs @@ -111,7 +111,11 @@ impl ParachainConsensus for Box + Send + /// Parachain specific block import. /// -/// Specialized block import for parachains. It supports to delay setting the best block until the relay chain has included a candidate in its best block. By default the delayed best block setting is disabled. The block import also monitors the imported blocks and prunes by default if there are too many blocks at the same height. Too many blocks at the same height can for example happen if the relay chain is rejecting the parachain blocks in the validation. +/// Specialized block import for parachains. It supports to delay setting the best block until the +/// relay chain has included a candidate in its best block. By default the delayed best block +/// setting is disabled. The block import also monitors the imported blocks and prunes by default if +/// there are too many blocks at the same height. Too many blocks at the same height can for example +/// happen if the relay chain is rejecting the parachain blocks in the validation. pub struct ParachainBlockImport { inner: BI, monitor: Option>>, @@ -140,7 +144,7 @@ impl> ParachainBlockImport let monitor = level_limit.map(|level_limit| SharedData::new(LevelMonitor::new(level_limit, backend))); - Self { inner, monitor, update_fork_choice: false } + Self { inner, monitor, delayed_best_block: false } } /// Create a new instance which delays setting the best block. @@ -148,7 +152,7 @@ impl> ParachainBlockImport /// The number of leaves per level limit is set to `LevelLimit::Default`. pub fn new_with_delayed_best_block(inner: BI, backend: Arc) -> Self { Self { - update_fork_choice: true, + delayed_best_block: true, ..Self::new_with_limit(inner, backend, LevelLimit::Default) } } @@ -159,7 +163,7 @@ impl Clone for ParachainBlockImport { ParachainBlockImport { inner: self.inner.clone(), monitor: self.monitor.clone(), - update_fork_choice: self.update_fork_choice, + delayed_best_block: self.delayed_best_block, } } } @@ -195,7 +199,7 @@ where params.finalized = true; } - if self.update_fork_choice { + if self.delayed_best_block { // Best block is determined by the relay chain, or if we are doing the initial sync // we import all blocks as new best. params.fork_choice = Some(sc_consensus::ForkChoiceStrategy::Custom( diff --git a/cumulus/client/consensus/common/src/tests.rs b/cumulus/client/consensus/common/src/tests.rs index 3e5409af3b800..597d1ab2acc2c 100644 --- a/cumulus/client/consensus/common/src/tests.rs +++ b/cumulus/client/consensus/common/src/tests.rs @@ -1125,7 +1125,7 @@ fn find_potential_parents_aligned_with_pending() { let backend = Arc::new(Backend::new_test(1000, 1)); let client = Arc::new(TestClientBuilder::with_backend(backend.clone()).build()); let mut para_import = - ParachainBlockImport::new_with_update_fork_choice_enabled(client.clone(), backend.clone()); + ParachainBlockImport::new_with_delayed_best_block(client.clone(), backend.clone()); let relay_parent = relay_hash_from_block_num(10); // Choose different relay parent for alternative chain to get new hashes. @@ -1281,7 +1281,7 @@ fn find_potential_parents_aligned_no_pending() { let backend = Arc::new(Backend::new_test(1000, 1)); let client = Arc::new(TestClientBuilder::with_backend(backend.clone()).build()); let mut para_import = - ParachainBlockImport::new_with_update_fork_choice_enabled(client.clone(), backend.clone()); + ParachainBlockImport::new_with_delayed_best_block(client.clone(), backend.clone()); let relay_parent = relay_hash_from_block_num(10); // Choose different relay parent for alternative chain to get new hashes.