Support sending and receiving multiple Justifications (#8266)

* client/network: support sending multiple justifications

* network: flag support for multiple justifications in protobuf request

* Update client/network/src/protocol.rs

Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

* network: update comment on protobuf field

Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

substrate/client/network/src/block_request_handler.rs

@@ -80,6 +80,7 @@ struct SeenRequestsKey<B: BlockT> {
 	max_blocks: usize,
 	direction: Direction,
 	attributes: BlockAttributes,
+	support_multiple_justifications: bool,
 }
 
 impl<B: BlockT> Hash for SeenRequestsKey<B> {
@@ -180,12 +181,15 @@ impl<B: BlockT> BlockRequestHandler<B> {
 
 		let attributes = BlockAttributes::from_be_u32(request.fields)?;
 
+		let support_multiple_justifications = request.support_multiple_justifications;
+
 		let key = SeenRequestsKey {
 			peer: *peer,
 			max_blocks,
 			direction,
 			from: from_block_id.clone(),
 			attributes,
+			support_multiple_justifications,
 		};
 
 		let mut reputation_changes = Vec::new();
@@ -221,6 +225,7 @@ impl<B: BlockT> BlockRequestHandler<B> {
 				from_block_id,
 				direction,
 				max_blocks,
+				support_multiple_justifications,
 			)?;
 
 			// If any of the blocks contains nay data, we can consider it as successful request.
@@ -259,6 +264,7 @@ impl<B: BlockT> BlockRequestHandler<B> {
 		mut block_id: BlockId<B>,
 		direction: Direction,
 		max_blocks: usize,
+		support_multiple_justifications: bool,
 	) -> Result<BlockResponse, HandleRequestError> {
 		let get_header = attributes.contains(BlockAttributes::HEADER);
 		let get_body = attributes.contains(BlockAttributes::BODY);
@@ -277,22 +283,33 @@ impl<B: BlockT> BlockRequestHandler<B> {
 				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 (justifications, justification, is_empty_justification) =
+				if support_multiple_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 is_empty_justification = justification
+						.as_ref()
+						.map(|j| j.is_empty())
+						.unwrap_or(false);
 
-			let justification = justification.unwrap_or_default();
+					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 +337,7 @@ impl<B: BlockT> BlockRequestHandler<B> {
 				message_queue: Vec::new(),
 				justification,
 				is_empty_justification,
+				justifications,
 			};
 
 			total_size += block_data.body.len();

substrate/client/network/src/light_client_requests/sender.rs

@@ -722,6 +722,7 @@ impl<B: Block> Request<B> {
 					to_block: Default::default(),
 					direction: schema::v1::Direction::Ascending as i32,
 					max_blocks: 1,
+					support_multiple_justifications: true,
 				};
 
 				let mut buf = Vec::with_capacity(rq.encoded_len());

substrate/client/network/src/protocol.rs

@@ -580,6 +580,11 @@ impl<B: BlockT> Protocol<B> {
 				} else {
 					None
 				},
+				justifications: if !block_data.justifications.is_empty() {
+					Some(DecodeAll::decode_all(&mut block_data.justifications.as_ref())?)
+				} else {
+					None
+				},
 			})
 		}).collect::<Result<Vec<_>, codec::Error>>();
 
@@ -908,6 +913,7 @@ impl<B: BlockT> Protocol<B> {
 						receipt: None,
 						message_queue: None,
 						justification: None,
+						justifications: None,
 					},
 				],
 			},
@@ -1123,6 +1129,7 @@ fn prepare_block_request<B: BlockT>(
 		to_block: request.to.map(|h| h.encode()).unwrap_or_default(),
 		direction: request.direction as i32,
 		max_blocks: request.max.unwrap_or(0),
+		support_multiple_justifications: true,
 	};
 
 	CustomMessageOutcome::BlockRequest {

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

@@ -168,7 +168,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 +254,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.

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

@@ -833,8 +833,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 +855,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 +967,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 +1050,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 +1065,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 +1662,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())
 }
@@ -2163,6 +2170,7 @@ mod test {
 					receipt: None,
 					message_queue: None,
 					justification: None,
+					justifications: None,
 				}
 			).collect(),
 		}

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

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

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

@@ -29,6 +29,10 @@ message BlockRequest {
 	Direction direction = 5;
 	// Maximum number of blocks to return. An implementation defined maximum is used when unspecified.
 	uint32 max_blocks = 6; // optional
+	// Indicate to the receiver that we support multiple justifications. If the responder also
+	// supports this it will populate the multiple justifications field in `BlockData` instead of
+	// the single justification field.
+	bool support_multiple_justifications = 7; // optional
 }
 
 // Response to `BlockRequest`
@@ -56,5 +60,11 @@ 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 field for a single justification, this field does not required an associated
+	// boolean to differentiate between the lack of justifications and empty justification(s). This
+	// is because empty justifications, like all justifications, are paired with a non-empty
+	// consensus engine ID.
+	bytes justifications = 8; // optional
 }