diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 40ec720c4e7fe..a9aa204f12e2d 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -1213,7 +1213,7 @@ where // 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 { + let mut best_header = match select_chain.best_chain().await { Ok(best_header) => best_header, Err(err) => { warn!( @@ -1227,12 +1227,30 @@ where }, }; - if target_header.number() > best_header.number() { - return Err(Error::Safety( - "SelectChain returned a finality target higher than its best block".into(), - )) + let is_descendent_of = is_descendent_of(&*client, None); + + if target_header.number() > best_header.number() || + target_header.number() == best_header.number() && + target_header.hash() != best_header.hash() || + !is_descendent_of(&target_header.hash(), &best_header.hash())? + { + debug!( + target: LOG_TARGET, + "SelectChain returned a finality target inconsistent with its best block. Restricting best block to target block" + ); + + best_header = target_header.clone(); } + debug!( + target: LOG_TARGET, + "SelectChain: finality target: #{} ({}), best block: #{} ({})", + target_header.number(), + target_header.hash(), + best_header.number(), + best_header.hash(), + ); + // 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()) { @@ -1254,6 +1272,13 @@ where .header(*target_header.parent_hash())? .expect("Header known to exist after `finality_target` call; qed"); } + + debug!( + target: LOG_TARGET, + "Finality target restricted to #{} ({}) due to pending authority set change", + target_header.number(), + target_header.hash() + ) } // restrict vote according to the given voting rule, if the voting rule diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 07ea4649148fb..05561ad856119 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -244,6 +244,35 @@ impl SelectChain for MockSelectChain { } } +// A mock voting rule that allows asserting an expected value for best block +#[derive(Clone, Default)] +struct AssertBestBlock(Arc>>); + +impl VotingRule for AssertBestBlock +where + B: HeaderBackend, +{ + fn restrict_vote( + &self, + _backend: Arc, + _base: &::Header, + best_target: &::Header, + _current_target: &::Header, + ) -> VotingRuleResult { + if let Some(expected) = *self.0.lock() { + assert_eq!(best_target.hash(), expected); + } + + Box::pin(std::future::ready(None)) + } +} + +impl AssertBestBlock { + fn set_expected_best_block(&self, hash: Hash) { + *self.0.lock() = Some(hash); + } +} + const TEST_GOSSIP_DURATION: Duration = Duration::from_millis(500); fn make_ids(keys: &[Ed25519Keyring]) -> AuthorityList { @@ -1566,16 +1595,115 @@ async fn grandpa_environment_passes_actual_best_block_to_voting_rules() { .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()); +#[tokio::test] +async fn grandpa_environment_checks_if_best_block_is_descendent_of_finality_target() { + 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(); + let voting_rule = AssertBestBlock::default(); + let env = test_environment_with_select_chain( + &link, + None, + network_service.clone(), + select_chain.clone(), + voting_rule.clone(), + ); + + // create a chain that is 10 blocks long + peer.push_blocks(10, false); + + let hashof5_a = client.expect_block_hash_from_id(&BlockId::Number(5)).unwrap(); + let hashof10_a = client.expect_block_hash_from_id(&BlockId::Number(10)).unwrap(); + + // create a fork starting at block 4 that is 6 blocks long + let fork = peer.generate_blocks_at( + BlockId::Number(4), + 6, + BlockOrigin::File, + |builder| { + let mut block = builder.build().unwrap().block; + block.header.digest_mut().push(DigestItem::Other(vec![1])); + block + }, + false, + false, + true, + ForkChoiceStrategy::LongestChain, + ); + + let hashof5_b = *fork.first().unwrap(); + let hashof10_b = *fork.last().unwrap(); + + // returning a finality target that's higher than the best block is inconsistent, + // therefore the best block should be set to be the same block as the target + select_chain.set_best_chain(client.expect_header(hashof5_a).unwrap()); + select_chain.set_finality_target(client.expect_header(hashof10_a).unwrap().hash()); + voting_rule.set_expected_best_block(hashof10_a); + + // the voting rule will internally assert that the best block that was passed was `hashof10_a`, + // instead of the one returned by `SelectChain` + assert_eq!( + env.best_chain_containing(peer.client().info().finalized_hash) + .await + .unwrap() + .unwrap() + .0, + hashof10_a, + ); + + // best block and finality target are blocks at the same height but on different forks, + // we should override the initial best block (#5B) with the target block (#5A) + select_chain.set_best_chain(client.expect_header(hashof5_b).unwrap()); + select_chain.set_finality_target(client.expect_header(hashof5_a).unwrap().hash()); + voting_rule.set_expected_best_block(hashof5_a); + + assert_eq!( + env.best_chain_containing(peer.client().info().finalized_hash) + .await + .unwrap() + .unwrap() + .0, + hashof5_a, + ); - assert_matches!( - env.best_chain_containing(peer.client().info().finalized_hash).await, - Err(CommandOrError::Error(Error::Safety(_))) + // best block is higher than finality target but it's on a different fork, + // we should override the initial best block (#5A) with the target block (#5B) + select_chain.set_best_chain(client.expect_header(hashof10_b).unwrap()); + select_chain.set_finality_target(client.expect_header(hashof5_a).unwrap().hash()); + voting_rule.set_expected_best_block(hashof5_a); + + assert_eq!( + env.best_chain_containing(peer.client().info().finalized_hash) + .await + .unwrap() + .unwrap() + .0, + hashof5_a, + ); + + // best block is higher than finality target and it's on the same fork, + // the best block passed to the voting rule should not be overriden + select_chain.set_best_chain(client.expect_header(hashof10_a).unwrap()); + select_chain.set_finality_target(client.expect_header(hashof5_a).unwrap().hash()); + voting_rule.set_expected_best_block(hashof10_a); + + assert_eq!( + env.best_chain_containing(peer.client().info().finalized_hash) + .await + .unwrap() + .unwrap() + .0, + hashof5_a, ); }