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

Remove dead code that still fetches lamports-per-signature from nonce or from blockhash_queue #34943

Closed

Conversation

tao-stones
Copy link
Contributor

Problem

calculate_fee() function no longer uses lamports_per_signature parameter, yet there are code still fetches it from nonce account, or lock and read from blockhash_queue. Can gain some perf if these dead code are removed.

Summary of Changes

  • Remove lamports_per_signature before calculate_fee(...);
  • Removed bank.get_fee_for_message_with_lamports_per_signature() as it is redundant.

Fixes #34864

@tao-stones tao-stones force-pushed the remove-lamports-per-sig-param branch from 446abbd to fa585b3 Compare January 25, 2024 04:52
…t_fee_for_message_with_lamports_per_signature() as it is redandent.
@tao-stones tao-stones force-pushed the remove-lamports-per-sig-param branch from fa585b3 to 6241072 Compare January 25, 2024 16:21
@@ -80,7 +80,6 @@ impl FeeStructure {
pub fn calculate_fee(
&self,
message: &SanitizedMessage,
_lamports_per_signature: u64,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rest of changes are results of removing this unused parameter.

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 132 lines in your changes are missing coverage. Please review.

Comparison is base (b04765f) 81.6% compared to head (595eac2) 81.6%.
Report is 12 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34943     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         828      830      +2     
  Lines      224339   224681    +342     
=========================================
+ Hits       183274   183432    +158     
- Misses      41065    41249    +184     

@tao-stones tao-stones marked this pull request as ready for review January 25, 2024 17:56
LucasSte
LucasSte previously approved these changes Jan 25, 2024
Copy link
Member

@jstarry jstarry 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.. I was concerned about some potential consensus breaking changes here in all the nonce handling but it looks like check_transaction_age -> check_transaction_for_nonce already filters out any transactions that use invalid nonces properly. cc @t-nelson for a second look if he wishes

@tao-stones
Copy link
Contributor Author

Close this without merge; we decided to keep original calculate_fee(...) public api unchanged in #34757, so instead of one big-bang removal, I will break them into smaller pieces as it goes.

@tao-stones tao-stones closed this Jan 29, 2024
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.

lamports_per_signature in nonce account is not used
3 participants