From a80baaef4688c99b1eb35bef83fd6c5a08e9eab9 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 8 Mar 2022 20:08:07 +0100 Subject: [PATCH 01/14] First rough draft for BABE revert --- bin/node-template/node/src/command.rs | 2 +- bin/node/cli/src/command.rs | 11 ++- client/cli/src/commands/revert_cmd.rs | 14 +++- client/consensus/babe/src/lib.rs | 38 +++++++++ client/consensus/babe/src/tests.rs | 62 +++++++++++++++ client/consensus/epochs/src/lib.rs | 54 ++++++++++--- utils/fork-tree/src/lib.rs | 108 ++++++++++++++++++++++++-- 7 files changed, 271 insertions(+), 18 deletions(-) diff --git a/bin/node-template/node/src/command.rs b/bin/node-template/node/src/command.rs index 72c7a75b387bb..66ee9fe45a55c 100644 --- a/bin/node-template/node/src/command.rs +++ b/bin/node-template/node/src/command.rs @@ -95,7 +95,7 @@ pub fn run() -> sc_cli::Result<()> { runner.async_run(|config| { let PartialComponents { client, task_manager, backend, .. } = service::new_partial(&config)?; - Ok((cmd.run(client, backend), task_manager)) + Ok((cmd.run(client, backend, None), task_manager)) }) }, Some(Subcommand::Benchmark(cmd)) => diff --git a/bin/node/cli/src/command.rs b/bin/node/cli/src/command.rs index cc6480bb90d55..36006776b91dd 100644 --- a/bin/node/cli/src/command.rs +++ b/bin/node/cli/src/command.rs @@ -157,7 +157,16 @@ pub fn run() -> Result<()> { let runner = cli.create_runner(cmd)?; runner.async_run(|config| { let PartialComponents { client, task_manager, backend, .. } = new_partial(&config)?; - Ok((cmd.run(client, backend), task_manager)) + let client_clone = client.clone(); + let revert_aux = Box::new(move |blocks| { + sc_consensus_babe::revert::( + client_clone, + blocks, + )?; + Ok(()) + }); + + Ok((cmd.run(client, backend, Some(revert_aux)), task_manager)) }) }, #[cfg(feature = "try-runtime")] diff --git a/client/cli/src/commands/revert_cmd.rs b/client/cli/src/commands/revert_cmd.rs index c207d198d5a27..a015e937875af 100644 --- a/client/cli/src/commands/revert_cmd.rs +++ b/client/cli/src/commands/revert_cmd.rs @@ -24,7 +24,7 @@ use crate::{ use clap::Parser; use sc_client_api::{Backend, UsageProvider}; use sc_service::chain_ops::revert_chain; -use sp_runtime::traits::{Block as BlockT, Header as HeaderT}; +use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; use std::{fmt::Debug, str::FromStr, sync::Arc}; /// The `revert` command used revert the chain to a previous state. @@ -43,9 +43,16 @@ pub struct RevertCmd { pub pruning_params: PruningParams, } +type AuxRevertHandler = Box) -> error::Result<()>>; + impl RevertCmd { /// Run the revert command - pub async fn run(&self, client: Arc, backend: Arc) -> error::Result<()> + pub async fn run( + &self, + client: Arc, + backend: Arc, + aux_revert: Option>, + ) -> error::Result<()> where B: BlockT, BA: Backend, @@ -53,6 +60,9 @@ impl RevertCmd { <<::Header as HeaderT>::Number as FromStr>::Err: Debug, { let blocks = self.num.parse()?; + if let Some(aux_revert) = aux_revert { + aux_revert(blocks)?; + } revert_chain(client, backend, blocks)?; Ok(()) diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 442dbab77e120..fd6ee43150dc6 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -1830,3 +1830,41 @@ where Ok(BasicQueue::new(verifier, Box::new(block_import), justification_import, spawner, registry)) } + +/// Reverts aux data. +pub fn revert(client: Arc, blocks: NumberFor) -> ClientResult<()> +where + Block: BlockT, + Client: AuxStore + + HeaderMetadata + + HeaderBackend + + ProvideRuntimeApi + + UsageProvider, + Client::Api: BabeApi, +{ + let best_number = client.info().best_number; + let finalized = client.info().finalized_number; + let revertible = blocks.min(best_number - finalized); + + let number = best_number.saturating_sub(revertible); + let hash = client + .block_hash_from_id(&BlockId::Number(number))? + .ok_or(ClientError::Backend(format!("Hash lookup failure for block number: {}", number)))?; + + let config = Config::get(&*client)?; + let epoch_changes = + aux_schema::load_epoch_changes::(&*client, config.genesis_config())?; + let mut epoch_changes = epoch_changes.shared_data(); + + if number == Zero::zero() { + // Special case, no epoch changes data were present on genesis. + *epoch_changes = EpochChangesFor::::default(); + } else { + epoch_changes.revert(descendent_query(&*client), hash, number); + } + + aux_schema::write_epoch_changes::(&epoch_changes, |values| { + client.insert_aux(values, &[]).unwrap(); + }); + Ok(()) +} diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index d2de05bc91952..14eaa319baa9f 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -735,6 +735,68 @@ fn importing_block_one_sets_genesis_epoch() { assert_eq!(epoch_for_second_block, genesis_epoch); } +#[test] +fn revert_prunes_epoch_changes_tree() { + let mut net = BabeTestNet::new(1); + + let peer = net.peer(0); + let data = peer.data.as_ref().expect("babe link set up during initialization"); + + let client = peer.client().as_client(); + let mut block_import = data.block_import.lock().take().expect("import set up during init"); + let epoch_changes = data.link.epoch_changes.clone(); + + let mut proposer_factory = DummyFactory { + client: client.clone(), + config: data.link.config.clone(), + epoch_changes: data.link.epoch_changes.clone(), + mutator: Arc::new(|_, _| ()), + }; + + let mut propose_and_import_blocks_wrap = |parent_id, n| { + propose_and_import_blocks(&client, &mut proposer_factory, &mut block_import, parent_id, n) + }; + + // Test scenario. + // Information for epoch 19 is produced on three different forks at block #13. + // One branch starts before the revert point (epoch data should be maintained). + // One branch starts after the revert point (epoch data should be removed). + // + // *----------------- F(#13) < fork #2 + // / + // A(#1) ---- B(#7) ----#8----+-----#12----- C(#13) ---- D(#19) < canon + // \ ^ \ + // \ revert *---- G(#13) -----H(#19) < fork #3 + // \ here (#9) + // *-----E(#7) < fork #1 + + let canon = propose_and_import_blocks_wrap(BlockId::Number(0), 21); + let _fork_1 = propose_and_import_blocks_wrap(BlockId::Hash(canon[0]), 10); + let _fork_2 = propose_and_import_blocks_wrap(BlockId::Hash(canon[7]), 10); + let _fork_3 = propose_and_import_blocks_wrap(BlockId::Hash(canon[11]), 10); + + // We should be tracking a total of 9 epochs in the fork tree + assert_eq!(epoch_changes.shared_data().tree().iter().count(), 8); + // And only one root + assert_eq!(epoch_changes.shared_data().tree().roots().count(), 1); + + { + let epoch_changes = epoch_changes.shared_data(); + let it: Vec<_> = epoch_changes.tree().iter().collect(); + dbg!(it); + } + + // Revert to block #9 + revert(client, 12).unwrap(); + + { + // THIS IS NOT REFRESHED... + // let epoch_changes = epoch_changes.shared_data(); + // let it: Vec<_> = epoch_changes.tree().iter().collect(); + // dbg!(it); + } +} + #[test] fn importing_epoch_change_block_prunes_tree() { let mut net = BabeTestNet::new(1); diff --git a/client/consensus/epochs/src/lib.rs b/client/consensus/epochs/src/lib.rs index b380d8ed54904..1584b138bad95 100644 --- a/client/consensus/epochs/src/lib.rs +++ b/client/consensus/epochs/src/lib.rs @@ -660,15 +660,6 @@ where parent_number: Number, slot: E::Slot, ) -> Result>, fork_tree::Error> { - // find_node_where will give you the node in the fork-tree which is an ancestor - // of the `parent_hash` by default. if the last epoch was signalled at the parent_hash, - // then it won't be returned. we need to create a new fake chain head hash which - // "descends" from our parent-hash. - let fake_head_hash = fake_head_hash(parent_hash); - - let is_descendent_of = - descendent_of_builder.build_is_descendent_of(Some((fake_head_hash, *parent_hash))); - if parent_number == Zero::zero() { // need to insert the genesis epoch. return Ok(Some(ViableEpochDescriptor::UnimportedGenesis(slot))) @@ -683,6 +674,15 @@ where } } + // find_node_where will give you the node in the fork-tree which is an ancestor + // of the `parent_hash` by default. if the last epoch was signalled at the parent_hash, + // then it won't be returned. we need to create a new fake chain head hash which + // "descends" from our parent-hash. + let fake_head_hash = fake_head_hash(parent_hash); + + let is_descendent_of = + descendent_of_builder.build_is_descendent_of(Some((fake_head_hash, *parent_hash))); + // We want to find the deepest node in the tree which is an ancestor // of our block and where the start slot of the epoch was before the // slot of our block. The genesis special-case doesn't need to look @@ -798,6 +798,42 @@ where }); self.epochs.insert((hash, number), persisted); } + + /// TODO: document appropriately. + /// + /// Revert a branch down to the node after the specified block. + pub fn revert>( + &mut self, + descendent_of_builder: D, + hash: Hash, + number: Number, + ) { + let is_descendent_of = descendent_of_builder.build_is_descendent_of(None); + + let mut some_removed = false; + let mut predicate = |node_hash: &Hash, node_num: &Number, _: &PersistedEpochHeader| { + if number >= *node_num && + (is_descendent_of(node_hash, &hash).unwrap_or_default() || *node_hash == hash) + { + // Continue the search in this subtree. + (false, None) + } else if is_descendent_of(&hash, node_hash).unwrap_or_default() { + // Found a node to be removed. + some_removed = true; + (true, None) + } else if some_removed && *node_num < number { + // Backtrack detected, we can early stop the overall filtering operation. + (false, Some(true)) + } else { + // Not a parent or child of the one we're looking for, stop processing this branch. + (false, Some(false)) + } + }; + + self.inner.filter(&mut predicate).for_each(|(h, n, _)| { + self.epochs.remove(&(h, n)); + }); + } } /// Type alias to produce the epoch-changes tree from a block type. diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index a718ff26213e4..471a0d64e7cd0 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -624,6 +624,35 @@ where (None, false) => Ok(FinalizationResult::Unchanged), } } + + /// Remove from the tree some nodes (and their subtrees) using a `predicate`. + /// The `predicate` should return a tuple where the first `bool` indicates + /// if a node and its subtree should be removed, the second `Option` + /// indicates if the filtering operation should be stopped. + /// If `Some(false)` stop is requested only for the current node subtree, + /// if `Some(true)` stop is requested for the full filtering operation. + /// Tree traversal is performed using an pre-order depth-first search. + /// An iterator over all the pruned nodes is returned. + pub fn filter(&mut self, predicate: &mut F) -> impl Iterator + where + F: FnMut(&H, &N, &V) -> (bool, Option), + { + let mut removed = Vec::new(); + let mut i = 0; + while i < self.roots.len() { + let (remove, stop) = self.roots[i].filter(predicate, &mut removed); + if remove { + removed.push(self.roots.remove(i)); + } else { + i += 1; + } + if stop { + break + } + } + self.rebalance(); + RemovedIterator { stack: removed } + } } // Workaround for: https://github.com/rust-lang/rust/issues/34537 @@ -849,6 +878,45 @@ mod node_implementation { }, } } + + /// Calls a `predicate` for the given node. + /// The `predicate` should return a tuple where the first `bool` indicates + /// if the node and its subtree should be removed, the second `Option` + /// indicates if the filtering operation should be stopped. + /// If `Some(false)` stop is requested only for the node subtree, + /// if `Some(true)` stop is requested for the full filtering operation. + /// Pruned subtrees are added to the `removed` list. + /// Removal of this node optionally enacted by the caller. + /// Returns a couple of booleans where the fist indicates if a node + /// (and its subtree) should be removed, the second indicates if the + /// overall filtering operation stopped. + pub fn filter( + &mut self, + predicate: &mut F, + removed: &mut Vec>, + ) -> (bool, bool) + where + F: FnMut(&H, &N, &V) -> (bool, Option), + { + let (remove, pred_stop) = predicate(&self.hash, &self.number, &self.data); + let mut stop = pred_stop == Some(true); + if !remove && pred_stop.is_none() { + let mut i = 0; + while i < self.children.len() { + let (child_remove, child_stop) = self.children[i].filter(predicate, removed); + if child_remove { + removed.push(self.children.remove(i)); + } else { + i += 1; + } + if child_stop { + stop = child_stop; + break + } + } + } + (remove, stop) + } } } @@ -919,11 +987,11 @@ mod test { // / - G // / / // A - F - H - I - // \ - // - L - M \ - // - O - // \ - // — J - K + // \ \ + // \ - L - M + // \ \ + // \ - O + // - J - K // // (where N is not a part of fork tree) // @@ -1458,4 +1526,34 @@ mod test { ["A", "F", "H", "L", "O", "P", "M", "I", "G", "B", "C", "D", "E", "J", "K"] ); } + + #[test] + fn tree_filter() { + let (mut tree, _) = test_fork_tree(); + + let mut predicate = |h: &&str, _: &u64, _: &()| { + match *h { + // Don't remove and continue filtering. + "A" | "B" | "F" => (false, None), + // Don't remove and don't filter the node subtree. + "C" => (false, Some(false)), + // Don't remove and stop the overall filtering operation. + "G" => (false, Some(true)), + // Remove and continue filtering. + "H" => (true, None), + // Should never happen because of the stop and pruning conditions. + "D" | "E" | "I" | "L" | "M" | "O" | "J" | "K" | _ => + panic!("Unexpected filtering for: {}", *h), + } + }; + + let removed = tree.filter(&mut predicate); + + assert_eq!( + tree.iter().map(|(h, _, _)| *h).collect::>(), + ["A", "B", "C", "D", "E", "F", "G", "J", "K"] + ); + + assert_eq!(removed.map(|(h, _, _)| h).collect::>(), vec!["H", "L", "M", "O", "I"]); + } } From a494b1263f756a740310b2a9729b2ee25e8f339b Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Mon, 14 Mar 2022 13:16:07 +0100 Subject: [PATCH 02/14] Proper babe revert test --- client/consensus/babe/src/lib.rs | 5 ++-- client/consensus/babe/src/tests.rs | 40 ++++++++++++++++-------------- client/consensus/epochs/src/lib.rs | 6 ++--- 3 files changed, 26 insertions(+), 25 deletions(-) diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index fd6ee43150dc6..61529d550381b 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -1864,7 +1864,6 @@ where } aux_schema::write_epoch_changes::(&epoch_changes, |values| { - client.insert_aux(values, &[]).unwrap(); - }); - Ok(()) + client.insert_aux(values, &[]) + }) } diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index 14eaa319baa9f..e58a71e241177 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -767,34 +767,36 @@ fn revert_prunes_epoch_changes_tree() { // A(#1) ---- B(#7) ----#8----+-----#12----- C(#13) ---- D(#19) < canon // \ ^ \ // \ revert *---- G(#13) -----H(#19) < fork #3 - // \ here (#9) + // \ to #10 // *-----E(#7) < fork #1 let canon = propose_and_import_blocks_wrap(BlockId::Number(0), 21); - let _fork_1 = propose_and_import_blocks_wrap(BlockId::Hash(canon[0]), 10); - let _fork_2 = propose_and_import_blocks_wrap(BlockId::Hash(canon[7]), 10); - let _fork_3 = propose_and_import_blocks_wrap(BlockId::Hash(canon[11]), 10); + let fork1 = propose_and_import_blocks_wrap(BlockId::Hash(canon[0]), 10); + let fork2 = propose_and_import_blocks_wrap(BlockId::Hash(canon[7]), 10); + let _fork3 = propose_and_import_blocks_wrap(BlockId::Hash(canon[11]), 10); // We should be tracking a total of 9 epochs in the fork tree assert_eq!(epoch_changes.shared_data().tree().iter().count(), 8); // And only one root assert_eq!(epoch_changes.shared_data().tree().roots().count(), 1); - { - let epoch_changes = epoch_changes.shared_data(); - let it: Vec<_> = epoch_changes.tree().iter().collect(); - dbg!(it); - } - - // Revert to block #9 - revert(client, 12).unwrap(); - - { - // THIS IS NOT REFRESHED... - // let epoch_changes = epoch_changes.shared_data(); - // let it: Vec<_> = epoch_changes.tree().iter().collect(); - // dbg!(it); - } + // Revert to block #10 (best(22) - 12) + revert(client.clone(), 12).expect("revert should work for the baked test scenario"); + + // Load and check epoch changes. + let config = Config::get(&*client).expect("config created during initialization"); + let epoch_changes = + aux_schema::load_epoch_changes::(&*client, config.genesis_config()) + .expect("epoch changes available"); + let epoch_changes = epoch_changes.shared_data(); + + let expected_nodes = vec![ + canon[0], // A + canon[6], // B + fork2[4], // F + fork1[5], // E + ]; + assert_eq!(epoch_changes.tree().iter().map(|(h, _, _)| *h).collect::>(), expected_nodes); } #[test] diff --git a/client/consensus/epochs/src/lib.rs b/client/consensus/epochs/src/lib.rs index 1584b138bad95..e35249a8d37ea 100644 --- a/client/consensus/epochs/src/lib.rs +++ b/client/consensus/epochs/src/lib.rs @@ -799,9 +799,9 @@ where self.epochs.insert((hash, number), persisted); } - /// TODO: document appropriately. - /// - /// Revert a branch down to the node after the specified block. + /// Revert to a specified block given its `hash` and `number`. + /// This removes all the epoch changes information that were announced by + /// all the given block descendents. pub fn revert>( &mut self, descendent_of_builder: D, From 3e95228011917feb41d25bd08232e669261cfcac Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Mon, 14 Mar 2022 15:36:33 +0100 Subject: [PATCH 03/14] Cleanup --- bin/node/cli/src/command.rs | 6 ++---- client/cli/src/commands/revert_cmd.rs | 1 + 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/bin/node/cli/src/command.rs b/bin/node/cli/src/command.rs index 36006776b91dd..fd709a2cb308a 100644 --- a/bin/node/cli/src/command.rs +++ b/bin/node/cli/src/command.rs @@ -159,10 +159,8 @@ pub fn run() -> Result<()> { let PartialComponents { client, task_manager, backend, .. } = new_partial(&config)?; let client_clone = client.clone(); let revert_aux = Box::new(move |blocks| { - sc_consensus_babe::revert::( - client_clone, - blocks, - )?; + sc_consensus_babe::revert(client_clone, blocks)?; + // TODO: grandpa Ok(()) }); diff --git a/client/cli/src/commands/revert_cmd.rs b/client/cli/src/commands/revert_cmd.rs index a015e937875af..27e6d31d1e620 100644 --- a/client/cli/src/commands/revert_cmd.rs +++ b/client/cli/src/commands/revert_cmd.rs @@ -43,6 +43,7 @@ pub struct RevertCmd { pub pruning_params: PruningParams, } +/// Revert handler for auxiliary data (e.g. consensus). type AuxRevertHandler = Box) -> error::Result<()>>; impl RevertCmd { From cbbd61fded3056023b234f3e2d487041d799a382 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Mon, 14 Mar 2022 16:25:24 +0100 Subject: [PATCH 04/14] Test trivial cleanup --- client/consensus/babe/src/tests.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index e58a71e241177..b304c73f5e985 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -784,11 +784,17 @@ fn revert_prunes_epoch_changes_tree() { revert(client.clone(), 12).expect("revert should work for the baked test scenario"); // Load and check epoch changes. - let config = Config::get(&*client).expect("config created during initialization"); - let epoch_changes = - aux_schema::load_epoch_changes::(&*client, config.genesis_config()) - .expect("epoch changes available"); - let epoch_changes = epoch_changes.shared_data(); + + let actual_nodes = aux_schema::load_epoch_changes::( + &*client, + data.link.config.genesis_config(), + ) + .expect("load epoch changes") + .shared_data() + .tree() + .iter() + .map(|(h, _, _)| *h) + .collect::>(); let expected_nodes = vec![ canon[0], // A @@ -796,7 +802,8 @@ fn revert_prunes_epoch_changes_tree() { fork2[4], // F fork1[5], // E ]; - assert_eq!(epoch_changes.tree().iter().map(|(h, _, _)| *h).collect::>(), expected_nodes); + + assert_eq!(actual_nodes, expected_nodes); } #[test] From 301f55a0421d3e13aba4725030e4964dadea79f7 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Mon, 14 Mar 2022 16:43:00 +0100 Subject: [PATCH 05/14] Fix to make clippy happy --- utils/fork-tree/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index 471a0d64e7cd0..c3fbf4f2a567f 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -1542,8 +1542,7 @@ mod test { // Remove and continue filtering. "H" => (true, None), // Should never happen because of the stop and pruning conditions. - "D" | "E" | "I" | "L" | "M" | "O" | "J" | "K" | _ => - panic!("Unexpected filtering for: {}", *h), + _ => panic!("Unexpected filtering for node: {}", *h), } }; From 2cf3e1c85e467c12f3333299eb72637c0678a691 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Mon, 14 Mar 2022 19:34:40 +0100 Subject: [PATCH 06/14] Check polkadot companion From 71ae24d027ad8f5c2bf5eb89316d488e750bb683 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Tue, 15 Mar 2022 09:53:15 +0100 Subject: [PATCH 07/14] Check cumulus companion From c052a281c80da689cb2ef93ac0a5fccbe46dbca2 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 16 Mar 2022 18:02:39 +0100 Subject: [PATCH 08/14] Remove babe's blocks weight on revert --- bin/node/cli/src/command.rs | 7 ++-- client/cli/src/commands/revert_cmd.rs | 7 ++-- client/consensus/babe/src/lib.rs | 48 +++++++++++++++++++++++---- client/consensus/babe/src/tests.rs | 29 +++++++++++----- 4 files changed, 69 insertions(+), 22 deletions(-) diff --git a/bin/node/cli/src/command.rs b/bin/node/cli/src/command.rs index fd709a2cb308a..1b3158028681d 100644 --- a/bin/node/cli/src/command.rs +++ b/bin/node/cli/src/command.rs @@ -157,10 +157,9 @@ pub fn run() -> Result<()> { let runner = cli.create_runner(cmd)?; runner.async_run(|config| { let PartialComponents { client, task_manager, backend, .. } = new_partial(&config)?; - let client_clone = client.clone(); - let revert_aux = Box::new(move |blocks| { - sc_consensus_babe::revert(client_clone, blocks)?; - // TODO: grandpa + let revert_aux = Box::new(|client, backend, blocks| { + sc_consensus_babe::revert(client, backend, blocks)?; + // TODO: grandpa revert Ok(()) }); diff --git a/client/cli/src/commands/revert_cmd.rs b/client/cli/src/commands/revert_cmd.rs index 27e6d31d1e620..f65e348b37b89 100644 --- a/client/cli/src/commands/revert_cmd.rs +++ b/client/cli/src/commands/revert_cmd.rs @@ -44,7 +44,8 @@ pub struct RevertCmd { } /// Revert handler for auxiliary data (e.g. consensus). -type AuxRevertHandler = Box) -> error::Result<()>>; +type AuxRevertHandler = + Box, Arc, NumberFor) -> error::Result<()>>; impl RevertCmd { /// Run the revert command @@ -52,7 +53,7 @@ impl RevertCmd { &self, client: Arc, backend: Arc, - aux_revert: Option>, + aux_revert: Option>, ) -> error::Result<()> where B: BlockT, @@ -62,7 +63,7 @@ impl RevertCmd { { let blocks = self.num.parse()?; if let Some(aux_revert) = aux_revert { - aux_revert(blocks)?; + aux_revert(client.clone(), backend.clone(), blocks)?; } revert_chain(client, backend, blocks)?; diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 61529d550381b..5c7e3ec915d89 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -92,8 +92,8 @@ use retain_mut::RetainMut; use schnorrkel::SignatureError; use sc_client_api::{ - backend::AuxStore, AuxDataOperations, BlockchainEvents, FinalityNotification, PreCommitActions, - ProvideUncles, UsageProvider, + backend::AuxStore, AuxDataOperations, Backend as BackendT, BlockchainEvents, + FinalityNotification, PreCommitActions, ProvideUncles, UsageProvider, }; use sc_consensus::{ block_import::{ @@ -113,7 +113,9 @@ use sc_telemetry::{telemetry, TelemetryHandle, CONSENSUS_DEBUG, CONSENSUS_TRACE} use sp_api::{ApiExt, ProvideRuntimeApi}; use sp_application_crypto::AppKey; use sp_block_builder::BlockBuilder as BlockBuilderApi; -use sp_blockchain::{Error as ClientError, HeaderBackend, HeaderMetadata, Result as ClientResult}; +use sp_blockchain::{ + Backend as _, Error as ClientError, HeaderBackend, HeaderMetadata, Result as ClientResult, +}; use sp_consensus::{ BlockOrigin, CacheKeyId, CanAuthorWith, Environment, Error as ConsensusError, Proposer, SelectChain, @@ -1832,7 +1834,11 @@ where } /// Reverts aux data. -pub fn revert(client: Arc, blocks: NumberFor) -> ClientResult<()> +pub fn revert( + client: Arc, + backend: Arc, + blocks: NumberFor, +) -> ClientResult<()> where Block: BlockT, Client: AuxStore @@ -1841,6 +1847,7 @@ where + ProvideRuntimeApi + UsageProvider, Client::Api: BabeApi, + Backend: BackendT, { let best_number = client.info().best_number; let finalized = client.info().finalized_number; @@ -1849,7 +1856,12 @@ where let number = best_number.saturating_sub(revertible); let hash = client .block_hash_from_id(&BlockId::Number(number))? - .ok_or(ClientError::Backend(format!("Hash lookup failure for block number: {}", number)))?; + .ok_or(ClientError::Backend(format!( + "Unexpected hash lookup failure for block number: {}", + number + )))?; + + // Revert epoch changes tree. let config = Config::get(&*client)?; let epoch_changes = @@ -1863,7 +1875,31 @@ where epoch_changes.revert(descendent_query(&*client), hash, number); } + // Remove block weights added after the revert point. + + let mut weight_keys = HashSet::with_capacity(revertible.saturated_into()); + let leaves = backend.blockchain().leaves()?.into_iter().filter(|&leaf| { + sp_blockchain::tree_route(&*client, hash, leaf) + .map(|route| route.retracted().is_empty()) + .unwrap_or_default() + }); + for leaf in leaves { + let mut hash = leaf; + // Insert parent after parent until we don't hit an already processed + // branch or we reach a direct child of the rollback point. + while weight_keys.insert(aux_schema::block_weight_key(hash)) { + let meta = client.header_metadata(hash)?; + if meta.number == number + One::one() { + // We've reached a child of the revert point, stop here. + break + } + hash = client.header_metadata(hash)?.parent; + } + } + let weight_keys: Vec<_> = weight_keys.iter().map(|val| val.as_slice()).collect(); + + // Write epoch changes and remove weights in one shot. aux_schema::write_epoch_changes::(&epoch_changes, |values| { - client.insert_aux(values, &[]) + client.insert_aux(values, weight_keys.iter()) }) } diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index b304c73f5e985..080387c88655c 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -736,13 +736,14 @@ fn importing_block_one_sets_genesis_epoch() { } #[test] -fn revert_prunes_epoch_changes_tree() { +fn revert_prunes_epoch_changes_and_removes_weights() { let mut net = BabeTestNet::new(1); let peer = net.peer(0); let data = peer.data.as_ref().expect("babe link set up during initialization"); let client = peer.client().as_client(); + let backend = peer.client().as_backend(); let mut block_import = data.block_import.lock().take().expect("import set up during init"); let epoch_changes = data.link.epoch_changes.clone(); @@ -762,26 +763,25 @@ fn revert_prunes_epoch_changes_tree() { // One branch starts before the revert point (epoch data should be maintained). // One branch starts after the revert point (epoch data should be removed). // - // *----------------- F(#13) < fork #2 + // *----------------- F(#13) --#18 < fork #2 // / - // A(#1) ---- B(#7) ----#8----+-----#12----- C(#13) ---- D(#19) < canon + // A(#1) ---- B(#7) ----#8----+-----#12----- C(#13) ---- D(#19) ------#21 < canon // \ ^ \ - // \ revert *---- G(#13) -----H(#19) < fork #3 + // \ revert *---- G(#13) ---- H(#19) ---#20 < fork #3 // \ to #10 - // *-----E(#7) < fork #1 - + // *-----E(#7)---#11 < fork #1 let canon = propose_and_import_blocks_wrap(BlockId::Number(0), 21); let fork1 = propose_and_import_blocks_wrap(BlockId::Hash(canon[0]), 10); let fork2 = propose_and_import_blocks_wrap(BlockId::Hash(canon[7]), 10); - let _fork3 = propose_and_import_blocks_wrap(BlockId::Hash(canon[11]), 10); + let fork3 = propose_and_import_blocks_wrap(BlockId::Hash(canon[11]), 8); // We should be tracking a total of 9 epochs in the fork tree assert_eq!(epoch_changes.shared_data().tree().iter().count(), 8); // And only one root assert_eq!(epoch_changes.shared_data().tree().roots().count(), 1); - // Revert to block #10 (best(22) - 12) - revert(client.clone(), 12).expect("revert should work for the baked test scenario"); + // Revert canon chain to block #10 (best(21) - 11) + revert(client.clone(), backend, 11).expect("revert should work for baked test scenario"); // Load and check epoch changes. @@ -804,6 +804,17 @@ fn revert_prunes_epoch_changes_tree() { ]; assert_eq!(actual_nodes, expected_nodes); + + let weight_data_check = |hashes: &[Hash], expected: bool| { + hashes.iter().all(|hash| { + aux_schema::load_block_weight(&*client, hash).unwrap().is_some() == expected + }) + }; + assert!(weight_data_check(&canon[..10], true)); + assert!(weight_data_check(&canon[10..], false)); + assert!(weight_data_check(&fork1, true)); + assert!(weight_data_check(&fork2, true)); + assert!(weight_data_check(&fork3, false)); } #[test] From 81f8f854703140c002a4b76d776dc9ba1abb060f Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 16 Mar 2022 19:24:20 +0100 Subject: [PATCH 09/14] Handle "empty" blockchain edge case --- client/consensus/babe/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 5c7e3ec915d89..bcd8f5f7c455d 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -1889,7 +1889,7 @@ where // branch or we reach a direct child of the rollback point. while weight_keys.insert(aux_schema::block_weight_key(hash)) { let meta = client.header_metadata(hash)?; - if meta.number == number + One::one() { + if meta.number <= number + One::one() { // We've reached a child of the revert point, stop here. break } From a418f81babccedc6a92bdfb44bf7ed14e60688a1 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 16 Mar 2022 20:15:20 +0100 Subject: [PATCH 10/14] Run companions From de1ede9e465f7748b4b3426f68596830ec1e318c Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 23 Mar 2022 10:53:08 +0100 Subject: [PATCH 11/14] Simplify the filter predicate --- client/consensus/epochs/src/lib.rs | 19 ++--- utils/fork-tree/src/lib.rs | 117 +++++++++++++---------------- 2 files changed, 61 insertions(+), 75 deletions(-) diff --git a/client/consensus/epochs/src/lib.rs b/client/consensus/epochs/src/lib.rs index e35249a8d37ea..90081bf9af442 100644 --- a/client/consensus/epochs/src/lib.rs +++ b/client/consensus/epochs/src/lib.rs @@ -21,7 +21,7 @@ pub mod migration; use codec::{Decode, Encode}; -use fork_tree::ForkTree; +use fork_tree::{FilterAction, ForkTree}; use sc_client_api::utils::is_descendent_of; use sp_blockchain::{Error as ClientError, HeaderBackend, HeaderMetadata}; use sp_runtime::traits::{Block as BlockT, NumberFor, One, Zero}; @@ -810,27 +810,22 @@ where ) { let is_descendent_of = descendent_of_builder.build_is_descendent_of(None); - let mut some_removed = false; - let mut predicate = |node_hash: &Hash, node_num: &Number, _: &PersistedEpochHeader| { + let filter = |node_hash: &Hash, node_num: &Number, _: &PersistedEpochHeader| { if number >= *node_num && (is_descendent_of(node_hash, &hash).unwrap_or_default() || *node_hash == hash) { // Continue the search in this subtree. - (false, None) - } else if is_descendent_of(&hash, node_hash).unwrap_or_default() { + FilterAction::KeepNode + } else if number < *node_num && is_descendent_of(&hash, node_hash).unwrap_or_default() { // Found a node to be removed. - some_removed = true; - (true, None) - } else if some_removed && *node_num < number { - // Backtrack detected, we can early stop the overall filtering operation. - (false, Some(true)) + FilterAction::Remove } else { // Not a parent or child of the one we're looking for, stop processing this branch. - (false, Some(false)) + FilterAction::KeepTree } }; - self.inner.filter(&mut predicate).for_each(|(h, n, _)| { + self.inner.drain_filter(filter).for_each(|(h, n, _)| { self.epochs.remove(&(h, n)); }); } diff --git a/utils/fork-tree/src/lib.rs b/utils/fork-tree/src/lib.rs index c3fbf4f2a567f..1d9b39f7dc04b 100644 --- a/utils/fork-tree/src/lib.rs +++ b/utils/fork-tree/src/lib.rs @@ -69,6 +69,17 @@ pub enum FinalizationResult { Unchanged, } +/// Filtering action. +#[derive(Debug, PartialEq)] +pub enum FilterAction { + /// Remove the node and its subtree. + Remove, + /// Maintain the node. + KeepNode, + /// Maintain the node and its subtree. + KeepTree, +} + /// A tree data structure that stores several nodes across multiple branches. /// Top-level branches are called roots. The tree has functionality for /// finalizing nodes, which means that that node is traversed, and all competing @@ -625,30 +636,24 @@ where } } - /// Remove from the tree some nodes (and their subtrees) using a `predicate`. - /// The `predicate` should return a tuple where the first `bool` indicates - /// if a node and its subtree should be removed, the second `Option` - /// indicates if the filtering operation should be stopped. - /// If `Some(false)` stop is requested only for the current node subtree, - /// if `Some(true)` stop is requested for the full filtering operation. - /// Tree traversal is performed using an pre-order depth-first search. + /// Remove from the tree some nodes (and their subtrees) using a `filter` predicate. + /// The `filter` is called over tree nodes and returns a filter action: + /// - `Remove` if the node and its subtree should be removed; + /// - `KeepNode` if we should maintain the node and keep processing the tree. + /// - `KeepTree` if we should maintain the node and its entire subtree. /// An iterator over all the pruned nodes is returned. - pub fn filter(&mut self, predicate: &mut F) -> impl Iterator + pub fn drain_filter(&mut self, mut filter: F) -> impl Iterator where - F: FnMut(&H, &N, &V) -> (bool, Option), + F: FnMut(&H, &N, &V) -> FilterAction, { let mut removed = Vec::new(); let mut i = 0; while i < self.roots.len() { - let (remove, stop) = self.roots[i].filter(predicate, &mut removed); - if remove { + if self.roots[i].drain_filter(&mut filter, &mut removed) { removed.push(self.roots.remove(i)); } else { i += 1; } - if stop { - break - } } self.rebalance(); RemovedIterator { stack: removed } @@ -879,43 +884,32 @@ mod node_implementation { } } - /// Calls a `predicate` for the given node. - /// The `predicate` should return a tuple where the first `bool` indicates - /// if the node and its subtree should be removed, the second `Option` - /// indicates if the filtering operation should be stopped. - /// If `Some(false)` stop is requested only for the node subtree, - /// if `Some(true)` stop is requested for the full filtering operation. + /// Calls a `filter` predicate for the given node. + /// The `filter` is called over tree nodes and returns a filter action: + /// - `Remove` if the node and its subtree should be removed; + /// - `KeepNode` if we should maintain the node and keep processing the tree; + /// - `KeepTree` if we should maintain the node and its entire subtree. /// Pruned subtrees are added to the `removed` list. - /// Removal of this node optionally enacted by the caller. - /// Returns a couple of booleans where the fist indicates if a node - /// (and its subtree) should be removed, the second indicates if the - /// overall filtering operation stopped. - pub fn filter( - &mut self, - predicate: &mut F, - removed: &mut Vec>, - ) -> (bool, bool) + /// Returns a booleans indicateing if this node (and its subtree) should be removed. + pub fn drain_filter(&mut self, filter: &mut F, removed: &mut Vec>) -> bool where - F: FnMut(&H, &N, &V) -> (bool, Option), + F: FnMut(&H, &N, &V) -> FilterAction, { - let (remove, pred_stop) = predicate(&self.hash, &self.number, &self.data); - let mut stop = pred_stop == Some(true); - if !remove && pred_stop.is_none() { - let mut i = 0; - while i < self.children.len() { - let (child_remove, child_stop) = self.children[i].filter(predicate, removed); - if child_remove { - removed.push(self.children.remove(i)); - } else { - i += 1; - } - if child_stop { - stop = child_stop; - break + match filter(&self.hash, &self.number, &self.data) { + FilterAction::KeepNode => { + let mut i = 0; + while i < self.children.len() { + if self.children[i].drain_filter(filter, removed) { + removed.push(self.children.remove(i)); + } else { + i += 1; + } } - } + false + }, + FilterAction::KeepTree => false, + FilterAction::Remove => true, } - (remove, stop) } } } @@ -963,6 +957,8 @@ impl Iterator for RemovedIterator { #[cfg(test)] mod test { + use crate::FilterAction; + use super::{Error, FinalizationResult, ForkTree}; #[derive(Debug, PartialEq)] @@ -1528,31 +1524,26 @@ mod test { } #[test] - fn tree_filter() { + fn tree_drain_filter() { let (mut tree, _) = test_fork_tree(); - let mut predicate = |h: &&str, _: &u64, _: &()| { - match *h { - // Don't remove and continue filtering. - "A" | "B" | "F" => (false, None), - // Don't remove and don't filter the node subtree. - "C" => (false, Some(false)), - // Don't remove and stop the overall filtering operation. - "G" => (false, Some(true)), - // Remove and continue filtering. - "H" => (true, None), - // Should never happen because of the stop and pruning conditions. - _ => panic!("Unexpected filtering for node: {}", *h), - } + let filter = |h: &&str, _: &u64, _: &()| match *h { + "A" | "B" | "F" | "G" => FilterAction::KeepNode, + "C" => FilterAction::KeepTree, + "H" | "J" => FilterAction::Remove, + _ => panic!("Unexpected filtering for node: {}", *h), }; - let removed = tree.filter(&mut predicate); + let removed = tree.drain_filter(filter); assert_eq!( tree.iter().map(|(h, _, _)| *h).collect::>(), - ["A", "B", "C", "D", "E", "F", "G", "J", "K"] + ["A", "B", "C", "D", "E", "F", "G"] ); - assert_eq!(removed.map(|(h, _, _)| h).collect::>(), vec!["H", "L", "M", "O", "I"]); + assert_eq!( + removed.map(|(h, _, _)| h).collect::>(), + ["J", "K", "H", "L", "M", "O", "I"] + ); } } From 41b192ef2dae2f4ae8ec3305bc273958de923332 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 23 Mar 2022 14:57:00 +0100 Subject: [PATCH 12/14] Saturating sub is not required --- client/consensus/babe/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index bcd8f5f7c455d..4f3139a013d4b 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -1853,7 +1853,7 @@ where let finalized = client.info().finalized_number; let revertible = blocks.min(best_number - finalized); - let number = best_number.saturating_sub(revertible); + let number = best_number - revertible; let hash = client .block_hash_from_id(&BlockId::Number(number))? .ok_or(ClientError::Backend(format!( From 042b0eabd2308cf4ae3c4aacd2dcf8e4151dbbf6 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 23 Mar 2022 18:47:35 +0100 Subject: [PATCH 13/14] Run pipeline From b9d15499b6a568e2bea739711d9696596077da00 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Wed, 23 Mar 2022 19:35:00 +0100 Subject: [PATCH 14/14] Run pipeline again...