Remove congestion multiplier from calculate_fee function #34537

tao-stones · 2023-12-19T23:27:39Z

Problem

bank.calculate_fee() takes a parameter with misleading name (lamports_per_signature) for testing congestion multiplier to be zero, which has been removed everywhere. That parameter should also be removed.

Due to legacy or perhaps misleading name of the parameter, call sites are calling extra code to resolve lamports_per_signature before calling calculate_fee(), in some instances, it locks hash_queue to do so. All of these are not necessary.

Summary of Changes

remove congestion_multiplier within calculate_fee()
remove lamports_per_signature input parameter of calculate_fee(), durable transaction also uses fee rate from time of execution.
remove code that fetches lamports_per_signature from nonce account leading to call of calculate_fee()

Fixes #

sdk/src/fee.rs

tao-stones · 2023-12-20T05:04:01Z

runtime/src/bank.rs

@@ -4058,22 +4055,6 @@ impl Bank {
            .verification_complete()
    }

-    pub fn get_fee_for_message_with_lamports_per_signature(


fee_structure.calculate_fee() doesn't take lamports_per_signature anymore, this function can be fold into above bank.get_fee_for_message().

tao-stones · 2023-12-20T05:05:49Z

runtime/src/bank.rs

-                })
-        })?;
-        Some(self.get_fee_for_message_with_lamports_per_signature(message, lamports_per_signature))
+    pub fn get_fee_for_message(&self, message: &SanitizedMessage) -> u64 {


the return type changed from option<u64> to u64, that leads to many updates on callsite

tao-stones · 2023-12-20T05:07:07Z

runtime/src/accounts/mod.rs

@@ -52,7 +51,6 @@ pub(super) fn load_accounts(
    ancestors: &Ancestors,
    txs: &[SanitizedTransaction],
    lock_results: Vec<TransactionCheckResult>,
-    hash_queue: &BlockhashQueue,


hash_queue was used to resolve lamports_per_signature, which is no longer needed.

tao-stones · 2023-12-20T23:02:34Z

banks-client/src/lib.rs

-        let genesis = create_genesis_config(10);
+        // Fund payer account with minimal transaction fee for one signature, plus some extra.
+        let mint_lamports = 10 + solana_sdk::fee::FeeStructure::default().lamports_per_signature;
+        let genesis = create_genesis_config(mint_lamports);


Previously, load_accounts() read BlockHashQueue for fee_calculator.lamports_per_signature, then use it as "congestion_multiplier". fee_calculator perhaps should not be used anymore. In any case, fee_calculator.lamports_per_signature would be zero in this test, therefore would have set transaction fee to 0.

With the change in this PR, eg removing congestion_multiplier all together, this transaction transaction will have fee of 1 signature (eg 5000 lamports). Account this in mint account corrects the test.

Bunch of other tests, including those form local-cluster tests, might have been broken for the same reason:

genesis is configured with 0 lamports_per_signature in fee_calculator.

it is read in as congestion_multipler, that effectively set transaction fee to 0.

tests were setup with low balance since transaction has zero fee.

with changes in this PR, transaction will have 5000 lamports per signature fee, that fails transactions with low payer balance.

One option is to initialize bank.fee_structure from blockhash_queue.fee_calculate.lamport_per_signature, instead of default, that should keep all exist tests success without further changes. But that itself has risk, should be in its own pr.

jstarry · 2024-01-10T00:42:02Z

I agree that this code is messy but I'm not sure that this is the right way to clean it up. I believe the reason that the lamports_per_signature is passed here is so that it can be overridden for durable nonce transactions. Maybe @t-nelson can add more context on what the latest discussion on durable nonce tx fees was?

t-nelson · 2024-01-10T20:10:16Z

I agree that this code is messy but I'm not sure that this is the right way to clean it up. I believe the reason that the lamports_per_signature is passed here is so that it can be overridden for durable nonce transactions. Maybe @t-nelson can add more context on what the latest discussion on durable nonce tx fees was?

was this supposed to be a line comment? where's "here"?

tao-stones · 2024-01-10T20:51:58Z

sdk/src/fee.rs

-        } else {
-            1.0 // multiplier that has no effect
-        };
-
        let signature_fee = message
            .num_signatures()
            .saturating_mul(self.lamports_per_signature);


while removing "no-longer necessary" congestion_multipler, realized there might be a bug in this line 88 above. The signature_fee is always calculated with current bank's fee_structure's lamports_per_signature, it should be using passed in parameter lamports_per_signature, which is determined differently for nonce tx and normal tx.

Isn't this a bug? @t-nelson @jstarry

I traced it back to #23095 where behavior seems changed.

mmm, i don't remember the intent of this "congestion" fee. lamports_per_signature already contains the signature rate based congestion fee. it's calculated at the start of the slot, https://github.com/jackcmay/solana/blob/176ef5aeb25d7b3f331a12655654e801fed06235/runtime/src/bank.rs#L1563-L1578.

@t-nelson can you comment on why durable nonces store lamports_per_signature and if we still intend to use that value in the future. The main thing we need to figure out to accept or reject this PR is whether or not we should be using the nonce-stored lamports_per_signature value from a previous bank here instead of the one calculated by the current bank.

durable nonces store lamports_per_signature because some opinionated people were very adamant about preserving the "fee-payer pays deterministic fees" meme. afaik we've already unhooked them and just use fee rate from time of execution

~~another way to look at it: if #34754 is legit bugfix?~~ (somehow, I didn't see trent's comment early) So #34754 is not legit, and durable nonce does not need to store lamports_per_signature?

tao-stones · 2024-01-10T20:57:05Z

I agree that this code is messy but I'm not sure that this is the right way to clean it up. I believe the reason that the lamports_per_signature is passed here is so that it can be overridden for durable nonce transactions. Maybe @t-nelson can add more context on what the latest discussion on durable nonce tx fees was?

was this supposed to be a line comment? where's "here"?

"here" is calculate_fee().

It suppose to be an innocent PR to remove unused stuff 😇 But it revealed few issues. The first one is: have we been wrong in calculating nonce tx signature fee? line commente here.

2. simply bank.get_fee_for_message(), no need to resolve lamports_per_signature before calling; 3. remove a dead test that was for congestion multiplier;

…nature fee

tao-stones · 2024-01-14T00:52:53Z

Convert this PR to Draft, going to break it into smaller and separated PRs:

just remove congestion_multiplier from fee calculation, and fix/harden all tests
new issue/PR to state durable transaction will be charged with current bank's fee rate, not by fee rate in nonce account, remove related code.

tao-stones mentioned this pull request Dec 19, 2023

cleanup feature code after activated everywhere #34509

Merged

tao-stones commented Dec 20, 2023

View reviewed changes

tao-stones requested review from jstarry and apfitzge December 20, 2023 05:09

tao-stones force-pushed the remove-congestion-multipler-from-calc-fee branch from cec0ec8 to 068db48 Compare December 20, 2023 15:54

tao-stones marked this pull request as ready for review December 20, 2023 15:54

tao-stones commented Dec 20, 2023

View reviewed changes

tao-stones mentioned this pull request Dec 20, 2023

Is it still necessary to include FeeCalculator in BlockHashQueue #34553

Closed

tao-stones force-pushed the remove-congestion-multipler-from-calc-fee branch 2 times, most recently from 3238dcf to e3d9b1a Compare December 21, 2023 04:02

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 4, 2024

github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 10, 2024

tao-stones commented Jan 10, 2024

View reviewed changes

tao-stones mentioned this pull request Jan 12, 2024

fee calculated with provided lamports_per_signature #34754

Closed

tao-stones added 5 commits January 12, 2024 17:22

1. remove congestion multiplier from calculate_fee parameter list;

c312356

2. simply bank.get_fee_for_message(), no need to resolve lamports_per_signature before calling; 3. remove a dead test that was for congestion multiplier;

removed hash_queue from load accounts

36e3d15

fmt

587de0b

update sbf tests

f8dfc6a

update tests to fund payer account with extra to cover now-needed sig…

2743154

…nature fee

tao-stones force-pushed the remove-congestion-multipler-from-calc-fee branch from e3d9b1a to 2743154 Compare January 12, 2024 17:22

rebase conflict

ed69c7a

tao-stones mentioned this pull request Jan 12, 2024

separate priority fee and transaction fee from fee calculation #34757

Merged

tao-stones marked this pull request as draft January 14, 2024 00:49

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

tao-stones closed this Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove congestion multiplier from calculate_fee function #34537

Remove congestion multiplier from calculate_fee function #34537

tao-stones commented Dec 19, 2023 •

edited

Loading

tao-stones Dec 20, 2023

tao-stones Dec 20, 2023

tao-stones Dec 20, 2023

tao-stones Dec 20, 2023

tao-stones Dec 21, 2023

jstarry commented Jan 10, 2024

t-nelson commented Jan 10, 2024

tao-stones Jan 10, 2024

t-nelson Jan 11, 2024

jstarry Jan 11, 2024

t-nelson Jan 11, 2024

tao-stones Jan 11, 2024 •

edited

Loading

tao-stones commented Jan 10, 2024

tao-stones commented Jan 14, 2024

Remove congestion multiplier from calculate_fee function #34537

Remove congestion multiplier from calculate_fee function #34537

Conversation

tao-stones commented Dec 19, 2023 • edited Loading

Problem

Summary of Changes

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

jstarry commented Jan 10, 2024

t-nelson commented Jan 10, 2024

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

tao-stones Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

tao-stones commented Jan 10, 2024

tao-stones commented Jan 14, 2024

tao-stones commented Dec 19, 2023 •

edited

Loading

tao-stones Jan 11, 2024 •

edited

Loading