diff --git a/bin/node-template/node/src/command.rs b/bin/node-template/node/src/command.rs index 66ee9fe45a55c..f033e779543a0 100644 --- a/bin/node-template/node/src/command.rs +++ b/bin/node-template/node/src/command.rs @@ -95,7 +95,11 @@ 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, None), task_manager)) + let aux_revert = Box::new(move |client, _, blocks| { + sc_finality_grandpa::revert(client, blocks)?; + Ok(()) + }); + Ok((cmd.run(client, backend, Some(aux_revert)), task_manager)) }) }, Some(Subcommand::Benchmark(cmd)) => diff --git a/bin/node/cli/src/command.rs b/bin/node/cli/src/command.rs index bd324b20fb019..db243ff6f597b 100644 --- a/bin/node/cli/src/command.rs +++ b/bin/node/cli/src/command.rs @@ -16,7 +16,11 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -use crate::{chain_spec, service, service::new_partial, Cli, Subcommand}; +use crate::{ + chain_spec, service, + service::{new_partial, FullClient}, + Cli, Subcommand, +}; use node_executor::ExecutorDispatch; use node_primitives::Block; use node_runtime::RuntimeApi; @@ -175,13 +179,12 @@ pub fn run() -> Result<()> { let runner = cli.create_runner(cmd)?; runner.async_run(|config| { let PartialComponents { client, task_manager, backend, .. } = new_partial(&config)?; - let revert_aux = Box::new(|client, backend, blocks| { - sc_consensus_babe::revert(client, backend, blocks)?; - // TODO: grandpa revert + let aux_revert = Box::new(move |client: Arc, backend, blocks| { + sc_consensus_babe::revert(client.clone(), backend, blocks)?; + grandpa::revert(client, blocks)?; Ok(()) }); - - Ok((cmd.run(client, backend, Some(revert_aux)), task_manager)) + Ok((cmd.run(client, backend, Some(aux_revert)), task_manager)) }) }, #[cfg(feature = "try-runtime")] diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 818384a5c3d7e..3d3a7f24df816 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -1832,7 +1832,9 @@ where Ok(BasicQueue::new(verifier, Box::new(block_import), justification_import, spawner, registry)) } -/// Reverts aux data. +/// Reverts protocol aux data to at most the last finalized block. +/// In particular, epoch-changes and block weights announced after the revert +/// point are removed. pub fn revert( client: Arc, backend: Arc, diff --git a/client/finality-grandpa/src/authorities.rs b/client/finality-grandpa/src/authorities.rs index 033a1c4bbb239..668fe5f269051 100644 --- a/client/finality-grandpa/src/authorities.rs +++ b/client/finality-grandpa/src/authorities.rs @@ -21,7 +21,7 @@ use std::{cmp::Ord, fmt::Debug, ops::Add}; use finality_grandpa::voter_set::VoterSet; -use fork_tree::ForkTree; +use fork_tree::{FilterAction, ForkTree}; use log::debug; use parity_scale_codec::{Decode, Encode}; use parking_lot::MappedMutexGuard; @@ -220,6 +220,37 @@ where pub(crate) fn current(&self) -> (u64, &[(AuthorityId, u64)]) { (self.set_id, &self.current_authorities[..]) } + + /// Revert to a specified block given its `hash` and `number`. + /// This removes all the authority set changes that were announced after + /// the revert point. + /// Revert point is identified by `number` and `hash`. + pub(crate) fn revert(&mut self, hash: H, number: N, is_descendent_of: &F) + where + F: Fn(&H, &H) -> Result, + { + let mut filter = |node_hash: &H, node_num: &N, _: &PendingChange| { + 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 + } + }; + + // Remove standard changes. + let _ = self.pending_standard_changes.drain_filter(&mut filter); + + // Remove forced changes. + self.pending_forced_changes + .retain(|change| !is_descendent_of(&hash, &change.canon_hash).unwrap_or_default()); + } } impl AuthoritySet diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index fef16286381ea..34d5b6bb1f70c 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -63,6 +63,7 @@ use parking_lot::RwLock; use prometheus_endpoint::{PrometheusError, Registry}; use sc_client_api::{ backend::{AuxStore, Backend}, + utils::is_descendent_of, BlockchainEvents, CallExecutor, ExecutionStrategy, ExecutorProvider, Finalizer, LockImportRun, StorageProvider, TransactionFor, }; @@ -71,7 +72,7 @@ use sc_telemetry::{telemetry, TelemetryHandle, CONSENSUS_DEBUG, CONSENSUS_INFO}; use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver}; use sp_api::ProvideRuntimeApi; use sp_application_crypto::AppKey; -use sp_blockchain::{Error as ClientError, HeaderBackend, HeaderMetadata}; +use sp_blockchain::{Error as ClientError, HeaderBackend, HeaderMetadata, Result as ClientResult}; use sp_consensus::SelectChain; use sp_core::crypto::ByteArray; use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr}; @@ -1162,3 +1163,49 @@ fn local_authority_id( .map(|(p, _)| p.clone()) }) } + +/// Reverts protocol aux data to at most the last finalized block. +/// In particular, standard and forced authority set changes announced after the +/// revert point are removed. +pub fn revert(client: Arc, blocks: NumberFor) -> ClientResult<()> +where + Block: BlockT, + Client: AuxStore + + HeaderMetadata + + HeaderBackend + + ProvideRuntimeApi, +{ + 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 + )))?; + + let info = client.info(); + let persistent_data: PersistentData = + aux_schema::load_persistent(&*client, info.genesis_hash, Zero::zero(), || unreachable!())?; + + let shared_authority_set = persistent_data.authority_set; + let mut authority_set = shared_authority_set.inner(); + + let is_descendent_of = is_descendent_of(&*client, None); + authority_set.revert(hash, number, &is_descendent_of); + + // The following has the side effect to properly reset the current voter state. + let (set_id, set_ref) = authority_set.current(); + let new_set = Some(NewAuthoritySet { + canon_hash: info.finalized_hash, + canon_number: info.finalized_number, + set_id, + authorities: set_ref.to_vec(), + }); + aux_schema::update_authority_set::(&authority_set, new_set.as_ref(), |values| { + client.insert_aux(values, None) + }) +} diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 2e545b6e88ebf..5083cbfc21d1d 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -57,7 +57,7 @@ use tokio::runtime::{Handle, Runtime}; use authorities::AuthoritySet; use communication::grandpa_protocol_name; -use sc_block_builder::BlockBuilderProvider; +use sc_block_builder::{BlockBuilder, BlockBuilderProvider}; use sc_consensus::LongestChain; use sc_keystore::LocalKeystore; use sp_application_crypto::key_types::GRANDPA; @@ -1685,3 +1685,128 @@ fn grandpa_environment_doesnt_send_equivocation_reports_for_itself() { let equivocation_proof = sp_finality_grandpa::Equivocation::Prevote(equivocation); assert!(environment.report_equivocation(equivocation_proof).is_ok()); } + +#[test] +fn revert_prunes_authority_changes() { + sp_tracing::try_init_simple(); + let runtime = Runtime::new().unwrap(); + + let peers = &[Ed25519Keyring::Alice, Ed25519Keyring::Bob, Ed25519Keyring::Charlie]; + + type TestBlockBuilder<'a> = + BlockBuilder<'a, Block, PeersFullClient, substrate_test_runtime_client::Backend>; + let edit_block = |builder: TestBlockBuilder| { + let mut block = builder.build().unwrap().block; + add_scheduled_change( + &mut block, + ScheduledChange { next_authorities: make_ids(peers), delay: 0 }, + ); + block + }; + + let api = TestApi::new(make_ids(peers)); + let mut net = GrandpaTestNet::new(api, 3, 0); + runtime.spawn(initialize_grandpa(&mut net, peers)); + + let peer = net.peer(0); + let client = peer.client().as_client(); + + // Test scenario: (X) = auth-change, 24 = revert-point + // + // +---------(27) + // / + // 0---(21)---23---24---25---(28)---30 + // ^ \ + // revert-point +------(29) + + // Construct canonical chain + + // add 20 blocks + peer.push_blocks(20, false); + // at block 21 we add an authority transition + peer.generate_blocks(1, BlockOrigin::File, edit_block); + // add more blocks on top of it (until we have 24) + peer.push_blocks(3, false); + // add more blocks on top of it (until we have 27) + peer.push_blocks(3, false); + // at block 28 we add an authority transition + peer.generate_blocks(1, BlockOrigin::File, edit_block); + // add more blocks on top of it (until we have 30) + peer.push_blocks(2, false); + + // Fork before revert point + + // add more blocks on top of block 23 (until we have 26) + let hash = peer.generate_blocks_at( + BlockId::Number(23), + 3, + BlockOrigin::File, + |builder| { + let mut block = builder.build().unwrap().block; + block.header.digest_mut().push(DigestItem::Other(vec![1])); + block + }, + false, + false, + true, + ForkChoiceStrategy::LongestChain, + ); + // at block 27 of the fork add an authority transition + peer.generate_blocks_at( + BlockId::Hash(hash), + 1, + BlockOrigin::File, + edit_block, + false, + false, + true, + ForkChoiceStrategy::LongestChain, + ); + + // Fork after revert point + + // add more block on top of block 25 (until we have 28) + let hash = peer.generate_blocks_at( + BlockId::Number(25), + 3, + BlockOrigin::File, + |builder| { + let mut block = builder.build().unwrap().block; + block.header.digest_mut().push(DigestItem::Other(vec![2])); + block + }, + false, + false, + true, + ForkChoiceStrategy::LongestChain, + ); + // at block 29 of the fork add an authority transition + peer.generate_blocks_at( + BlockId::Hash(hash), + 1, + BlockOrigin::File, + edit_block, + false, + false, + true, + ForkChoiceStrategy::LongestChain, + ); + + revert(client.clone(), 6).unwrap(); + + let persistent_data: PersistentData = aux_schema::load_persistent( + &*client, + client.info().genesis_hash, + Zero::zero(), + || unreachable!(), + ) + .unwrap(); + let changes_num: Vec<_> = persistent_data + .authority_set + .inner() + .pending_standard_changes + .iter() + .map(|(_, n, _)| *n) + .collect(); + assert_eq!(changes_num, [21, 27]); +} diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 3986ac47f3616..552879f35d934 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -334,7 +334,7 @@ where /// Add blocks to the peer -- edit the block before adding. The chain will /// start at the given block iD. - fn generate_blocks_at( + pub fn generate_blocks_at( &mut self, at: BlockId, count: usize,