diff --git a/substrate/client/network/src/block_request_handler.rs b/substrate/client/network/src/block_request_handler.rs index 85b4acf687..148bc01302 100644 --- a/substrate/client/network/src/block_request_handler.rs +++ b/substrate/client/network/src/block_request_handler.rs @@ -72,13 +72,14 @@ pub(crate) fn generate_protocol_name(protocol_id: &ProtocolId) -> String { s } -/// The key for [`BlockRequestHandler::seen_requests`]. -#[derive(Eq, PartialEq)] +/// The key of [`BlockRequestHandler::seen_requests`]. +#[derive(Eq, PartialEq, Clone)] struct SeenRequestsKey { peer: PeerId, from: BlockId, max_blocks: usize, direction: Direction, + attributes: BlockAttributes, } impl Hash for SeenRequestsKey { @@ -86,6 +87,7 @@ impl Hash for SeenRequestsKey { self.peer.hash(state); self.max_blocks.hash(state); self.direction.hash(state); + self.attributes.hash(state); match self.from { BlockId::Hash(h) => h.hash(state), @@ -94,6 +96,14 @@ impl Hash for SeenRequestsKey { } } +/// The value of [`BlockRequestHandler::seen_requests`]. +enum SeenRequestsValue { + /// First time we have seen the request. + First, + /// We have fulfilled the request `n` times. + Fulfilled(usize), +} + /// Handler for incoming block requests from a remote peer. pub struct BlockRequestHandler { client: Arc>, @@ -101,7 +111,7 @@ pub struct BlockRequestHandler { /// Maps from request to number of times we have seen this request. /// /// This is used to check if a peer is spamming us with the same request. - seen_requests: LruCache, usize>, + seen_requests: LruCache, SeenRequestsValue>, } impl BlockRequestHandler { @@ -173,37 +183,43 @@ impl BlockRequestHandler { let direction = Direction::from_i32(request.direction) .ok_or(HandleRequestError::ParseDirection)?; + let attributes = BlockAttributes::from_be_u32(request.fields)?; + let key = SeenRequestsKey { peer: *peer, max_blocks, direction, from: from_block_id.clone(), + attributes, }; let mut reputation_changes = Vec::new(); - if let Some(requests) = self.seen_requests.get_mut(&key) { - *requests = requests.saturating_add(1); + match self.seen_requests.get_mut(&key) { + Some(SeenRequestsValue::First) => {}, + Some(SeenRequestsValue::Fulfilled(ref mut requests)) => { + *requests = requests.saturating_add(1); - if *requests > MAX_NUMBER_OF_SAME_REQUESTS_PER_PEER { - reputation_changes.push(rep::SAME_REQUEST); + if *requests > MAX_NUMBER_OF_SAME_REQUESTS_PER_PEER { + reputation_changes.push(rep::SAME_REQUEST); + } + }, + None => { + self.seen_requests.put(key.clone(), SeenRequestsValue::First); } - } else { - self.seen_requests.put(key, 1); } debug!( target: LOG_TARGET, "Handling block request from {}: Starting at `{:?}` with maximum blocks \ - of `{}` and direction `{:?}`.", + of `{}`, direction `{:?}` and attributes `{:?}`.", peer, from_block_id, max_blocks, direction, + attributes, ); - let attributes = BlockAttributes::from_be_u32(request.fields)?; - let result = if reputation_changes.is_empty() { let block_response = self.get_block_response( attributes, @@ -212,6 +228,21 @@ impl BlockRequestHandler { max_blocks, )?; + // If any of the blocks contains nay data, we can consider it as successful request. + if block_response + .blocks + .iter() + .any(|b| !b.header.is_empty() || !b.body.is_empty() || b.is_empty_justification) + { + if let Some(value) = self.seen_requests.get_mut(&key) { + // If this is the first time we have processed this request, we need to change + // it to `Fulfilled`. + if let SeenRequestsValue::First = value { + *value = SeenRequestsValue::Fulfilled(1); + } + } + } + let mut data = Vec::with_capacity(block_response.encoded_len()); block_response.encode(&mut data)?; diff --git a/substrate/client/network/test/src/sync.rs b/substrate/client/network/test/src/sync.rs index 46fbb8f82d..b11dbaca75 100644 --- a/substrate/client/network/test/src/sync.rs +++ b/substrate/client/network/test/src/sync.rs @@ -935,3 +935,47 @@ fn continue_to_sync_after_some_block_announcement_verifications_failed() { net.block_until_sync(); assert!(net.peer(1).has_block(&block_hash)); } + +/// When being spammed by the same request of a peer, we ban this peer. However, we should only ban +/// this peer if the request was successful. In the case of a justification request for example, +/// we ask our peers multiple times until we got the requested justification. This test ensures that +/// asking for the same justification multiple times doesn't ban a peer. +#[test] +fn multiple_requests_are_accepted_as_long_as_they_are_not_fulfilled() { + sp_tracing::try_init_simple(); + let mut net = JustificationTestNet::new(2); + net.peer(0).push_blocks(10, false); + net.block_until_sync(); + + // there's currently no justification for block #10 + assert_eq!(net.peer(0).client().justification(&BlockId::Number(10)).unwrap(), None); + assert_eq!(net.peer(1).client().justification(&BlockId::Number(10)).unwrap(), None); + + let h1 = net.peer(1).client().header(&BlockId::Number(10)).unwrap().unwrap(); + + // Let's assume block 10 was finalized, but we still need the justification from the network. + net.peer(1).request_justification(&h1.hash().into(), 10); + + // Let's build some more blocks and wait always for the network to have synced them + for _ in 0..5 { + // We need to sleep 10 seconds as this is the time we wait between sending a new + // justification request. + std::thread::sleep(std::time::Duration::from_secs(10)); + net.peer(0).push_blocks(1, false); + net.block_until_sync(); + assert_eq!(1, net.peer(0).num_peers()); + } + + // Finalize the block and make the justification available. + net.peer(0).client().finalize_block(BlockId::Number(10), Some(Vec::new()), true).unwrap(); + + block_on(futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); + + if net.peer(1).client().justification(&BlockId::Number(10)).unwrap() != Some(Vec::new()) { + return Poll::Pending; + } + + Poll::Ready(()) + })); +}