From 95d6d05c3bc38a9e1ff4927f265304424e112bbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 11 Oct 2022 17:50:30 +0100 Subject: [PATCH 1/2] grandpa: pass the actual best block to voting rules --- client/finality-grandpa/src/environment.rs | 126 +++++++++++---------- 1 file changed, 67 insertions(+), 59 deletions(-) diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 3d708a95f41cb..c319e9bbf60c6 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -1158,7 +1158,7 @@ where let base_header = match client.header(BlockId::Hash(block))? { Some(h) => h, None => { - debug!(target: "afg", + warn!(target: "afg", "Encountered error finding best chain containing {:?}: couldn't find base block", block, ); @@ -1174,72 +1174,80 @@ where let limit = authority_set.current_limit(*base_header.number()); debug!(target: "afg", "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(BlockId::Hash(best_hash))? - .expect("Header known to exist after `finality_target` call; qed"); + let mut target_header = match select_chain.finality_target(block, None).await { + Ok(target_hash) => client + .header(BlockId::Hash(target_hash))? + .expect("Header known to exist after `finality_target` call; qed"), + Err(err) => { + warn!(target: "afg", + "Encountered error finding best chain containing {:?}: couldn't find target block: {}", + block, + err, + ); - // check if our vote is currently being limited due to a pending change - let limit = limit.filter(|limit| limit < best_header.number()); + return Ok(None) + }, + }; - let (base_header, best_header, target_header) = if let Some(target_number) = limit { - let mut target_header = best_header.clone(); + // 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: "afg", + "Encountered error finding best chain containing {:?}: couldn't find best block: {}", + block, + err, + ); - // 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" - ); - } + return Ok(None) + }, + }; - if *target_header.number() == target_number { - break - } + if target_header.number() > best_header.number() { + return Err(Error::Safety( + "SelectChain returned a finality target higher than its best block".into(), + )) + } - target_header = client - .header(BlockId::Hash(*target_header.parent_hash()))? - .expect("Header known to exist after `finality_target` call; qed"); - } + // 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" + ); + } - (base_header, best_header, target_header) - } else { - // otherwise just use the given best as the target - (base_header, best_header.clone(), best_header) - }; + if *target_header.number() == target_number { + break + } - // 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()))) - }, - Err(e) => { - warn!(target: "afg", "Encountered error finding best chain containing {:?}: {}", block, e); - None - }, - }; + target_header = client + .header(BlockId::Hash(*target_header.parent_hash()))? + .expect("Header known to exist after `finality_target` call; qed"); + } + } - Ok(result) + // 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 From 1f2b79d692da8613dfc00f5476e85702e22c1fa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 11 Oct 2022 18:49:40 +0100 Subject: [PATCH 2/2] grandpa: add test for checking best header is passed to voting rule --- client/finality-grandpa/src/tests.rs | 147 ++++++++++++++++++++++++--- 1 file changed, 133 insertions(+), 14 deletions(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index b1e46be5cabde..010dfcd0dc846 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::executor::block_on; use futures_timer::Delay; @@ -34,8 +35,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, @@ -202,11 +202,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 { @@ -1342,21 +1381,16 @@ fn voter_catches_up_to_latest_round_when_behind() { runtime.block_on(future::select(test, drive_to_completion)); } -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, @@ -1381,7 +1415,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()), @@ -1394,6 +1428,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, + ) +} + #[test] fn grandpa_environment_respects_voting_rules() { use finality_grandpa::voter::Environment; @@ -1493,6 +1546,72 @@ fn grandpa_environment_respects_voting_rules() { ); } +#[test] +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().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) + select_chain.set_best_chain(client.header(&BlockId::Number(21)).unwrap().unwrap()); + select_chain.set_finality_target(client.header(&BlockId::Number(21)).unwrap().unwrap().hash()); + + assert_eq!( + block_on(env.best_chain_containing(peer.client().info().finalized_hash)) + .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 + select_chain.set_best_chain(client.header(&BlockId::Number(21)).unwrap().unwrap()); + select_chain.set_finality_target(client.header(&BlockId::Number(10)).unwrap().unwrap().hash()); + + assert_eq!( + block_on(env.best_chain_containing(peer.client().info().finalized_hash)) + .unwrap() + .unwrap() + .1, + 10, + ); + + // returning a finality target that's higher than the best block is an + // inconsistent state that should be handled + select_chain.set_best_chain(client.header(&BlockId::Number(21)).unwrap().unwrap()); + select_chain.set_finality_target(client.header(&BlockId::Number(42)).unwrap().unwrap().hash()); + + assert_matches!( + block_on(env.best_chain_containing(peer.client().info().finalized_hash)), + Err(CommandOrError::Error(Error::Safety(_))) + ); +} + #[test] fn grandpa_environment_never_overwrites_round_voter_state() { use finality_grandpa::voter::Environment;