Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

grandpa: don't error if best block and finality target are inconsistent #13364

Merged
merged 4 commits into from
Feb 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions client/finality-grandpa/src/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand All @@ -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() &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire check here is a little bit shit and that we then log some warning. This doesn't need to be an issue as between both of these calls there can be a reorg, leading to the case that the finality target isn't in the chain of the best block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andresilva what about applying the strategy you initially suggested? That is don't have to separate calls as to modify the finality_target() method to return a tuple (target, best) in one shot?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkchr I don't know what you mean, or rather I don't understand what I should do with your comment. This can happen due to a race condition but it can also happen that SelectChain is returning inconsistent results (which I suspect is the root cause of the issue on Westend).

@davxy I think that should still be the goal but wanted to get a quick fix PR to address the issue on Westend. After we change SelectChain::finality_target we might still want to be defensive against "bad" implementations and do this check anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to say that there can be a race between getting the finality target and getting the best block. I mean it is totally legit that the best block for example goes backwards when the longest/best chain has a valid dispute. And in this case the finality target could also be greater than the "New best block"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, and that is the result of doing two distinct async calls where the underlying blockchain state may change. We can avoid that by making sure that we get all the data in a single call to SelectChain::finality_target (what @davxy talks about in his comment). But we still don't control what is returned by SelectChain, and it probably still makes sense to have this check when we fix that and/or we need to make sure that the Polkadot SelectChain respects this invariant (e.g. after #13289 we know that invariant holds for LongestChain).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove the logging (or downgrade to debug/trace) if you think it will cause confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I downgraded the logging to debug. I think it's useful to log this as it can help us catch "bad" SelectChain implementations, but it's useless for end users to be presented with this warning.

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()) {
Expand All @@ -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
Expand Down
144 changes: 136 additions & 8 deletions client/finality-grandpa/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,35 @@ impl SelectChain<Block> for MockSelectChain {
}
}

// A mock voting rule that allows asserting an expected value for best block
#[derive(Clone, Default)]
struct AssertBestBlock(Arc<Mutex<Option<Hash>>>);

impl<B> VotingRule<Block, B> for AssertBestBlock
where
B: HeaderBackend<Block>,
{
fn restrict_vote(
&self,
_backend: Arc<B>,
_base: &<Block as BlockT>::Header,
best_target: &<Block as BlockT>::Header,
_current_target: &<Block as BlockT>::Header,
) -> VotingRuleResult<Block> {
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 {
Expand Down Expand Up @@ -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,
);
}

Expand Down