From 9241bdc3b5304b74aa5f367f0708a79e10d64361 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 28 Jan 2023 02:35:32 -0500 Subject: [PATCH] Fix #183 --- .../tendermint/client/src/authority/mod.rs | 36 ++++++++++--------- substrate/tendermint/machine/src/block.rs | 12 +++---- substrate/tendermint/machine/src/ext.rs | 2 +- substrate/tendermint/machine/src/lib.rs | 20 ++++++++--- substrate/tendermint/machine/tests/ext.rs | 4 +-- 5 files changed, 41 insertions(+), 33 deletions(-) diff --git a/substrate/tendermint/client/src/authority/mod.rs b/substrate/tendermint/client/src/authority/mod.rs index 379784f0..4181b5ee 100644 --- a/substrate/tendermint/client/src/authority/mod.rs +++ b/substrate/tendermint/client/src/authority/mod.rs @@ -22,7 +22,7 @@ use sp_runtime::{ }; use sp_blockchain::HeaderBackend; -use sp_consensus::{Error, BlockOrigin, Proposer, Environment}; +use sp_consensus::{Error, BlockOrigin, BlockStatus, Proposer, Environment}; use sc_consensus::import_queue::IncomingBlock; use sc_service::ImportQueue; @@ -86,7 +86,6 @@ async fn get_proposal( env: &Arc>, import: &TendermintImport, header: &::Header, - stub: bool, ) -> T::Block { let proposer = env.lock().await.init(header).await.expect("Failed to create a proposer for the new block"); @@ -95,15 +94,11 @@ async fn get_proposal( .propose( import.inherent_data(*header.parent_hash()).await, Digest::default(), - if stub { - Duration::ZERO - } else { - // The first processing time is to build the block - // The second is for it to be downloaded (assumes a block won't take longer to download - // than it'll take to process) - // The third is for it to actually be processed - Duration::from_secs((T::BLOCK_PROCESSING_TIME_IN_SECONDS / 3).into()) - }, + // The first processing time is to build the block + // The second is for it to be downloaded (assumes a block won't take longer to download + // than it'll take to process) + // The third is for it to actually be processed + Duration::from_secs((T::BLOCK_PROCESSING_TIME_IN_SECONDS / 3).into()), Some(T::PROPOSED_BLOCK_SIZE_LIMIT), ) .await @@ -118,7 +113,7 @@ impl TendermintAuthority { } async fn get_proposal(&self, header: &::Header) -> T::Block { - get_proposal(&self.active.as_ref().unwrap().env, &self.import, header, false).await + get_proposal(&self.active.as_ref().unwrap().env, &self.import, header).await } /// Create and run a new Tendermint Authority, proposing and voting on blocks. @@ -263,9 +258,10 @@ impl TendermintAuthority { step.send(( BlockNumber(number), Commit::decode(&mut justifications.get(CONSENSUS_ID).unwrap().as_ref()).unwrap(), - // This will fail if syncing occurs radically faster than machine stepping takes - // TODO: Set true when initial syncing - get_proposal(&env, &import, ¬if.header, false).await + // Creating a proposal will fail if syncing occurs radically faster than machine + // stepping takes + // Don't create proposals when stepping accordingly + None )).await.unwrap(); } else { debug!( @@ -437,10 +433,16 @@ impl Network for TendermintAuthority { &mut self, block: T::Block, commit: Commit>, - ) -> T::Block { + ) -> Option { // Prevent import_block from being called while we run let _lock = self.import.sync_lock.lock().await; + // If we didn't import this block already, return + // If it's a legitimate block, we'll pick it up in the standard sync loop + if self.import.client.block_status(block.hash()).unwrap() != BlockStatus::InChainWithState { + return None; + } + // Check if we already imported this externally if self.import.client.justifications(block.hash()).unwrap().is_some() { debug!(target: "tendermint", "Machine produced a commit after we already synced it"); @@ -487,6 +489,6 @@ impl Network for TendermintAuthority { // Clear any blocks for the previous slot which we were willing to recheck *self.import.recheck.write().unwrap() = HashSet::new(); - self.get_proposal(block.header()).await + Some(self.get_proposal(block.header()).await) } } diff --git a/substrate/tendermint/machine/src/block.rs b/substrate/tendermint/machine/src/block.rs index 92923ae9..8136f888 100644 --- a/substrate/tendermint/machine/src/block.rs +++ b/substrate/tendermint/machine/src/block.rs @@ -14,7 +14,7 @@ use crate::{ pub(crate) struct BlockData { pub(crate) number: BlockNumber, pub(crate) validator_id: Option, - pub(crate) proposal: N::Block, + pub(crate) proposal: Option, pub(crate) log: MessageLog, pub(crate) slashes: HashSet, @@ -35,7 +35,7 @@ impl BlockData { weights: Arc, number: BlockNumber, validator_id: Option, - proposal: N::Block, + proposal: Option, ) -> BlockData { BlockData { number, @@ -106,12 +106,8 @@ impl BlockData { // 14-21 if Some(proposer) == self.validator_id { - let (round, block) = if let Some((round, block)) = &self.valid { - (Some(*round), block.clone()) - } else { - (None, self.proposal.clone()) - }; - Some(Data::Proposal(round, block)) + let (round, block) = self.valid.clone().unzip(); + block.or_else(|| self.proposal.clone()).map(|block| Data::Proposal(round, block)) } else { self.round_mut().set_timeout(Step::Propose); None diff --git a/substrate/tendermint/machine/src/ext.rs b/substrate/tendermint/machine/src/ext.rs index daa684c3..f78b3608 100644 --- a/substrate/tendermint/machine/src/ext.rs +++ b/substrate/tendermint/machine/src/ext.rs @@ -270,5 +270,5 @@ pub trait Network: Send + Sync { &mut self, block: Self::Block, commit: Commit, - ) -> Self::Block; + ) -> Option; } diff --git a/substrate/tendermint/machine/src/lib.rs b/substrate/tendermint/machine/src/lib.rs index 487f4cbf..4146dbb5 100644 --- a/substrate/tendermint/machine/src/lib.rs +++ b/substrate/tendermint/machine/src/lib.rs @@ -135,7 +135,8 @@ pub struct TendermintMachine { queue: VecDeque>, msg_recv: mpsc::UnboundedReceiver>, - step_recv: mpsc::UnboundedReceiver<(BlockNumber, Commit, N::Block)>, + #[allow(clippy::type_complexity)] + step_recv: mpsc::UnboundedReceiver<(BlockNumber, Commit, Option)>, block: BlockData, } @@ -143,7 +144,7 @@ pub struct TendermintMachine { pub type StepSender = mpsc::UnboundedSender<( BlockNumber, Commit<::SignatureScheme>, - ::Block, + Option<::Block>, )>; pub type MessageSender = mpsc::UnboundedSender>; @@ -186,7 +187,7 @@ impl TendermintMachine { } // 53-54 - async fn reset(&mut self, end_round: RoundNumber, proposal: N::Block) { + async fn reset(&mut self, end_round: RoundNumber, proposal: Option) { // Ensure we have the end time data for the last round self.block.populate_end_time(end_round); @@ -209,7 +210,11 @@ impl TendermintMachine { self.round(RoundNumber(0), Some(round_end)); } - async fn reset_by_commit(&mut self, commit: Commit, proposal: N::Block) { + async fn reset_by_commit( + &mut self, + commit: Commit, + proposal: Option, + ) { let mut round = self.block.round().number; // If this commit is for a round we don't have, jump up to it while self.block.end_time[&round].canonical() < commit.end_time { @@ -271,7 +276,12 @@ impl TendermintMachine { msg_recv, step_recv, - block: BlockData::new(weights, BlockNumber(last_block.0 + 1), validator_id, proposal), + block: BlockData::new( + weights, + BlockNumber(last_block.0 + 1), + validator_id, + Some(proposal), + ), }; // The end time of the last block is the start time for this one diff --git a/substrate/tendermint/machine/tests/ext.rs b/substrate/tendermint/machine/tests/ext.rs index a02afc12..818f2acd 100644 --- a/substrate/tendermint/machine/tests/ext.rs +++ b/substrate/tendermint/machine/tests/ext.rs @@ -140,11 +140,11 @@ impl Network for TestNetwork { &mut self, block: TestBlock, commit: Commit, - ) -> TestBlock { + ) -> Option { dbg!("Adding ", &block); assert!(block.valid.is_ok()); assert!(self.verify_commit(block.id(), &commit)); - TestBlock { id: (u32::from_le_bytes(block.id) + 1).to_le_bytes(), valid: Ok(()) } + Some(TestBlock { id: (u32::from_le_bytes(block.id) + 1).to_le_bytes(), valid: Ok(()) }) } }