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

feat: improved preemtive rate limit for file transactions #2922

Conversation

quiet-node
Copy link
Member

@quiet-node quiet-node commented Aug 30, 2024

Description:
A preemptive rate limit was introduced as a hotfix in version 0.52.1, using a logic that preemptively rate-limited file transactions by comparing a hard-coded amount of tinybars that was assumed to be the cost for FileCreate and FileAppend transactions. However, this logic was incorrect and the tinybar amount was a random figure agreed among the team for the purposes of the hotfix.

This PR improves the rate-limiting logic by leveraging the Hedera Fee Calculator to estimate fees for FileCreate and FileAppend transactions based on a file size of FILE_APPEND_CHUNK_SIZE bytes. This approach ensures the estimated fees more closely align with the actual transaction fees charged for these transactions.

Fixes #2921

Notes for reviewer:

Checklist

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

@quiet-node quiet-node added the enhancement New feature or request label Aug 30, 2024
@quiet-node quiet-node added this to the 0.55.0 milestone Aug 30, 2024
@quiet-node quiet-node self-assigned this Aug 30, 2024
@quiet-node quiet-node linked an issue Aug 30, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Aug 30, 2024

Acceptance Tests

  20 files  274 suites   34m 4s ⏱️
605 tests 597 ✔️ 4 💤 4
811 runs  801 ✔️ 4 💤 6

Results for commit a431881.

♻️ This comment has been updated with latest results.

@quiet-node quiet-node marked this pull request as draft August 30, 2024 22:09
packages/relay/src/lib/eth.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/hbarlimiter/index.ts Show resolved Hide resolved
@quiet-node quiet-node force-pushed the 2921-hbar-rate-limit-redesign-improved-the-current-preemtive-rate-limit-logic branch 2 times, most recently from 6fd9899 to ec0904e Compare September 3, 2024 19:46
Copy link

github-actions bot commented Sep 3, 2024

Tests

       3 files     297 suites   19s ⏱️
1 353 tests 1 352 ✔️ 1 💤 0
1 362 runs  1 361 ✔️ 1 💤 0

Results for commit a431881.

♻️ This comment has been updated with latest results.

@quiet-node quiet-node marked this pull request as ready for review September 3, 2024 20:54
packages/relay/src/lib/clients/sdkClient.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/eth.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/eth.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/hbarlimiter/index.ts Show resolved Hide resolved
packages/relay/src/lib/hbarlimiter/index.ts Outdated Show resolved Hide resolved
packages/relay/tests/lib/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
packages/relay/tests/lib/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
packages/relay/tests/lib/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
packages/relay/tests/lib/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
packages/server/tests/acceptance/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
@quiet-node quiet-node force-pushed the 2921-hbar-rate-limit-redesign-improved-the-current-preemtive-rate-limit-logic branch 3 times, most recently from 16f93e4 to 9150900 Compare September 9, 2024 13:28
packages/relay/src/lib/hbarlimiter/index.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/hbarlimiter/index.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/hbarlimiter/index.ts Outdated Show resolved Hide resolved
packages/relay/src/lib/hbarlimiter/index.ts Show resolved Hide resolved
@quiet-node quiet-node force-pushed the 2921-hbar-rate-limit-redesign-improved-the-current-preemtive-rate-limit-logic branch 2 times, most recently from 3a29b5e to d26375d Compare September 9, 2024 19:41
Copy link
Contributor

@victor-yanev victor-yanev left a comment

Choose a reason for hiding this comment

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

Looks good, a couple of questions and nits

packages/relay/src/lib/clients/sdkClient.ts Outdated Show resolved Hide resolved
packages/relay/tests/lib/hbarLimiter.spec.ts Outdated Show resolved Hide resolved
packages/relay/tests/lib/openrpc.spec.ts Outdated Show resolved Hide resolved
packages/relay/tests/lib/openrpc.spec.ts Show resolved Hide resolved
ebadiere
ebadiere previously approved these changes Sep 10, 2024
Copy link
Collaborator

@ebadiere ebadiere left a comment

Choose a reason for hiding this comment

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

LG.

Copy link
Contributor

@victor-yanev victor-yanev left a comment

Choose a reason for hiding this comment

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

Btw, when I come to think about those fees we use from the hedera fee calculator - aren't those estimates for the whole fee of the transaction? What part of that is paid by the relay operator? Isn't most of that paid by the signer of the transaction? Are you sure that those estimates reflect what is actually being paid by the relay operator - please add an acceptance test that verifies the estimates are accurate by comparing the difference in the operator's balance after performing the transaction to the result from the estimateFileTransactionFee.

@quiet-node quiet-node force-pushed the 2921-hbar-rate-limit-redesign-improved-the-current-preemtive-rate-limit-logic branch from c8eaf17 to 1add014 Compare September 11, 2024 15:41
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
@quiet-node quiet-node force-pushed the 2921-hbar-rate-limit-redesign-improved-the-current-preemtive-rate-limit-logic branch from 52f79be to a431881 Compare September 17, 2024 14:49
Copy link

sonarcloud bot commented Sep 17, 2024

@ebadiere ebadiere self-requested a review September 17, 2024 14:59
@quiet-node quiet-node merged commit 50b023e into main Sep 17, 2024
39 checks passed
@quiet-node quiet-node deleted the 2921-hbar-rate-limit-redesign-improved-the-current-preemtive-rate-limit-logic branch September 17, 2024 15:19
Copy link

codecov bot commented Sep 17, 2024

Codecov Report

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

Project coverage is 89.92%. Comparing base (ef3dd83) to head (a431881).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
packages/relay/src/lib/eth.ts 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2922      +/-   ##
==========================================
+ Coverage   89.32%   89.92%   +0.59%     
==========================================
  Files          56       58       +2     
  Lines        3832     3900      +68     
  Branches      774      779       +5     
==========================================
+ Hits         3423     3507      +84     
+ Misses        362      346      -16     
  Partials       47       47              
Flag Coverage Δ
relay 90.14% <97.50%> (+0.38%) ⬆️
server 88.15% <ø> (+0.89%) ⬆️
ws-server 100.00% <ø> (?)

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

Files with missing lines Coverage Δ
packages/relay/src/lib/clients/mirrorNodeClient.ts 95.65% <100.00%> (+<0.01%) ⬆️
packages/relay/src/lib/clients/sdkClient.ts 65.81% <100.00%> (+3.05%) ⬆️
packages/relay/src/lib/constants.ts 91.11% <100.00%> (+0.20%) ⬆️
packages/relay/src/lib/errors/JsonRpcError.ts 74.50% <ø> (ø)
packages/relay/src/lib/hbarlimiter/index.ts 100.00% <100.00%> (+1.61%) ⬆️
packages/relay/src/lib/eth.ts 88.96% <91.66%> (+0.14%) ⬆️

... and 4 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improved the current preemptive rate limit logic
3 participants