From 3462a4b7fadb5562bb4000013636ad5be770f18c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= 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 +++ client/consensus/common/src/tests.rs | 73 +++++++++++++++++-- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/client/consensus/common/src/parachain_consensus.rs b/client/consensus/common/src/parachain_consensus.rs index 6373563d467c..b872b4366c1c 100644 --- a/client/consensus/common/src/parachain_consensus.rs +++ b/client/consensus/common/src/parachain_consensus.rs @@ -346,6 +346,18 @@ where P: UsageProvider + Send + Sync + BlockBackend, for<'a> &'a P: BlockImport, { + 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/client/consensus/common/src/tests.rs b/client/consensus/common/src/tests.rs index 7f6b2d6df49b..4365e496d7f5 100644 --- a/client/consensus/common/src/tests.rs +++ b/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) -> Block { +fn build_and_import_block(mut client: Arc, 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::>(); + + 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); +}