don't increment counters for outgoing messages blocked by the rate limit #3518

arvidn · 2021-05-03T11:34:54Z

This is best reviewed with white space changed hidden, as there's indentation changes.

This was causing a problem where outbound messages, blocked by the rate limiter,
would still increment the counters as-if they had been sent. This, in turn,
could cause other message types to get blocked becuase the rate limiter thought
we had sent a lot of the other (blocked) message type.

…mit. This was causing a problem where outbound messages, blocked by the rate limiter, would still increment the counters as-if they had been sent. This, in turn, could cause other message types to get blocked becuase the rate limiter thought we had sent a lot of the other (blocked) message type.

arvidn · 2021-05-03T13:38:58Z

chia/server/rate_limits.py

@@ -116,7 +133,7 @@ def process_msg_and_check(self, message: Message) -> bool:
        Returns True if message can be processed successfully, false if a rate limit is passed.
        """

-        current_minute = time.time() // self.reset_seconds
+        current_minute = int(time.time() // self.reset_seconds)


for some reason, integer division returns a float (according to mypy)

It's because // is really floor( div (x) )

arvidn · 2021-05-03T13:40:05Z

chia/server/rate_limits.py

+        new_cumulative_size: int = self.message_cumulative_sizes[message_type] + len(message.data)
+        new_non_tx_count: int = self.non_tx_message_counts
+        new_non_tx_size: int = self.non_tx_cumulative_size
+        proportion_of_limit: float = self.percentage_of_limit / 100


this is the key part of this patch. Rather than updating the counters immediately, compute the new counters if this message were to be sent. Check rates against these counters and if we end up accepting sending the message, then commit the new values to the counters (see the finally block)

This would make a kick ass comment, as it perfectly describes the intent.

I was hoping the comment in the finally block would suffice

arvidn · 2021-05-03T13:40:57Z

chia/server/rate_limits.py

+            ret = True
+            return True
+        finally:
+            if self.incoming or ret:


only commit the new counters if we're returning True or if this was an incoming rate limiter, and we already received the message. In the latter case we should always increment the counters.

arvidn · 2021-05-03T13:42:02Z

tests/core/server/test_rate_limits.py

@@ -217,3 +217,49 @@ async def test_percentage_limits(self):
            if not response:
                saw_disconnect = True
        assert saw_disconnect
+
+    @pytest.mark.asyncio
+    async def test_too_many_outgoing_messages(self):


these two tests demonstrate the problem. Prior to this patch, we would have the "incoming" behavior in both cases, both incoming and outgoing messages.

aqk · 2021-05-03T16:00:15Z

tests/core/server/test_rate_limits.py

+            else:
+                blocked += 1
+
+        assert passed == 10


Why is passed == 10 here?

ah, the rate limit for this type of message is 10 (per the unit of time specified in the RateLimit constructor)

@Chida82

* 2 harvesting features (#3331) * 3 harvesting features: - Debug level shows the time for every quality lookup - Warning level if takes longer than 5 seconds - Allow configuration of plot loading interval (default 2 minutes) * Comment out super logging * Improve wallet consistency (#3305) * Improve wallet consistency * Improve CLI significantly, and fix self-tx balances * Fix await * Fix deadlock and test * Remove spam.sh * Changelog for 1.1.3 (#3345) * Changelog for 1.1.3 * minor updates * updates part 3 * Those engineers who don't update changelogs... :) * Apologies to @Chida82 who added log rotate count! (#3369) * Rust parse serialized (#3444) * use rust implementation for finding length of a serialized clvm program * bump clvm_rs version * Don't retry respond_peers message (#3508) * don't increment counters for outgoing messages blocked by the rate limit. (#3518) This was causing a problem where outbound messages, blocked by the rate limiter, would still increment the counters as-if they had been sent. This, in turn, could cause other message types to get blocked becuase the rate limiter thought we had sent a lot of the other (blocked) message type. Co-authored-by: Mariano Sorgente <3069354+mariano54@users.noreply.github.com> Co-authored-by: Gene Hoffman <30377676+hoffmang9@users.noreply.github.com> Co-authored-by: Arvid Norberg <arvid@libtorrent.org>

@Chida82

* 2 harvesting features (#3331) * 3 harvesting features: - Debug level shows the time for every quality lookup - Warning level if takes longer than 5 seconds - Allow configuration of plot loading interval (default 2 minutes) * Comment out super logging * Improve wallet consistency (#3305) * Improve wallet consistency * Improve CLI significantly, and fix self-tx balances * Fix await * Fix deadlock and test * Remove spam.sh * Changelog for 1.1.3 (#3345) * Changelog for 1.1.3 * minor updates * updates part 3 * Those engineers who don't update changelogs... :) * Apologies to @Chida82 who added log rotate count! (#3369) * Rust parse serialized (#3444) * use rust implementation for finding length of a serialized clvm program * bump clvm_rs version * Don't retry respond_peers message (#3508) * don't increment counters for outgoing messages blocked by the rate limit. (#3518) This was causing a problem where outbound messages, blocked by the rate limiter, would still increment the counters as-if they had been sent. This, in turn, could cause other message types to get blocked becuase the rate limiter thought we had sent a lot of the other (blocked) message type. * hide secret wallet key by default with 'chia keys show' (#3565) Co-authored-by: Adam Kelly <aqk@aqk.im> Co-authored-by: Mariano Sorgente <3069354+mariano54@users.noreply.github.com> Co-authored-by: Gene Hoffman <30377676+hoffmang9@users.noreply.github.com> Co-authored-by: Arvid Norberg <arvid@libtorrent.org> Co-authored-by: Adam Kelly <338792+aqk@users.noreply.github.com> Co-authored-by: Adam Kelly <aqk@aqk.im>

@Chida82

* 2 harvesting features (#3331) * 3 harvesting features: - Debug level shows the time for every quality lookup - Warning level if takes longer than 5 seconds - Allow configuration of plot loading interval (default 2 minutes) * Comment out super logging * Improve wallet consistency (#3305) * Improve wallet consistency * Improve CLI significantly, and fix self-tx balances * Fix await * Fix deadlock and test * Remove spam.sh * Changelog for 1.1.3 (#3345) * Changelog for 1.1.3 * minor updates * updates part 3 * Those engineers who don't update changelogs... :) * Apologies to @Chida82 who added log rotate count! (#3369) * Rust parse serialized (#3444) * use rust implementation for finding length of a serialized clvm program * bump clvm_rs version * Don't retry respond_peers message (#3508) * don't increment counters for outgoing messages blocked by the rate limit. (#3518) This was causing a problem where outbound messages, blocked by the rate limiter, would still increment the counters as-if they had been sent. This, in turn, could cause other message types to get blocked becuase the rate limiter thought we had sent a lot of the other (blocked) message type. * hide secret wallet key by default with 'chia keys show' (#3565) Co-authored-by: Adam Kelly <aqk@aqk.im> * brake if end of ses list (#3583) * brake if end of ses list * log * throw if wp peak is not heavier * comment * handle sync from scratch * comment Co-authored-by: Mariano Sorgente <3069354+mariano54@users.noreply.github.com> Co-authored-by: Gene Hoffman <30377676+hoffmang9@users.noreply.github.com> Co-authored-by: Arvid Norberg <arvid@libtorrent.org> Co-authored-by: Adam Kelly <338792+aqk@users.noreply.github.com> Co-authored-by: Adam Kelly <aqk@aqk.im> Co-authored-by: Almog De Paz <almogdepaz@gmail.com>

@Chida82

* 2 harvesting features (#3331) * 3 harvesting features: - Debug level shows the time for every quality lookup - Warning level if takes longer than 5 seconds - Allow configuration of plot loading interval (default 2 minutes) * Comment out super logging * Improve wallet consistency (#3305) * Improve wallet consistency * Improve CLI significantly, and fix self-tx balances * Fix await * Fix deadlock and test * Remove spam.sh * Changelog for 1.1.3 (#3345) * Changelog for 1.1.3 * minor updates * updates part 3 * Those engineers who don't update changelogs... :) * Apologies to @Chida82 who added log rotate count! (#3369) * Rust parse serialized (#3444) * use rust implementation for finding length of a serialized clvm program * bump clvm_rs version * Don't retry respond_peers message (#3508) * don't increment counters for outgoing messages blocked by the rate limit. (#3518) This was causing a problem where outbound messages, blocked by the rate limiter, would still increment the counters as-if they had been sent. This, in turn, could cause other message types to get blocked becuase the rate limiter thought we had sent a lot of the other (blocked) message type. * hide secret wallet key by default with 'chia keys show' (#3565) Co-authored-by: Adam Kelly <aqk@aqk.im> * brake if end of ses list (#3583) * brake if end of ses list * log * throw if wp peak is not heavier * comment * handle sync from scratch * comment * fix indentation issue in message send retry logic. factor out retry coroutine (#3629) * Fix test (#3350) * Mempool sorting and accept reverted pending transactions (#3683) * Sort by fee/cost, and fix pending tx issue in reorgs * Fix test name * Bring the seen list size back to normal. Co-authored-by: Mariano Sorgente <3069354+mariano54@users.noreply.github.com> Co-authored-by: Gene Hoffman <30377676+hoffmang9@users.noreply.github.com> Co-authored-by: Arvid Norberg <arvid@libtorrent.org> Co-authored-by: Adam Kelly <338792+aqk@users.noreply.github.com> Co-authored-by: Adam Kelly <aqk@aqk.im> Co-authored-by: Almog De Paz <almogdepaz@gmail.com>

arvidn requested a review from mariano54 May 3, 2021 11:34

arvidn force-pushed the rate-limit-fix branch 2 times, most recently from bf4d609 to c3de1da Compare May 3, 2021 13:38

arvidn commented May 3, 2021

View reviewed changes

arvidn marked this pull request as ready for review May 3, 2021 13:44

arvidn mentioned this pull request May 3, 2021

Don't retry respond_peers message #3508

Merged

arvidn requested a review from Yostra May 3, 2021 13:55

arvidn force-pushed the rate-limit-fix branch from c3de1da to 32a6e4d Compare May 3, 2021 14:09

aqk approved these changes May 3, 2021

View reviewed changes

wjblanke merged commit 912dc84 into main May 3, 2021

wjblanke deleted the rate-limit-fix branch May 3, 2021 18:18

tedshroyer mentioned this pull request May 4, 2021

[BUG] **Rate Limiting / Retries / Or Both impair stability of nodes* #3605

Closed

This was referenced Nov 28, 2023

[Snyk] Security upgrade lerna from 5.5.2 to 6.5.0 asmrprog/chia-blockchain#5

Open

[Snyk] Security upgrade lerna from 5.5.2 to 6.5.0 asmrprog/chia-blockchain#6

Open

asmrprog mentioned this pull request Dec 19, 2023

[Snyk] Security upgrade lerna from 5.5.2 to 6.5.0 asmrprog/chia-blockchain#11

Open

This was referenced Feb 11, 2024

[Snyk] Fix for 1 vulnerabilities asmrprog/chia-blockchain#15

Open

[Snyk] Fix for 1 vulnerabilities asmrprog/chia-blockchain#16

Open

This was referenced Mar 22, 2024

[Snyk] Security upgrade lerna from 5.5.2 to 6.5.0 asmrprog/chia-blockchain#21

Open

[Snyk] Security upgrade lerna from 5.5.2 to 6.5.0 asmrprog/chia-blockchain#22

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't increment counters for outgoing messages blocked by the rate limit #3518

don't increment counters for outgoing messages blocked by the rate limit #3518

arvidn commented May 3, 2021 •

edited

Loading

arvidn May 3, 2021

aqk May 3, 2021 •

edited

Loading

arvidn May 3, 2021

aqk May 3, 2021

arvidn May 3, 2021

arvidn May 3, 2021

arvidn May 3, 2021

aqk May 3, 2021

arvidn May 3, 2021

don't increment counters for outgoing messages blocked by the rate limit #3518

don't increment counters for outgoing messages blocked by the rate limit #3518

Conversation

arvidn commented May 3, 2021 • edited Loading

Choose a reason for hiding this comment

aqk May 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvidn commented May 3, 2021 •

edited

Loading

aqk May 3, 2021 •

edited

Loading