Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Allow the HBar Rate Limiter to be disabled. #3252

Merged
merged 14 commits into from
Nov 15, 2024

Conversation

ebadiere
Copy link
Contributor

This fix allows the HBar Rate Limiter to be disabled by setting the HBAR_RATE_LIMIT_TINYBAR to 0

Related issue(s):

Fixes #3251

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@ebadiere ebadiere added the bug Something isn't working label Nov 12, 2024
@ebadiere ebadiere added this to the 0.61.0 milestone Nov 12, 2024
@ebadiere ebadiere self-assigned this Nov 12, 2024
Copy link

github-actions bot commented Nov 12, 2024

Test Results

    4 files  +    3    421 suites  +414   27s ⏱️ -3s
1 494 tests +1 489  1 493 ✅ +1 489  1 💤 +1  0 ❌  - 1 
1 503 runs  +1 467  1 502 ✅ +1 467  1 💤 +1  0 ❌  - 1 

Results for commit bfa8093. ± Comparison against base commit ce43b76.

This pull request removes 5 and adds 1494 tests. Note that renamed tests count towards both.
"before all" hook in "@api-batch-2 RPC Server Acceptance Tests" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before all" hook in "@api-batch-2 RPC Server Acceptance Tests"
@release-light, @release should execute "eth_getTransactionReceipt" for hash of 2930 transaction ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-1 RPC Server Acceptance Tests RPC Server Acceptance Tests Transaction related RPC Calls @release-light, @release should execute "eth_getTransactionReceipt" for hash of 2930 transaction
@release-light, @release should execute "eth_getTransactionReceipt" for hash of London transaction ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-1 RPC Server Acceptance Tests RPC Server Acceptance Tests Transaction related RPC Calls @release-light, @release should execute "eth_getTransactionReceipt" for hash of London transaction
@release-light, @release should execute "eth_getTransactionReceipt" for hash of legacy transaction ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-1 RPC Server Acceptance Tests RPC Server Acceptance Tests Transaction related RPC Calls @release-light, @release should execute "eth_getTransactionReceipt" for hash of legacy transaction
@release-light, @release should execute "eth_sendRawTransaction" for legacy EIP 155 transactions ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-1 RPC Server Acceptance Tests RPC Server Acceptance Tests Transaction related RPC Calls @release-light, @release should execute "eth_sendRawTransaction" for legacy EIP 155 transactions
"eth_blockNumber" return the latest block number on second try ‑ @ethGetBlockByNumber using MirrorNode "eth_blockNumber" return the latest block number on second try
"eth_blockNumber" should return the latest block number using cache ‑ @ethGetBlockByNumber using MirrorNode "eth_blockNumber" should return the latest block number using cache
"eth_blockNumber" should return the latest block number ‑ @ethGetBlockByNumber using MirrorNode "eth_blockNumber" should return the latest block number
"eth_blockNumber" should throw an error if no blocks are found after third try ‑ @ethGetBlockByNumber using MirrorNode "eth_blockNumber" should throw an error if no blocks are found after third try
"eth_blockNumber" should throw an error if no blocks are found ‑ @ethGetBlockByNumber using MirrorNode "eth_blockNumber" should throw an error if no blocks are found
Adds a revertReason field for receipts with errorMessage ‑ @ethGetTransactionReceipt eth_getTransactionReceipt tests Adds a revertReason field for receipts with errorMessage
BLOCK_HASH filter timeouts and throws the expected error ‑ @ethGetLogs using MirrorNode timeout BLOCK_HASH filter timeouts and throws the expected error
BLOCK_HASH filter ‑ @ethGetLogs using MirrorNode BLOCK_HASH filter
Can extract the account number out of an account pagination next link url ‑ MirrorNodeClient Can extract the account number out of an account pagination next link url
Can extract the evm address out of an account pagination next link url ‑ MirrorNodeClient Can extract the evm address out of an account pagination next link url
…

♻️ This comment has been updated with latest results.

Signed-off-by: Eric Badiere <[email protected]>
@ebadiere ebadiere marked this pull request as ready for review November 13, 2024 15:12
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Good approach.
Question on the config changes.
Also I provided a suggestion to be a bit more strict

packages/server/tests/acceptance/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
packages/server/tests/acceptance/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
packages/server/tests/acceptance/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
packages/server/tests/acceptance/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
packages/server/tests/acceptance/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
trigger rate limiting, and added isEnabled() method.

Signed-off-by: Eric Badiere <[email protected]>
Signed-off-by: Eric Badiere <[email protected]>

fix: clean up of experimental code.

Signed-off-by: Eric Badiere <[email protected]>
@ebadiere ebadiere force-pushed the 3251-habr-rate-limiter-can-be-disabled branch from 240a06c to 1938973 Compare November 13, 2024 22:38
mishomihov00
mishomihov00 previously approved these changes Nov 14, 2024
Copy link
Contributor

@mishomihov00 mishomihov00 left a comment

Choose a reason for hiding this comment

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

Reviewed and approved only this file:
.github/workflows/acceptance.yml

quiet-node
quiet-node previously approved these changes Nov 14, 2024
Copy link
Member

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

LGTM jsut some comments to clean up code

packages/server/tests/acceptance/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
packages/server/tests/acceptance/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
Signed-off-by: Eric Badiere <[email protected]>
quiet-node
quiet-node previously approved these changes Nov 14, 2024
Nana-EC
Nana-EC previously approved these changes Nov 14, 2024
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG, see note on config issue to open

packages/relay/src/lib/constants.ts Show resolved Hide resolved
when the HBar Rate Limiter is disabled the test should not
use the expenses aggregated to determine if the maxSpendingLimit
has been passed, but simply use the relay operator's before and after balances.

Signed-off-by: Eric Badiere <[email protected]>
@ebadiere ebadiere dismissed stale reviews from Nana-EC and quiet-node via af3a44b November 14, 2024 19:58
and renamed flag to more meaningful name.

Signed-off-by: Eric Badiere <[email protected]>
Copy link

sonarcloud bot commented Nov 14, 2024

Copy link
Member

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

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

LGTM! Great work!

@ebadiere ebadiere merged commit b99655e into main Nov 15, 2024
46 checks passed
@ebadiere ebadiere deleted the 3251-habr-rate-limiter-can-be-disabled branch November 15, 2024 13:49
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.33%. Comparing base (264d2cb) to head (bfa8093).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
packages/relay/src/lib/constants.ts 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3252      +/-   ##
==========================================
+ Coverage   77.85%   81.33%   +3.47%     
==========================================
  Files          66       69       +3     
  Lines        4453     4629     +176     
  Branches      993     1041      +48     
==========================================
+ Hits         3467     3765     +298     
+ Misses        613      516      -97     
+ Partials      373      348      -25     
Flag Coverage Δ
config-service 98.14% <ø> (ø)
relay 78.63% <88.88%> (+0.03%) ⬆️
server 83.28% <ø> (ø)
ws-server 36.66% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...s/relay/src/lib/services/hbarLimitService/index.ts 92.70% <100.00%> (+9.75%) ⬆️
packages/relay/src/lib/constants.ts 90.19% <0.00%> (+3.92%) ⬆️

... and 19 files with indirect coverage changes

quiet-node pushed a commit that referenced this pull request Nov 15, 2024
* fix: Allow the HBar Rate Limiter to be disabled.

Signed-off-by: Eric Badiere <[email protected]>

* fix: Added acceptance test.

Signed-off-by: Eric Badiere <[email protected]>

* fix:  Improved test by adding condition that would normally
trigger rate limiting, and added isEnabled() method.

Signed-off-by: Eric Badiere <[email protected]>

* fix:  Cleaned up and tightened test.

Signed-off-by: Eric Badiere <[email protected]>

* fix:  Updated workflow for new tests.

Signed-off-by: Eric Badiere <[email protected]>

* fix: Updated comment.

Signed-off-by: Eric Badiere <[email protected]>

* fix: added the new hbarlimiter_batch_3 to the public_result in
the workflow

Signed-off-by: Eric Badiere <[email protected]>

* fix: Test clean up.

Signed-off-by: Eric Badiere <[email protected]>

fix: clean up of experimental code.

Signed-off-by: Eric Badiere <[email protected]>

* fix: added isEanbled check to addExpense.

Signed-off-by: Eric Badiere <[email protected]>

* fix: Check totalBudget instead of remainingBudget in
constructor.

Signed-off-by: Eric Badiere <[email protected]>

* fix: clean up.

Signed-off-by: Eric Badiere <[email protected]>

* fix: Test fix.  Now that the addExpense is also skipped
when the HBar Rate Limiter is disabled the test should not
use the expenses aggregated to determine if the maxSpendingLimit
has been passed, but simply use the relay operator's before and after balances.

Signed-off-by: Eric Badiere <[email protected]>

* fix: Added note around nullish coalescing operator.

Signed-off-by: Eric Badiere <[email protected]>

* fix: Added check for remaining budget at start of test
and renamed flag to more meaningful name.

Signed-off-by: Eric Badiere <[email protected]>

---------

Signed-off-by: Eric Badiere <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

HBar Rate Limiter should have the ability to be disabled.
5 participants