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(ws): Add flow control to the tx history streamer #1126

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

msbrogli
Copy link
Member

@msbrogli msbrogli commented Aug 29, 2024

Motivation

The current implementation of the tx history streaming does not have a flow control mechanism. After running some stress tests, we learned that wallets might not be able to process transactions at the same rate the full node is sending them. So, this PR introduces a flow control mechanism using a sequence number and an ACK number.

Here's an incomplete design of the flow control mechanism: https://github.com/HathorNetwork/internal-rfcs/pull/25

Acceptance Criteria

  1. Add seq: int to each stream message (except begin, end and error).
  2. The flow control is disabled by default.
  3. Add StreamBeginMessage.sliding_window_size: Optional[int] where none means "no flow control".
  4. Add an optional window-size attribute to the create stream websocket commands xpub and manual.
  5. If window-size is missing, the default value is used.
  6. If window-size is a number, the provided value is used.
  7. If window-size is the number is negative, the flow control is disabled.
  8. Add a command request:history:ack that sets the ack number and optionally modifies the window size.
  9. If flow control is enabled, the streamer will send up to _sliding_window_size messages without receiving an ack. Then it will wait for an ack (or a change in the window size) to resume.
  10. Fix a bug in which the streamer was skipping one message every time the streamer was paused.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@msbrogli msbrogli requested a review from jansegre as a code owner August 29, 2024 16:55
Copy link

github-actions bot commented Aug 29, 2024

🐰Bencher

ReportTue, September 3, 2024 at 20:46:43 UTC
Projecthathor-core
Branchfeat/tx-history-streaming-flow-control
Testbedubuntu-22.04
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Lower Boundary
nanoseconds (ns) | (%)
Latency Upper Boundary
nanoseconds (ns) | (%)
sync-v2 (up to 20000 blocks)✅ (view plot)101,390,691,250.28 (-0.47%)91,685,880,126.34 (90.43%)112,060,520,154.42 (90.48%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@msbrogli msbrogli force-pushed the feat/tx-history-streaming-flow-control branch 3 times, most recently from 219ada2 to 1163fe5 Compare August 29, 2024 18:02
@msbrogli msbrogli force-pushed the feat/tx-history-streaming-flow-control branch 2 times, most recently from 61054c1 to 670dd92 Compare August 29, 2024 18:24
@msbrogli msbrogli self-assigned this Aug 29, 2024
@msbrogli msbrogli requested a review from r4mmer August 29, 2024 19:41
@msbrogli msbrogli force-pushed the feat/tx-history-streaming-flow-control branch from 670dd92 to c3a634b Compare August 29, 2024 19:43
jansegre
jansegre previously approved these changes Aug 29, 2024
hathor/websocket/protocol.py Outdated Show resolved Hide resolved
@msbrogli msbrogli force-pushed the feat/tx-history-streaming-flow-control branch from c3a634b to b0ae958 Compare August 29, 2024 21:22
hathor/websocket/protocol.py Outdated Show resolved Hide resolved
hathor/websocket/streamer.py Outdated Show resolved Hide resolved
tests/websocket/test_streamer.py Outdated Show resolved Hide resolved
hathor/websocket/streamer.py Outdated Show resolved Hide resolved
hathor/websocket/protocol.py Show resolved Hide resolved
tests/websocket/test_streamer.py Outdated Show resolved Hide resolved
@msbrogli msbrogli force-pushed the feat/tx-history-streaming-flow-control branch from 6c9da5e to 303abdd Compare September 2, 2024 19:06
Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 34.44444% with 59 lines in your changes missing coverage. Please review.

Project coverage is 84.50%. Comparing base (4f80786) to head (f3e3ab5).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
hathor/websocket/protocol.py 5.55% 34 Missing ⚠️
hathor/websocket/streamer.py 50.00% 22 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1126      +/-   ##
==========================================
- Coverage   84.61%   84.50%   -0.11%     
==========================================
  Files         317      317              
  Lines       24150    24231      +81     
  Branches     3662     3680      +18     
==========================================
+ Hits        20434    20477      +43     
- Misses       3009     3050      +41     
+ Partials      707      704       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@msbrogli msbrogli force-pushed the feat/tx-history-streaming-flow-control branch 2 times, most recently from f997cd2 to a551bb4 Compare September 3, 2024 14:36
jansegre
jansegre previously approved these changes Sep 3, 2024
Copy link
Member

@jansegre jansegre 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 to me.

@msbrogli msbrogli force-pushed the feat/tx-history-streaming-flow-control branch 2 times, most recently from aa9eaca to a53373c Compare September 3, 2024 15:48
tests/websocket/test_streamer.py Outdated Show resolved Hide resolved
@msbrogli msbrogli force-pushed the feat/tx-history-streaming-flow-control branch from e32fa63 to f3e3ab5 Compare September 3, 2024 20:44
@msbrogli msbrogli merged commit f3e3ab5 into master Sep 3, 2024
13 checks passed
@msbrogli msbrogli deleted the feat/tx-history-streaming-flow-control branch September 3, 2024 21:52
@jansegre jansegre mentioned this pull request Oct 4, 2024
2 tasks
@jansegre jansegre mentioned this pull request Dec 11, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants