chain-selection: allow reverting current block (#4103)

Block reversion of the current block is technically possible as can be
seen from

https://github.com/paritytech/polkadot-sdk/blob/39b1f50f1c251def87c1625d68567ed252dc6272/polkadot/runtime/parachains/src/disputes.rs#L1215-L1223

- [x] Fix the test

polkadot/node/core/chain-selection/src/lib.rs

@@ -619,7 +619,7 @@ async fn handle_active_leaf(
 
 // Extract all reversion logs from a header in ascending order.
 //
-// Ignores logs with number >= the block header number.
+// Ignores logs with number > the block header number.
 fn extract_reversion_logs(header: &Header) -> Vec<BlockNumber> {
 	let number = header.number;
 	let mut logs = header
@@ -639,14 +639,14 @@ fn extract_reversion_logs(header: &Header) -> Vec<BlockNumber> {
 
 				None
 			},
-			Ok(Some(ConsensusLog::Revert(b))) if b < number => Some(b),
+			Ok(Some(ConsensusLog::Revert(b))) if b <= number => Some(b),
 			Ok(Some(ConsensusLog::Revert(b))) => {
 				gum::warn!(
 					target: LOG_TARGET,
 					revert_target = b,
 					block_number = number,
 					block_hash = ?header.hash(),
-					"Block issued invalid revert digest targeting itself or future"
+					"Block issued invalid revert digest targeting future"
 				);
 
 				None

polkadot/node/core/chain-selection/src/tests.rs

@@ -966,19 +966,54 @@ fn ancestor_of_unviable_is_not_leaf_if_has_children() {
 }
 
 #[test]
-fn self_and_future_reversions_are_ignored() {
+fn self_reversions_are_not_ignored() {
 	test_harness(|backend, _, mut virtual_overseer| async move {
 		let finalized_number = 0;
 		let finalized_hash = Hash::repeat_byte(0);
 
 		// F <- A1 <- A2 <- A3.
 		//
-		// A3 reverts itself and future blocks. ignored.
+		// A3 reverts itself
+
+		let (_, chain_a) =
+			construct_chain_on_base(vec![1, 2, 3], finalized_number, finalized_hash, |h| {
+				if h.number == 3 {
+					add_reversions(h, vec![3])
+				}
+			});
+
+		let a2_hash = chain_a.iter().rev().nth(1).unwrap().0.hash();
+
+		import_blocks_into(
+			&mut virtual_overseer,
+			&backend,
+			Some((finalized_number, finalized_hash)),
+			chain_a.clone(),
+		)
+		.await;
+
+		assert_backend_contains(&backend, chain_a.iter().map(|(h, _)| h));
+		assert_leaves(&backend, vec![a2_hash]);
+		assert_leaves_query(&mut virtual_overseer, vec![a2_hash]).await;
+
+		virtual_overseer
+	});
+}
+
+#[test]
+fn future_reversions_are_ignored() {
+	test_harness(|backend, _, mut virtual_overseer| async move {
+		let finalized_number = 0;
+		let finalized_hash = Hash::repeat_byte(0);
+
+		// F <- A1 <- A2 <- A3.
+		//
+		// A3 reverts future blocks. ignored.
 
 		let (a3_hash, chain_a) =
 			construct_chain_on_base(vec![1, 2, 3], finalized_number, finalized_hash, |h| {
 				if h.number == 3 {
-					add_reversions(h, vec![3, 4, 100])
+					add_reversions(h, vec![4, 100])
 				}
 			});
 
@@ -1006,7 +1041,7 @@ fn revert_finalized_is_ignored() {
 
 		// F <- A1 <- A2 <- A3.
 		//
-		// A3 reverts itself and future blocks. ignored.
+		// A3 reverts finalized F and its ancestors. ignored.
 
 		let (a3_hash, chain_a) =
 			construct_chain_on_base(vec![1, 2, 3], finalized_number, finalized_hash, |h| {

polkadot/node/core/chain-selection/src/tree.rs

@@ -236,7 +236,7 @@ fn propagate_viability_update(
 	Ok(())
 }
 
-/// Imports a new block and applies any reversions to ancestors.
+/// Imports a new block and applies any reversions to ancestors or the block itself.
 pub(crate) fn import_block(
 	backend: &mut OverlayedBackend<impl Backend>,
 	block_hash: Hash,
@@ -246,25 +246,29 @@ pub(crate) fn import_block(
 	weight: BlockWeight,
 	stagnant_at: Timestamp,
 ) -> Result<(), Error> {
+	let block_entry =
 		add_block(backend, block_hash, block_number, parent_hash, weight, stagnant_at)?;
-	apply_ancestor_reversions(backend, block_hash, block_number, reversion_logs)?;
+	apply_reversions(backend, block_entry, reversion_logs)?;
 
 	Ok(())
 }
 
 // Load the given ancestor's block entry, in descending order from the `block_hash`.
-// The ancestor_number must be at least one block less than the `block_number`.
+// The ancestor_number must be not higher than the `block_entry`'s.
 //
 // The returned entry will be `None` if the range is invalid or any block in the path had
 // no entry present. If any block entry was missing, it can safely be assumed to
 // be finalized.
 fn load_ancestor(
 	backend: &mut OverlayedBackend<impl Backend>,
-	block_hash: Hash,
+	block_entry: &BlockEntry,
-	block_number: BlockNumber,
 	ancestor_number: BlockNumber,
 ) -> Result<Option<BlockEntry>, Error> {
-	if block_number <= ancestor_number {
+	let block_hash = block_entry.block_hash;
+	let block_number = block_entry.block_number;
+	if block_number == ancestor_number {
+		return Ok(Some(block_entry.clone()))
+	} else if block_number < ancestor_number {
 		return Ok(None)
 	}
 
@@ -300,7 +304,7 @@ fn add_block(
 	parent_hash: Hash,
 	weight: BlockWeight,
 	stagnant_at: Timestamp,
-) -> Result<(), Error> {
+) -> Result<BlockEntry, Error> {
 	let mut leaves = backend.load_leaves()?;
 	let parent_entry = backend.load_block_entry(&parent_hash)?;
 
@@ -308,7 +312,7 @@ fn add_block(
 		parent_entry.as_ref().and_then(|parent| parent.non_viable_ancestor_for_child());
 
 	// 1. Add the block to the DB assuming it's not reverted.
-	backend.write_block_entry(BlockEntry {
+	let block_entry = BlockEntry {
 		block_hash,
 		block_number,
 		parent_hash,
@@ -319,7 +323,8 @@ fn add_block(
 			approval: Approval::Unapproved,
 		},
 		weight,
-	});
+	};
+	backend.write_block_entry(block_entry.clone());
 
 	// 2. Update leaves if inherited viability is fine.
 	if inherited_viability.is_none() {
@@ -344,26 +349,25 @@ fn add_block(
 	stagnant_at_list.push(block_hash);
 	backend.write_stagnant_at(stagnant_at, stagnant_at_list);
 
-	Ok(())
+	Ok(block_entry)
 }
 
 /// Assuming that a block is already imported, accepts the number of the block
 /// as well as a list of reversions triggered by the block in ascending order.
-fn apply_ancestor_reversions(
+fn apply_reversions(
 	backend: &mut OverlayedBackend<impl Backend>,
-	block_hash: Hash,
+	block_entry: BlockEntry,
-	block_number: BlockNumber,
 	reversions: Vec<BlockNumber>,
 ) -> Result<(), Error> {
 	// Note: since revert numbers are  in ascending order, the expensive propagation
 	// of unviability is only heavy on the first log.
 	for revert_number in reversions {
-		let maybe_block_entry = load_ancestor(backend, block_hash, block_number, revert_number)?;
+		let maybe_block_entry = load_ancestor(backend, &block_entry, revert_number)?;
-		if let Some(block_entry) = &maybe_block_entry {
+		if let Some(entry) = &maybe_block_entry {
 			gum::trace!(
 				target: LOG_TARGET,
 				?revert_number,
-				revert_hash = ?block_entry.block_hash,
+				revert_hash = ?entry.block_hash,
 				"Block marked as reverted via scraped on-chain reversions"
 			);
 		}
@@ -372,8 +376,8 @@ fn apply_ancestor_reversions(
 			maybe_block_entry,
 			None,
 			revert_number,
-			Some(block_hash),
+			Some(block_entry.block_hash),
-			Some(block_number),
+			Some(block_entry.block_number),
 		)?;
 	}