Refactor remove fee rate governor from bank #5388
Conversation
| assert_eq!( | ||
| bank.fee_rate_governor.lamports_per_signature, | ||
| dbank.fee_rate_governor.lamports_per_signature | ||
| ); |
There was a problem hiding this comment.
does removing fee_rate_governor as extra modified fields break the test integrity? wdyt @brooksprumo
| @@ -4317,24 +4316,6 @@ fn test_bank_inherit_tx_count() { | |||
| assert_eq!(bank6.non_vote_transaction_count_since_restart(), 1); | |||
| } | |||
|
|
|||
| #[test] | |||
| @@ -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; | |||
There was a problem hiding this comment.
this line was unnecessary at first place
tao-stones
force-pushed
the
refactor-remove-fee-rate-governor-from-bank
branch
from
1c57657 to
24be257
Compare
| @@ -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, | |||
There was a problem hiding this comment.
bank is created as default_for_tests(), effectively same as FeeRateGovernor::default().
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
tao-stones
force-pushed
the
refactor-remove-fee-rate-governor-from-bank
branch
from
24be257 to
4acf88a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5388 +/- ##
=======================================
Coverage 83.3% 83.3%
=======================================
Files 818 818
Lines 371732 371664 -68
=======================================
- Hits 309938 309889 -49
+ Misses 61794 61775 -19 🚀 New features to boost your workflow:
|
| // Bank no longer has fee-rate-governor, it can be removed from snapshot in followup | ||
| // refactoring. | ||
| fee_rate_governor: FeeRateGovernor::default(), |
There was a problem hiding this comment.
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).
| // Bank no longer has fee-rate-governor, it can be removed from snapshot in followup | |
| // refactoring. | |
| fee_rate_governor: FeeRateGovernor::default(), |
There was a problem hiding this comment.
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 +?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
we have not needed it since before I started working on solana 😆
| lamports_per_signature: solana_sdk::fee_calculator::FeeRateGovernor::default() | ||
| .lamports_per_signature, |
There was a problem hiding this comment.
Please import FeeRateGovernor at the top of this module.
| 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.
Problem
How
bankchecksis_zero_fees_for_test, and howbankburns transaction fee have been refactored in previous PRs that contribute to #3303,bank.fee_rate_governorhas become obsoleted.Summary of Changes
fee_rate_governorfrombank.Note: this PR does not remove
fee_rate_governorfrom snapshot, which can be done in followup PRFixes #