diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 9e63bef78a6a9..40ec720c4e7fe 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -1174,7 +1174,7 @@ where let base_header = match client.header(block)? { Some(h) => h, None => { - debug!( + warn!( target: LOG_TARGET, "Encountered error finding best chain containing {:?}: couldn't find base block", block, @@ -1194,75 +1194,82 @@ where "Finding best chain containing block {:?} with number limit {:?}", block, limit ); - let result = match select_chain.finality_target(block, None).await { - Ok(best_hash) => { - let best_header = client - .header(best_hash)? - .expect("Header known to exist after `finality_target` call; qed"); - - // check if our vote is currently being limited due to a pending change - let limit = limit.filter(|limit| limit < best_header.number()); - - let (base_header, best_header, target_header) = if let Some(target_number) = limit { - let mut target_header = best_header.clone(); - - // walk backwards until we find the target block - loop { - if *target_header.number() < target_number { - unreachable!( - "we are traversing backwards from a known block; \ - blocks are stored contiguously; \ - qed" - ); - } - - if *target_header.number() == target_number { - break - } - - target_header = client - .header(*target_header.parent_hash())? - .expect("Header known to exist after `finality_target` call; qed"); - } - - (base_header, best_header, target_header) - } else { - // otherwise just use the given best as the target - (base_header, best_header.clone(), best_header) - }; + let mut target_header = match select_chain.finality_target(block, None).await { + Ok(target_hash) => client + .header(target_hash)? + .expect("Header known to exist after `finality_target` call; qed"), + Err(err) => { + warn!( + target: LOG_TARGET, + "Encountered error finding best chain containing {:?}: couldn't find target block: {}", + block, + err, + ); - // restrict vote according to the given voting rule, if the - // voting rule doesn't restrict the vote then we keep the - // previous target. - // - // note that we pass the original `best_header`, i.e. before the - // authority set limit filter, which can be considered a - // mandatory/implicit voting rule. - // - // we also make sure that the restricted vote is higher than the - // round base (i.e. last finalized), otherwise the value - // returned by the given voting rule is ignored and the original - // target is used instead. - voting_rule - .restrict_vote(client.clone(), &base_header, &best_header, &target_header) - .await - .filter(|(_, restricted_number)| { - // we can only restrict votes within the interval [base, target] - restricted_number >= base_header.number() && - restricted_number < target_header.number() - }) - .or_else(|| Some((target_header.hash(), *target_header.number()))) + return Ok(None) }, - Err(e) => { + }; + + // NOTE: this is purposefully done after `finality_target` to prevent a case + // where in-between these two requests there is a block import and + // `finality_target` returns something higher than `best_chain`. + let best_header = match select_chain.best_chain().await { + Ok(best_header) => best_header, + Err(err) => { warn!( target: LOG_TARGET, - "Encountered error finding best chain containing {:?}: {}", block, e + "Encountered error finding best chain containing {:?}: couldn't find best block: {}", + block, + err, ); - None + + return Ok(None) }, }; - Ok(result) + if target_header.number() > best_header.number() { + return Err(Error::Safety( + "SelectChain returned a finality target higher than its best block".into(), + )) + } + + // check if our vote is currently being limited due to a pending change, + // in which case we will restrict our target header to the given limit + if let Some(target_number) = limit.filter(|limit| limit < target_header.number()) { + // walk backwards until we find the target block + loop { + if *target_header.number() < target_number { + unreachable!( + "we are traversing backwards from a known block; \ + blocks are stored contiguously; \ + qed" + ); + } + + if *target_header.number() == target_number { + break + } + + target_header = client + .header(*target_header.parent_hash())? + .expect("Header known to exist after `finality_target` call; qed"); + } + } + + // restrict vote according to the given voting rule, if the voting rule + // doesn't restrict the vote then we keep the previous target. + // + // we also make sure that the restricted vote is higher than the round base + // (i.e. last finalized), otherwise the value returned by the given voting + // rule is ignored and the original target is used instead. + Ok(voting_rule + .restrict_vote(client.clone(), &base_header, &best_header, &target_header) + .await + .filter(|(_, restricted_number)| { + // we can only restrict votes within the interval [base, target] + restricted_number >= base_header.number() && restricted_number < target_header.number() + }) + .or_else(|| Some((target_header.hash(), *target_header.number())))) } /// Finalize the given block and apply any authority set changes. If an diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 4abcb05406e5b..07ea4649148fb 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -20,6 +20,7 @@ use super::*; use assert_matches::assert_matches; +use async_trait::async_trait; use environment::HasVoted; use futures_timer::Delay; use parking_lot::{Mutex, RwLock}; @@ -33,8 +34,7 @@ use sc_network_test::{ PeersFullClient, TestClient, TestNetFactory, }; use sp_api::{ApiRef, ProvideRuntimeApi}; -use sp_blockchain::Result; -use sp_consensus::BlockOrigin; +use sp_consensus::{BlockOrigin, Error as ConsensusError, SelectChain}; use sp_core::H256; use sp_finality_grandpa::{ AuthorityList, EquivocationProof, GrandpaApi, OpaqueKeyOwnershipProof, GRANDPA_ENGINE_ID, @@ -200,11 +200,50 @@ sp_api::mock_impl_runtime_apis! { } impl GenesisAuthoritySetProvider for TestApi { - fn get(&self) -> Result { + fn get(&self) -> sp_blockchain::Result { Ok(self.genesis_authorities.clone()) } } +/// A mock `SelectChain` that allows the user to set the return values for each +/// method. After the `SelectChain` methods are called the pending value is +/// discarded and another call to set new values must be performed. +#[derive(Clone, Default)] +struct MockSelectChain { + leaves: Arc>>>, + best_chain: Arc::Header>>>, + finality_target: Arc>>, +} + +impl MockSelectChain { + fn set_best_chain(&self, best: ::Header) { + *self.best_chain.lock() = Some(best); + } + + fn set_finality_target(&self, target: Hash) { + *self.finality_target.lock() = Some(target); + } +} + +#[async_trait] +impl SelectChain for MockSelectChain { + async fn leaves(&self) -> Result, ConsensusError> { + Ok(self.leaves.lock().take().unwrap()) + } + + async fn best_chain(&self) -> Result<::Header, ConsensusError> { + Ok(self.best_chain.lock().take().unwrap()) + } + + async fn finality_target( + &self, + _target_hash: Hash, + _maybe_max_number: Option>, + ) -> Result { + Ok(self.finality_target.lock().take().unwrap()) + } +} + const TEST_GOSSIP_DURATION: Duration = Duration::from_millis(500); fn make_ids(keys: &[Ed25519Keyring]) -> AuthorityList { @@ -1291,21 +1330,16 @@ async fn voter_catches_up_to_latest_round_when_behind() { future::select(test, drive_to_completion).await; } -type TestEnvironment = Environment< - substrate_test_runtime_client::Backend, - Block, - TestClient, - N, - LongestChain, - VR, ->; +type TestEnvironment = + Environment; -fn test_environment( +fn test_environment_with_select_chain( link: &TestLinkHalf, keystore: Option, network_service: N, + select_chain: SC, voting_rule: VR, -) -> TestEnvironment +) -> TestEnvironment where N: NetworkT, VR: VotingRule, @@ -1330,7 +1364,7 @@ where authority_set: authority_set.clone(), config: config.clone(), client: link.client.clone(), - select_chain: link.select_chain.clone(), + select_chain, set_id: authority_set.set_id(), voter_set_state: set_state.clone(), voters: Arc::new(authority_set.current_authorities()), @@ -1343,6 +1377,25 @@ where } } +fn test_environment( + link: &TestLinkHalf, + keystore: Option, + network_service: N, + voting_rule: VR, +) -> TestEnvironment, VR> +where + N: NetworkT, + VR: VotingRule, +{ + test_environment_with_select_chain( + link, + keystore, + network_service, + link.select_chain.clone(), + voting_rule, + ) +} + #[tokio::test] async fn grandpa_environment_respects_voting_rules() { use finality_grandpa::voter::Environment; @@ -1455,6 +1508,77 @@ async fn grandpa_environment_respects_voting_rules() { ); } +#[tokio::test] +async fn grandpa_environment_passes_actual_best_block_to_voting_rules() { + // NOTE: this is a "regression" test since initially we were not passing the + // best block to the voting rules + use finality_grandpa::voter::Environment; + + let peers = &[Ed25519Keyring::Alice]; + let voters = make_ids(peers); + + let mut net = GrandpaTestNet::new(TestApi::new(voters), 1, 0); + let peer = net.peer(0); + let network_service = peer.network_service().clone(); + let link = peer.data.lock().take().unwrap(); + let client = peer.client().as_client().clone(); + let select_chain = MockSelectChain::default(); + + // add 42 blocks + peer.push_blocks(42, false); + + // create an environment with a voting rule that always restricts votes to + // before the best block by 5 blocks + let env = test_environment_with_select_chain( + &link, + None, + network_service.clone(), + select_chain.clone(), + voting_rule::BeforeBestBlockBy(5), + ); + + // both best block and finality target are pointing to the same latest block, + // therefore we must restrict our vote on top of the given target (#21) + let hashof21 = client.expect_block_hash_from_id(&BlockId::Number(21)).unwrap(); + select_chain.set_best_chain(client.expect_header(hashof21).unwrap()); + select_chain.set_finality_target(client.expect_header(hashof21).unwrap().hash()); + + assert_eq!( + env.best_chain_containing(peer.client().info().finalized_hash) + .await + .unwrap() + .unwrap() + .1, + 16, + ); + + // the returned finality target is already 11 blocks from the best block, + // therefore there should be no further restriction by the voting rule + let hashof10 = client.expect_block_hash_from_id(&BlockId::Number(10)).unwrap(); + select_chain.set_best_chain(client.expect_header(hashof21).unwrap()); + select_chain.set_finality_target(client.expect_header(hashof10).unwrap().hash()); + + assert_eq!( + env.best_chain_containing(peer.client().info().finalized_hash) + .await + .unwrap() + .unwrap() + .1, + 10, + ); + + // returning a finality target that's higher than the best block is an + // inconsistent state that should be handled + let hashof42 = client.expect_block_hash_from_id(&BlockId::Number(42)).unwrap(); + select_chain.set_best_chain(client.expect_header(hashof21).unwrap()); + select_chain.set_finality_target(client.expect_header(hashof42).unwrap().hash()); + + assert_matches!( + env.best_chain_containing(peer.client().info().finalized_hash).await, + Err(CommandOrError::Error(Error::Safety(_))) + ); +} + #[tokio::test] async fn grandpa_environment_never_overwrites_round_voter_state() { use finality_grandpa::voter::Environment;