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.

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<B: BlockT> {
 	peer: PeerId,
 	from: BlockId<B>,
 	max_blocks: usize,
 	direction: Direction,
+	attributes: BlockAttributes,
 }
 
 impl<B: BlockT> Hash for SeenRequestsKey<B> {
@@ -86,6 +87,7 @@ impl<B: BlockT> Hash for SeenRequestsKey<B> {
 		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<B: BlockT> Hash for SeenRequestsKey<B> {
 	}
 }
 
+/// 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<B: BlockT> {
 	client: Arc<dyn Client<B>>,
@@ -101,7 +111,7 @@ pub struct BlockRequestHandler<B: BlockT> {
 	/// 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<SeenRequestsKey<B>, usize>,
+	seen_requests: LruCache<SeenRequestsKey<B>, SeenRequestsValue>,
 }
 
 impl<B: BlockT> BlockRequestHandler<B> {
@@ -173,37 +183,43 @@ impl<B: BlockT> BlockRequestHandler<B> {
 		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<B: BlockT> BlockRequestHandler<B> {
 				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)?;
 

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(())
+	}));
+}