Skip to content

Commit

Permalink
grandpa: handle error from SelectChain::finality_target (#5153) (#5172)
Browse files Browse the repository at this point in the history
Fix #3487.

---------

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
Co-authored-by: Dmitry Lavrenov <39522748+dmitrylavrenov@users.noreply.github.com>
  • Loading branch information
3 people authored Jul 29, 2024
1 parent 740b3ac commit ed3657a
Show file tree
Hide file tree
Showing 3 changed files with 130 additions and 2 deletions.
12 changes: 12 additions & 0 deletions prdoc/pr_5153.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
title: "Grandpa: Ensure voting doesn't fail after a re-org"

doc:
- audience: Node Operator
description: |
Ensures that a node is still able to vote with Grandpa, when a re-org happened that
changed the best chain. This ultimately prevents that a network may runs into a
potential finality stall.

crates:
- name: sc-consensus-grandpa
bump: patch
10 changes: 8 additions & 2 deletions substrate/client/consensus/grandpa/src/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1214,14 +1214,20 @@ where
.header(target_hash)?
.expect("Header known to exist after `finality_target` call; qed"),
Err(err) => {
warn!(
debug!(
target: LOG_TARGET,
"Encountered error finding best chain containing {:?}: couldn't find target block: {}",
block,
err,
);

return Ok(None)
// NOTE: in case the given `SelectChain` doesn't provide any block we fallback to using
// the given base block provided by the GRANDPA voter.
//
// For example, `LongestChain` will error if the given block to use as base isn't part
// of the best chain (as defined by `LongestChain`), which could happen if there was a
// re-org.
base_header.clone()
},
};

Expand Down
110 changes: 110 additions & 0 deletions substrate/client/consensus/grandpa/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1820,6 +1820,116 @@ async fn grandpa_environment_checks_if_best_block_is_descendent_of_finality_targ
);
}

// This is a regression test for an issue that was triggered by a reorg
// - https://github.com/paritytech/polkadot-sdk/issues/3487
// - https://github.com/humanode-network/humanode/issues/1104
#[tokio::test]
async fn grandpa_environment_uses_round_base_block_for_voting_if_finality_target_errors() {
use finality_grandpa::voter::Environment;
use sp_consensus::SelectChain;

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 sync_service = peer.sync_service().clone();
let notification_service =
peer.take_notification_service(&grandpa_protocol_name::NAME.into()).unwrap();
let link = peer.data.lock().take().unwrap();
let client = peer.client().as_client().clone();
let select_chain = sc_consensus::LongestChain::new(peer.client().as_backend());

// create a chain that is 10 blocks long
peer.push_blocks(10, false);

let env = test_environment_with_select_chain(
&link,
None,
network_service.clone(),
sync_service,
notification_service,
select_chain.clone(),
VotingRulesBuilder::default().build(),
);

let hashof7 = client.expect_block_hash_from_id(&BlockId::Number(7)).unwrap();
let hashof8_a = client.expect_block_hash_from_id(&BlockId::Number(8)).unwrap();

// finalize the 7th block
peer.client().finalize_block(hashof7, None, false).unwrap();

assert_eq!(peer.client().info().finalized_hash, hashof7);

// simulate completed grandpa round
env.completed(
1,
finality_grandpa::round::State {
prevote_ghost: Some((hashof8_a, 8)),
finalized: Some((hashof7, 7)),
estimate: Some((hashof8_a, 8)),
completable: true,
},
Default::default(),
&finality_grandpa::HistoricalVotes::new(),
)
.unwrap();

// check simulated last completed round
assert_eq!(
env.voter_set_state.read().last_completed_round().state,
finality_grandpa::round::State {
prevote_ghost: Some((hashof8_a, 8)),
finalized: Some((hashof7, 7)),
estimate: Some((hashof8_a, 8)),
completable: true
}
);

// `hashof8_a` should be finalized next, `best_chain_containing` should return `hashof8_a`
assert_eq!(env.best_chain_containing(hashof8_a).await.unwrap().unwrap().0, hashof8_a);

// simulate reorg on block 8 by creating a fork starting at block 7 that is 10 blocks long
peer.generate_blocks_at(
BlockId::Number(7),
10,
BlockOrigin::File,
|mut builder| {
builder.push_deposit_log_digest_item(DigestItem::Other(vec![1])).unwrap();
builder.build().unwrap().block
},
false,
false,
true,
ForkChoiceStrategy::LongestChain,
);

// check that new best chain is on longest chain
assert_eq!(env.select_chain.best_chain().await.unwrap().number, 17);

// verify that last completed round has `prevote_ghost` and `estimate` blocks related to
// `hashof8_a`
assert_eq!(
env.voter_set_state.read().last_completed_round().state,
finality_grandpa::round::State {
prevote_ghost: Some((hashof8_a, 8)),
finalized: Some((hashof7, 7)),
estimate: Some((hashof8_a, 8)),
completable: true
}
);

// `hashof8_a` should be finalized next, `best_chain_containing` should still return `hashof8_a`
assert_eq!(env.best_chain_containing(hashof8_a).await.unwrap().unwrap().0, hashof8_a);

// simulate finalization of the `hashof8_a` block
peer.client().finalize_block(hashof8_a, None, false).unwrap();

// check that best chain is reorged back
assert_eq!(env.select_chain.best_chain().await.unwrap().number, 10);
}

#[tokio::test]
async fn grandpa_environment_never_overwrites_round_voter_state() {
use finality_grandpa::voter::Environment;
Expand Down

0 comments on commit ed3657a

Please sign in to comment.