grandpa: remove light-client specific block import pipeline (#7546)

* grandpa: remove light-client specific block import

* consensus, network: remove finality proofs

substrate/client/network/src/protocol/message.rs

@@ -25,7 +25,6 @@ pub use self::generic::{
 	BlockAnnounce, RemoteCallRequest, RemoteReadRequest,
 	RemoteHeaderRequest, RemoteHeaderResponse,
 	RemoteChangesRequest, RemoteChangesResponse,
-	FinalityProofRequest, FinalityProofResponse,
 	FromBlock, RemoteReadChildRequest, Roles,
 };
 use sc_client_api::StorageProof;
@@ -280,11 +279,10 @@ pub mod generic {
 		RemoteChangesResponse(RemoteChangesResponse<Number, Hash>),
 		/// Remote child storage read request.
 		RemoteReadChildRequest(RemoteReadChildRequest<Hash>),
-		/// Finality proof request.
-		FinalityProofRequest(FinalityProofRequest<Hash>),
-		/// Finality proof response.
-		FinalityProofResponse(FinalityProofResponse<Hash>),
 		/// Batch of consensus protocol messages.
+		// NOTE: index is incremented by 2 due to finality proof related
+		// messages that were removed.
+		#[codec(index = "17")]
 		ConsensusBatch(Vec<ConsensusMessage>),
 	}
 
@@ -307,8 +305,6 @@ pub mod generic {
 				Message::RemoteChangesRequest(_) => "RemoteChangesRequest",
 				Message::RemoteChangesResponse(_) => "RemoteChangesResponse",
 				Message::RemoteReadChildRequest(_) => "RemoteReadChildRequest",
-				Message::FinalityProofRequest(_) => "FinalityProofRequest",
-				Message::FinalityProofResponse(_) => "FinalityProofResponse",
 				Message::ConsensusBatch(_) => "ConsensusBatch",
 			}
 		}
@@ -546,26 +542,4 @@ pub mod generic {
 		/// Missing changes tries roots proof.
 		pub roots_proof: StorageProof,
 	}
-
-	#[derive(Debug, PartialEq, Eq, Clone, Encode, Decode)]
-	/// Finality proof request.
-	pub struct FinalityProofRequest<H> {
-		/// Unique request id.
-		pub id: RequestId,
-		/// Hash of the block to request proof for.
-		pub block: H,
-		/// Additional data blob (that both requester and provider understood) required for proving finality.
-		pub request: Vec<u8>,
-	}
-
-	#[derive(Debug, PartialEq, Eq, Clone, Encode, Decode)]
-	/// Finality proof response.
-	pub struct FinalityProofResponse<H> {
-		/// Id of a request this response was made for.
-		pub id: RequestId,
-		/// Hash of the block (the same as in the FinalityProofRequest).
-		pub block: H,
-		/// Finality proof (if available).
-		pub proof: Option<Vec<u8>>,
-	}
 }

substrate/client/network/src/protocol/sync.rs

@@ -35,9 +35,7 @@ use sp_consensus::{BlockOrigin, BlockStatus,
 	import_queue::{IncomingBlock, BlockImportResult, BlockImportError}
 };
 use crate::{
-	config::BoxFinalityProofRequestBuilder,
-	protocol::message::{self, generic::FinalityProofRequest, BlockAnnounce, BlockAttributes, BlockRequest, BlockResponse,
-	FinalityProofResponse, Roles},
+	protocol::message::{self, BlockAnnounce, BlockAttributes, BlockRequest, BlockResponse, Roles},
 };
 use either::Either;
 use extra_requests::ExtraRequests;
@@ -116,9 +114,6 @@ mod rep {
 	/// Reputation change for peers which send us a block with bad justifications.
 	pub const BAD_JUSTIFICATION: Rep = Rep::new(-(1 << 16), "Bad justification");
 
-	/// Reputation change for peers which send us a block with bad finality proof.
-	pub const BAD_FINALITY_PROOF: Rep = Rep::new(-(1 << 16), "Bad finality proof");
-
 	/// Reputation change when a peer sent us invlid ancestry result.
 	pub const UNKNOWN_ANCESTOR:Rep = Rep::new(-(1 << 16), "DB Error");
 }
@@ -185,8 +180,6 @@ pub struct ChainSync<B: BlockT> {
 	/// What block attributes we require for this node, usually derived from
 	/// what role we are, but could be customized
 	required_block_attributes: message::BlockAttributes,
-	/// Any extra finality proof requests.
-	extra_finality_proofs: ExtraRequests<B>,
 	/// Any extra justification requests.
 	extra_justifications: ExtraRequests<B>,
 	/// A set of hashes of blocks that are being downloaded or have been
@@ -195,8 +188,6 @@ pub struct ChainSync<B: BlockT> {
 	/// The best block number that was successfully imported into the chain.
 	/// This can not decrease.
 	best_imported_number: NumberFor<B>,
-	/// Finality proof handler.
-	request_builder: Option<BoxFinalityProofRequestBuilder<B>>,
 	/// Fork sync targets.
 	fork_targets: HashMap<B::Hash, ForkTarget<B>>,
 	/// A set of peers for which there might be potential block requests
@@ -270,8 +261,6 @@ pub enum PeerSyncState<B: BlockT> {
 	DownloadingStale(B::Hash),
 	/// Downloading justification for given block hash.
 	DownloadingJustification(B::Hash),
-	/// Downloading finality proof for given block hash.
-	DownloadingFinalityProof(B::Hash)
 }
 
 impl<B: BlockT> PeerSyncState<B> {
@@ -402,20 +391,6 @@ pub enum OnBlockJustification<B: BlockT> {
 	}
 }
 
-/// Result of [`ChainSync::on_block_finality_proof`].
-#[derive(Debug, Clone, PartialEq, Eq)]
-pub enum OnBlockFinalityProof<B: BlockT> {
-	/// The proof needs no further handling.
-	Nothing,
-	/// The proof should be imported.
-	Import {
-		peer: PeerId,
-		hash: B::Hash,
-		number: NumberFor<B>,
-		proof: Vec<u8>
-	}
-}
-
 /// Result of [`ChainSync::has_slot_for_block_announce_validation`].
 enum HasSlotForBlockAnnounceValidation {
 	/// Yes, there is a slot for the block announce validation.
@@ -432,7 +407,6 @@ impl<B: BlockT> ChainSync<B> {
 		role: Roles,
 		client: Arc<dyn crate::chain::Client<B>>,
 		info: &BlockchainInfo<B>,
-		request_builder: Option<BoxFinalityProofRequestBuilder<B>>,
 		block_announce_validator: Box<dyn BlockAnnounceValidator<B> + Send>,
 		max_parallel_downloads: u32,
 	) -> Self {
@@ -449,12 +423,10 @@ impl<B: BlockT> ChainSync<B> {
 			best_queued_hash: info.best_hash,
 			best_queued_number: info.best_number,
 			best_imported_number: info.best_number,
-			extra_finality_proofs: ExtraRequests::new("finality proof"),
 			extra_justifications: ExtraRequests::new("justification"),
 			role,
 			required_block_attributes,
 			queue_blocks: Default::default(),
-			request_builder,
 			fork_targets: Default::default(),
 			pending_requests: Default::default(),
 			block_announce_validator,
@@ -613,14 +585,6 @@ impl<B: BlockT> ChainSync<B> {
 		})
 	}
 
-	/// Schedule a finality proof request for the given block.
-	pub fn request_finality_proof(&mut self, hash: &B::Hash, number: NumberFor<B>) {
-		let client = &self.client;
-		self.extra_finality_proofs.schedule((*hash, number), |base, block| {
-			is_descendent_of(&**client, base, block)
-		})
-	}
-
 	/// Request syncing for the given block from given set of peers.
 	// The implementation is similar to on_block_announce with unknown parent hash.
 	pub fn set_sync_fork_request(
@@ -700,30 +664,6 @@ impl<B: BlockT> ChainSync<B> {
 		})
 	}
 
-	/// Get an iterator over all scheduled finality proof requests.
-	pub fn finality_proof_requests(&mut self) -> impl Iterator<Item = (PeerId, FinalityProofRequest<B::Hash>)> + '_ {
-		let peers = &mut self.peers;
-		let request_builder = &mut self.request_builder;
-		let mut matcher = self.extra_finality_proofs.matcher();
-		std::iter::from_fn(move || {
-			if let Some((peer, request)) = matcher.next(&peers) {
-				peers.get_mut(&peer)
-					.expect("`Matcher::next` guarantees the `PeerId` comes from the given peers; qed")
-					.state = PeerSyncState::DownloadingFinalityProof(request.0);
-				let req = message::generic::FinalityProofRequest {
-					id: 0,
-					block: request.0,
-					request: request_builder.as_mut()
-						.map(|builder| builder.build_request_data(&request.0))
-						.unwrap_or_default()
-				};
-				Some((peer, req))
-			} else {
-				None
-			}
-		})
-	}
-
 	/// Get an iterator over all block requests of all peers.
 	pub fn block_requests(&mut self) -> impl Iterator<Item = (&PeerId, BlockRequest<B>)> + '_ {
 		if self.pending_requests.is_empty() {
@@ -920,8 +860,7 @@ impl<B: BlockT> ChainSync<B> {
 						}
 
 						| PeerSyncState::Available
-						| PeerSyncState::DownloadingJustification(..)
-						| PeerSyncState::DownloadingFinalityProof(..) => Vec::new()
+						| PeerSyncState::DownloadingJustification(..) => Vec::new()
 					}
 				} else {
 					// When request.is_none() this is a block announcement. Just accept blocks.
@@ -1033,41 +972,6 @@ impl<B: BlockT> ChainSync<B> {
 		Ok(OnBlockJustification::Nothing)
 	}
 
-	/// Handle new finality proof data.
-	pub fn on_block_finality_proof
-		(&mut self, who: PeerId, resp: FinalityProofResponse<B::Hash>) -> Result<OnBlockFinalityProof<B>, BadPeer>
-	{
-		let peer =
-			if let Some(peer) = self.peers.get_mut(&who) {
-				peer
-			} else {
-				error!(target: "sync", "💔 Called on_block_finality_proof_data with a bad peer ID");
-				return Ok(OnBlockFinalityProof::Nothing)
-			};
-
-		self.pending_requests.add(&who);
-		if let PeerSyncState::DownloadingFinalityProof(hash) = peer.state {
-			peer.state = PeerSyncState::Available;
-
-			// We only request one finality proof at a time.
-			if hash != resp.block {
-				info!(
-					target: "sync",
-					"💔 Invalid block finality proof provided: requested: {:?} got: {:?}",
-					hash,
-					resp.block
-				);
-				return Err(BadPeer(who, rep::BAD_FINALITY_PROOF));
-			}
-
-			if let Some((peer, hash, number, p)) = self.extra_finality_proofs.on_response(who, resp.proof) {
-				return Ok(OnBlockFinalityProof::Import { peer, hash, number, proof: p })
-			}
-		}
-
-		Ok(OnBlockFinalityProof::Nothing)
-	}
-
 	/// A batch of blocks have been processed, with or without errors.
 	///
 	/// Call this when a batch of blocks have been processed by the import
@@ -1122,11 +1026,6 @@ impl<B: BlockT> ChainSync<B> {
 						}
 					}
 
-					if aux.needs_finality_proof {
-						trace!(target: "sync", "Block imported but requires finality proof {}: {:?}", number, hash);
-						self.request_finality_proof(&hash, number);
-					}
-
 					if number > self.best_imported_number {
 						self.best_imported_number = number;
 					}
@@ -1178,22 +1077,8 @@ impl<B: BlockT> ChainSync<B> {
 		self.pending_requests.set_all();
 	}
 
-	pub fn on_finality_proof_import(&mut self, req: (B::Hash, NumberFor<B>), res: Result<(B::Hash, NumberFor<B>), ()>) {
-		self.extra_finality_proofs.try_finalize_root(req, res, true);
-		self.pending_requests.set_all();
-	}
-
 	/// Notify about finalization of the given block.
 	pub fn on_block_finalized(&mut self, hash: &B::Hash, number: NumberFor<B>) {
-		let client = &self.client;
-		let r = self.extra_finality_proofs.on_block_finalized(hash, number, |base, block| {
-			is_descendent_of(&**client, base, block)
-		});
-
-		if let Err(err) = r {
-			warn!(target: "sync", "💔 Error cleaning up pending extra finality proof data requests: {:?}", err)
-		}
-
 		let client = &self.client;
 		let r = self.extra_justifications.on_block_finalized(hash, number, |base, block| {
 			is_descendent_of(&**client, base, block)
@@ -1506,14 +1391,12 @@ impl<B: BlockT> ChainSync<B> {
 		self.blocks.clear_peer_download(who);
 		self.peers.remove(who);
 		self.extra_justifications.peer_disconnected(who);
-		self.extra_finality_proofs.peer_disconnected(who);
 		self.pending_requests.set_all();
 	}
 
 	/// Restart the sync process. This will reset all pending block requests and return an iterator
 	/// of new block requests to make to peers. Peers that were downloading finality data (i.e.
-	/// their state was `DownloadingJustification` or `DownloadingFinalityProof`) are unaffected and
-	/// will stay in the same state.
+	/// their state was `DownloadingJustification`) are unaffected and will stay in the same state.
 	fn restart<'a>(
 		&'a mut self,
 	) -> impl Iterator<Item = Result<(PeerId, BlockRequest<B>), BadPeer>> + 'a {
@@ -1526,11 +1409,10 @@ impl<B: BlockT> ChainSync<B> {
 		let old_peers = std::mem::take(&mut self.peers);
 
 		old_peers.into_iter().filter_map(move |(id, p)| {
-			// peers that were downloading justifications or finality proofs
+			// peers that were downloading justifications
 			// should be kept in that state.
 			match p.state {
-				PeerSyncState::DownloadingJustification(_)
-				| PeerSyncState::DownloadingFinalityProof(_) => {
+				PeerSyncState::DownloadingJustification(_) => {
 					self.peers.insert(id, p);
 					return None;
 				}
@@ -1570,7 +1452,6 @@ impl<B: BlockT> ChainSync<B> {
 		Metrics {
 			queued_blocks: self.queue_blocks.len().try_into().unwrap_or(std::u32::MAX),
 			fork_targets: self.fork_targets.len().try_into().unwrap_or(std::u32::MAX),
-			finality_proofs: self.extra_finality_proofs.metrics(),
 			justifications: self.extra_justifications.metrics(),
 			_priv: ()
 		}
@@ -1581,7 +1462,6 @@ impl<B: BlockT> ChainSync<B> {
 pub(crate) struct Metrics {
 	pub(crate) queued_blocks: u32,
 	pub(crate) fork_targets: u32,
-	pub(crate) finality_proofs: extra_requests::Metrics,
 	pub(crate) justifications: extra_requests::Metrics,
 	_priv: ()
 }
@@ -1835,7 +1715,6 @@ mod test {
 			Roles::AUTHORITY,
 			client.clone(),
 			&info,
-			None,
 			block_announce_validator,
 			1,
 		);
@@ -1907,7 +1786,6 @@ mod test {
 			Roles::AUTHORITY,
 			client.clone(),
 			&info,
-			None,
 			Box::new(DefaultBlockAnnounceValidator),
 			1,
 		);
@@ -1915,7 +1793,6 @@ mod test {
 		let peer_id1 = PeerId::random();
 		let peer_id2 = PeerId::random();
 		let peer_id3 = PeerId::random();
-		let peer_id4 = PeerId::random();
 
 		let mut new_blocks = |n| {
 			for _ in 0..n {
@@ -1928,7 +1805,6 @@ mod test {
 		};
 
 		let (b1_hash, b1_number) = new_blocks(50);
-		let (b2_hash, b2_number) = new_blocks(10);
 
 		// add 2 peers at blocks that we don't have locally
 		sync.new_peer(peer_id1.clone(), Hash::random(), 42).unwrap();
@@ -1958,38 +1834,16 @@ mod test {
 			PeerSyncState::DownloadingJustification(b1_hash),
 		);
 
-		// add another peer at a known later block
-		sync.new_peer(peer_id4.clone(), b2_hash, b2_number).unwrap();
-
-		// we request a finality proof for a block we have locally
-		sync.request_finality_proof(&b2_hash, b2_number);
-
-		// the finality proof request should be scheduled to peer 4
-		// which is at that block
-		assert!(
-			sync.finality_proof_requests().any(|(p, r)| { p == peer_id4 && r.block == b2_hash })
-		);
-
-		assert_eq!(
-			sync.peers.get(&peer_id4).unwrap().state,
-			PeerSyncState::DownloadingFinalityProof(b2_hash),
-		);
-
 		// we restart the sync state
 		let block_requests = sync.restart();
 
 		// which should make us send out block requests to the first two peers
 		assert!(block_requests.map(|r| r.unwrap()).all(|(p, _)| { p == peer_id1 || p == peer_id2 }));
 
-		// peer 3 and 4 should be unaffected as they were downloading finality data
+		// peer 3 should be unaffected it was downloading finality data
 		assert_eq!(
 			sync.peers.get(&peer_id3).unwrap().state,
 			PeerSyncState::DownloadingJustification(b1_hash),
 		);
-
-		assert_eq!(
-			sync.peers.get(&peer_id4).unwrap().state,
-			PeerSyncState::DownloadingFinalityProof(b2_hash),
-		);
 	}
 }

substrate/client/network/src/protocol/sync/extra_requests.rs

@@ -528,13 +528,12 @@ mod tests {
 
 	impl Arbitrary for ArbitraryPeerSyncState {
 		fn arbitrary<G: Gen>(g: &mut G) -> Self {
-			let s = match g.gen::<u8>() % 5 {
+			let s = match g.gen::<u8>() % 4 {
 				0 => PeerSyncState::Available,
 				// TODO: 1 => PeerSyncState::AncestorSearch(g.gen(), AncestorSearchState<B>),
 				1 => PeerSyncState::DownloadingNew(g.gen::<BlockNumber>()),
 				2 => PeerSyncState::DownloadingStale(Hash::random()),
-				3 => PeerSyncState::DownloadingJustification(Hash::random()),
-				_ => PeerSyncState::DownloadingFinalityProof(Hash::random())
+				_ => PeerSyncState::DownloadingJustification(Hash::random()),
 			};
 			ArbitraryPeerSyncState(s)
 		}