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: calculated transaction record query cost instead of querying operator balance #2848

Merged
merged 4 commits into from
Aug 22, 2024

Conversation

quiet-node
Copy link
Member

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

Description:
This PR introduces a solution to calculate the transaction record query based on the exchange rate instead of querying the operator's balance. This fix eliminates the need for the two balance queries in each metrics capture request, which could cause significant timeouts when the callData size is large.

Fixes #2846
Related issue: #2808
Related issue: #2847

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 19, 2024
@quiet-node quiet-node added this to the 0.55.0 milestone Aug 19, 2024
@quiet-node quiet-node self-assigned this Aug 19, 2024
@quiet-node quiet-node linked an issue Aug 19, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Aug 19, 2024

Tests

       3 files     207 suites   16s ⏱️
1 046 tests 1 045 ✔️ 1 💤 0
1 058 runs  1 057 ✔️ 1 💤 0

Results for commit e5cab22.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Aug 19, 2024

Acceptance Tests

  21 files  261 suites   31m 10s ⏱️
613 tests 589 ✔️ 4 💤 20
795 runs  769 ✔️ 4 💤 22

Results for commit 7c78887.

♻️ This comment has been updated with latest results.

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.

Instead of extending the timeout from 45 seconds to 90 seconds to address the flakiness, I think we should focus on the fact that this transaction used to complete in less than 45 seconds, and now it’s taking twice as long.

The delay is especially evident when the relay executes multiple file appends and has to retry operator balance queries during hbar limiter metrics capture. To improve user experience and avoid unnecessary slowdowns, I suggest we handle these metrics asynchronously and address the actual problem which we have - that this transaction no longer executes in the time frame it used to.

@quiet-node
Copy link
Member Author

Instead of extending the timeout from 45 seconds to 90 seconds to address the flakiness, I think we should focus on the fact that this transaction used to complete in less than 45 seconds, and now it’s taking twice as long.

The delay is especially evident when the relay executes multiple file appends and has to retry operator balance queries during hbar limiter metrics capture. To improve user experience and avoid unnecessary slowdowns, I suggest we handle these metrics asynchronously and address the actual problem which we have - that this transaction no longer executes in the time frame it used to.

Hey @victor-yanev, thanks for the comment! I also noticed that the main issue behind this timeout problem stems from the TransactionService class, as I mentioned in the "notes for reviewer" section. That’s why I labeled it as a "temporary" solution to help unblock the PRs for now. But yes, I definitely agree with you and appreciate the great suggestion—I’ll look into making the account balance queries asynchronous to improve the situation.

@quiet-node quiet-node marked this pull request as draft August 20, 2024 17:16
@quiet-node quiet-node changed the title fix: temporarily increased the timeout for the SdkClient test suite to 90000ms to reduce flakiness fix: calculated transaction record query cost instead of querying operator balance Aug 21, 2024
@quiet-node quiet-node marked this pull request as ready for review August 21, 2024 17:47
victor-yanev
victor-yanev previously approved these changes Aug 21, 2024
@ebadiere ebadiere self-requested a review August 22, 2024 15:01
ebadiere
ebadiere previously approved these changes Aug 22, 2024
@quiet-node quiet-node force-pushed the 2846-flaky-hbarlimiter-class-due-to-time-out-issue branch from 7c78887 to bf11a76 Compare August 22, 2024 15:13
quiet-node and others added 4 commits August 22, 2024 10:14
…,000ms to reduce flakiness

Signed-off-by: Logan Nguyen <[email protected]>
…ator balance

Signed-off-by: Victor Yanev <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Co-Authored-By: Victor Yanev <[email protected]>
@quiet-node quiet-node force-pushed the 2846-flaky-hbarlimiter-class-due-to-time-out-issue branch from bf11a76 to e5cab22 Compare August 22, 2024 15:21
Copy link

@quiet-node quiet-node merged commit d03bedb into main Aug 22, 2024
39 of 40 checks passed
@quiet-node quiet-node deleted the 2846-flaky-hbarlimiter-class-due-to-time-out-issue branch August 22, 2024 15:56
Copy link

codecov bot commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.81%. Comparing base (af09470) to head (e5cab22).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2848      +/-   ##
==========================================
- Coverage   82.66%   81.81%   -0.86%     
==========================================
  Files          49       46       -3     
  Lines        3554     3403     -151     
  Branches      751      718      -33     
==========================================
- Hits         2938     2784     -154     
- Misses        370      391      +21     
+ Partials      246      228      -18     
Flag Coverage Δ
relay 81.67% <100.00%> (-0.18%) ⬇️
server 81.19% <ø> (ø)
ws-server 97.87% <ø> (ø)

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

Files Coverage Δ
packages/relay/src/lib/constants.ts 90.90% <ø> (-9.10%) ⬇️
.../services/transactionService/transactionService.ts 87.50% <100.00%> (+0.65%) ⬆️

... and 14 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.

Flaky HBarLimiter class due to time out issue
3 participants