Skip to content

Refactor remove fee rate governor from bank #5388

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.

Sign up for GitHub

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

Closed
tao-stones wants to merge 2 commits into from
Closed

Refactor remove fee rate governor from bank #5388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
639016d
remove fee-rate-governor from bank
tao-stones Mar 20, 2025
4acf88a
fix abi test
tao-stones Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 3 additions & 19 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,6 @@ impl PartialEq for Bank {
block_height,
collector_id,
collector_fees,
fee_rate_governor,
collected_rent,
rent_collector,
epoch_schedule,
Expand Down Expand Up @@ -610,7 +609,6 @@ impl PartialEq for Bank {
&& block_height == &other.block_height
&& collector_id == &other.collector_id
&& collector_fees.load(Relaxed) == other.collector_fees.load(Relaxed)
&& fee_rate_governor == &other.fee_rate_governor
&& collected_rent.load(Relaxed) == other.collected_rent.load(Relaxed)
&& rent_collector == &other.rent_collector
&& epoch_schedule == &other.epoch_schedule
Expand Down Expand Up @@ -827,9 +825,6 @@ pub struct Bank {
/// Fees that have been collected
collector_fees: AtomicU64,

/// Track cluster signature throughput and adjust fee rate
pub(crate) fee_rate_governor: FeeRateGovernor,

/// Rent that has been collected
collected_rent: AtomicU64,

Expand Down Expand Up @@ -1105,7 +1100,6 @@ impl Bank {
block_height: u64::default(),
collector_id: Pubkey::default(),
collector_fees: AtomicU64::default(),
fee_rate_governor: FeeRateGovernor::default(),
collected_rent: AtomicU64::default(),
rent_collector: RentCollector::default(),
epoch_schedule: EpochSchedule::default(),
Expand Down Expand Up @@ -1283,10 +1277,6 @@ impl Bank {

let (status_cache, status_cache_time_us) = measure_us!(Arc::clone(&parent.status_cache));

let (fee_rate_governor, fee_components_time_us) = measure_us!(
FeeRateGovernor::new_derived(&parent.fee_rate_governor, parent.signature_count())
);

let bank_id = rc.bank_id_generator.fetch_add(1, Relaxed) + 1;
let (blockhash_queue, blockhash_queue_time_us) =
measure_us!(RwLock::new(parent.blockhash_queue.read().unwrap().clone()));
Expand Down Expand Up @@ -1339,7 +1329,6 @@ impl Bank {
.block_height
.checked_add(1)
.expect("block height addition overflowed"),
fee_rate_governor,
capitalization: AtomicU64::new(parent.capitalization()),
vote_only_bank,
inflation: parent.inflation.clone(),
Expand Down Expand Up @@ -1487,7 +1476,6 @@ impl Bank {
bank_rc_creation_time_us,
total_elapsed_time_us: time.as_us(),
status_cache_time_us,
fee_components_time_us,
blockhash_queue_time_us,
stakes_cache_time_us,
epoch_stakes_time_us,
Expand Down Expand Up @@ -1758,7 +1746,6 @@ impl Bank {
block_height: fields.block_height,
collector_id: fields.collector_id,
collector_fees: AtomicU64::new(fields.collector_fees),
fee_rate_governor: fields.fee_rate_governor,
collected_rent: AtomicU64::new(fields.collected_rent),
// clone()-ing is needed to consider a gated behavior in rent_collector
rent_collector: Self::get_rent_collector_from(&fields.rent_collector, fields.epoch),
Expand Down Expand Up @@ -1951,7 +1938,9 @@ impl Bank {
block_height: self.block_height,
collector_id: self.collector_id,
collector_fees: self.collector_fees.load(Relaxed),
fee_rate_governor: self.fee_rate_governor.clone(),
// Bank no longer has fee-rate-governor, it can be removed from snapshot in followup
// refactoring.
fee_rate_governor: FeeRateGovernor::default(),
Comment on lines +1941 to +1943
Copy link
Copy Markdown

@brooksprumo brooksprumo Mar 21, 2025

Choose a reason for hiding this comment

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

This struct, BankFieldsToSerialize, should reflect all the fields in Bank that should be serialized in the snapshot. Since we no longer have a fee rate governor in Bank, we can (and should) remove it here as well.

Note that we can't change the actual snapshot format, so we'll keep writing FeeRateGovernor::default(), but that'll be handled by the actual snapshot code later. This is intended to make it clearer which fields are actually needed or not (from the POV of either the Bank or the Snapshot).

Suggested change
// Bank no longer has fee-rate-governor, it can be removed from snapshot in followup
// refactoring.
fee_rate_governor: FeeRateGovernor::default(),

Copy link
Copy Markdown

@apfitzge apfitzge Mar 21, 2025

Choose a reason for hiding this comment

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

maybe a dumb question - do we actually need to serialize this specific struct anymore? can we just serialize [0u8; core::mem::size_of<FeeRateGovernor>()]?

maybe we because validators could still be deserializing (checking structure is correct) until all are whatever version this lands in +?

Copy link
Copy Markdown

@brooksprumo brooksprumo Mar 21, 2025

Choose a reason for hiding this comment

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

This is a good point. In what version is FeeRateGovernor not needed? If we still need is in v2.2, then we cannot remove it from the snapshot here in v2.3 (master). We must wait until v2.4 instead.

Copy link
Copy Markdown

@brooksprumo brooksprumo Mar 21, 2025

Choose a reason for hiding this comment

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

The other point, we cannot remove fields/bytes from the actual snapshot. We can write zeroes and ignore them if the field is not needed anymore.

Copy link
Copy Markdown

@apfitzge apfitzge Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

we have not needed it since before I started working on solana 😆

collected_rent: self.collected_rent.load(Relaxed),
rent_collector: self.rent_collector.clone(),
epoch_schedule: self.epoch_schedule.clone(),
Expand Down Expand Up @@ -2705,9 +2694,6 @@ impl Bank {
#[cfg(feature = "dev-context-only-utils")] collector_id_for_tests: Option<Pubkey>,
#[cfg(feature = "dev-context-only-utils")] genesis_hash: Option<Hash>,
) {
// Bootstrap validator collects fees until `new_from_parent` is called.
self.fee_rate_governor = genesis_config.fee_rate_governor.clone();

for (pubkey, account) in genesis_config.accounts.iter() {
assert!(
self.get_account(pubkey).is_none(),
Expand Down Expand Up @@ -6483,14 +6469,12 @@ impl Bank {

if new_feature_activations.contains(&feature_set::pico_inflation::id()) {
*self.inflation.write().unwrap() = Inflation::pico();
self.fee_rate_governor.burn_percent = solana_sdk::fee_calculator::DEFAULT_BURN_PERCENT; // 50% fee burn
self.rent_collector.rent.burn_percent = 50; // 50% rent burn
}

if !new_feature_activations.is_disjoint(&self.feature_set.full_inflation_features_enabled())
{
*self.inflation.write().unwrap() = Inflation::full();
self.fee_rate_governor.burn_percent = solana_sdk::fee_calculator::DEFAULT_BURN_PERCENT; // 50% fee burn
self.rent_collector.rent.burn_percent = 50; // 50% rent burn
}

Expand Down
2 changes: 0 additions & 2 deletions runtime/src/bank/metrics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ pub(crate) struct NewBankTimings {
pub(crate) bank_rc_creation_time_us: u64,
pub(crate) total_elapsed_time_us: u64,
pub(crate) status_cache_time_us: u64,
pub(crate) fee_components_time_us: u64,
pub(crate) blockhash_queue_time_us: u64,
pub(crate) stakes_cache_time_us: u64,
pub(crate) epoch_stakes_time_us: u64,
Expand Down Expand Up @@ -121,7 +120,6 @@ pub(crate) fn report_new_bank_metrics(
("bank_rc_creation_us", timings.bank_rc_creation_time_us, i64),
("total_elapsed_us", timings.total_elapsed_time_us, i64),
("status_cache_us", timings.status_cache_time_us, i64),
("fee_components_us", timings.fee_components_time_us, i64),
("blockhash_queue_us", timings.blockhash_queue_time_us, i64),
("stakes_cache_us", timings.stakes_cache_time_us, i64),
("epoch_stakes_time_us", timings.epoch_stakes_time_us, i64),
Expand Down
33 changes: 8 additions & 25 deletions runtime/src/bank/serde_snapshot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,8 @@ mod tests {

let versioned_epoch_stakes = mem::take(&mut bank_fields.versioned_epoch_stakes);
let accounts_lt_hash = bank_fields.accounts_lt_hash.clone().map(Into::into);
let (_last_hash, last_lamports_per_signature) =
bank2.last_blockhash_and_lamports_per_signature();
serde_snapshot::serialize_bank_snapshot_into(
&mut writer,
bank_fields,
Expand All @@ -225,7 +227,7 @@ mod tests {
expected_accounts_hash,
&get_storages_to_serialize(&bank2.get_snapshot_storages(None)),
ExtraFieldsToSerialize {
lamports_per_signature: bank2.fee_rate_governor.lamports_per_signature,
lamports_per_signature: last_lamports_per_signature,
incremental_snapshot_persistence: expected_incremental_snapshot_persistence
.as_ref(),
epoch_accounts_hash: expected_epoch_accounts_hash,
Expand Down Expand Up @@ -334,8 +336,6 @@ mod tests {
(AccountsHash(Hash::new_unique()), u64::default()),
);

// Set extra fields
bank.fee_rate_governor.lamports_per_signature = 7000;
// Note that epoch_stakes already has two epoch stakes entries for epochs 0 and 1
// which will also be serialized to the versioned epoch stakes extra field. Those
// entries are of type Stakes<StakeAccount> so add a new entry for Stakes<Stake>.
Expand Down Expand Up @@ -394,10 +394,6 @@ mod tests {
.unwrap();

assert_eq!(bank.epoch_stakes, dbank.epoch_stakes);
assert_eq!(
bank.fee_rate_governor.lamports_per_signature,
dbank.fee_rate_governor.lamports_per_signature
);
Copy link
Copy Markdown
Author

@tao-stones tao-stones Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

does removing fee_rate_governor as extra modified fields break the test integrity? wdyt @brooksprumo

Copy link
Copy Markdown

@brooksprumo brooksprumo Mar 21, 2025

Choose a reason for hiding this comment

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

Looks fine to me.

}

#[test]
Expand All @@ -408,14 +404,11 @@ mod tests {
activate_all_features(&mut genesis_config);

let bank0 = Arc::new(Bank::new_for_tests(&genesis_config));
let mut bank = Bank::new_from_parent(bank0, &Pubkey::default(), 1);
let bank = Bank::new_from_parent(bank0, &Pubkey::default(), 1);
while !bank.is_complete() {
bank.fill_bank_with_ticks_for_tests();
}

// Set extra field
bank.fee_rate_governor.lamports_per_signature = 7000;

let (_tmp_dir, accounts_dir) = create_tmp_accounts_dir_for_tests();
let bank_snapshots_dir = TempDir::new().unwrap();
let full_snapshot_archives_dir = TempDir::new().unwrap();
Expand All @@ -433,7 +426,7 @@ mod tests {
.unwrap();

// Deserialize
let (dbank, _) = snapshot_bank_utils::bank_from_snapshot_archives(
let (_dbank, _) = snapshot_bank_utils::bank_from_snapshot_archives(
&[accounts_dir],
bank_snapshots_dir.path(),
&snapshot_archive_info,
Expand All @@ -452,11 +445,6 @@ mod tests {
Arc::default(),
)
.unwrap();

assert_eq!(
bank.fee_rate_governor.lamports_per_signature,
dbank.fee_rate_governor.lamports_per_signature
);
}

#[test_case(StorageAccess::Mmap)]
Expand All @@ -467,7 +455,7 @@ mod tests {

let bank0 = Arc::new(Bank::new_for_tests(&genesis_config));
bank0.squash();
let mut bank = Bank::new_from_parent(bank0.clone(), &Pubkey::default(), 1);
let bank = Bank::new_from_parent(bank0.clone(), &Pubkey::default(), 1);
bank.freeze();
add_root_and_flush_write_cache(&bank0);
bank.rc
Expand All @@ -479,9 +467,6 @@ mod tests {
(AccountsHash(Hash::new_unique()), u64::default()),
);

// Set extra fields
bank.fee_rate_governor.lamports_per_signature = 7000;

// Serialize, but don't serialize the extra fields
let snapshot_storages = bank.get_snapshot_storages(None);
let mut buf = vec![];
Expand Down Expand Up @@ -525,9 +510,6 @@ mod tests {
)
.unwrap();

// Defaults to 0
assert_eq!(0, dbank.fee_rate_governor.lamports_per_signature);

// The snapshot epoch_reward_status always equals `None`, so the bank
// field should default to `Inactive`
assert_eq!(dbank.epoch_reward_status, EpochRewardStatus::Inactive);
Expand Down Expand Up @@ -603,7 +585,8 @@ mod tests {
AccountsHash(Hash::new_unique()),
&get_storages_to_serialize(&snapshot_storages),
ExtraFieldsToSerialize {
lamports_per_signature: bank.fee_rate_governor.lamports_per_signature,
lamports_per_signature: solana_sdk::fee_calculator::FeeRateGovernor::default()
.lamports_per_signature,
Copy link
Copy Markdown
Author

@tao-stones tao-stones Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

bank is created as default_for_tests(), effectively same as FeeRateGovernor::default().

Copy link
Copy Markdown

@apfitzge apfitzge Mar 21, 2025

Choose a reason for hiding this comment

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

do we not have this constant anywhere else? if we don't maybe we just add it in fee crate instead of continue to use FeeRateGovernor code.

Copy link
Copy Markdown

@brooksprumo brooksprumo Mar 21, 2025

Choose a reason for hiding this comment

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

The value doesn't matter for this test, we can just write a constant here too.

lamports_per_signature: 123, // actual value is not important

Comment on lines +588 to +589
Copy link
Copy Markdown

@brooksprumo brooksprumo Mar 21, 2025

Choose a reason for hiding this comment

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

Please import FeeRateGovernor at the top of this module.

Suggested change
lamports_per_signature: solana_sdk::fee_calculator::FeeRateGovernor::default()
.lamports_per_signature,
// at the top of the module: use solana_sdk::fee_calculator::FeeRateGovernor
lamports_per_signature: FeeRateGovernor::default().lamports_per_signature,

Or alternatively, use bank.last_blockhash_and_lamports_per_signature() instead. The actual values aren't important for this "test", just the types.

incremental_snapshot_persistence: Some(&incremental_snapshot_persistence),
epoch_accounts_hash: Some(EpochAccountsHash::new(Hash::new_unique())),
versioned_epoch_stakes,
Expand Down
21 changes: 1 addition & 20 deletions runtime/src/bank/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4180,8 +4180,7 @@ fn test_bank_epoch_vote_accounts() {
fn test_zero_signatures() {
solana_logger::setup();
let (genesis_config, mint_keypair) = create_genesis_config(500);
let mut bank = Bank::new_for_tests(&genesis_config);
bank.fee_rate_governor.lamports_per_signature = 2;
Copy link
Copy Markdown
Author

@tao-stones tao-stones Mar 20, 2025

Choose a reason for hiding this comment

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

this line was unnecessary at first place

let bank = Bank::new_for_tests(&genesis_config);
let key = solana_pubkey::new_rand();

let mut transfer_instruction = system_instruction::transfer(&mint_keypair.pubkey(), &key, 0);
Expand Down Expand Up @@ -4317,24 +4316,6 @@ fn test_bank_inherit_tx_count() {
assert_eq!(bank6.non_vote_transaction_count_since_restart(), 1);
}

#[test]
Copy link
Copy Markdown
Author

@tao-stones tao-stones Mar 20, 2025

Choose a reason for hiding this comment

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

this test is obsoleted

fn test_bank_inherit_fee_rate_governor() {
let (mut genesis_config, _mint_keypair) = create_genesis_config(500);
genesis_config
.fee_rate_governor
.target_lamports_per_signature = 123;

let bank0 = Arc::new(Bank::new_for_tests(&genesis_config));
let bank1 = Arc::new(new_from_parent(bank0.clone()));
assert_eq!(
bank0.fee_rate_governor.target_lamports_per_signature / 2,
bank1
.fee_rate_governor
.create_fee_calculator()
.lamports_per_signature
);
}

#[test]
fn test_bank_vote_accounts() {
let GenesisConfigInfo {
Expand Down