integrate dispute finality (#3484)

* finality_target adjustments * fn finality_target * partially address review comments * fixins * more rustic if condition * fix tests * fixins * Update node/core/approval-voting/src/lib.rs Co-authored-by: Andronik Ordian <[email protected]> * Update node/core/approval-voting/src/lib.rs Co-authored-by: Robert Habermeier <[email protected]> * review comments part one * rename candidates -> block_descriptions * testing outline (incomplete, WIP) * test foo * split RelayChainSelection into RelayChainSelection{,WithFallback}, introduce HeaderProvider{,Provider} * make some stuff public (revert this soon™) * some test improvements * slips of pens * test fixins * add another trait abstraction * pending edge case tests + warnings fixes * more test cases * fin * chore fmt * fix cargo.lock * Undo obsolete changes * // comments * make mod pub(crate) * fix * minimize static bounds * resolve number() as before * fmt * post merge fix * address some nits Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: Robert Habermeier <[email protected]>
paritytech · Jul 26, 2021 · 7aed7a2 · 7aed7a2
1 parent 2d19780
commit 7aed7a2
Show file tree

Hide file tree

Showing 11 changed files with 1,090 additions and 100 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/node/core/approval-voting/src/lib.rs b/node/core/approval-voting/src/lib.rs
@@ -27,7 +27,7 @@ use polkadot_node_subsystem::{
 		ApprovalVotingMessage, RuntimeApiMessage, RuntimeApiRequest, ChainApiMessage,
 		ApprovalDistributionMessage, CandidateValidationMessage,
 		AvailabilityRecoveryMessage, ChainSelectionMessage, DisputeCoordinatorMessage,
-		ImportStatementsResult,
+		ImportStatementsResult, HighestApprovedAncestorBlock, BlockDescription,
 	},
 	errors::RecoveryError,
 	overseer::{self, SubsystemSender as _}, SubsystemContext, SubsystemError, SubsystemResult, SpawnedSubsystem,
@@ -1180,7 +1180,7 @@ async fn handle_approved_ancestor(
 	target: Hash,
 	lower_bound: BlockNumber,
 	wakeups: &Wakeups,
-) -> SubsystemResult<Option<(Hash, BlockNumber)>> {
+) -> SubsystemResult<Option<HighestApprovedAncestorBlock>> {
 	const MAX_TRACING_WINDOW: usize = 200;
 	const ABNORMAL_DEPTH_THRESHOLD: usize = 5;
 
@@ -1228,6 +1228,8 @@ async fn handle_approved_ancestor(
 		Vec::new()
 	};
 
+	let mut block_descriptions = Vec::new();
+
 	let mut bits: BitVec<Lsb0, u8> = Default::default();
 	for (i, block_hash) in std::iter::once(target).chain(ancestry).enumerate() {
 		// Block entries should be present as the assumption is that
@@ -1259,8 +1261,10 @@ async fn handle_approved_ancestor(
 			}
 		} else if bits.len() <= ABNORMAL_DEPTH_THRESHOLD {
 			all_approved_max = None;
+			block_descriptions.clear();
 		} else {
 			all_approved_max = None;
+			block_descriptions.clear();
 
 			let unapproved: Vec<_> = entry.unapproved_candidates().collect();
 			tracing::debug!(
@@ -1338,6 +1342,11 @@ async fn handle_approved_ancestor(
 				}
 			}
 		}
+		block_descriptions.push(BlockDescription {
+			block_hash,
+			session: entry.session(),
+			candidates: entry.candidates().iter().map(|(_idx, candidate_hash)| *candidate_hash ).collect(),
+		});
 	}
 
 	tracing::trace!(
@@ -1366,8 +1375,19 @@ async fn handle_approved_ancestor(
 		},
 	);
 
+	// `reverse()` to obtain the ascending order from lowest to highest
+	// block within the candidates, which is the expected order
+	block_descriptions.reverse();
+
+	let all_approved_max = all_approved_max.map(|(hash, block_number)| {
+		HighestApprovedAncestorBlock{
+			hash,
+			number: block_number,
+			descriptions: block_descriptions,
+		}
+	});
 	match all_approved_max {
-		Some((ref hash, ref number)) => {
+		Some(HighestApprovedAncestorBlock { ref hash, ref number, .. }) => {
 			span.add_uint_tag("approved-number", *number as u64);
 			span.add_string_fmt_debug_tag("approved-hash", hash);
 		}

diff --git a/node/core/approval-voting/src/old_tests.rs b/node/core/approval-voting/src/old_tests.rs
@@ -26,7 +26,7 @@ use polkadot_node_primitives::approval::{
 	RELAY_VRF_MODULO_CONTEXT, DelayTranche,
 };
 use polkadot_node_subsystem_test_helpers::make_subsystem_context;
-use polkadot_node_subsystem::messages::AllMessages;
+use polkadot_node_subsystem::messages::{AllMessages, HighestApprovedAncestorBlock};
 use sp_core::testing::TaskExecutor;
 
 use parking_lot::Mutex;
@@ -1612,10 +1612,12 @@ fn approved_ancestor_all_approved() {
 
 	let test_fut = Box::pin(async move {
 		let overlay_db = OverlayedBackend::new(&db);
-		assert_eq!(
+		assert_matches!(
 			handle_approved_ancestor(&mut ctx, &overlay_db, block_hash_4, 0, &Default::default())
-				.await.unwrap(),
-			Some((block_hash_4, 4)),
+				.await,
+			Ok(Some(HighestApprovedAncestorBlock { hash, number, .. } )) => {
+				assert_eq!((block_hash_4, 4), (hash, number));
+			}
 		)
 	});
 
@@ -1686,10 +1688,12 @@ fn approved_ancestor_missing_approval() {
 
 	let test_fut = Box::pin(async move {
 		let overlay_db = OverlayedBackend::new(&db);
-		assert_eq!(
+		assert_matches!(
 			handle_approved_ancestor(&mut ctx, &overlay_db, block_hash_4, 0, &Default::default())
-				.await.unwrap(),
-			Some((block_hash_2, 2)),
+				.await,
+			Ok(Some(HighestApprovedAncestorBlock { hash, number, .. })) => {
+				assert_eq!((block_hash_2, 2), (hash, number));
+			}
 		)
 	});
 

diff --git a/node/core/dispute-coordinator/src/lib.rs b/node/core/dispute-coordinator/src/lib.rs
@@ -35,7 +35,7 @@ use polkadot_node_subsystem::{
 	errors::{ChainApiError, RuntimeApiError},
 	messages::{
 		ChainApiMessage, DisputeCoordinatorMessage, DisputeDistributionMessage,
-		DisputeParticipationMessage, ImportStatementsResult,
+		DisputeParticipationMessage, ImportStatementsResult, BlockDescription,
 	}
 };
 use polkadot_node_subsystem_util::rolling_session_window::{
@@ -961,14 +961,16 @@ fn make_dispute_message(
 	).map_err(MakeDisputeMessageError::InvalidStatementCombination)
 }
 
+/// Determine the the best block and its block number.
+/// Assumes `block_descriptions` are sorted from the one
+/// with the lowest `BlockNumber` to the highest.
 fn determine_undisputed_chain(
 	overlay_db: &mut OverlayedBackend<'_, impl Backend>,
 	base_number: BlockNumber,
-	block_descriptions: Vec<(Hash, SessionIndex, Vec<CandidateHash>)>,
+	block_descriptions: Vec<BlockDescription>,
 ) -> Result<Option<(BlockNumber, Hash)>, Error> {
 	let last = block_descriptions.last()
-
-		.map(|e| (base_number + block_descriptions.len() as BlockNumber, e.0));
+		.map(|e| (base_number + block_descriptions.len() as BlockNumber, e.block_hash));
 
 	// Fast path for no disputes.
 	let recent_disputes = match overlay_db.load_recent_disputes()? {
@@ -984,14 +986,14 @@ fn determine_undisputed_chain(
 		)
 	};
 
-	for (i, (_, session, candidates)) in block_descriptions.iter().enumerate() {
+	for (i, BlockDescription { session, candidates, .. }) in block_descriptions.iter().enumerate() {
 		if candidates.iter().any(|c| is_possibly_invalid(*session, *c)) {
 			if i == 0 {
 				return Ok(None);
 			} else {
 				return Ok(Some((
 					base_number + i as BlockNumber,
-					block_descriptions[i - 1].0,
+					block_descriptions[i - 1].block_hash,
 				)));
 			}
 		}

diff --git a/node/core/dispute-coordinator/src/tests.rs b/node/core/dispute-coordinator/src/tests.rs
@@ -22,6 +22,7 @@ use polkadot_primitives::v1::{BlakeTwo256, HashT, ValidatorId, Header, SessionIn
 use polkadot_node_subsystem::{jaeger, ActiveLeavesUpdate, ActivatedLeaf, LeafStatus};
 use polkadot_node_subsystem::messages::{
 	AllMessages, ChainApiMessage, RuntimeApiMessage, RuntimeApiRequest,
+	BlockDescription,
 };
 use polkadot_node_subsystem_test_helpers::{make_subsystem_context, TestSubsystemContextHandle};
 use sp_core::testing::TaskExecutor;
@@ -672,10 +673,10 @@ fn finality_votes_ignore_disputed_candidates() {
 			let block_hash_b = Hash::repeat_byte(0x0b);
 
 			virtual_overseer.send(FromOverseer::Communication {
-				msg: DisputeCoordinatorMessage::DetermineUndisputedChain {
+				msg: DisputeCoordinatorMessage::DetermineUndisputedChain{
 					base_number: 10,
 					block_descriptions: vec![
-						(block_hash_a, session, vec![candidate_hash]),
+						BlockDescription { block_hash: block_hash_a, session, candidates: vec![candidate_hash] },
 					],
 					tx,
 				},
@@ -688,8 +689,8 @@ fn finality_votes_ignore_disputed_candidates() {
 				msg: DisputeCoordinatorMessage::DetermineUndisputedChain {
 					base_number: 10,
 					block_descriptions: vec![
-						(block_hash_a, session, vec![]),
-						(block_hash_b, session, vec![candidate_hash]),
+						BlockDescription { block_hash: block_hash_a, session, candidates: vec![] },
+						BlockDescription { block_hash: block_hash_b, session, candidates: vec![candidate_hash] },
 					],
 					tx,
 				},
@@ -804,7 +805,7 @@ fn supermajority_valid_dispute_may_be_finalized() {
 				msg: DisputeCoordinatorMessage::DetermineUndisputedChain {
 					base_number: 10,
 					block_descriptions: vec![
-						(block_hash_a, session, vec![candidate_hash]),
+						BlockDescription { block_hash: block_hash_a, session, candidates: vec![candidate_hash] },
 					],
 					tx,
 				},
@@ -817,8 +818,8 @@ fn supermajority_valid_dispute_may_be_finalized() {
 				msg: DisputeCoordinatorMessage::DetermineUndisputedChain {
 					base_number: 10,
 					block_descriptions: vec![
-						(block_hash_a, session, vec![]),
-						(block_hash_b, session, vec![candidate_hash]),
+						BlockDescription { block_hash: block_hash_a, session, candidates: vec![] },
+						BlockDescription { block_hash: block_hash_b, session, candidates: vec![candidate_hash] },
 					],
 					tx,
 				},

diff --git a/node/service/Cargo.toml b/node/service/Cargo.toml
@@ -112,7 +112,10 @@ polkadot-statement-distribution = { path = "../network/statement-distribution",
 
 [dev-dependencies]
 polkadot-test-client = { path = "../test/client" }
-env_logger = "0.8.4"
+polkadot-node-subsystem-test-helpers = { path = "../subsystem-test-helpers" }
+env_logger = "0.9.0"
+log = "0.4.14"
+assert_matches = "1.5.0"
 
 [features]
 default = ["db", "full-node"]

diff --git a/node/service/src/grandpa_support.rs b/node/service/src/grandpa_support.rs
@@ -18,23 +18,24 @@
 
 use std::sync::Arc;
 
-use sp_runtime::traits::{Block as BlockT, NumberFor};
-use sp_runtime::generic::BlockId;
 use sp_runtime::traits::Header as _;
+use sp_runtime::traits::{Block as BlockT, NumberFor};
+
+use crate::HeaderProvider;
 
 #[cfg(feature = "full-node")]
 use polkadot_primitives::v1::Hash;
 
 /// Returns the block hash of the block at the given `target_number` by walking
 /// backwards from the given `current_header`.
-pub(super) fn walk_backwards_to_target_block<Block, B>(
-	backend: &B,
+pub(super) fn walk_backwards_to_target_block<Block, HP>(
+	backend: &HP,
 	target_number: NumberFor<Block>,
 	current_header: &Block::Header,
 ) -> Result<(Block::Hash, NumberFor<Block>), sp_blockchain::Error>
 where
 	Block: BlockT,
-	B: sp_blockchain::HeaderBackend<Block>,
+	HP: HeaderProvider<Block>,
 {
 	let mut target_hash = current_header.hash();
 	let mut target_header = current_header.clone();
@@ -54,7 +55,7 @@ where
 
 		target_hash = *target_header.parent_hash();
 		target_header = backend
-			.header(BlockId::Hash(target_hash))?
+			.header(target_hash)?
 			.expect("Header known to exist due to the existence of one of its descendants; qed");
 	}
 }
@@ -69,7 +70,7 @@ pub(crate) struct PauseAfterBlockFor<N>(pub(crate) N, pub(crate) N);
 impl<Block, B> grandpa::VotingRule<Block, B> for PauseAfterBlockFor<NumberFor<Block>>
 where
 	Block: BlockT,
-	B: sp_blockchain::HeaderBackend<Block>,
+	B: sp_blockchain::HeaderBackend<Block> + 'static,
 {
 	fn restrict_vote(
 		&self,