Grandpa revert procedure (#11162)

* Grandpa revert procedure

* Trigger ci pipeline

* Test rename

* Update client/finality-grandpa/src/authorities.rs

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

substrate/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)) =>

substrate/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 <https://www.gnu.org/licenses/>.
 
-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| {
+				let aux_revert = Box::new(move |client: Arc<FullClient>, backend, blocks| {
-					sc_consensus_babe::revert(client, backend, blocks)?;
+					sc_consensus_babe::revert(client.clone(), backend, blocks)?;
-					// TODO: grandpa revert
+					grandpa::revert(client, blocks)?;
 					Ok(())
 				});
-
+				Ok((cmd.run(client, backend, Some(aux_revert)), task_manager))
-				Ok((cmd.run(client, backend, Some(revert_aux)), task_manager))
 			})
 		},
 		#[cfg(feature = "try-runtime")]

substrate/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<Block, Client, Backend>(
 	client: Arc<Client>,
 	backend: Arc<Backend>,

substrate/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<F, E>(&mut self, hash: H, number: N, is_descendent_of: &F)
+	where
+		F: Fn(&H, &H) -> Result<bool, E>,
+	{
+		let mut filter = |node_hash: &H, node_num: &N, _: &PendingChange<H, N>| {
+			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<H: Eq, N> AuthoritySet<H, N>

substrate/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<Block, Client>(client: Arc<Client>, blocks: NumberFor<Block>) -> ClientResult<()>
+where
+	Block: BlockT,
+	Client: AuxStore
+		+ HeaderMetadata<Block, Error = sp_blockchain::Error>
+		+ HeaderBackend<Block>
+		+ ProvideRuntimeApi<Block>,
+{
+	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<Block> =
+		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::<Block, _, _>(&authority_set, new_set.as_ref(), |values| {
+		client.insert_aux(values, None)
+	})
+}

substrate/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<Block> = 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]);
+}

substrate/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<F>(
+	pub fn generate_blocks_at<F>(
 		&mut self,
 		at: BlockId<Block>,
 		count: usize,