From befa8606cf05d22fdc32350c605c8e0a244b0310 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 29 Jul 2024 10:37:23 +0200 Subject: [PATCH] grandpa: handle error from SelectChain::finality_target (#5153) (#5172) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix https://github.com/paritytech/polkadot-sdk/issues/3487. --------- Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> Co-authored-by: Dmitry Lavrenov <39522748+dmitrylavrenov@users.noreply.github.com> --- prdoc/pr_5153.prdoc | 12 ++ .../consensus/grandpa/src/environment.rs | 10 +- .../client/consensus/grandpa/src/tests.rs | 110 ++++++++++++++++++ 3 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 prdoc/pr_5153.prdoc diff --git a/prdoc/pr_5153.prdoc b/prdoc/pr_5153.prdoc new file mode 100644 index 000000000000..4f43b52d8edf --- /dev/null +++ b/prdoc/pr_5153.prdoc @@ -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 diff --git a/substrate/client/consensus/grandpa/src/environment.rs b/substrate/client/consensus/grandpa/src/environment.rs index 6199e8a97d99..a618b7ff07ad 100644 --- a/substrate/client/consensus/grandpa/src/environment.rs +++ b/substrate/client/consensus/grandpa/src/environment.rs @@ -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() }, }; diff --git a/substrate/client/consensus/grandpa/src/tests.rs b/substrate/client/consensus/grandpa/src/tests.rs index 14708cc89e89..2aa1b5f6ee1b 100644 --- a/substrate/client/consensus/grandpa/src/tests.rs +++ b/substrate/client/consensus/grandpa/src/tests.rs @@ -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;