FinalityNotification: Directly include stale blocks (#9904)

The finality notification was already carrying the information about the
stale heads. However, most users of the stale heads were expanding these
stale heads to all the stale blocks. So, we were iterating the same
forks multiple times in the node for each finality notification. Also in
a possible future where we start actually pruning headers as well,
expanding these forks would fail.

So, this pull request is changing the finality notification to directly
carry the stale blocks (which were calculated any way already).

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: 2 people

cumulus/client/pov-recovery/src/tests.rs

@@ -1203,7 +1203,7 @@ async fn candidate_is_finalized_while_awaiting_recovery() {
 	let (unpin_sender, _unpin_receiver) = sc_utils::mpsc::tracing_unbounded("test_unpin", 10);
 	finality_notifications_tx
 		.unbounded_send(FinalityNotification::from_summary(
-			FinalizeSummary { header: header.clone(), finalized: vec![], stale_heads: vec![] },
+			FinalizeSummary { header: header.clone(), finalized: vec![], stale_blocks: vec![] },
 			unpin_sender,
 		))
 		.unwrap();

prdoc/pr_9904.prdoc

@@ -0,0 +1,24 @@
+title: 'FinalityNotification: Directly include stale blocks'
+doc:
+- audience: Node Dev
+  description: |-
+    The finality notification was already carrying the information about the stale heads. However, most users of the stale heads were expanding these stale heads to all the stale blocks. So, we were iterating the same forks multiple times in the node for each finality notification. Also in a possible future where we start actually pruning headers as well, expanding these forks would fail.
+
+    So, this pull request is changing the finality notification to directly carry the stale blocks (which were calculated any way already).
+crates:
+- name: cumulus-client-pov-recovery
+  bump: major
+- name: sc-client-api
+  bump: major
+- name: sc-consensus-babe
+  bump: major
+- name: sc-client-db
+  bump: major
+- name: mmr-gadget
+  bump: major
+- name: sc-rpc-spec-v2
+  bump: major
+- name: sc-service
+  bump: major
+- name: sp-blockchain
+  bump: major

substrate/client/api/src/backend.rs

@@ -79,6 +79,15 @@ pub struct ImportSummary<Block: BlockT> {
 	pub import_notification_action: ImportNotificationAction,
 }
 
+/// A stale block.
+#[derive(Clone, Debug)]
+pub struct StaleBlock<Block: BlockT> {
+	/// The hash of this block.
+	pub hash: Block::Hash,
+	/// Is this a head?
+	pub is_head: bool,
+}
+
 /// Finalization operation summary.
 ///
 /// Contains information about the block that just got finalized,
@@ -87,10 +96,11 @@ pub struct FinalizeSummary<Block: BlockT> {
 	/// Last finalized block header.
 	pub header: Block::Header,
 	/// Blocks that were finalized.
+	///
 	/// The last entry is the one that has been explicitly finalized.
 	pub finalized: Vec<Block::Hash>,
-	/// Heads that became stale during this finalization operation.
-	pub stale_heads: Vec<Block::Hash>,
+	/// Blocks that became stale during this finalization operation.
+	pub stale_blocks: Vec<StaleBlock<Block>>,
 }
 
 /// Import operation wrapper.

substrate/client/api/src/client.rs

@@ -31,7 +31,9 @@ use std::{
 	sync::Arc,
 };
 
-use crate::{blockchain::Info, notifications::StorageEventStream, FinalizeSummary, ImportSummary};
+use crate::{
+	blockchain::Info, notifications::StorageEventStream, FinalizeSummary, ImportSummary, StaleBlock,
+};
 
 use sc_transaction_pool_api::ChainEvent;
 use sc_utils::mpsc::{TracingUnboundedReceiver, TracingUnboundedSender};
@@ -404,8 +406,8 @@ pub struct FinalityNotification<Block: BlockT> {
 	///
 	/// This maps to the range `(old_finalized, new_finalized)`.
 	pub tree_route: Arc<[Block::Hash]>,
-	/// Stale branches heads.
-	pub stale_heads: Arc<[Block::Hash]>,
+	/// Stale blocks.
+	pub stale_blocks: Arc<[Arc<StaleBlock<Block>>]>,
 	/// Handle to unpin the block this notification is for
 	unpin_handle: UnpinHandle<Block>,
 }
@@ -439,7 +441,9 @@ impl<Block: BlockT> FinalityNotification<Block> {
 			hash,
 			header: summary.header,
 			tree_route: Arc::from(summary.finalized),
-			stale_heads: Arc::from(summary.stale_heads),
+			stale_blocks: Arc::from(
+				summary.stale_blocks.into_iter().map(Arc::from).collect::<Vec<_>>(),
+			),
 			unpin_handle: UnpinHandle::new(hash, unpin_worker_sender),
 		}
 	}

substrate/client/consensus/babe/src/lib.rs

@@ -113,7 +113,7 @@ use sp_api::{ApiExt, ProvideRuntimeApi};
 use sp_application_crypto::AppCrypto;
 use sp_block_builder::BlockBuilder as BlockBuilderApi;
 use sp_blockchain::{
-	Backend as _, BlockStatus, Error as ClientError, ForkBackend, HeaderBackend, HeaderMetadata,
+	Backend as _, BlockStatus, Error as ClientError, HeaderBackend, HeaderMetadata,
 	Result as ClientResult,
 };
 use sp_consensus::{BlockOrigin, Environment, Error as ConsensusError, Proposer, SelectChain};
@@ -561,16 +561,7 @@ fn aux_storage_cleanup<C: HeaderMetadata<Block> + HeaderBackend<Block>, Block: B
 			.filter(|h| **h != notification.hash),
 	);
 
-	// Cleans data for stale forks.
-	let stale_forks = match client.expand_forks(&notification.stale_heads) {
-		Ok(stale_forks) => stale_forks,
-		Err(e) => {
-			warn!(target: LOG_TARGET, "{:?}", e);
-
-			Default::default()
-		},
-	};
-	hashes.extend(stale_forks.iter());
+	hashes.extend(notification.stale_blocks.iter().map(|b| b.hash));
 
 	hashes
 		.into_iter()

substrate/client/db/src/lib.rs

@@ -1898,7 +1898,11 @@ impl<Block: BlockT> Backend<Block> {
 		}
 
 		if remove_displaced {
-			let new_displaced = self.blockchain.displaced_leaves_after_finalizing(f_hash, f_num)?;
+			let new_displaced = self.blockchain.displaced_leaves_after_finalizing(
+				f_hash,
+				f_num,
+				*f_header.parent_hash(),
+			)?;
 
 			self.blockchain.leaves.write().remove_displaced_leaves(FinalizationOutcome::new(
 				new_displaced.displaced_leaves.iter().copied(),
@@ -3289,16 +3293,18 @@ pub(crate) mod tests {
 		let a4_hash =
 			insert_disconnected_header(&backend, a4_number, a3_hash, H256::from([2; 32]), true);
 		{
-			let displaced =
-				blockchain.displaced_leaves_after_finalizing(a3_hash, a3_number).unwrap();
+			let displaced = blockchain
+				.displaced_leaves_after_finalizing(a3_hash, a3_number, H256::from([200; 32]))
+				.unwrap();
 			assert_eq!(blockchain.leaves().unwrap(), vec![a4_hash, genesis_hash]);
 			assert_eq!(displaced.displaced_leaves, vec![(genesis_number, genesis_hash)]);
 			assert_eq!(displaced.displaced_blocks, vec![]);
 		}
 
 		{
-			let displaced =
-				blockchain.displaced_leaves_after_finalizing(a4_hash, a4_number).unwrap();
+			let displaced = blockchain
+				.displaced_leaves_after_finalizing(a4_hash, a4_number, a3_hash)
+				.unwrap();
 			assert_eq!(blockchain.leaves().unwrap(), vec![a4_hash, genesis_hash]);
 			assert_eq!(displaced.displaced_leaves, vec![(genesis_number, genesis_hash)]);
 			assert_eq!(displaced.displaced_blocks, vec![]);
@@ -3315,8 +3321,9 @@ pub(crate) mod tests {
 			false,
 		);
 		{
-			let displaced =
-				blockchain.displaced_leaves_after_finalizing(a3_hash, a3_number).unwrap();
+			let displaced = blockchain
+				.displaced_leaves_after_finalizing(a3_hash, a3_number, H256::from([2; 32]))
+				.unwrap();
 			assert_eq!(blockchain.leaves().unwrap(), vec![a4_hash, a1_hash]);
 			assert_eq!(displaced.displaced_leaves, vec![]);
 			assert_eq!(displaced.displaced_blocks, vec![]);
@@ -3334,8 +3341,9 @@ pub(crate) mod tests {
 			false,
 		);
 		{
-			let displaced =
-				blockchain.displaced_leaves_after_finalizing(a3_hash, a3_number).unwrap();
+			let displaced = blockchain
+				.displaced_leaves_after_finalizing(a3_hash, a3_number, H256::from([2; 32]))
+				.unwrap();
 			assert_eq!(blockchain.leaves().unwrap(), vec![a4_hash, a1_hash, b1_hash]);
 			assert_eq!(displaced.displaced_leaves, vec![]);
 			assert_eq!(displaced.displaced_blocks, vec![]);
@@ -3358,8 +3366,9 @@ pub(crate) mod tests {
 		let b5_hash =
 			insert_disconnected_header(&backend, b5_number, b4_hash, H256::from([43; 32]), false);
 		{
-			let displaced =
-				blockchain.displaced_leaves_after_finalizing(a3_hash, a3_number).unwrap();
+			let displaced = blockchain
+				.displaced_leaves_after_finalizing(a3_hash, a3_number, H256::from([2; 32]))
+				.unwrap();
 			assert_eq!(blockchain.leaves().unwrap(), vec![b5_hash, a4_hash, a1_hash]);
 			assert_eq!(displaced.displaced_leaves, vec![]);
 			assert_eq!(displaced.displaced_blocks, vec![]);
@@ -3374,8 +3383,9 @@ pub(crate) mod tests {
 		let c4_hash =
 			insert_disconnected_header(&backend, c4_number, a3_hash, H256::from([44; 32]), false);
 		{
-			let displaced =
-				blockchain.displaced_leaves_after_finalizing(a4_hash, a4_number).unwrap();
+			let displaced = blockchain
+				.displaced_leaves_after_finalizing(a4_hash, a4_number, a3_hash)
+				.unwrap();
 			assert_eq!(blockchain.leaves().unwrap(), vec![b5_hash, a4_hash, c4_hash, a1_hash]);
 			assert_eq!(displaced.displaced_leaves, vec![(c4_number, c4_hash)]);
 			assert_eq!(displaced.displaced_blocks, vec![c4_hash]);
@@ -3404,33 +3414,37 @@ pub(crate) mod tests {
 
 		{
 			let displaced = blockchain
-				.displaced_leaves_after_finalizing(genesis_hash, genesis_number)
+				.displaced_leaves_after_finalizing(genesis_hash, genesis_number, Default::default())
 				.unwrap();
 			assert_eq!(displaced.displaced_leaves, vec![]);
 			assert_eq!(displaced.displaced_blocks, vec![]);
 		}
 		{
-			let displaced_a1 =
-				blockchain.displaced_leaves_after_finalizing(a1_hash, a1_number).unwrap();
+			let displaced_a1 = blockchain
+				.displaced_leaves_after_finalizing(a1_hash, a1_number, genesis_hash)
+				.unwrap();
 			assert_eq!(displaced_a1.displaced_leaves, vec![]);
 			assert_eq!(displaced_a1.displaced_blocks, vec![]);
 
-			let displaced_a2 =
-				blockchain.displaced_leaves_after_finalizing(a2_hash, a3_number).unwrap();
+			let displaced_a2 = blockchain
+				.displaced_leaves_after_finalizing(a2_hash, a2_number, a1_hash)
+				.unwrap();
 			assert_eq!(displaced_a2.displaced_leaves, vec![]);
 			assert_eq!(displaced_a2.displaced_blocks, vec![]);
 
-			let displaced_a3 =
-				blockchain.displaced_leaves_after_finalizing(a3_hash, a3_number).unwrap();
+			let displaced_a3 = blockchain
+				.displaced_leaves_after_finalizing(a3_hash, a3_number, a2_hash)
+				.unwrap();
 			assert_eq!(displaced_a3.displaced_leaves, vec![]);
 			assert_eq!(displaced_a3.displaced_blocks, vec![]);
 		}
 		{
 			// Finalized block is above leaves and not imported yet.
 			// We will not be able to make a connection,
 			// nothing can be marked as displaced.
-			let displaced =
-				blockchain.displaced_leaves_after_finalizing(H256::from([57; 32]), 10).unwrap();
+			let displaced = blockchain
+				.displaced_leaves_after_finalizing(H256::from([57; 32]), 10, H256::from([56; 32]))
+				.unwrap();
 			assert_eq!(displaced.displaced_leaves, vec![]);
 			assert_eq!(displaced.displaced_blocks, vec![]);
 		}
@@ -3454,8 +3468,9 @@ pub(crate) mod tests {
 		let d2_hash = insert_header(&backend, d2_number, d1_hash, None, Default::default());
 
 		{
-			let displaced_a1 =
-				blockchain.displaced_leaves_after_finalizing(a1_hash, a1_number).unwrap();
+			let displaced_a1 = blockchain
+				.displaced_leaves_after_finalizing(a1_hash, a1_number, genesis_hash)
+				.unwrap();
 			assert_eq!(
 				displaced_a1.displaced_leaves,
 				vec![(c2_number, c2_hash), (d2_number, d2_hash)]
@@ -3464,27 +3479,31 @@ pub(crate) mod tests {
 			displaced_blocks.sort();
 			assert_eq!(displaced_a1.displaced_blocks, displaced_blocks);
 
-			let displaced_a2 =
-				blockchain.displaced_leaves_after_finalizing(a2_hash, a2_number).unwrap();
+			let displaced_a2 = blockchain
+				.displaced_leaves_after_finalizing(a2_hash, a2_number, a1_hash)
+				.unwrap();
 			assert_eq!(displaced_a1.displaced_leaves, displaced_a2.displaced_leaves);
 			assert_eq!(displaced_a1.displaced_blocks, displaced_a2.displaced_blocks);
 
-			let displaced_a3 =
-				blockchain.displaced_leaves_after_finalizing(a3_hash, a3_number).unwrap();
+			let displaced_a3 = blockchain
+				.displaced_leaves_after_finalizing(a3_hash, a3_number, a2_hash)
+				.unwrap();
 			assert_eq!(displaced_a1.displaced_leaves, displaced_a3.displaced_leaves);
 			assert_eq!(displaced_a1.displaced_blocks, displaced_a3.displaced_blocks);
 		}
 		{
-			let displaced =
-				blockchain.displaced_leaves_after_finalizing(b1_hash, b1_number).unwrap();
+			let displaced = blockchain
+				.displaced_leaves_after_finalizing(b1_hash, b1_number, genesis_hash)
+				.unwrap();
 			assert_eq!(displaced.displaced_leaves, vec![(a3_number, a3_hash)]);
 			let mut displaced_blocks = vec![a1_hash, a2_hash, a3_hash];
 			displaced_blocks.sort();
 			assert_eq!(displaced.displaced_blocks, displaced_blocks);
 		}
 		{
-			let displaced =
-				blockchain.displaced_leaves_after_finalizing(b2_hash, b2_number).unwrap();
+			let displaced = blockchain
+				.displaced_leaves_after_finalizing(b2_hash, b2_number, b1_hash)
+				.unwrap();
 			assert_eq!(
 				displaced.displaced_leaves,
 				vec![(a3_number, a3_hash), (d2_number, d2_hash)]
@@ -3494,8 +3513,9 @@ pub(crate) mod tests {
 			assert_eq!(displaced.displaced_blocks, displaced_blocks);
 		}
 		{
-			let displaced =
-				blockchain.displaced_leaves_after_finalizing(c2_hash, c2_number).unwrap();
+			let displaced = blockchain
+				.displaced_leaves_after_finalizing(c2_hash, c2_number, c1_hash)
+				.unwrap();
 			assert_eq!(
 				displaced.displaced_leaves,
 				vec![(a3_number, a3_hash), (d2_number, d2_hash)]

substrate/client/merkle-mountain-range/src/offchain_mmr.rs

@@ -25,15 +25,15 @@ use crate::{aux_schema, MmrClient, LOG_TARGET};
 use log::{debug, error, info, warn};
 use sc_client_api::{Backend, FinalityNotification};
 use sc_offchain::OffchainDb;
-use sp_blockchain::{CachedHeaderMetadata, ForkBackend};
+use sp_blockchain::CachedHeaderMetadata;
 use sp_consensus_beefy::MmrRootHash;
 use sp_core::offchain::{DbExternalities, StorageKind};
 use sp_mmr_primitives::{utils, utils::NodesUtils, MmrApi, NodeIndex};
 use sp_runtime::{
 	traits::{Block, Header, NumberFor, One},
 	Saturating,
 };
-use std::{collections::VecDeque, default::Default, sync::Arc};
+use std::{collections::VecDeque, sync::Arc};
 
 /// `OffchainMMR` exposes MMR offchain canonicalization and pruning logic.
 pub struct OffchainMmr<B: Block, BE: Backend<B>, C> {
@@ -273,14 +273,7 @@ where
 		self.write_gadget_state_or_log();
 
 		// Remove offchain MMR nodes for stale forks.
-		let stale_forks = self.client.expand_forks(&notification.stale_heads).unwrap_or_else(|e| {
-			warn!(target: LOG_TARGET, "{:?}", e);
-
-			Default::default()
-		});
-		for hash in stale_forks.iter() {
-			self.prune_branch(hash);
-		}
+		notification.stale_blocks.iter().for_each(|s| self.prune_branch(&s.hash));
 	}
 }