diff --git a/substrate/core/finality-grandpa/src/lib.rs b/substrate/core/finality-grandpa/src/lib.rs index fa9703c8cd..d00c94cdbf 100644 --- a/substrate/core/finality-grandpa/src/lib.rs +++ b/substrate/core/finality-grandpa/src/lib.rs @@ -95,7 +95,7 @@ use consensus_common::{BlockImport, JustificationImport, Error as ConsensusError use runtime_primitives::Justification; use runtime_primitives::traits::{ NumberFor, Block as BlockT, Header as HeaderT, DigestFor, ProvideRuntimeApi, Hash as HashT, - DigestItemFor, DigestItem, As, Zero, + DigestItemFor, DigestItem, As, One, Zero, }; use fg_primitives::GrandpaApi; use runtime_primitives::generic::BlockId; @@ -422,16 +422,57 @@ impl, B, E, N, RA> grandpa::Chain { - let header = self.inner.header(&BlockId::Hash(hash)).ok()? + match self.inner.best_containing(block, None) { + Ok(Some(mut best_hash)) => { + let base_header = self.inner.header(&BlockId::Hash(block)).ok()? .expect("Header known to exist after `best_containing` call; qed"); - Some((hash, header.number().clone())) + if let Some(limit) = limit { + // this is a rare case which might cause issues, + // might be better to return the header itself. + if *base_header.number() > limit { + debug!(target: "afg", "Encountered error finding best chain containing {:?} with limit {:?}: target block is after limit", + block, + limit, + ); + return None; + } + } + + let mut best_header = self.inner.header(&BlockId::Hash(best_hash)).ok()? + .expect("Header known to exist after `best_containing` call; qed"); + + // we target a vote towards 3/4 of the unfinalized chain (rounding up) + let target = { + let two = NumberFor::::one() + One::one(); + let three = two + One::one(); + let four = three + One::one(); + + let diff = *best_header.number() - *base_header.number(); + let diff = ((diff * three) + two) / four; + + *base_header.number() + diff + }; + + // unless our vote is currently being limited due to a pending change + let target = limit.map(|limit| limit.min(target)).unwrap_or(target); + + // walk backwards until we find the target block + loop { + if *best_header.number() < target { unreachable!(); } + if *best_header.number() == target { + return Some((best_hash, *best_header.number())); + } + + best_hash = *best_header.parent_hash(); + best_header = self.inner.header(&BlockId::Hash(best_hash)).ok()? + .expect("Header known to exist after `best_containing` call; qed"); + } + }, + Ok(None) => { + debug!(target: "afg", "Encountered error finding best chain containing {:?}: couldn't find target block", block); + None } - // Ok(None) can be returned when `block` is after `limit`. That might cause issues. - // might be better to return the header itself in this (rare) case. - Ok(None) => None, Err(e) => { debug!(target: "afg", "Encountered error finding best chain containing {:?}: {:?}", block, e); None diff --git a/substrate/core/finality-grandpa/src/tests.rs b/substrate/core/finality-grandpa/src/tests.rs index d025796f08..dc2ccd9a53 100644 --- a/substrate/core/finality-grandpa/src/tests.rs +++ b/substrate/core/finality-grandpa/src/tests.rs @@ -329,11 +329,16 @@ fn make_ids(keys: &[Keyring]) -> Vec<(Ed25519AuthorityId, u64)> { .collect() } -fn run_to_completion(blocks: u64, net: Arc>, peers: &[Keyring]) { +fn run_to_completion(blocks: u64, net: Arc>, peers: &[Keyring]) -> u64 { + use parking_lot::RwLock; + let mut finality_notifications = Vec::new(); let mut runtime = current_thread::Runtime::new().unwrap(); + let highest_finalized = Arc::new(RwLock::new(0)); + for (peer_id, key) in peers.iter().enumerate() { + let highest_finalized = highest_finalized.clone(); let (client, link) = { let mut net = net.lock(); // temporary needed for some reason @@ -345,7 +350,13 @@ fn run_to_completion(blocks: u64, net: Arc>, peers: &[Keyr }; finality_notifications.push( client.finality_notification_stream() - .take_while(|n| Ok(n.header.number() < &blocks)) + .take_while(move |n| { + let mut highest_finalized = highest_finalized.write(); + if *n.header.number() > *highest_finalized { + *highest_finalized = *n.header.number(); + } + Ok(n.header.number() < &blocks) + }) .for_each(|_| Ok(())) ); fn assert_send(_: &T) { } @@ -378,6 +389,10 @@ fn run_to_completion(blocks: u64, net: Arc>, peers: &[Keyr .map_err(|_| ()); runtime.block_on(wait_for.select(drive_to_completion).map_err(|_| ())).unwrap(); + + let highest_finalized = *highest_finalized.read(); + + highest_finalized } #[test] @@ -683,8 +698,6 @@ fn consensus_changes_works() { #[test] fn sync_justifications_on_change_blocks() { - ::env_logger::init(); - let peers_a = &[Keyring::Alice, Keyring::Bob, Keyring::Charlie]; let peers_b = &[Keyring::Alice, Keyring::Bob]; let voters = make_ids(peers_b); @@ -731,3 +744,28 @@ fn sync_justifications_on_change_blocks() { net.lock().sync_steps(100); } } + +#[test] +fn doesnt_vote_on_the_tip_of_the_chain() { + ::env_logger::init(); + + let peers_a = &[Keyring::Alice, Keyring::Bob, Keyring::Charlie]; + let voters = make_ids(peers_a); + let api = TestApi::new(voters); + let mut net = GrandpaTestNet::new(api, 3); + + // add 100 blocks + net.peer(0).push_blocks(100, false); + net.sync(); + + for i in 0..3 { + assert_eq!(net.peer(i).client().info().unwrap().chain.best_number, 100, + "Peer #{} failed to sync", i); + } + + let net = Arc::new(Mutex::new(net)); + let highest = run_to_completion(75, net.clone(), peers_a); + + // the highest block to be finalized will be 3/4 deep in the unfinalized chain + assert_eq!(highest, 75); +}