From da3d708ad9e75a49555056ce1f76fe600ef0effc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Mon, 29 Mar 2021 12:13:26 +0200 Subject: [PATCH 1/4] client/network: support sending multiple justifications --- client/network/src/block_request_handler.rs | 47 +++++++++++++-------- client/network/src/protocol.rs | 10 ++++- client/network/src/protocol/message.rs | 8 +++- client/network/src/protocol/sync.rs | 34 ++++++++++----- client/network/src/protocol/sync/blocks.rs | 1 + client/network/src/schema/api.v1.proto | 4 ++ 6 files changed, 74 insertions(+), 30 deletions(-) diff --git a/client/network/src/block_request_handler.rs b/client/network/src/block_request_handler.rs index 2cc888c220f62..44cc9b35fb00a 100644 --- a/client/network/src/block_request_handler.rs +++ b/client/network/src/block_request_handler.rs @@ -267,6 +267,7 @@ impl BlockRequestHandler { 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(); @@ -275,28 +276,39 @@ impl BlockRequestHandler { 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))? { @@ -324,6 +336,7 @@ impl BlockRequestHandler { message_queue: Vec::new(), justification, is_empty_justification, + justifications, }; total_size += block_data.body.len(); diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 84b5285b38ada..7c88cf635faa5 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -578,6 +578,11 @@ impl Protocol { } else { None }, + justifications: if !block_data.justifications.is_empty() { + Some(Decode::decode(&mut block_data.justifications.as_ref())?) + } else { + None + }, }) }).collect::, codec::Error>>(); @@ -610,7 +615,9 @@ impl Protocol { 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 }) => @@ -906,6 +913,7 @@ impl Protocol { receipt: None, message_queue: None, justification: None, + justifications: None, }, ], }, diff --git a/client/network/src/protocol/message.rs b/client/network/src/protocol/message.rs index 01e9a5d7215af..5ffe0702dfc32 100644 --- a/client/network/src/protocol/message.rs +++ b/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; } } @@ -148,7 +152,7 @@ pub struct RemoteReadResponse { 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, @@ -234,6 +238,8 @@ pub mod generic { pub message_queue: Option>, /// Justification if requested. pub justification: Option, + /// Justifications if requested. + pub justifications: Option, } /// Identifies starting point of a block sequence. diff --git a/client/network/src/protocol/sync.rs b/client/network/src/protocol/sync.rs index 37f9a451b67d3..06eb1d7803d60 100644 --- a/client/network/src/protocol/sync.rs +++ b/client/network/src/protocol/sync.rs @@ -448,7 +448,9 @@ impl ChainSync { block_announce_validator: Box + 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 @@ -697,7 +699,7 @@ impl ChainSync { .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, @@ -823,8 +825,9 @@ impl ChainSync { .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, @@ -844,11 +847,14 @@ impl ChainSync { } validate_blocks::(&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, @@ -953,11 +959,14 @@ impl ChainSync { // When request.is_none() this is a block announcement. Just accept blocks. validate_blocks::(&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, @@ -1028,7 +1037,7 @@ impl ChainSync { 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). @@ -1043,7 +1052,7 @@ impl ChainSync { 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 }) } @@ -1605,7 +1614,7 @@ impl ChainSync { // 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) -> Option { justification.map(|just| (*b"FRNK", just).into()) } @@ -1623,7 +1632,9 @@ pub(crate) struct Metrics { fn ancestry_request(block: NumberFor) -> BlockRequest { 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, @@ -2043,7 +2054,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 })); @@ -2105,6 +2116,7 @@ mod test { receipt: None, message_queue: None, justification: None, + justifications: None, } ).collect(), } diff --git a/client/network/src/protocol/sync/blocks.rs b/client/network/src/protocol/sync/blocks.rs index 60492f24ed8c3..81f9cffacaab4 100644 --- a/client/network/src/protocol/sync/blocks.rs +++ b/client/network/src/protocol/sync/blocks.rs @@ -228,6 +228,7 @@ mod test { message_queue: None, receipt: None, justification: None, + justifications: None, }).collect() } diff --git a/client/network/src/schema/api.v1.proto b/client/network/src/schema/api.v1.proto index a933c5811c109..e9f8506a36a56 100644 --- a/client/network/src/schema/api.v1.proto +++ b/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 } From c9028713a003177eaa57f99709945c142a0d1b9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 27 Apr 2021 09:20:08 +0200 Subject: [PATCH 2/4] network: flag support for multiple justifications in protobuf request --- client/network/src/block_request_handler.rs | 11 ++++++++--- client/network/src/light_client_requests/sender.rs | 1 + client/network/src/protocol.rs | 5 ++--- client/network/src/protocol/message.rs | 4 ---- client/network/src/protocol/sync.rs | 12 ++++-------- client/network/src/schema/api.v1.proto | 4 ++++ 6 files changed, 19 insertions(+), 18 deletions(-) diff --git a/client/network/src/block_request_handler.rs b/client/network/src/block_request_handler.rs index faafb45e093ee..633b6b5935edc 100644 --- a/client/network/src/block_request_handler.rs +++ b/client/network/src/block_request_handler.rs @@ -80,6 +80,7 @@ struct SeenRequestsKey { max_blocks: usize, direction: Direction, attributes: BlockAttributes, + support_multiple_justifications: bool, } impl Hash for SeenRequestsKey { @@ -180,12 +181,15 @@ impl BlockRequestHandler { 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 BlockRequestHandler { 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,11 +264,11 @@ impl BlockRequestHandler { mut block_id: BlockId, direction: Direction, max_blocks: usize, + support_multiple_justifications: bool, ) -> Result { 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(); @@ -272,14 +277,14 @@ impl BlockRequestHandler { let number = *header.number(); let hash = header.hash(); let parent_hash = *header.parent_hash(); - let justifications = if get_justification || get_justifications { + let justifications = if get_justification { self.client.justifications(&BlockId::Hash(hash))? } else { None }; let (justifications, justification, is_empty_justification) = - if get_justifications { + if support_multiple_justifications { let justifications = match justifications { Some(v) => v.encode(), None => Vec::new(), diff --git a/client/network/src/light_client_requests/sender.rs b/client/network/src/light_client_requests/sender.rs index 652f465d6f250..bf832ea13aedf 100644 --- a/client/network/src/light_client_requests/sender.rs +++ b/client/network/src/light_client_requests/sender.rs @@ -722,6 +722,7 @@ impl Request { 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()); diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 3def1f61cfdff..1e25272082096 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -617,9 +617,7 @@ impl Protocol { blocks_range(), ); - if (message::BlockAttributes::JUSTIFICATION | message::BlockAttributes::JUSTIFICATIONS) - .contains(request.fields) - { + if request.fields == message::BlockAttributes::JUSTIFICATION { match self.sync.on_block_justification(peer_id, block_response) { Ok(sync::OnBlockJustification::Nothing) => CustomMessageOutcome::None, Ok(sync::OnBlockJustification::Import { peer, hash, number, justifications }) => @@ -1131,6 +1129,7 @@ fn prepare_block_request( 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 { diff --git a/client/network/src/protocol/message.rs b/client/network/src/protocol/message.rs index e2093aa36ca32..dc6beac99aa01 100644 --- a/client/network/src/protocol/message.rs +++ b/client/network/src/protocol/message.rs @@ -77,10 +77,6 @@ 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; } } diff --git a/client/network/src/protocol/sync.rs b/client/network/src/protocol/sync.rs index 8da768b252688..d3ab2912a9dc5 100644 --- a/client/network/src/protocol/sync.rs +++ b/client/network/src/protocol/sync.rs @@ -448,9 +448,7 @@ impl ChainSync { block_announce_validator: Box + Send>, max_parallel_downloads: u32, ) -> Self { - let mut required_block_attributes = BlockAttributes::HEADER - | BlockAttributes::JUSTIFICATION - | BlockAttributes::JUSTIFICATIONS; + let mut required_block_attributes = BlockAttributes::HEADER | BlockAttributes::JUSTIFICATION; if role.is_full() { required_block_attributes |= BlockAttributes::BODY @@ -700,7 +698,7 @@ impl ChainSync { .state = PeerSyncState::DownloadingJustification(request.0); let req = message::generic::BlockRequest { id: 0, - fields: BlockAttributes::JUSTIFICATION | BlockAttributes::JUSTIFICATIONS, + fields: BlockAttributes::JUSTIFICATION, from: message::FromBlock::Hash(request.0), to: None, direction: message::Direction::Ascending, @@ -1682,9 +1680,7 @@ pub(crate) struct Metrics { fn ancestry_request(block: NumberFor) -> BlockRequest { message::generic::BlockRequest { id: 0, - fields: BlockAttributes::HEADER - | BlockAttributes::JUSTIFICATION - | BlockAttributes::JUSTIFICATIONS, + fields: BlockAttributes::HEADER | BlockAttributes::JUSTIFICATION, from: message::FromBlock::Number(block), to: None, direction: message::Direction::Ascending, @@ -2104,7 +2100,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 | BlockAttributes::JUSTIFICATIONS + && r.fields == BlockAttributes::JUSTIFICATION && r.from == message::FromBlock::Hash(b1_hash) && r.to == None })); diff --git a/client/network/src/schema/api.v1.proto b/client/network/src/schema/api.v1.proto index e9f8506a36a56..f7c9a34a866fc 100644 --- a/client/network/src/schema/api.v1.proto +++ b/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` From 9c09c7122e662675df06d4233d75352957ec03e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 27 Apr 2021 13:29:20 +0200 Subject: [PATCH 3/4] Update client/network/src/protocol.rs Co-authored-by: Pierre Krieger --- client/network/src/protocol.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 1e25272082096..ff64b9d599c04 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -581,7 +581,7 @@ impl Protocol { None }, justifications: if !block_data.justifications.is_empty() { - Some(Decode::decode(&mut block_data.justifications.as_ref())?) + Some(DecodeAll::decode_all(&mut block_data.justifications.as_ref())?) } else { None }, From e002a1569dcb5dbb697efefd5400a50952dbf45a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 27 Apr 2021 15:28:46 +0200 Subject: [PATCH 4/4] network: update comment on protobuf field --- client/network/src/schema/api.v1.proto | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/client/network/src/schema/api.v1.proto b/client/network/src/schema/api.v1.proto index f7c9a34a866fc..23d585b05e9cd 100644 --- a/client/network/src/schema/api.v1.proto +++ b/client/network/src/schema/api.v1.proto @@ -61,8 +61,10 @@ message BlockData { // 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. + // 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 }