From 3743cec9bdea339f89fed358ce212f9dbaa3f429 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Fri, 12 Mar 2021 12:13:44 +0100 Subject: [PATCH] Do not ban peers for sending multiple valid requests (#8325) We introduced banning of peers who spam us with the same request (more than 2 times). However, we missed that it is completely legal to send the same request multiple times as long as we did not provide any answer. An example for that is the justification request. This request is send multiple times until we could fetch the justification from one of our peers. So, the solution to this problem is to tag requests as fulfilled and to start counting these fulfilled requests. If the number is higher than what we allow, the peer should be banned. --- .../network/src/block_request_handler.rs | 55 +++++++++++++++---- substrate/client/network/test/src/sync.rs | 44 +++++++++++++++ 2 files changed, 87 insertions(+), 12 deletions(-) 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(()) + })); +}