From b1447b9367d6a0f353935a2cbe6887dfc0ed5423 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 26 Jul 2024 12:36:27 +0100 Subject: [PATCH 1/5] grandpa: handle error from SelectChain::finality_target --- substrate/client/consensus/grandpa/src/environment.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/substrate/client/consensus/grandpa/src/environment.rs b/substrate/client/consensus/grandpa/src/environment.rs index 6199e8a97d990..a618b7ff07ad8 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() }, }; From 2d6390500bf856ac53673a5226fdff28534a0407 Mon Sep 17 00:00:00 2001 From: Dmitry Lavrenov <39522748+dmitrylavrenov@users.noreply.github.com> Date: Fri, 26 Jul 2024 16:10:29 +0300 Subject: [PATCH 2/5] Add `selecting_block_outside_of_best_chain_works` test (#1) * Initial implementation of race condition of grandpa voting and babe reorg * Add comments * Remove toolchain * Add finalization check logic * Improve test logic --- .../client/consensus/grandpa/src/tests.rs | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+) diff --git a/substrate/client/consensus/grandpa/src/tests.rs b/substrate/client/consensus/grandpa/src/tests.rs index 14708cc89e890..8d8074c7d81d2 100644 --- a/substrate/client/consensus/grandpa/src/tests.rs +++ b/substrate/client/consensus/grandpa/src/tests.rs @@ -1820,6 +1820,138 @@ async fn grandpa_environment_checks_if_best_block_is_descendent_of_finality_targ ); } +// might happen in reorg case - +#[tokio::test] +async fn selecting_block_outside_of_best_chain_works() { + 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 + let hashes = 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 hashof8_a = client.expect_block_hash_from_id(&BlockId::Number(8)).unwrap(); + + // finalize the 7th block + peer.client().finalize_block(hashes[6], None, false).unwrap(); + + assert_eq!( + peer.client().info().finalized_hash, + hashes[6], + ); + + // simulate completed grandpa round + env.completed( + 1, + finality_grandpa::round::State { + prevote_ghost: Some((hashof8_a, 8)), + finalized: Some((hashes[6], 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((hashes[6], 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 8 that is 10 blocks long + let fork = 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((hashes[6], 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 finalizion 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; From 11efbf6d902b196da2ec3f37972f1cf062c141af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 26 Jul 2024 14:14:33 +0100 Subject: [PATCH 3/5] rustfmt --- .../client/consensus/grandpa/src/tests.rs | 49 +++++-------------- 1 file changed, 12 insertions(+), 37 deletions(-) diff --git a/substrate/client/consensus/grandpa/src/tests.rs b/substrate/client/consensus/grandpa/src/tests.rs index 8d8074c7d81d2..c300deb7f3af8 100644 --- a/substrate/client/consensus/grandpa/src/tests.rs +++ b/substrate/client/consensus/grandpa/src/tests.rs @@ -1857,10 +1857,7 @@ async fn selecting_block_outside_of_best_chain_works() { // finalize the 7th block peer.client().finalize_block(hashes[6], None, false).unwrap(); - assert_eq!( - peer.client().info().finalized_hash, - hashes[6], - ); + assert_eq!(peer.client().info().finalized_hash, hashes[6]); // simulate completed grandpa round env.completed( @@ -1869,20 +1866,16 @@ async fn selecting_block_outside_of_best_chain_works() { prevote_ghost: Some((hashof8_a, 8)), finalized: Some((hashes[6], 7)), estimate: Some((hashof8_a, 8)), - completable: true + completable: true, }, Default::default(), - &finality_grandpa::HistoricalVotes::new() - ).unwrap(); + &finality_grandpa::HistoricalVotes::new(), + ) + .unwrap(); // check simulated last completed round assert_eq!( - env - .voter_set_state - .read() - .last_completed_round() - .state - , + env.voter_set_state.read().last_completed_round().state, finality_grandpa::round::State { prevote_ghost: Some((hashof8_a, 8)), finalized: Some((hashes[6], 7)), @@ -1892,17 +1885,10 @@ async fn selecting_block_outside_of_best_chain_works() { ); // `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, - ); + 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 8 that is 10 blocks long - let fork = peer.generate_blocks_at( + peer.generate_blocks_at( BlockId::Number(7), 10, BlockOrigin::File, @@ -1919,14 +1905,10 @@ async fn selecting_block_outside_of_best_chain_works() { // 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` + // 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 - , + env.voter_set_state.read().last_completed_round().state, finality_grandpa::round::State { prevote_ghost: Some((hashof8_a, 8)), finalized: Some((hashes[6], 7)), @@ -1936,14 +1918,7 @@ async fn selecting_block_outside_of_best_chain_works() { ); // `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, - ); + assert_eq!(env.best_chain_containing(hashof8_a).await.unwrap().unwrap().0, hashof8_a); // simulate finalizion of the `hashof8_a` block peer.client().finalize_block(hashof8_a, None, false).unwrap(); From d5484fcb4f4c6abccbdb29479a05095f77b6cfa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Fri, 26 Jul 2024 14:24:27 +0100 Subject: [PATCH 4/5] nits in test --- .../client/consensus/grandpa/src/tests.rs | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/substrate/client/consensus/grandpa/src/tests.rs b/substrate/client/consensus/grandpa/src/tests.rs index c300deb7f3af8..2aa1b5f6ee1b2 100644 --- a/substrate/client/consensus/grandpa/src/tests.rs +++ b/substrate/client/consensus/grandpa/src/tests.rs @@ -1820,9 +1820,11 @@ async fn grandpa_environment_checks_if_best_block_is_descendent_of_finality_targ ); } -// might happen in reorg case - +// 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 selecting_block_outside_of_best_chain_works() { +async fn grandpa_environment_uses_round_base_block_for_voting_if_finality_target_errors() { use finality_grandpa::voter::Environment; use sp_consensus::SelectChain; @@ -1840,7 +1842,7 @@ async fn selecting_block_outside_of_best_chain_works() { let select_chain = sc_consensus::LongestChain::new(peer.client().as_backend()); // create a chain that is 10 blocks long - let hashes = peer.push_blocks(10, false); + peer.push_blocks(10, false); let env = test_environment_with_select_chain( &link, @@ -1852,19 +1854,20 @@ async fn selecting_block_outside_of_best_chain_works() { 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(hashes[6], None, false).unwrap(); + peer.client().finalize_block(hashof7, None, false).unwrap(); - assert_eq!(peer.client().info().finalized_hash, hashes[6]); + 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((hashes[6], 7)), + finalized: Some((hashof7, 7)), estimate: Some((hashof8_a, 8)), completable: true, }, @@ -1878,7 +1881,7 @@ async fn selecting_block_outside_of_best_chain_works() { env.voter_set_state.read().last_completed_round().state, finality_grandpa::round::State { prevote_ghost: Some((hashof8_a, 8)), - finalized: Some((hashes[6], 7)), + finalized: Some((hashof7, 7)), estimate: Some((hashof8_a, 8)), completable: true } @@ -1887,7 +1890,7 @@ async fn selecting_block_outside_of_best_chain_works() { // `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 8 that is 10 blocks long + // 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, @@ -1911,7 +1914,7 @@ async fn selecting_block_outside_of_best_chain_works() { env.voter_set_state.read().last_completed_round().state, finality_grandpa::round::State { prevote_ghost: Some((hashof8_a, 8)), - finalized: Some((hashes[6], 7)), + finalized: Some((hashof7, 7)), estimate: Some((hashof8_a, 8)), completable: true } @@ -1920,7 +1923,7 @@ async fn selecting_block_outside_of_best_chain_works() { // `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 finalizion of the `hashof8_a` block + // simulate finalization of the `hashof8_a` block peer.client().finalize_block(hashof8_a, None, false).unwrap(); // check that best chain is reorged back From f18e3a2654a6ba3e2d36636e2c5731a02e48a841 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sun, 28 Jul 2024 21:32:51 +0200 Subject: [PATCH 5/5] PRDOC --- prdoc/pr_5153.prdoc | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 prdoc/pr_5153.prdoc 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