Skip to content

Feature Gated: remove obsoleted FeeRateGovernor from fee calculation#5749

Closed
tao-stones wants to merge 3 commits intoanza-xyz:masterfrom
tao-stones:feat-remove-fee-rate-governor-from-bank
Closed

Feature Gated: remove obsoleted FeeRateGovernor from fee calculation#5749
tao-stones wants to merge 3 commits intoanza-xyz:masterfrom
tao-stones:feat-remove-fee-rate-governor-from-bank

Conversation

@tao-stones
Copy link
Copy Markdown

@tao-stones tao-stones commented Apr 10, 2025

Problem

  • lamports_per_signature in blockhash_queue and nonce accounts is not used for fee calculation.
  • Actual fees are fixed via bank.fee_structure.lamports_per_signature = 5_000.
  • The only role of lamports_per_signature in the blockhash_queue is to enable test-mode zero fees when it's set to zero.
  • Current behavior derives this value through fee_rate_governor, which is overly complex and introduces inconsistency, especially in early banks. (example)

Summary of Changes

  • At genesis, store target_lamports_per_signature from genesis_config into the blockhash queue.
  • For subsequent banks, copy lamports_per_signature from the parent bank.
  • This keeps the intended behavior (e.g., zero-fee testing) and removes reliance on fee_rate_governor.
  • Feature Gated, to avoid unintended fork divergence, this change is behind a feature gate (see Consensus divergence between v2.2 and master on Testnet #5454).

Note: impact on testnet

Genesis Config for three public clusters
Creation time: 2020-03-16T14:29:00+00:00
Cluster type: MainnetBeta
Genesis hash: 5eykt4UsFv8P8NJdTREpY1vzqKqZKvdpKuc147dw2N9d
...
FeeRateGovernor { lamports_per_signature: 0, target_lamports_per_signature: 10000, ... }

Creation time: 2020-02-04T16:35:32+00:00
Cluster type: Testnet
Genesis hash: 4uhcVJyU9pJkvQyS88uRDiswHXSCkY3zQawwpjk2NsNY
...
FeeRateGovernor { lamports_per_signature: 0, target_lamports_per_signature: 0, ... }

Creation time: 2020-08-10T17:36:56+00:00
Cluster type: Devnet
Genesis hash: EtWTRABZaYq6iMfeYKouRu166VU2xqa1wcaWoxPkrZBG
...
FeeRateGovernor { lamports_per_signature: 0, target_lamports_per_signature: 10000, ... }

Notice that testnet sets target_lamports_per_signature to 0 in its genesis config, but it is not a zero-fee environment today. (At some point, the lamports_per_signature value in the blockhash queue was updated to a non-zero value.)

With this PR enabled, a validator restarted from an existing snapshot will inherit the non-zero lamports_per_signature from its parent bank and continue charging normal fees.

However, if the validator were to restart from genesis, it would enter a zero-fee mode, since the genesis bank would initialize the blockhash queue with lamports_per_signature = 0.

Fixes #5782

@tao-stones tao-stones marked this pull request as draft April 10, 2025 17:01
@tao-stones tao-stones force-pushed the feat-remove-fee-rate-governor-from-bank branch from 7860020 to b08383b Compare April 11, 2025 18:04
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 11, 2025

Codecov Report

❌ Patch coverage is 86.20690% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.4%. Comparing base (13799bc) to head (1025017).
⚠️ Report is 3058 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #5749    +/-   ##
========================================
  Coverage    83.4%    83.4%            
========================================
  Files         850      850            
  Lines      377790   377648   -142     
========================================
- Hits       315166   315049   -117     
+ Misses      62624    62599    -25     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tao-stones tao-stones changed the title Feature Gated: remove obsoleted FeeRateGovernor from Bank Feature Gated: remove obsoleted FeeRateGovernor from fee calculation Apr 11, 2025
@tao-stones tao-stones marked this pull request as ready for review April 12, 2025 02:58
@apfitzge apfitzge requested review from apfitzge and removed request for apfitzge May 6, 2025 13:29
Copy link
Copy Markdown

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Checking in, what's the status of this PR?

@tao-stones
Copy link
Copy Markdown
Author

Checking in, what's the status of this PR?

PR is ready for review. Have run it in testnet for several days, looked OK. Not on the top of list atm, but appreciate reviews.

@tao-stones tao-stones force-pushed the feat-remove-fee-rate-governor-from-bank branch from b1a1a77 to f32b13a Compare May 6, 2025 17:09
@brooksprumo brooksprumo self-requested a review May 7, 2025 14:07
@tao-stones
Copy link
Copy Markdown
Author

After rebase, it seems to break sbf stable test due to test program is not deployed. I'm taking it back to draft for investigation. Please hold on reviewing. Sorry about back-n-forth.

failed stable-sbf test
---- test_no_panic_rpc_client stdout ----
  | Waiting for fees to stabilize 1...
  |  
  | thread 'test_no_panic_rpc_client' panicked at tests/simulation.rs:86:10:
  | called `Result::unwrap()` on an `Err` value: Error { request: Some(SendTransaction), kind: RpcError(RpcResponseError { code: -32002, message: "Transaction simulation failed: Error processing Instruction 0: Unsupported program id", data: SendTransactionPreflightFailure(RpcSimulateTransactionResult { err: Some(InstructionError(0, UnsupportedProgramId)), logs: Some(["Program 11157t3sqMV725NVRLrVQbAu98Jjfk1uCKehJnXXQs invoke [1]", "Program is not deployed", "Program 11157t3sqMV725NVRLrVQbAu98Jjfk1uCKehJnXXQs failed: Unsupported program id"]), accounts: None, units_consumed: Some(0), return_data: None, inner_instructions: None, replacement_blockhash: None }) }) }
  |  

<br class="Apple-interchange-newline">

@tao-stones tao-stones marked this pull request as draft May 7, 2025 16:17
@tao-stones
Copy link
Copy Markdown
Author

After rebase, it seems to break sbf stable test due to test program is not deployed. I'm taking it back to draft for investigation. Please hold on reviewing. Sorry about back-n-forth.

failed stable-sbf test

The failing test is flaky, pretty sure is a timing issue - test transaction could be sent before simulation program is deployed. This PR makes TestValidator starts up quicker by effective skip this step. Plan to address test's flakiness in own PR.

@tao-stones tao-stones force-pushed the feat-remove-fee-rate-governor-from-bank branch from be93f97 to 570405c Compare May 12, 2025 02:28
@tao-stones tao-stones force-pushed the feat-remove-fee-rate-governor-from-bank branch 3 times, most recently from ff5188c to 4fe1f20 Compare June 17, 2025 15:54
@tao-stones
Copy link
Copy Markdown
Author

After rebase, it seems to break sbf stable test due to test program is not deployed. I'm taking it back to draft for investigation. Please hold on reviewing. Sorry about back-n-forth.
failed stable-sbf test

The failing test is flaky, pretty sure is a timing issue - test transaction could be sent before simulation program is deployed. This PR makes TestValidator starts up quicker by effective skip this step. Plan to address test's flakiness in own PR.

#6587 updated TestValidator that addressed the flaky downstream tests. Rebased to bringing this PR back for review.

@brooksprumo @apfitzge, for refreshment, it is part of #3303

@tao-stones tao-stones marked this pull request as ready for review June 17, 2025 17:23
@tao-stones tao-stones force-pushed the feat-remove-fee-rate-governor-from-bank branch from 4fe1f20 to 1025017 Compare June 24, 2025 17:17
@tao-stones
Copy link
Copy Markdown
Author

tao-stones commented Jun 24, 2025

To clarify what's expected behavior when this feature gate is activated:

This feature gate changes how lamports_per_signature is managed in the blockhash_queue.

  1. Behavior After Feature Activation (Normal Operation)
  • lamports_per_signature is copied from the parent bank into the blockhash_queue.
  • The fee_rate_governor is no longer involved in computing or updating LPS.

Impact on fees: None.

  • If LPS was 0, it remains 0.
  • If LPS was non-zero, it will never become zero — this preserves current FeeRateGovernor behavior.
  1. Validator Restart Scenarios
  • Restart from Snapshot: the validator resumes by copying lamports_per_signature from the parent bank in the snapshot.
  • Restart from Genesis: bank0's LPS is initialized from genesis_config.target_lamports_per_signature. This corrects two current inconsistencies:
    1. If target_lamports_per_signature != 0: bank0 will not be zero-fee (as it incorrectly is today).
    2. If target_lamports_per_signature == 0: the validator becomes a zero-fee cluster permanently, since LPS will remain 0 in all descendant banks.
      • This unlikely case could unintentionally turn Testnet into a permanent zero-fee environment if it restarts from current genesis.

I'll test it out by restarting this PR-built validator (with feature enabled hardcodedly) on three public clusters, make sure it's catchup, and stay up for an epoch.

(edited)

Comment thread runtime/src/bank/tests.rs
Comment on lines +5700 to +5703
activate_feature(
&mut genesis_config,
agave_feature_set::remove_fee_rate_governor_from_fee_calculation::id(),
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we should follow the comments on this test and use a test matrix for with and without this feature gate.

let fee_details = solana_fee::calculate_fee_details(
transaction,
last_lamports_per_signature == 0,
self.is_zero_fees_for_test(),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this calculate_rewards_for_transaction() a test-only/dcou function? If not, seems fishy to call a _for_test() fn here.

@tao-stones
Copy link
Copy Markdown
Author

@brooksprumo Thanks for picking this up! I honestly thought the bot had already put this PR to rest 😄. It’s a nice cleanup, but it didn’t really gain traction, and doesn’t seem to be blocking anything at the moment. I’m going to close it for now — we can always resurrect it later if FeeRateGovernor starts getting too itchy again! 🧼

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.

Feature Gate: remove fee-rate-governor from fee calculation path

3 participants