Support sending and receiving multiple Justifications

client/network/src/block_request_handler.rs

@@ -263,6 +263,7 @@ impl<B: BlockT> BlockRequestHandler<B> {
 		let get_header = attributes.contains(BlockAttributes::HEADER);
 		let get_body = attributes.contains(BlockAttributes::BODY);
 		let get_justification = attributes.contains(BlockAttributes::JUSTIFICATION);
+		let get_justifications = attributes.contains(BlockAttributes::JUSTIFICATIONS);
 
 		let mut blocks = Vec::new();
 
@@ -271,28 +272,39 @@ impl<B: BlockT> BlockRequestHandler<B> {
 			let number = *header.number();
 			let hash = header.hash();
 			let parent_hash = *header.parent_hash();
-			let justifications = if get_justification {
+			let justifications = if get_justification || get_justifications {
 				self.client.justifications(&BlockId::Hash(hash))?
 			} else {
 				None
 			};
 
-			// TODO: In a follow up PR tracked by https://github.com/paritytech/substrate/issues/8172
-			// we want to send/receive all justifications.
-			// For now we keep compatibility by selecting precisely the GRANDPA one, and not just
-			// the first one. When sending we could have just taken the first one, since we don't
-			// expect there to be any other kind currently, but when receiving we need to add the
-			// engine ID tag.
-			// The ID tag is hardcoded here to avoid depending on the GRANDPA crate, and will be
-			// removed when resolving the above issue.
-			let justification = justifications.and_then(|just| just.into_justification(*b"FRNK"));
-
-			let is_empty_justification = justification
-				.as_ref()
-				.map(|j| j.is_empty())
-				.unwrap_or(false);
-
-			let justification = justification.unwrap_or_default();
+			let (justifications, justification, is_empty_justification) =
+				if get_justifications {
+					let justifications = match justifications {
+						Some(v) => v.encode(),
+						None => Vec::new(),
+					};
+					(justifications, Vec::new(), false)
+				} else {
+					// For now we keep compatibility by selecting precisely the GRANDPA one, and not just
+					// the first one. When sending we could have just taken the first one, since we don't
+					// expect there to be any other kind currently, but when receiving we need to add the
+					// engine ID tag.
+					// The ID tag is hardcoded here to avoid depending on the GRANDPA crate, and will be
+					// removed once we remove the backwards compatibility.
+					// See: https://github.com/paritytech/substrate/issues/8172
+					let justification =
+						justifications.and_then(|just| just.into_justification(*b"FRNK"));
+
+					let is_empty_justification = justification
+						.as_ref()
+						.map(|j| j.is_empty())
+						.unwrap_or(false);
+
+					let justification = justification.unwrap_or_default();
+
+					(Vec::new(), justification, is_empty_justification)
+				};
 
 			let body = if get_body {
 				match self.client.block_body(&BlockId::Hash(hash))? {
@@ -320,6 +332,7 @@ impl<B: BlockT> BlockRequestHandler<B> {
 				message_queue: Vec::new(),
 				justification,
 				is_empty_justification,
+				justifications,
 			};
 
 			total_size += block_data.body.len();

client/network/src/protocol.rs

@@ -580,6 +580,11 @@ impl<B: BlockT> Protocol<B> {
 				} else {
 					None
 				},
+				justifications: if !block_data.justifications.is_empty() {
+					Some(Decode::decode(&mut block_data.justifications.as_ref())?)
+				} else {
+					None
+				},
 			})
 		}).collect::<Result<Vec<_>, codec::Error>>();
 
@@ -612,7 +617,9 @@ impl<B: BlockT> Protocol<B> {
 			blocks_range(),
 		);
 
-		if request.fields == message::BlockAttributes::JUSTIFICATION {
+		if (message::BlockAttributes::JUSTIFICATION | message::BlockAttributes::JUSTIFICATIONS)
+			.contains(request.fields)
+		{
 			match self.sync.on_block_justification(peer_id, block_response) {
 				Ok(sync::OnBlockJustification::Nothing) => CustomMessageOutcome::None,
 				Ok(sync::OnBlockJustification::Import { peer, hash, number, justifications }) =>
@@ -908,6 +915,7 @@ impl<B: BlockT> Protocol<B> {
 						receipt: None,
 						message_queue: None,
 						justification: None,
+						justifications: None,
 					},
 				],
 			},

client/network/src/protocol/message.rs

@@ -77,6 +77,10 @@ bitflags! {
 		const MESSAGE_QUEUE = 0b00001000;
 		/// Include a justification for the block.
 		const JUSTIFICATION = 0b00010000;
+		/// Include multiple justifications for the block. For backwards compatibility we request
+		/// both a single justification as well as the full container. The response contains either
+		/// (not both).
+		const JUSTIFICATIONS = 0b00100000;
 	}
 }
 
@@ -168,7 +172,7 @@ impl<H: HeaderT> generic::BlockAnnounce<H> {
 pub mod generic {
 	use bitflags::bitflags;
 	use codec::{Encode, Decode, Input, Output};
-	use sp_runtime::EncodedJustification;
+	use sp_runtime::{EncodedJustification, Justifications};
 	use super::{
 		RemoteReadResponse, Transactions, Direction,
 		RequestId, BlockAttributes, RemoteCallResponse, ConsensusEngineId,
@@ -254,6 +258,8 @@ pub mod generic {
 		pub message_queue: Option<Vec<u8>>,
 		/// Justification if requested.
 		pub justification: Option<EncodedJustification>,
+		/// Justifications if requested.
+		pub justifications: Option<Justifications>,
 	}
 
 	/// Identifies starting point of a block sequence.

client/network/src/protocol/sync.rs

@@ -448,7 +448,9 @@ impl<B: BlockT> ChainSync<B> {
 		block_announce_validator: Box<dyn BlockAnnounceValidator<B> + Send>,
 		max_parallel_downloads: u32,
 	) -> Self {
-		let mut required_block_attributes = BlockAttributes::HEADER | BlockAttributes::JUSTIFICATION;
+		let mut required_block_attributes = BlockAttributes::HEADER
+			| BlockAttributes::JUSTIFICATION
+			| BlockAttributes::JUSTIFICATIONS;
 
 		if role.is_full() {
 			required_block_attributes |= BlockAttributes::BODY
@@ -698,7 +700,7 @@ impl<B: BlockT> ChainSync<B> {
 					.state = PeerSyncState::DownloadingJustification(request.0);
 				let req = message::generic::BlockRequest {
 					id: 0,
-					fields: BlockAttributes::JUSTIFICATION,
+					fields: BlockAttributes::JUSTIFICATION | BlockAttributes::JUSTIFICATIONS,
 					from: message::FromBlock::Hash(request.0),
 					to: None,
 					direction: message::Direction::Ascending,
@@ -833,8 +835,9 @@ impl<B: BlockT> ChainSync<B> {
 								.drain(self.best_queued_number + One::one())
 								.into_iter()
 								.map(|block_data| {
-									let justifications =
-										legacy_justification_mapping(block_data.block.justification);
+									let justifications = block_data.block.justifications.or(
+										legacy_justification_mapping(block_data.block.justification)
+									);
 									IncomingBlock {
 										hash: block_data.block.hash,
 										header: block_data.block.header,
@@ -854,11 +857,14 @@ impl<B: BlockT> ChainSync<B> {
 							}
 							validate_blocks::<B>(&blocks, who, Some(request))?;
 							blocks.into_iter().map(|b| {
+								let justifications = b.justifications.or(
+									legacy_justification_mapping(b.justification)
+								);
 								IncomingBlock {
 									hash: b.hash,
 									header: b.header,
 									body: b.body,
-									justifications: legacy_justification_mapping(b.justification),
+									justifications,
 									origin: Some(who.clone()),
 									allow_missing_state: true,
 									import_existing: false,
@@ -963,11 +969,14 @@ impl<B: BlockT> ChainSync<B> {
 					// When request.is_none() this is a block announcement. Just accept blocks.
 					validate_blocks::<B>(&blocks, who, None)?;
 					blocks.into_iter().map(|b| {
+						let justifications = b.justifications.or(
+							legacy_justification_mapping(b.justification)
+						);
 						IncomingBlock {
 							hash: b.hash,
 							header: b.header,
 							body: b.body,
-							justifications: legacy_justification_mapping(b.justification),
+							justifications,
 							origin: Some(who.clone()),
 							allow_missing_state: true,
 							import_existing: false,
@@ -1043,7 +1052,7 @@ impl<B: BlockT> ChainSync<B> {
 					return Err(BadPeer(who, rep::BAD_JUSTIFICATION));
 				}
 
-				block.justification
+				block.justifications.or(legacy_justification_mapping(block.justification))
 			} else {
 				// we might have asked the peer for a justification on a block that we assumed it
 				// had but didn't (regardless of whether it had a justification for it or not).
@@ -1058,7 +1067,7 @@ impl<B: BlockT> ChainSync<B> {
 
 			if let Some((peer, hash, number, j)) = self
 				.extra_justifications
-				.on_response(who, legacy_justification_mapping(justification))
+				.on_response(who, justification)
 			{
 				return Ok(OnBlockJustification::Import { peer, hash, number, justifications: j })
 			}
@@ -1655,7 +1664,7 @@ impl<B: BlockT> ChainSync<B> {
 // This is purely during a backwards compatible transitionary period and should be removed
 // once we can assume all nodes can send and receive multiple Justifications
 // The ID tag is hardcoded here to avoid depending on the GRANDPA crate.
-// TODO: https://github.com/paritytech/substrate/issues/8172
+// See: https://github.com/paritytech/substrate/issues/8172
 fn legacy_justification_mapping(justification: Option<EncodedJustification>) -> Option<Justifications> {
 	justification.map(|just| (*b"FRNK", just).into())
 }
@@ -1673,7 +1682,9 @@ pub(crate) struct Metrics {
 fn ancestry_request<B: BlockT>(block: NumberFor<B>) -> BlockRequest<B> {
 	message::generic::BlockRequest {
 		id: 0,
-		fields: BlockAttributes::HEADER | BlockAttributes::JUSTIFICATION,
+		fields: BlockAttributes::HEADER
+			| BlockAttributes::JUSTIFICATION
+			| BlockAttributes::JUSTIFICATIONS,
 		from: message::FromBlock::Number(block),
 		to: None,
 		direction: message::Direction::Ascending,
@@ -2093,7 +2104,7 @@ mod test {
 		// new peer which is at the given block
 		assert!(sync.justification_requests().any(|(p, r)| {
 			p == peer_id3
-				&& r.fields == BlockAttributes::JUSTIFICATION
+				&& r.fields == BlockAttributes::JUSTIFICATION | BlockAttributes::JUSTIFICATIONS
 				&& r.from == message::FromBlock::Hash(b1_hash)
 				&& r.to == None
 		}));
@@ -2163,6 +2174,7 @@ mod test {
 					receipt: None,
 					message_queue: None,
 					justification: None,
+					justifications: None,
 				}
 			).collect(),
 		}

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

@@ -228,6 +228,7 @@ mod test {
 			message_queue: None,
 			receipt: None,
 			justification: None,
+			justifications: None,
 		}).collect()
 	}
 

client/network/src/schema/api.v1.proto

@@ -56,5 +56,9 @@ message BlockData {
 	// doesn't make in possible to differentiate between a lack of justification and an empty
 	// justification.
 	bool is_empty_justification = 7; // optional, false if absent
+	// Justifications if requested.
+	// Unlike the case for single Justifications this will not be empty even if the contained
+	// Justifications itself are present but empty, since they contain the conensus engine ID.
+	bytes justifications = 8; // optional
 }