From 91d4a207af43f8f81f56e4f24af74f7c6f590148 Mon Sep 17 00:00:00 2001
From: ordian <write@reusable.software>
Date: Thu, 18 Apr 2024 16:32:14 +0200
Subject: [PATCH] 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 | 6 +--
.../node/core/chain-selection/src/tests.rs | 43 +++++++++++++++++--
.../node/core/chain-selection/src/tree.rs | 42 ++++++++++--------
3 files changed, 65 insertions(+), 26 deletions(-)
diff --git a/polkadot/node/core/chain-selection/src/lib.rs b/polkadot/node/core/chain-selection/src/lib.rs
index 6f864fefb61..07c245e839b 100644
--- a/polkadot/node/core/chain-selection/src/lib.rs
+++ b/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
diff --git a/polkadot/node/core/chain-selection/src/tests.rs b/polkadot/node/core/chain-selection/src/tests.rs
index bc998f268a0..1fe87f04cd5 100644
--- a/polkadot/node/core/chain-selection/src/tests.rs
+++ b/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| {
diff --git a/polkadot/node/core/chain-selection/src/tree.rs b/polkadot/node/core/chain-selection/src/tree.rs
index b4aba30368a..1eb6c13a7f8 100644
--- a/polkadot/node/core/chain-selection/src/tree.rs
+++ b/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> {
- add_block(backend, block_hash, block_number, parent_hash, weight, stagnant_at)?;
- apply_ancestor_reversions(backend, block_hash, block_number, reversion_logs)?;
+ let block_entry =
+ add_block(backend, block_hash, block_number, parent_hash, weight, stagnant_at)?;
+ 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_number: BlockNumber,
+ block_entry: &BlockEntry,
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_number: BlockNumber,
+ block_entry: BlockEntry,
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)?;
- if let Some(block_entry) = &maybe_block_entry {
+ let maybe_block_entry = load_ancestor(backend, &block_entry, revert_number)?;
+ 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_number),
+ Some(block_entry.block_hash),
+ Some(block_entry.block_number),
)?;
}
--
GitLab