-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Split signature throughput tracking out of FeeCalculator
#8447
Conversation
74a7539
to
f303562
Compare
Codecov Report
@@ Coverage Diff @@
## master #8447 +/- ##
========================================
- Coverage 80.3% 80.1% -0.2%
========================================
Files 256 256
Lines 56554 55279 -1275
========================================
- Hits 45413 44297 -1116
+ Misses 11141 10982 -159 |
f6fb667
to
4c5f81a
Compare
@mvines how are we looking here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, fwiw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the JSON RPC API docs too
Leaving `FeeCalculator` to *only* calculate transaction fees
Appease BPF tests
This reverts commit f303562.
4c5f81a
to
54c4254
Compare
Pull request has been modified.
@mvines last 4 should resolve the outstandings. While updating the docs I was reminded that we have a solana/sdk/src/fee_calculator.rs Lines 104 to 106 in fe63a19
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just that one last comment
* SDK: Split new `FeeRateGovernor` out of `FeeCalculator` Leaving `FeeCalculator` to *only* calculate transaction fees * Replace `FeeCalculator` with `FeeRateGovernor` as appropriate * Expose recent `FeeRateGovernor` to clients * Move `burn()` back into `FeeCalculator` Appease BPF tests * Revert "Move `burn()` back into `FeeCalculator`" This reverts commit f303562. * Adjust BPF `Fee` sysvar test to reflect removal of `burn()` from `FeeCalculator` * Make `FeeRateGovernor`'s `lamports_per_signature` private * rebase artifacts * fmt * Drop 'Recent' * Drop _with_commitment variant * Use a more portable integer for `target_signatures_per_slot` * Add docs for `getReeRateCalculator` JSON RPC method * Don't return `lamports_per_signature` in `getFeeRateGovernor` JSONRPC reply (cherry picked from commit 90bedd7)
…bs#8447) * SDK: Split new `FeeRateGovernor` out of `FeeCalculator` Leaving `FeeCalculator` to *only* calculate transaction fees * Replace `FeeCalculator` with `FeeRateGovernor` as appropriate * Expose recent `FeeRateGovernor` to clients * Move `burn()` back into `FeeCalculator` Appease BPF tests * Revert "Move `burn()` back into `FeeCalculator`" This reverts commit f303562. * Adjust BPF `Fee` sysvar test to reflect removal of `burn()` from `FeeCalculator` * Make `FeeRateGovernor`'s `lamports_per_signature` private * rebase artifacts * fmt * Drop 'Recent' * Drop _with_commitment variant * Use a more portable integer for `target_signatures_per_slot` * Add docs for `getReeRateCalculator` JSON RPC method * Don't return `lamports_per_signature` in `getFeeRateGovernor` JSONRPC reply
Problem
FeeCalculator
does too much. While initially intended as a utility to calculate transaction fees, it has grown to also track cluster signature throughput for calculating the next block'sFeeCalculator
. This requires significantly more stored state, making it prohibitive to store aFeeCalculator
on chain as required to resolve #7967.Summary of Changes
Factor out signature throughput tracking to new
FeeRateGovernor
typeReplace
FeeCalculator
withFeeRateGovernor
as appropriateExpose recent
FeeRateGovernor
via RPCToward #7967