-
Notifications
You must be signed in to change notification settings - Fork 231
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
v1.18: Scheduler - prioritization fees/cost (#34888) #187
v1.18: Scheduler - prioritization fees/cost (#34888) #187
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v1.18 #187 +/- ##
=========================================
+ Coverage 80.6% 81.6% +0.9%
=========================================
Files 827 827
Lines 224496 224469 -27
=========================================
+ Hits 181015 183209 +2194
+ Misses 43481 41260 -2221 |
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
} | ||
|
||
/// Returns a reference to the priority details of the transaction. | ||
pub(crate) fn transaction_priority_details(&self) -> &TransactionPriorityDetails { |
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.
for the sake of backport readability, can you read these removed functions and annotate with #[allow(dead_code)]
?
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.
The meta data stored in transaction_state changed, so we no longer have a TransactionPriorityDetails
. I don't think we should add back those unused metadata and code for calculating them.
I could add a block-comment around these removed fns if you think that'd help simplify the diff.
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.
Added block-comment from 50 to 97: c80d5fb
c80d5fb
to
03dbd60
Compare
had to rebase due to 1.18 version being tagged, was causing sanity CI check to fail. |
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.
great! much more readable with the dead code commented instead of removed
…or Solana transactions In an ideal world over-reserving compute units would not result in faster transaction confirmation but if we optimize the compute unit limit to match the amount of CUs the transaction requires (+ a 20% buffer) the transactions simply do not go through in time. Perhaps this could be revisited at some point in the future e.g. when [this PR](anza-xyz/agave#187) is released and validators update.
…-xyz#187) * v1.18: Scheduler - prioritization fees/cost (solana-labs#34888) * add commented out functions - simplify diff
Problem
Manual backport of solana-labs#34888
Summary of Changes
Fixes #