From 670e9824e0587c22941151f273ee86986f23a431 Mon Sep 17 00:00:00 2001 From: "Jeff Washington (jwash)" Date: Mon, 2 Jan 2023 09:51:08 -0600 Subject: [PATCH] feature: set rent_epoch to Epoch::MAX (#28690) * check android builds * feature: set rent_epoch to Epoch::MAX * tweaks * Update runtime/src/rent_collector.rs Co-authored-by: Brooks Prumo * simplify changes to tests * back out some test changes * calculate_rent_result passes through Exempt * move calc outside loop * if rent epoch is already max, use 'NoRentCollectionNow' Co-authored-by: Brooks Prumo --- programs/sbf/c/src/invoked/invoked.c | 6 +- programs/sbf/rust/invoked/src/processor.rs | 6 +- runtime/src/accounts.rs | 3 + runtime/src/bank.rs | 20 +- runtime/src/rent_collector.rs | 338 ++++++++++++--------- sdk/src/feature_set.rs | 5 + 6 files changed, 226 insertions(+), 152 deletions(-) diff --git a/programs/sbf/c/src/invoked/invoked.c b/programs/sbf/c/src/invoked/invoked.c index 940f67e600af24..f57896c47bf44c 100644 --- a/programs/sbf/c/src/invoked/invoked.c +++ b/programs/sbf/c/src/invoked/invoked.c @@ -45,7 +45,7 @@ extern uint64_t entrypoint(const uint8_t *input) { sol_assert(accounts[ARGUMENT_INDEX].data_len == 100); sol_assert(accounts[ARGUMENT_INDEX].is_signer); sol_assert(accounts[ARGUMENT_INDEX].is_writable); - sol_assert(accounts[ARGUMENT_INDEX].rent_epoch == 0); + sol_assert(accounts[ARGUMENT_INDEX].rent_epoch == UINT64_MAX); sol_assert(!accounts[ARGUMENT_INDEX].executable); for (int i = 0; i < accounts[ARGUMENT_INDEX].data_len; i++) { sol_assert(accounts[ARGUMENT_INDEX].data[i] == i); @@ -57,7 +57,7 @@ extern uint64_t entrypoint(const uint8_t *input) { sol_assert(accounts[INVOKED_ARGUMENT_INDEX].data_len == 10); sol_assert(accounts[INVOKED_ARGUMENT_INDEX].is_signer); sol_assert(accounts[INVOKED_ARGUMENT_INDEX].is_writable); - sol_assert(accounts[INVOKED_ARGUMENT_INDEX].rent_epoch == 0); + sol_assert(accounts[INVOKED_ARGUMENT_INDEX].rent_epoch == UINT64_MAX); sol_assert(!accounts[INVOKED_ARGUMENT_INDEX].executable); sol_assert( @@ -66,7 +66,7 @@ extern uint64_t entrypoint(const uint8_t *input) { &sbf_loader_id)); sol_assert(!accounts[INVOKED_PROGRAM_INDEX].is_signer); sol_assert(!accounts[INVOKED_PROGRAM_INDEX].is_writable); - sol_assert(accounts[INVOKED_PROGRAM_INDEX].rent_epoch == 0); + sol_assert(accounts[INVOKED_PROGRAM_INDEX].rent_epoch == UINT64_MAX); sol_assert(accounts[INVOKED_PROGRAM_INDEX].executable); sol_assert(SolPubkey_same(accounts[INVOKED_PROGRAM_INDEX].key, diff --git a/programs/sbf/rust/invoked/src/processor.rs b/programs/sbf/rust/invoked/src/processor.rs index f1af6d2f4a7bca..aa4d2fe801730d 100644 --- a/programs/sbf/rust/invoked/src/processor.rs +++ b/programs/sbf/rust/invoked/src/processor.rs @@ -48,7 +48,7 @@ fn process_instruction( assert_eq!(accounts[ARGUMENT_INDEX].data_len(), 100); assert!(accounts[ARGUMENT_INDEX].is_signer); assert!(accounts[ARGUMENT_INDEX].is_writable); - assert_eq!(accounts[ARGUMENT_INDEX].rent_epoch, 0); + assert_eq!(accounts[ARGUMENT_INDEX].rent_epoch, u64::MAX); assert!(!accounts[ARGUMENT_INDEX].executable); { let data = accounts[ARGUMENT_INDEX].try_borrow_data()?; @@ -65,14 +65,14 @@ fn process_instruction( assert_eq!(accounts[INVOKED_ARGUMENT_INDEX].data_len(), 10); assert!(accounts[INVOKED_ARGUMENT_INDEX].is_signer); assert!(accounts[INVOKED_ARGUMENT_INDEX].is_writable); - assert_eq!(accounts[INVOKED_ARGUMENT_INDEX].rent_epoch, 0); + assert_eq!(accounts[INVOKED_ARGUMENT_INDEX].rent_epoch, u64::MAX); assert!(!accounts[INVOKED_ARGUMENT_INDEX].executable); assert_eq!(accounts[INVOKED_PROGRAM_INDEX].key, program_id); assert_eq!(accounts[INVOKED_PROGRAM_INDEX].owner, &bpf_loader::id()); assert!(!accounts[INVOKED_PROGRAM_INDEX].is_signer); assert!(!accounts[INVOKED_PROGRAM_INDEX].is_writable); - assert_eq!(accounts[INVOKED_PROGRAM_INDEX].rent_epoch, 0); + assert_eq!(accounts[INVOKED_PROGRAM_INDEX].rent_epoch, u64::MAX); assert!(accounts[INVOKED_PROGRAM_INDEX].executable); assert_eq!( diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index a6228f5e3eb1eb..3477a90b09a581 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -275,6 +275,8 @@ impl Accounts { }; let mut accumulated_accounts_data_size: usize = 0; + let set_exempt_rent_epoch_max = + feature_set.is_active(&solana_sdk::feature_set::set_exempt_rent_epoch_max::id()); let mut accounts = account_keys .iter() .enumerate() @@ -309,6 +311,7 @@ impl Accounts { key, &mut account, self.accounts_db.filler_account_suffix.as_ref(), + set_exempt_rent_epoch_max, ) .rent_amount; (account, rent_due) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index bd4374d0f8476f..140d05c0cd905a 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -5338,12 +5338,16 @@ impl Bank { let mut time_hashing_skipped_rewrites_us = 0; let mut time_storing_accounts_us = 0; let can_skip_rewrites = self.rc.accounts.accounts_db.skip_rewrites; + let set_exempt_rent_epoch_max: bool = self + .feature_set + .is_active(&solana_sdk::feature_set::set_exempt_rent_epoch_max::id()); for (pubkey, account, _loaded_slot) in accounts.iter_mut() { let (rent_collected_info, measure) = measure!(self.rent_collector.collect_from_existing_account( pubkey, account, self.rc.accounts.accounts_db.filler_account_suffix.as_ref(), + set_exempt_rent_epoch_max, )); time_collecting_rent_us += measure.as_us(); @@ -7974,11 +7978,12 @@ pub(crate) mod tests { ancestors::Ancestors, bank_client::BankClient, genesis_utils::{ - self, activate_all_features, bootstrap_validator_stake_lamports, + self, activate_all_features, activate_feature, bootstrap_validator_stake_lamports, create_genesis_config_with_leader, create_genesis_config_with_vote_accounts, genesis_sysvar_and_builtin_program_lamports, GenesisConfigInfo, ValidatorVoteKeypairs, }, + rent_collector::TEST_SET_EXEMPT_RENT_EPOCH_MAX, rent_paying_accounts_by_partition::RentPayingAccountsByPartition, status_cache::MAX_CACHE_ENTRIES, }, @@ -8436,6 +8441,7 @@ pub(crate) mod tests { &keypairs[4].pubkey(), &mut account_copy, None, + TEST_SET_EXEMPT_RENT_EPOCH_MAX, ); assert_eq!(expected_rent.rent_amount, too_few_lamports); assert_eq!(account_copy.lamports(), 0); @@ -9900,12 +9906,15 @@ pub(crate) mod tests { solana_logger::setup(); let (mut genesis_config, _mint_keypair) = create_genesis_config(1_000_000); - activate_all_features(&mut genesis_config); + for feature_id in FeatureSet::default().inactive { + if feature_id != solana_sdk::feature_set::set_exempt_rent_epoch_max::id() { + activate_feature(&mut genesis_config, feature_id); + } + } let zero_lamport_pubkey = solana_sdk::pubkey::new_rand(); let rent_due_pubkey = solana_sdk::pubkey::new_rand(); let rent_exempt_pubkey = solana_sdk::pubkey::new_rand(); - let mut bank = Arc::new(Bank::new_for_tests(&genesis_config)); let zero_lamports = 0; let little_lamports = 1234; @@ -11942,7 +11951,9 @@ pub(crate) mod tests { let mut expected_next_slot = expected_previous_slot + 1; // First, initialize the clock sysvar - activate_all_features(&mut genesis_config); + for feature_id in FeatureSet::default().inactive { + activate_feature(&mut genesis_config, feature_id); + } let bank1 = Arc::new(Bank::new_for_tests_with_config( &genesis_config, BankTestConfig::default(), @@ -20033,6 +20044,7 @@ pub(crate) mod tests { &keypair.pubkey(), &mut account, None, + TEST_SET_EXEMPT_RENT_EPOCH_MAX, ); assert_eq!(info.account_data_len_reclaimed, data_size as u64); } diff --git a/runtime/src/rent_collector.rs b/runtime/src/rent_collector.rs index 5077e8bba4f29a..3794805a4669d0 100644 --- a/runtime/src/rent_collector.rs +++ b/runtime/src/rent_collector.rs @@ -29,6 +29,13 @@ impl Default for RentCollector { } } +/// When rent is collected from an exempt account, rent_epoch is set to this +/// value. The idea is to have a fixed, consistent value for rent_epoch for all accounts that do not collect rent. +/// This enables us to get rid of the field completely. +pub const RENT_EXEMPT_RENT_EPOCH: Epoch = Epoch::MAX; + +pub const TEST_SET_EXEMPT_RENT_EPOCH_MAX: bool = false; + /// when rent is collected for this account, this is the action to apply to the account #[derive(Debug)] enum RentResult { @@ -111,9 +118,16 @@ impl RentCollector { address: &Pubkey, account: &mut AccountSharedData, filler_account_suffix: Option<&Pubkey>, + set_exempt_rent_epoch_max: bool, ) -> CollectedInfo { match self.calculate_rent_result(address, account, filler_account_suffix) { - RentResult::Exempt | RentResult::NoRentCollectionNow => CollectedInfo::default(), + RentResult::Exempt => { + if set_exempt_rent_epoch_max { + account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH); + } + CollectedInfo::default() + } + RentResult::NoRentCollectionNow => CollectedInfo::default(), RentResult::CollectRent { new_rent_epoch, rent_due, @@ -145,8 +159,8 @@ impl RentCollector { account: &impl ReadableAccount, filler_account_suffix: Option<&Pubkey>, ) -> RentResult { - if account.rent_epoch() > self.epoch { - // potentially rent paying account + if account.rent_epoch() == RENT_EXEMPT_RENT_EPOCH || account.rent_epoch() > self.epoch { + // potentially rent paying account (or known and already marked exempt) // Maybe collect rent later, leave account alone for now. return RentResult::NoRentCollectionNow; } @@ -221,166 +235,201 @@ mod tests { // initialize rent_epoch as created at this epoch account.set_rent_epoch(self.epoch); self.collect_from_existing_account( - address, account, /*filler_account_suffix:*/ None, + address, + account, + /*filler_account_suffix:*/ None, + TEST_SET_EXEMPT_RENT_EPOCH_MAX, ) } } #[test] fn test_calculate_rent_result() { - let mut rent_collector = RentCollector::default(); - - let mut account = AccountSharedData::default(); - assert!(matches!( - rent_collector.calculate_rent_result(&Pubkey::default(), &account, None), - RentResult::NoRentCollectionNow, - )); - { - let mut account_clone = account.clone(); - assert_eq!( - rent_collector.collect_from_existing_account( - &Pubkey::default(), - &mut account_clone, - None - ), - CollectedInfo::default() - ); - assert_eq!(account_clone, account); - } - - account.set_executable(true); - assert!(matches!( - rent_collector.calculate_rent_result(&Pubkey::default(), &account, None), - RentResult::Exempt - )); - { - let mut account_clone = account.clone(); - assert_eq!( - rent_collector.collect_from_existing_account( - &Pubkey::default(), - &mut account_clone, - None - ), - CollectedInfo::default() - ); - assert_eq!(account_clone, account); - } + for set_exempt_rent_epoch_max in [false, true] { + let mut rent_collector = RentCollector::default(); + + let mut account = AccountSharedData::default(); + assert!(matches!( + rent_collector.calculate_rent_result(&Pubkey::default(), &account, None,), + RentResult::NoRentCollectionNow, + )); + { + let mut account_clone = account.clone(); + assert_eq!( + rent_collector.collect_from_existing_account( + &Pubkey::default(), + &mut account_clone, + None, + set_exempt_rent_epoch_max + ), + CollectedInfo::default() + ); + assert_eq!(account_clone, account); + } - account.set_executable(false); - assert!(matches!( - rent_collector.calculate_rent_result(&incinerator::id(), &account, None), - RentResult::Exempt - )); - { - let mut account_clone = account.clone(); - assert_eq!( - rent_collector.collect_from_existing_account( - &incinerator::id(), - &mut account_clone, - None - ), - CollectedInfo::default() - ); - assert_eq!(account_clone, account); - } + account.set_executable(true); + assert!(matches!( + rent_collector.calculate_rent_result(&Pubkey::default(), &account, None,), + RentResult::Exempt + )); + { + let mut account_clone = account.clone(); + let mut account_expected = account.clone(); + if set_exempt_rent_epoch_max { + account_expected.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH); + } + assert_eq!( + rent_collector.collect_from_existing_account( + &Pubkey::default(), + &mut account_clone, + None, + set_exempt_rent_epoch_max + ), + CollectedInfo::default() + ); + assert_eq!(account_clone, account_expected); + } - // try a few combinations of rent collector rent epoch and collecting rent with and without filler accounts specified (but we aren't a filler) - let filler_account = solana_sdk::pubkey::new_rand(); - - for filler_accounts in [None, Some(&filler_account)] { - for (rent_epoch, rent_due_expected) in [(2, 2), (3, 5)] { - rent_collector.epoch = rent_epoch; - account.set_lamports(10); - account.set_rent_epoch(1); - let new_rent_epoch_expected = rent_collector.epoch + 1; - assert!( - matches!( - rent_collector.calculate_rent_result(&Pubkey::default(), &account, filler_accounts), - RentResult::CollectRent{ new_rent_epoch, rent_due} if new_rent_epoch == new_rent_epoch_expected && rent_due == rent_due_expected, + account.set_executable(false); + assert!(matches!( + rent_collector.calculate_rent_result(&incinerator::id(), &account, None,), + RentResult::Exempt + )); + { + let mut account_clone = account.clone(); + let mut account_expected = account.clone(); + if set_exempt_rent_epoch_max { + account_expected.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH); + } + assert_eq!( + rent_collector.collect_from_existing_account( + &incinerator::id(), + &mut account_clone, + None, + set_exempt_rent_epoch_max ), - "{:?}", - rent_collector.calculate_rent_result(&Pubkey::default(), &account, None) + CollectedInfo::default() ); + assert_eq!(account_clone, account_expected); + } - { - let mut account_clone = account.clone(); - assert_eq!( - rent_collector.collect_from_existing_account( - &Pubkey::default(), - &mut account_clone, - filler_accounts + // try a few combinations of rent collector rent epoch and collecting rent with and without filler accounts specified (but we aren't a filler) + let filler_account = solana_sdk::pubkey::new_rand(); + + for filler_accounts in [None, Some(&filler_account)] { + for (rent_epoch, rent_due_expected) in [(2, 2), (3, 5)] { + rent_collector.epoch = rent_epoch; + account.set_lamports(10); + account.set_rent_epoch(1); + let new_rent_epoch_expected = rent_collector.epoch + 1; + assert!( + matches!( + rent_collector.calculate_rent_result(&Pubkey::default(), &account, filler_accounts), + RentResult::CollectRent{ new_rent_epoch, rent_due} if new_rent_epoch == new_rent_epoch_expected && rent_due == rent_due_expected, ), - CollectedInfo { - rent_amount: rent_due_expected, - account_data_len_reclaimed: 0 - } + "{:?}", + rent_collector.calculate_rent_result(&Pubkey::default(), &account, None,) ); - let mut account_expected = account.clone(); - account_expected.set_lamports(account.lamports() - rent_due_expected); - account_expected.set_rent_epoch(new_rent_epoch_expected); - assert_eq!(account_clone, account_expected); + + { + let mut account_clone = account.clone(); + assert_eq!( + rent_collector.collect_from_existing_account( + &Pubkey::default(), + &mut account_clone, + filler_accounts, + set_exempt_rent_epoch_max + ), + CollectedInfo { + rent_amount: rent_due_expected, + account_data_len_reclaimed: 0 + } + ); + let mut account_expected = account.clone(); + account_expected.set_lamports(account.lamports() - rent_due_expected); + account_expected.set_rent_epoch(new_rent_epoch_expected); + assert_eq!(account_clone, account_expected); + } } } - } - // enough lamports to make us exempt - account.set_lamports(1_000_000); - assert!(matches!( - rent_collector.calculate_rent_result(&Pubkey::default(), &account, None), - RentResult::Exempt, - )); - { - let mut account_clone = account.clone(); - assert_eq!( - rent_collector.collect_from_existing_account( - &Pubkey::default(), - &mut account_clone, - None - ), - CollectedInfo::default() + // enough lamports to make us exempt + account.set_lamports(1_000_000); + let result = rent_collector.calculate_rent_result(&Pubkey::default(), &account, None); + assert!( + matches!(result, RentResult::Exempt), + "{:?}, set_exempt_rent_epoch_max: {}", + result, + set_exempt_rent_epoch_max, ); - assert_eq!(account_clone, account); - } + { + let mut account_clone = account.clone(); + let mut account_expected = account.clone(); + if set_exempt_rent_epoch_max { + account_expected.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH); + } + assert_eq!( + rent_collector.collect_from_existing_account( + &Pubkey::default(), + &mut account_clone, + None, + set_exempt_rent_epoch_max + ), + CollectedInfo::default() + ); + assert_eq!(account_clone, account_expected); + } - // enough lamports to make us exempt - // but, our rent_epoch is set in the future, so we can't know if we are exempt yet or not. - // We don't calculate rent amount vs data if the rent_epoch is already in the future. - account.set_rent_epoch(1_000_000); - assert!(matches!( - rent_collector.calculate_rent_result(&Pubkey::default(), &account, None), - RentResult::NoRentCollectionNow, - )); - { - let mut account_clone = account.clone(); - assert_eq!( - rent_collector.collect_from_existing_account( - &Pubkey::default(), - &mut account_clone, - None - ), - CollectedInfo::default() - ); - assert_eq!(account_clone, account); - } + // enough lamports to make us exempt + // but, our rent_epoch is set in the future, so we can't know if we are exempt yet or not. + // We don't calculate rent amount vs data if the rent_epoch is already in the future. + account.set_rent_epoch(1_000_000); + assert!(matches!( + rent_collector.calculate_rent_result(&Pubkey::default(), &account, None,), + RentResult::NoRentCollectionNow, + )); + { + let mut account_clone = account.clone(); + assert_eq!( + rent_collector.collect_from_existing_account( + &Pubkey::default(), + &mut account_clone, + None, + set_exempt_rent_epoch_max + ), + CollectedInfo::default() + ); + assert_eq!(account_clone, account); + } - // filler accounts are exempt - account.set_rent_epoch(1); - account.set_lamports(10); - assert!(matches!( - rent_collector.calculate_rent_result(&filler_account, &account, Some(&filler_account)), - RentResult::Exempt, - )); - { - let mut account_clone = account.clone(); - assert_eq!( - rent_collector.collect_from_existing_account( + // filler accounts are exempt + account.set_rent_epoch(1); + account.set_lamports(10); + assert!(matches!( + rent_collector.calculate_rent_result( &filler_account, - &mut account_clone, - Some(&filler_account) + &account, + Some(&filler_account), ), - CollectedInfo::default() - ); - assert_eq!(account_clone, account); + RentResult::Exempt, + )); + { + let mut account_clone = account.clone(); + let mut account_expected = account.clone(); + if set_exempt_rent_epoch_max { + account_expected.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH); + } + assert_eq!( + rent_collector.collect_from_existing_account( + &filler_account, + &mut account_clone, + Some(&filler_account), + set_exempt_rent_epoch_max + ), + CollectedInfo::default() + ); + assert_eq!(account_clone, account_expected); + } } } @@ -418,6 +467,7 @@ mod tests { &solana_sdk::pubkey::new_rand(), &mut existing_account, None, // filler_account_suffix + TEST_SET_EXEMPT_RENT_EPOCH_MAX, ); assert!(existing_account.lamports() < old_lamports); assert_eq!( @@ -451,6 +501,7 @@ mod tests { &pubkey, &mut account, None, // filler_account_suffix + TEST_SET_EXEMPT_RENT_EPOCH_MAX, ); assert_eq!(account.lamports(), huge_lamports); assert_eq!(collected, CollectedInfo::default()); @@ -463,6 +514,7 @@ mod tests { &pubkey, &mut account, None, // filler_account_suffix + TEST_SET_EXEMPT_RENT_EPOCH_MAX, ); assert_eq!(account.lamports(), tiny_lamports - collected.rent_amount); assert_ne!(collected, CollectedInfo::default()); @@ -486,6 +538,7 @@ mod tests { &pubkey, &mut account, None, // filler_account_suffix + TEST_SET_EXEMPT_RENT_EPOCH_MAX, ); assert_eq!(account.lamports(), 0); assert_eq!(collected.rent_amount, 1); @@ -510,6 +563,7 @@ mod tests { &Pubkey::new_unique(), &mut account, None, // filler_account_suffix + TEST_SET_EXEMPT_RENT_EPOCH_MAX, ); assert_eq!(collected.rent_amount, account_lamports); diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 228dc56a0559da..12541a44d79047 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -498,6 +498,10 @@ pub mod account_hash_ignore_slot { solana_sdk::declare_id!("SVn36yVApPLYsa8koK3qUcy14zXDnqkNYWyUh1f4oK1"); } +pub mod set_exempt_rent_epoch_max { + solana_sdk::declare_id!("5wAGiy15X1Jb2hkHnPDCM8oB9V42VNA9ftNVFK84dEgv"); +} + pub mod relax_authority_signer_check_for_lookup_table_creation { solana_sdk::declare_id!("FKAcEvNgSY79RpqsPNUV5gDyumopH4cEHqUxyfm8b8Ap"); } @@ -678,6 +682,7 @@ lazy_static! { (enable_early_verification_of_account_modifications::id(), "enable early verification of account modifications #25899"), (disable_rehash_for_rent_epoch::id(), "on accounts hash calculation, do not try to rehash accounts #28934"), (account_hash_ignore_slot::id(), "ignore slot when calculating an account hash #28420"), + (set_exempt_rent_epoch_max::id(), "set rent epoch to Epoch::MAX for rent-exempt accounts #28683"), (on_load_preserve_rent_epoch_for_rent_exempt_accounts::id(), "on bank load account, do not try to fix up rent_epoch #28541"), (prevent_crediting_accounts_that_end_rent_paying::id(), "prevent crediting rent paying accounts #26606"), (cap_bpf_program_instruction_accounts::id(), "enforce max number of accounts per bpf program instruction #26628"),