From a7f4804749605ef2a362d8af9d6d88f1a5582d03 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Mon, 24 Oct 2022 05:28:21 -0400 Subject: [PATCH] Move Commit from including the round to including the round's end_time The round was usable to build the current clock in an accumulated fashion, relative to the previous round. The end time is the absolute metric of it, which can be used to calculate the round number (with all previous end times). Substrate now builds off the best block, not genesis, using the end time included in the justification to start its machine in a synchronized state. Knowing the end time of a round, or the round in which block was committed to, is necessary for nodes to sync up with Tendermint. Encoding it in the commit ensures it's long lasting and makes it readily available, without the load of an entire transaction. --- substrate/consensus/src/import_queue.rs | 30 +++++++++++++++-- substrate/consensus/src/tendermint.rs | 8 +++-- substrate/tendermint/src/ext.rs | 6 ++-- substrate/tendermint/src/lib.rs | 44 +++++++++++++++++++------ 4 files changed, 69 insertions(+), 19 deletions(-) diff --git a/substrate/consensus/src/import_queue.rs b/substrate/consensus/src/import_queue.rs index 64cefaa7..bf9b2ceb 100644 --- a/substrate/consensus/src/import_queue.rs +++ b/substrate/consensus/src/import_queue.rs @@ -3,9 +3,9 @@ use std::{ sync::{Arc, RwLock}, task::{Poll, Context}, future::Future, - time::SystemTime, }; +use sp_core::Decode; use sp_inherents::CreateInherentDataProviders; use sp_runtime::traits::{Header, Block}; use sp_api::{BlockId, TransactionFor}; @@ -18,9 +18,14 @@ use sc_client_api::Backend; use substrate_prometheus_endpoint::Registry; -use tendermint_machine::{ext::BlockNumber, TendermintMachine}; +use tendermint_machine::{ + ext::{BlockNumber, Commit}, + TendermintMachine, +}; use crate::{ + CONSENSUS_ID, + signature_scheme::TendermintSigner, tendermint::{TendermintClient, TendermintImport}, Announce, }; @@ -98,12 +103,31 @@ where let authority = { let machine_clone = import.machine.clone(); let mut import_clone = import.clone(); + let best = import.client.info().best_number; async move { *machine_clone.write().unwrap() = Some(TendermintMachine::new( import_clone.clone(), // TODO 0, - (BlockNumber(1), SystemTime::now()), + ( + // Header::Number: TryInto doesn't implement Debug and can't be unwrapped + match best.try_into() { + Ok(best) => BlockNumber(best), + Err(_) => panic!("BlockNumber exceeded u64"), + }, + Commit::::decode( + &mut import_clone + .client + .justifications(&BlockId::Number(best)) + .unwrap() + .unwrap() + .get(CONSENSUS_ID) + .unwrap() + .as_ref(), + ) + .unwrap() + .end_time, + ), import_clone .get_proposal(&import_clone.client.header(BlockId::Number(0u8.into())).unwrap().unwrap()) .await, diff --git a/substrate/consensus/src/tendermint.rs b/substrate/consensus/src/tendermint.rs index 80bf68ee..ec6a8d6c 100644 --- a/substrate/consensus/src/tendermint.rs +++ b/substrate/consensus/src/tendermint.rs @@ -24,7 +24,7 @@ use sp_consensus::{Error, BlockOrigin, Proposer, Environment}; use sc_consensus::{ForkChoiceStrategy, BlockImportParams, BlockImport, import_queue::IncomingBlock}; use sc_service::ImportQueue; -use sc_client_api::{Backend, Finalizer}; +use sc_client_api::{BlockBackend, Backend, Finalizer}; use tendermint_machine::{ ext::{BlockError, Commit, Network}, @@ -43,6 +43,7 @@ pub trait TendermintClient + 'static>: Send + Sync + HeaderBackend + + BlockBackend + BlockImport> + Finalizer + ProvideRuntimeApi @@ -55,6 +56,7 @@ impl< C: Send + Sync + HeaderBackend + + BlockBackend + BlockImport> + Finalizer + ProvideRuntimeApi @@ -379,8 +381,8 @@ where let info = self.client.info(); assert_eq!(info.best_hash, parent); assert_eq!(info.finalized_hash, parent); - assert_eq!(info.best_number, number - 1); - assert_eq!(info.finalized_number, number - 1); + assert_eq!(info.best_number, number - 1u8.into()); + assert_eq!(info.finalized_number, number - 1u8.into()); } Ok(()) diff --git a/substrate/tendermint/src/ext.rs b/substrate/tendermint/src/ext.rs index 96536e12..6becaa7a 100644 --- a/substrate/tendermint/src/ext.rs +++ b/substrate/tendermint/src/ext.rs @@ -64,8 +64,8 @@ pub trait SignatureScheme: Send + Sync { /// a valid commit. #[derive(Clone, PartialEq, Debug, Encode, Decode)] pub struct Commit { - /// Round which created this commit. - pub round: Round, + /// End time of the round, used as the start time of next round. + pub end_time: u64, /// Validators participating in the signature. pub validators: Vec, /// Aggregate signature. @@ -151,7 +151,7 @@ pub trait Network: Send + Sync { ) -> bool { if !self.signature_scheme().verify_aggregate( &commit.validators, - &commit_msg(commit.round, id.as_ref()), + &commit_msg(commit.end_time, id.as_ref()), &commit.signature, ) { return false; diff --git a/substrate/tendermint/src/lib.rs b/substrate/tendermint/src/lib.rs index f9b9de56..4a6a3884 100644 --- a/substrate/tendermint/src/lib.rs +++ b/substrate/tendermint/src/lib.rs @@ -2,7 +2,7 @@ use core::fmt::Debug; use std::{ sync::Arc, - time::{SystemTime, Instant, Duration}, + time::{UNIX_EPOCH, SystemTime, Instant, Duration}, collections::HashMap, }; @@ -24,8 +24,8 @@ use ext::*; mod message_log; use message_log::MessageLog; -pub(crate) fn commit_msg(round: Round, id: &[u8]) -> Vec { - [&round.0.to_le_bytes(), id].concat().to_vec() +pub(crate) fn commit_msg(end_time: u64, id: &[u8]) -> Vec { + [&end_time.to_le_bytes(), id].concat().to_vec() } #[derive(Clone, Copy, PartialEq, Eq, Hash, Debug, Encode, Decode)] @@ -95,6 +95,7 @@ pub struct TendermintMachine { proposer: N::ValidatorId, number: BlockNumber, + canonical_start_time: u64, start_time: Instant, personal_proposal: N::Block, @@ -126,9 +127,21 @@ pub struct TendermintHandle { } impl TendermintMachine { + // Get the canonical end time for a given round, represented as seconds since the epoch + // While we have the Instant already in end_time, converting it to a SystemTime would be lossy, + // potentially enough to cause a consensus failure. Independently tracking this variable ensures + // that won't happen + fn canonical_end_time(&self, round: Round) -> u64 { + let mut time = self.canonical_start_time; + for r in 0 .. u64::from(round.0 + 1) { + time += (r + 1) * u64::from(N::BLOCK_TIME); + } + time + } + fn timeout(&self, step: Step) -> Instant { let mut round_time = Duration::from_secs(N::BLOCK_TIME.into()); - round_time *= self.round.0.wrapping_add(1); + round_time *= self.round.0 + 1; let step_time = round_time / 3; let offset = match step { @@ -193,6 +206,7 @@ impl TendermintMachine { sleep(round_end.saturating_duration_since(Instant::now())).await; self.number.0 += 1; + self.canonical_start_time = self.canonical_end_time(end_round); self.start_time = round_end; self.personal_proposal = proposal; @@ -213,7 +227,7 @@ impl TendermintMachine { pub fn new( network: N, proposer: N::ValidatorId, - start: (BlockNumber, SystemTime), + start: (BlockNumber, u64), proposal: N::Block, ) -> TendermintHandle { // Convert the start time to an instant @@ -221,7 +235,10 @@ impl TendermintMachine { let start_time = { let instant_now = Instant::now(); let sys_now = SystemTime::now(); - instant_now - sys_now.duration_since(start.1).unwrap_or(Duration::ZERO) + instant_now - + sys_now + .duration_since(UNIX_EPOCH + Duration::from_secs(start.1)) + .unwrap_or(Duration::ZERO) }; let (msg_send, mut msg_recv) = mpsc::channel(100); // Backlog to accept. Currently arbitrary @@ -239,6 +256,7 @@ impl TendermintMachine { proposer, number: start.0, + canonical_start_time: start.1, start_time, personal_proposal: proposal, @@ -318,7 +336,7 @@ impl TendermintMachine { } let commit = Commit { - round: msg.round, + end_time: machine.canonical_end_time(msg.round), validators, signature: N::SignatureScheme::aggregate(&sigs), }; @@ -349,9 +367,13 @@ impl TendermintMachine { &mut self, msg: Message::Signature>, ) -> Result, TendermintError> { - // Verify the signature if this is a precommit + // Verify the end time and signature if this is a precommit if let Data::Precommit(Some((id, sig))) = &msg.data { - if !self.signer.verify(msg.sender, &commit_msg(msg.round, id.as_ref()), sig) { + if !self.signer.verify( + msg.sender, + &commit_msg(self.canonical_end_time(msg.round), id.as_ref()), + sig, + ) { // Since we verified this validator actually sent the message, they're malicious Err(TendermintError::Malicious(msg.sender))?; } @@ -495,7 +517,9 @@ impl TendermintMachine { self.locked = Some((self.round, block.id())); self.broadcast(Data::Precommit(Some(( block.id(), - self.signer.sign(&commit_msg(self.round, block.id().as_ref())), + self + .signer + .sign(&commit_msg(self.canonical_end_time(self.round), block.id().as_ref())), )))); return Ok(None); }