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

Add get_blocks and get_slot methods to bench-tps-client #94

Merged

Conversation

KirillLykov
Copy link

Problem

For the analysis of the confirmed transactions implemented in #92 BenchTpsClient needs some additional methods.

Summary of Changes

Add get_block, get_blocks and get_slot methods.

@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (85cfe23) to head (318a67b).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master      #94     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         838      838             
  Lines      226223   226223             
=========================================
- Hits       185240   185177     -63     
- Misses      40983    41046     +63     

@KirillLykov KirillLykov force-pushed the klykov/bench-tps-client-get-blocks branch from a2703f1 to 5287558 Compare March 6, 2024 14:35
bench-tps/src/bench_tps_client.rs Outdated Show resolved Hide resolved
bench-tps/src/bench_tps_client.rs Show resolved Hide resolved
@@ -1,6 +1,7 @@
use {

Choose a reason for hiding this comment

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

Plan is still for ThinClient to be deprecated then deleted, right?

Choose a reason for hiding this comment

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

Yes, been deprecated in PR: #35365. Fully removing it from other parts is WIP

bw-solana
bw-solana previously approved these changes Mar 6, 2024
Copy link

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

Design wise, I'm debating if bench-tps is the right tool for doing this. I can convince myself that verifying transaction results is a reasonable extension of the existing tool.

ilya-bobyr
ilya-bobyr previously approved these changes Mar 6, 2024
Copy link

@ilya-bobyr ilya-bobyr left a comment

Choose a reason for hiding this comment

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

Questions about the commitment level are quite reasonable.
This change is to support more metrics in bench-tps.
Specifically, Kirill is adding block fetching code here: #92
As bench-tps is supposed to be a major source of transactions, while it is running, fetching whole blocks and getting transaction stats this way it probably the most efficient approach.

There is a short summary here: https://github.com/anza-xyz/agave/pull/92/files#diff-1d0a43d64af632a7d6ecf76ff095dec3b9a6243758fb5818752e26f4c5383611R1-R2

But it would not hurt to provide a more extensive reasoning in the same or some other comment, relevant to the added service :P

For fetching blocks, I think, we want confirmed commitment level. Latency is not important, and we would not want to deal with forks.
We may want to add some fork related stats in there at a later point.

@KirillLykov
Copy link
Author

Code looks good to me.

Design wise, I'm debating if bench-tps is the right tool for doing this. I can convince myself that verifying transaction results is a reasonable extension of the existing tool.

Alternatively, I could have implemented a standalone tool which goes over all the provided signatures in the give span of blocks and does the same. This approach will make bench-tps runs execution to take much longer time and will lead to slightly more complex setup (create signature file, read it, what if something went wrong, etc)

Copy link

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

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

LGTM

@KirillLykov KirillLykov merged commit c6bd388 into anza-xyz:master Mar 7, 2024
46 checks passed
codebender828 pushed a commit to codebender828/agave that referenced this pull request Oct 3, 2024
* add get_block(s)/slot methods to BenchTpsClient

* Update Cargo.lock

* add commitment level for get_slot/blocks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants