separate priority fee and transaction fee from fee calculation #34757

tao-stones · 2024-01-11T21:29:59Z

Problem

To reward priority fee differently from transaction fee (SIMD-0096), priority fee should be separated from return of calculate_fee(...).

Summary of Changes

change calculate_fee() return type from u64 to FeeDetails
no other functional changes.

Fixes #

codecov · 2024-01-11T23:44:36Z

Codecov Report

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

Comparison is base (51c0649) 81.6% compared to head (f84b315) 81.6%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #34757   +/-   ##
=======================================
  Coverage    81.6%    81.6%           
=======================================
  Files         830      830           
  Lines      224746   224757   +11     
=======================================
+ Hits       183515   183547   +32     
+ Misses      41231    41210   -21

sdk/src/fee.rs

t-nelson · 2024-01-12T22:59:03Z

can this wait for 1.19? should be able to cut 1.18 in ~1wk if all goes well with mb 1.17 adoption, making this mergeable into master

tao-stones · 2024-01-12T23:51:53Z

can this wait for 1.19? should be able to cut 1.18 in ~1wk if all goes well with mb 1.17 adoption, making this mergeable into master

1.19 sounds good, need time to test anyway.

sdk/src/fee.rs

t-nelson · 2024-01-23T23:34:15Z

sdk/src/fee.rs

@@ -83,7 +96,7 @@ impl FeeStructure {
        _lamports_per_signature: u64,
        budget_limits: &FeeBudgetLimits,
        include_loaded_account_data_size_in_fee: bool,
-    ) -> u64 {
+    ) -> FeeDetails {


this is changing the signature of a public interface, which is technically against semver. who's consuming this?

Direct callers (besides tests) are bank (calculate tx fee for distribution) and accounts (to validate payer account), both are updated in this PR. Don't think it is exposed to external 🤞🏼

well it's exposed for sure 'cause sdk is a publicly consumable crate and this method is publicly exported. question is whether it's actually consumed by anyone, which we really ought not find out the hard way.

i suppose the most pedantic thing to do here would be to move this logic to a new method calculate_fee_details(...) -> FeeDetails, then deprecate calculate_fee(...) -> u64 replacing its body with a call to calculate_fee_details(...).total_fees()

I agree with Trent here, it's quite possible 3rd party code usees this function.
Though I'm not sure we need to actually deprecate calculate_fee, so long as the impl is just calculate_fee_details(...).total_fees().

Seems reasonable there are many call-sites that don't care about the details, just the total fee. Might simplify this PR as well.

Agree too. But does calculate_fee() need to be exposed to external? Users use simulator to get tx fee, are they? github says this function was moved from runtime::bank to sdk::fee_structure not too long ago. Maybe entire fee_structure belong to solana_runtime instead of sdk?

the ship has sailed on whether it needs to be or not. we've already shipped releases with it exposed in the public api of a publicly consumable crate that we've promised to follow semver on. removing that symbol for public availability anytime but a major release will be a violation of semver

033c98b for the sailed ship

tao-stones · 2024-01-25T21:45:20Z

sdk/src/fee.rs

@@ -75,15 +88,32 @@ impl FeeStructure {
            .saturating_mul(heap_cost)
    }

-    /// Calculate fee for `SanitizedMessage`
    #[cfg(not(target_os = "solana"))]
    pub fn calculate_fee(


Keep this function live & active. @apfitzge has a good point that most use case just want total_fee. Doesn't need to deprecate it.

…e and priority fee separated

apfitzge · 2024-01-25T22:18:39Z

sdk/src/fee.rs

+            transaction_fee: (signature_fee
+                .saturating_add(write_lock_fee)
+                .saturating_add(compute_fee) as f64)
+                .round() as u64,


unrelated, but why do we round here even after removing congestion? these are all integers, right?

Good catch! I'll patch it separately

t-nelson

thanks!

solana-labs#34757)" This reverts commit 5ecc47e.

#34757)" This reverts commit 5ecc47e.

#34757)" This reverts commit 5ecc47e. (cherry picked from commit df2ee12) # Conflicts: # sdk/src/fee.rs

* Revert "refactor unused parameter (#34970)" This reverts commit 0838909. (cherry picked from commit 1542392) # Conflicts: # sdk/src/fee.rs * Revert "separate priority fee and transaction fee from fee calculation (#34757)" This reverts commit 5ecc47e. (cherry picked from commit df2ee12) # Conflicts: # sdk/src/fee.rs * Revert "Remove congestion multiplier from calculate fee (#34865)" This reverts commit 73d3973. (cherry picked from commit 0dcac3f) # Conflicts: # sdk/src/fee.rs * fix merge conflicts --------- Co-authored-by: Tao Zhu <tao@solana.com>

tao-stones force-pushed the calculate-fee-returns-details branch 2 times, most recently from 2ae2a72 to 6725461 Compare January 11, 2024 22:01

tao-stones requested review from pgarg66 and t-nelson January 12, 2024 02:51

apfitzge reviewed Jan 12, 2024

View reviewed changes

sdk/src/fee.rs Outdated Show resolved Hide resolved

mergify bot added community Community contribution need:merge-assist labels Jan 23, 2024

tao-stones force-pushed the calculate-fee-returns-details branch from 6725461 to c877ee5 Compare January 23, 2024 19:06

tao-stones removed community Community contribution need:merge-assist labels Jan 23, 2024

tao-stones force-pushed the calculate-fee-returns-details branch 2 times, most recently from 9cd885f to 1f186d1 Compare January 23, 2024 20:24

tao-stones requested a review from apfitzge January 23, 2024 22:58

t-nelson requested changes Jan 23, 2024

View reviewed changes

tao-stones requested a review from t-nelson January 24, 2024 05:12

tao-stones force-pushed the calculate-fee-returns-details branch from 033c98b to f7f85de Compare January 25, 2024 16:47

tao-stones commented Jan 25, 2024

View reviewed changes

add function calculate_fee_details() that returns FeeDetails with bas…

f2f33f5

…e and priority fee separated

tao-stones force-pushed the calculate-fee-returns-details branch from 3f826ea to f2f33f5 Compare January 25, 2024 22:14

apfitzge approved these changes Jan 25, 2024

View reviewed changes

t-nelson approved these changes Jan 26, 2024

View reviewed changes

Merge branch 'master' into calculate-fee-returns-details

f84b315

tao-stones merged commit 5ecc47e into solana-labs:master Jan 26, 2024
45 checks passed

tao-stones deleted the calculate-fee-returns-details branch January 26, 2024 14:24

This was referenced Jan 29, 2024

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

Closed

fix fee resulted bank hash mismatch #35012

Closed

tao-stones added a commit to tao-stones/solana that referenced this pull request Jan 30, 2024

revert solana-labs#34757

da7c936

tao-stones mentioned this pull request Jan 30, 2024

Revert congestion multiplier PRs #35016

Closed

tao-stones added a commit to tao-stones/solana that referenced this pull request Jan 30, 2024

Revert "separate priority fee and transaction fee from fee calculation (

1c13fbb

solana-labs#34757)" This reverts commit 5ecc47e.

tao-stones mentioned this pull request Jan 30, 2024

Revert PRs caused hash mismatch #35018

Merged

tao-stones added a commit that referenced this pull request Jan 30, 2024

Revert "separate priority fee and transaction fee from fee calculation (

df2ee12

#34757)" This reverts commit 5ecc47e.

mergify bot pushed a commit that referenced this pull request Jan 30, 2024

Revert "separate priority fee and transaction fee from fee calculation (

0f603be

#34757)" This reverts commit 5ecc47e. (cherry picked from commit df2ee12) # Conflicts: # sdk/src/fee.rs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

separate priority fee and transaction fee from fee calculation #34757

separate priority fee and transaction fee from fee calculation #34757

tao-stones commented Jan 11, 2024

codecov bot commented Jan 11, 2024 •

edited

Loading

t-nelson commented Jan 12, 2024 •

edited

Loading

tao-stones commented Jan 12, 2024

t-nelson Jan 23, 2024

tao-stones Jan 24, 2024 •

edited

Loading

t-nelson Jan 24, 2024

apfitzge Jan 24, 2024

tao-stones Jan 24, 2024

t-nelson Jan 24, 2024

tao-stones Jan 25, 2024

tao-stones Jan 25, 2024

apfitzge Jan 25, 2024

tao-stones Jan 25, 2024

t-nelson left a comment

separate priority fee and transaction fee from fee calculation #34757

separate priority fee and transaction fee from fee calculation #34757

Conversation

tao-stones commented Jan 11, 2024

Problem

Summary of Changes

codecov bot commented Jan 11, 2024 • edited Loading

Codecov Report

t-nelson commented Jan 12, 2024 • edited Loading

tao-stones commented Jan 12, 2024

Choose a reason for hiding this comment

tao-stones Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 11, 2024 •

edited

Loading

t-nelson commented Jan 12, 2024 •

edited

Loading

tao-stones Jan 24, 2024 •

edited

Loading