From edbf56ee32ca277202edfb9cf95a03bd8b6ac2a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= <123550+andresilva@users.noreply.github.com> Date: Sun, 28 Jul 2024 21:03:48 +0100 Subject: [PATCH 1/2] grandpa: handle error from SelectChain::finality_target (#5153) 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: Dmitry Lavrenov <39522748+dmitrylavrenov@users.noreply.github.com> Co-authored-by: Bastian Köcher Co-authored-by: Bastian Köcher --- client/consensus/grandpa/src/environment.rs | 10 +- client/consensus/grandpa/src/tests.rs | 110 ++++++++++++++++++++ prdoc/pr_5153.prdoc | 12 +++ 3 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 prdoc/pr_5153.prdoc diff --git a/client/consensus/grandpa/src/environment.rs b/client/consensus/grandpa/src/environment.rs index 67820a59cc943..5e267ea280f65 100644 --- a/client/consensus/grandpa/src/environment.rs +++ b/client/consensus/grandpa/src/environment.rs @@ -1209,14 +1209,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/client/consensus/grandpa/src/tests.rs b/client/consensus/grandpa/src/tests.rs index f5e5c45e74207..7b3602fb41a55 100644 --- a/client/consensus/grandpa/src/tests.rs +++ b/client/consensus/grandpa/src/tests.rs @@ -1738,6 +1738,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; diff --git a/prdoc/pr_5153.prdoc b/prdoc/pr_5153.prdoc new file mode 100644 index 0000000000000..4f43b52d8edfb --- /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 From 85f7dc726d6bb666171b42e22e528bd676d6d568 Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov Date: Mon, 19 Aug 2024 20:02:10 +0300 Subject: [PATCH 2/2] Remove redundant prdoc --- prdoc/pr_5153.prdoc | 12 ------------ 1 file changed, 12 deletions(-) delete mode 100644 prdoc/pr_5153.prdoc diff --git a/prdoc/pr_5153.prdoc b/prdoc/pr_5153.prdoc deleted file mode 100644 index 4f43b52d8edfb..0000000000000 --- a/prdoc/pr_5153.prdoc +++ /dev/null @@ -1,12 +0,0 @@ -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