From a85c9f749494366f2a1f95389bb61ea84f0bf0cd Mon Sep 17 00:00:00 2001 From: eskimor Date: Sat, 19 Nov 2022 15:57:53 +0100 Subject: [PATCH 1/4] We actually don't need to rate limit redundant requests. Those redundant requests should not actually happen, but still. --- node/network/dispute-distribution/src/sender/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/node/network/dispute-distribution/src/sender/mod.rs b/node/network/dispute-distribution/src/sender/mod.rs index 331fe12c61e2..e6d98f587696 100644 --- a/node/network/dispute-distribution/src/sender/mod.rs +++ b/node/network/dispute-distribution/src/sender/mod.rs @@ -108,8 +108,6 @@ impl DisputeSender { runtime: &mut RuntimeInfo, msg: DisputeMessage, ) -> Result<()> { - self.rate_limit.limit().await; - let req: DisputeRequest = msg.into(); let candidate_hash = req.0.candidate_receipt.hash(); match self.disputes.entry(candidate_hash) { @@ -118,6 +116,8 @@ impl DisputeSender { return Ok(()) }, Entry::Vacant(vacant) => { + self.rate_limit.limit().await; + let send_task = SendTask::new( ctx, runtime, From 5c439ed6b9366adc9a1d9c5db45ed2c34da71e89 Mon Sep 17 00:00:00 2001 From: eskimor Date: Sat, 19 Nov 2022 16:35:12 +0100 Subject: [PATCH 2/4] Add some logging. --- .../dispute-distribution/src/sender/mod.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/node/network/dispute-distribution/src/sender/mod.rs b/node/network/dispute-distribution/src/sender/mod.rs index e6d98f587696..8c2d517141a8 100644 --- a/node/network/dispute-distribution/src/sender/mod.rs +++ b/node/network/dispute-distribution/src/sender/mod.rs @@ -116,7 +116,7 @@ impl DisputeSender { return Ok(()) }, Entry::Vacant(vacant) => { - self.rate_limit.limit().await; + self.rate_limit.limit("in start_sender", candidate_hash).await; let send_task = SendTask::new( ctx, @@ -169,10 +169,12 @@ impl DisputeSender { // Iterates in order of insertion: let mut should_rate_limit = true; - for dispute in self.disputes.values_mut() { + for (candidate_hash, dispute) in self.disputes.iter_mut() { if have_new_sessions || dispute.has_failed_sends() { if should_rate_limit { - self.rate_limit.limit().await; + self.rate_limit + .limit("in going through new sessions/failed sends", *candidate_hash) + .await; } let sends_happened = dispute .refresh_sends(ctx, runtime, &self.active_sessions, &self.metrics) @@ -193,7 +195,7 @@ impl DisputeSender { // recovered at startup will be relatively "old" anyway and we assume that no more than a // third of the validators will go offline at any point in time anyway. for dispute in unknown_disputes { - self.rate_limit.limit().await; + self.rate_limit.limit("while going through unknown disputes", dispute.1).await; self.start_send_for_dispute(ctx, runtime, dispute).await?; } Ok(()) @@ -383,7 +385,9 @@ impl RateLimit { } /// Wait until ready and prepare for next call. - async fn limit(&mut self) { + /// + /// String given as occasion and candidate hash are logged in case the rate limit hit. + async fn limit(&mut self, occasion: &'static str, candidate_hash: CandidateHash) { // Wait for rate limit and add some logging: poll_fn(|cx| { let old_limit = Pin::new(&mut self.limit); @@ -391,6 +395,8 @@ impl RateLimit { Poll::Pending => { gum::debug!( target: LOG_TARGET, + ?occasion, + ?candidate_hash, "Sending rate limit hit, slowing down requests" ); Poll::Pending From 99505d8591bf5c9077bac6008d06247619118949 Mon Sep 17 00:00:00 2001 From: eskimor Date: Mon, 21 Nov 2022 11:38:24 +0100 Subject: [PATCH 3/4] Also log message when the receiving side hit the rate limit. --- node/network/dispute-distribution/src/receiver/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/node/network/dispute-distribution/src/receiver/mod.rs b/node/network/dispute-distribution/src/receiver/mod.rs index 158c66e20655..9030fc0b3f96 100644 --- a/node/network/dispute-distribution/src/receiver/mod.rs +++ b/node/network/dispute-distribution/src/receiver/mod.rs @@ -302,6 +302,12 @@ where // Queue request: if let Err((authority_id, req)) = self.peer_queues.push_req(authority_id, req) { + gum::debug!( + target: LOG_TARGET, + ?authority_id, + ?peer, + "Peer hit the rate limit - dropping message." + ); req.send_outgoing_response(OutgoingResponse { result: Err(()), reputation_changes: vec![COST_APPARENT_FLOOD], From 4ecc6fd7a76700de274a27d0cec1e7616079849c Mon Sep 17 00:00:00 2001 From: eskimor Date: Wed, 23 Nov 2022 16:13:43 +0100 Subject: [PATCH 4/4] Update node/network/dispute-distribution/src/sender/mod.rs Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> --- node/network/dispute-distribution/src/sender/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/network/dispute-distribution/src/sender/mod.rs b/node/network/dispute-distribution/src/sender/mod.rs index 8c2d517141a8..b7df29b829ed 100644 --- a/node/network/dispute-distribution/src/sender/mod.rs +++ b/node/network/dispute-distribution/src/sender/mod.rs @@ -173,7 +173,7 @@ impl DisputeSender { if have_new_sessions || dispute.has_failed_sends() { if should_rate_limit { self.rate_limit - .limit("in going through new sessions/failed sends", *candidate_hash) + .limit("while going through new sessions/failed sends", *candidate_hash) .await; } let sends_happened = dispute