From 617c0df26493cfaafa097f57e7ec328f319686ca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <bkchr@users.noreply.github.com>
Date: Wed, 21 Jul 2021 16:21:52 +0200
Subject: [PATCH] Do not set best block when there already exists a best block
with an higher number (#544)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* Do not set best block when there already exists a best block with an higher number
* Apply suggestions from code review
Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
---
.../common/src/parachain_consensus.rs | 12 +++
cumulus/client/consensus/common/src/tests.rs | 73 +++++++++++++++++--
2 files changed, 77 insertions(+), 8 deletions(-)
diff --git a/cumulus/client/consensus/common/src/parachain_consensus.rs b/cumulus/client/consensus/common/src/parachain_consensus.rs
index 6373563d467..b872b4366c1 100644
--- a/cumulus/client/consensus/common/src/parachain_consensus.rs
+++ b/cumulus/client/consensus/common/src/parachain_consensus.rs
@@ -346,6 +346,18 @@ where
P: UsageProvider<Block> + Send + Sync + BlockBackend<Block>,
for<'a> &'a P: BlockImport<Block>,
{
+ let best_number = parachain.usage_info().chain.best_number;
+ if *header.number() < best_number {
+ tracing::debug!(
+ target: "cumulus-consensus",
+ %best_number,
+ block_number = %header.number(),
+ "Skipping importing block as new best block, because there already exists a \
+ best block with an higher number",
+ );
+ return;
+ }
+
// Make it the new best block
let mut block_import_params = BlockImportParams::new(BlockOrigin::ConsensusBroadcast, header);
block_import_params.fork_choice = Some(ForkChoiceStrategy::Custom(true));
diff --git a/cumulus/client/consensus/common/src/tests.rs b/cumulus/client/consensus/common/src/tests.rs
index 7f6b2d6df49..4365e496d7f 100644
--- a/cumulus/client/consensus/common/src/tests.rs
+++ b/cumulus/client/consensus/common/src/tests.rs
@@ -19,7 +19,7 @@ use crate::*;
use codec::Encode;
use cumulus_test_client::{
runtime::{Block, Header},
- Client, InitBlockBuilder, TestClientBuilder, TestClientBuilderExt,
+ Backend, Client, InitBlockBuilder, TestClientBuilder, TestClientBuilderExt,
};
use futures::{channel::mpsc, executor::block_on, select, FutureExt, Stream, StreamExt};
use futures_timer::Delay;
@@ -101,18 +101,17 @@ impl crate::parachain_consensus::RelaychainClient for Relaychain {
}
}
-fn build_and_import_block(mut client: Arc<Client>) -> Block {
+fn build_and_import_block(mut client: Arc<Client>, import_as_best: bool) -> Block {
let builder = client.init_block_builder(None, Default::default());
let block = builder.build().unwrap().block;
let (header, body) = block.clone().deconstruct();
let mut block_import_params = BlockImportParams::new(BlockOrigin::Own, header);
- block_import_params.fork_choice = Some(ForkChoiceStrategy::Custom(false));
+ block_import_params.fork_choice = Some(ForkChoiceStrategy::Custom(import_as_best));
block_import_params.body = Some(body);
block_on(client.import_block(block_import_params, Default::default())).unwrap();
- assert_eq!(0, client.chain_info().best_number);
block
}
@@ -123,7 +122,7 @@ fn follow_new_best_works() {
let client = Arc::new(TestClientBuilder::default().build());
- let block = build_and_import_block(client.clone());
+ let block = build_and_import_block(client.clone(), false);
let relay_chain = Relaychain::new();
let new_best_heads_sender = relay_chain
.inner
@@ -164,7 +163,7 @@ fn follow_finalized_works() {
let client = Arc::new(TestClientBuilder::default().build());
- let block = build_and_import_block(client.clone());
+ let block = build_and_import_block(client.clone(), false);
let relay_chain = Relaychain::new();
let finalized_sender = relay_chain
.inner
@@ -205,7 +204,7 @@ fn follow_finalized_does_not_stop_on_unknown_block() {
let client = Arc::new(TestClientBuilder::default().build());
- let block = build_and_import_block(client.clone());
+ let block = build_and_import_block(client.clone(), false);
let unknown_block = {
let block_builder =
@@ -264,7 +263,7 @@ fn follow_new_best_sets_best_after_it_is_imported() {
let mut client = Arc::new(TestClientBuilder::default().build());
- let block = build_and_import_block(client.clone());
+ let block = build_and_import_block(client.clone(), false);
let unknown_block = {
let block_builder =
@@ -336,3 +335,61 @@ fn follow_new_best_sets_best_after_it_is_imported() {
}
});
}
+
+/// When we import a new best relay chain block, we extract the best parachain block from it and set
+/// it. This works when we follow the relay chain and parachain at the tip of each other, but there
+/// can be race conditions when we are doing a full sync of both or just the relay chain.
+/// The problem is that we import parachain blocks as best as long as we are in major sync. So, we
+/// could import block 100 as best and then import a relay chain block that says that block 99 is
+/// the best parachain block. This should not happen, we should never set the best block to a lower
+/// block number.
+#[test]
+fn do_not_set_best_block_to_older_block() {
+ const NUM_BLOCKS: usize = 4;
+
+ sp_tracing::try_init_simple();
+
+ let backend = Arc::new(Backend::new_test(1000, 1));
+
+ let client = Arc::new(TestClientBuilder::with_backend(backend).build());
+
+ let blocks = (0..NUM_BLOCKS)
+ .into_iter()
+ .map(|_| build_and_import_block(client.clone(), true))
+ .collect::<Vec<_>>();
+
+ assert_eq!(NUM_BLOCKS as u32, client.usage_info().chain.best_number);
+
+ let relay_chain = Relaychain::new();
+ let new_best_heads_sender = relay_chain
+ .inner
+ .lock()
+ .unwrap()
+ .new_best_heads_sender
+ .clone();
+
+ let consensus =
+ run_parachain_consensus(100.into(), client.clone(), relay_chain, Arc::new(|_, _| {}));
+
+ let client2 = client.clone();
+ let work = async move {
+ new_best_heads_sender
+ .unbounded_send(blocks[NUM_BLOCKS - 2].header().clone())
+ .unwrap();
+ // Wait for it to be processed.
+ Delay::new(Duration::from_millis(300)).await;
+ };
+
+ block_on(async move {
+ futures::pin_mut!(consensus);
+ futures::pin_mut!(work);
+
+ select! {
+ r = consensus.fuse() => panic!("Consensus should not end: {:?}", r),
+ _ = work.fuse() => {},
+ }
+ });
+
+ // Build and import a new best block.
+ build_and_import_block(client2.clone(), true);
+}
--
GitLab