From a59ccd174949cf10ca41b1c9eef2c7a626b44b00 Mon Sep 17 00:00:00 2001 From: Davide Galassi Date: Thu, 24 Mar 2022 09:51:55 +0100 Subject: [PATCH] BABE's revert procedure (#11022) * First rough draft for BABE revert * Proper babe revert test * Cleanup * Test trivial cleanup * Fix to make clippy happy * Check polkadot companion * Check cumulus companion * Remove babe's blocks weight on revert * Handle "empty" blockchain edge case * Run companions * Simplify the filter predicate * Saturating sub is not required * Run pipeline * Run pipeline again... --- bin/node-template/node/src/command.rs | 2 +- bin/node/cli/src/command.rs | 8 ++- client/cli/src/commands/revert_cmd.rs | 16 ++++- client/consensus/babe/src/lib.rs | 79 ++++++++++++++++++++- client/consensus/babe/src/tests.rs | 82 ++++++++++++++++++++++ client/consensus/epochs/src/lib.rs | 51 +++++++++++--- utils/fork-tree/src/lib.rs | 98 +++++++++++++++++++++++++-- 7 files changed, 314 insertions(+), 22 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 e208e324ee2aa..3c2039fde4757 100644 --- a/bin/node/cli/src/command.rs +++ b/bin/node/cli/src/command.rs @@ -168,7 +168,13 @@ 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 revert_aux = Box::new(|client, backend, blocks| { + sc_consensus_babe::revert(client, backend, blocks)?; + // TODO: grandpa revert + 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..f65e348b37b89 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,18 @@ pub struct RevertCmd { pub pruning_params: PruningParams, } +/// Revert handler for auxiliary data (e.g. consensus). +type AuxRevertHandler = + Box, Arc, NumberFor) -> 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 +62,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(client.clone(), backend.clone(), 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..4f3139a013d4b 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, @@ -1830,3 +1832,74 @@ where Ok(BasicQueue::new(verifier, Box::new(block_import), justification_import, spawner, registry)) } + +/// Reverts aux data. +pub fn revert( + client: Arc, + backend: Arc, + blocks: NumberFor, +) -> ClientResult<()> +where + Block: BlockT, + Client: AuxStore + + HeaderMetadata + + HeaderBackend + + ProvideRuntimeApi + + UsageProvider, + Client::Api: BabeApi, + Backend: BackendT, +{ + let best_number = client.info().best_number; + let finalized = client.info().finalized_number; + let revertible = blocks.min(best_number - finalized); + + let number = best_number - revertible; + let hash = client + .block_hash_from_id(&BlockId::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 = + 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); + } + + // 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, weight_keys.iter()) + }) +} diff --git a/client/consensus/babe/src/tests.rs b/client/consensus/babe/src/tests.rs index d2de05bc91952..080387c88655c 100644 --- a/client/consensus/babe/src/tests.rs +++ b/client/consensus/babe/src/tests.rs @@ -735,6 +735,88 @@ fn importing_block_one_sets_genesis_epoch() { assert_eq!(epoch_for_second_block, genesis_epoch); } +#[test] +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(); + + 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) --#18 < fork #2 + // / + // A(#1) ---- B(#7) ----#8----+-----#12----- C(#13) ---- D(#19) ------#21 < canon + // \ ^ \ + // \ revert *---- G(#13) ---- H(#19) ---#20 < fork #3 + // \ to #10 + // *-----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]), 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 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. + + 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 + canon[6], // B + fork2[4], // F + fork1[5], // E + ]; + + 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] 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..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}; @@ -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,37 @@ where }); self.epochs.insert((hash, number), persisted); } + + /// 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, + hash: Hash, + number: Number, + ) { + let is_descendent_of = descendent_of_builder.build_is_descendent_of(None); + + 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. + FilterAction::KeepNode + } else if number < *node_num && is_descendent_of(&hash, node_hash).unwrap_or_default() { + // Found a node to be removed. + FilterAction::Remove + } else { + // Not a parent or child of the one we're looking for, stop processing this branch. + FilterAction::KeepTree + } + }; + + self.inner.drain_filter(filter).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..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 @@ -624,6 +635,29 @@ where (None, false) => Ok(FinalizationResult::Unchanged), } } + + /// 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 drain_filter(&mut self, mut filter: F) -> impl Iterator + where + F: FnMut(&H, &N, &V) -> FilterAction, + { + let mut removed = Vec::new(); + let mut i = 0; + while i < self.roots.len() { + if self.roots[i].drain_filter(&mut filter, &mut removed) { + removed.push(self.roots.remove(i)); + } else { + i += 1; + } + } + self.rebalance(); + RemovedIterator { stack: removed } + } } // Workaround for: https://github.com/rust-lang/rust/issues/34537 @@ -849,6 +883,34 @@ mod node_implementation { }, } } + + /// 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. + /// 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) -> FilterAction, + { + 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, + } + } } } @@ -895,6 +957,8 @@ impl Iterator for RemovedIterator { #[cfg(test)] mod test { + use crate::FilterAction; + use super::{Error, FinalizationResult, ForkTree}; #[derive(Debug, PartialEq)] @@ -919,11 +983,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 +1522,28 @@ mod test { ["A", "F", "H", "L", "O", "P", "M", "I", "G", "B", "C", "D", "E", "J", "K"] ); } + + #[test] + fn tree_drain_filter() { + let (mut tree, _) = test_fork_tree(); + + 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.drain_filter(filter); + + assert_eq!( + tree.iter().map(|(h, _, _)| *h).collect::>(), + ["A", "B", "C", "D", "E", "F", "G"] + ); + + assert_eq!( + removed.map(|(h, _, _)| h).collect::>(), + ["J", "K", "H", "L", "M", "O", "I"] + ); + } }