Skip to content

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Aug 26, 2025

This PR is best reviewed one commit at a time. In the one commit where the indentation level is reduced, it's best reviewed ignoring white space.

This is in preparation for further work on the rate limits system, which has a lot of potential to be simpler and more efficient (as in, not rate limiting messages as severely).

These are the changes, in order of commits:

  • use monotonic() clock to avoid being affected by system clock changes
  • in the rate limiter, the 1 minute window is configurable, so rename current_minute to current_slot
  • override the clock in tests, to make tests fully deterministic
  • remove (de-indent) the pytest class in test_rate_limits.py
  • small improvement to a comment
  • minor typo fixes in rate_limits.py. The log message would not have spaces as intended, and the default_settings (which doesn't appear to be used) can have its lookup deferred
  • replace the test_too_many_messages() test with a parameterized test covering almost all rate limit cases.

@arvidn arvidn added Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup labels Aug 26, 2025
@arvidn arvidn marked this pull request as ready for review August 26, 2025 16:27
@arvidn arvidn requested a review from a team as a code owner August 26, 2025 16:27
@arvidn arvidn requested a review from altendky August 26, 2025 16:29
@arvidn arvidn mentioned this pull request Aug 26, 2025
1 task
@arvidn arvidn force-pushed the rate-limits-cleanup branch from ae3a27a to bb3d479 Compare August 26, 2025 16:40
Copy link

coveralls-official bot commented Aug 27, 2025

Pull Request Test Coverage Report for Build 17302370298

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 274 of 275 (99.64%) changed or added relevant lines in 2 files are covered.
  • 31 unchanged lines in 12 files lost coverage.
  • Overall coverage decreased (-0.002%) to 91.294%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/_tests/core/server/test_rate_limits.py 265 266 99.62%
Files with Coverage Reduction New Missed Lines %
chia/daemon/client.py 1 74.16%
chia/daemon/keychain_proxy.py 1 70.57%
chia/farmer/farmer.py 1 73.32%
chia/full_node/pending_tx_cache.py 1 96.55%
chia/rpc/rpc_server.py 1 89.34%
chia/wallet/util/wallet_sync_utils.py 1 86.07%
chia/timelord/timelord_launcher.py 2 90.71%
chia/server/node_discovery.py 3 81.75%
chia/wallet/wallet_node.py 3 87.34%
chia/full_node/coin_store.py 5 98.58%
Totals Coverage Status
Change from base Build 17253036064: -0.002%
Covered Lines: 102756
Relevant Lines: 112423

💛 - Coveralls

@arvidn arvidn requested a review from altendky August 28, 2025 13:57
@arvidn arvidn requested a review from wjblanke August 28, 2025 20:40
Copy link
Contributor

File Coverage Missing Lines
chia/_tests/core/server/test_rate_limits.py 99.6% lines 441
Total Missing Coverage
275 lines Unknown 99%

Copy link
Contributor

@wjblanke wjblanke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aok

@pmaslana pmaslana merged commit 29c25fb into main Sep 2, 2025
713 of 716 checks passed
@pmaslana pmaslana deleted the rate-limits-cleanup branch September 2, 2025 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog Cleanup Code cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants