From c997030197451839874dd37a660f35ed8e1a991c Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Fri, 31 May 2024 12:43:24 -0500 Subject: [PATCH] refactor: collect rent from account (#1527) --- runtime/src/bank.rs | 36 +++------- svm/src/account_loader.rs | 140 ++++++++++++++++++++++++++++++-------- 2 files changed, 124 insertions(+), 52 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index e605c1ef605bef..ecee9f9de3be8e 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -140,8 +140,7 @@ use { packet::PACKET_DATA_SIZE, precompiles::get_precompiles, pubkey::Pubkey, - rent::RentDue, - rent_collector::{CollectedInfo, RentCollector, RENT_EXEMPT_RENT_EPOCH}, + rent_collector::{CollectedInfo, RentCollector}, rent_debits::RentDebits, reserved_account_keys::ReservedAccountKeys, reward_info::RewardInfo, @@ -165,7 +164,8 @@ use { }, solana_svm::{ account_loader::{ - CheckedTransactionDetails, TransactionCheckResult, TransactionLoadResult, + collect_rent_from_account, CheckedTransactionDetails, TransactionCheckResult, + TransactionLoadResult, }, account_overrides::AccountOverrides, nonce_info::NoncePartial, @@ -4422,28 +4422,14 @@ impl Bank { .test_skip_rewrites_but_include_in_bank_hash; let mut skipped_rewrites = Vec::default(); for (pubkey, account, _loaded_slot) in accounts.iter_mut() { - let rent_collected_info = if self.should_collect_rent() { - let (rent_collected_info, measure) = measure!(self - .rent_collector - .collect_from_existing_account(pubkey, account)); - time_collecting_rent_us += measure.as_us(); - rent_collected_info - } else { - // When rent fee collection is disabled, we won't collect rent for any account. If there - // are any rent paying accounts, their `rent_epoch` won't change either. However, if the - // account itself is rent-exempted but its `rent_epoch` is not u64::MAX, we will set its - // `rent_epoch` to u64::MAX. In such case, the behavior stays the same as before. - if account.rent_epoch() != RENT_EXEMPT_RENT_EPOCH - && self.rent_collector.get_rent_due( - account.lamports(), - account.data().len(), - account.rent_epoch(), - ) == RentDue::Exempt - { - account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH); - } - CollectedInfo::default() - }; + let (rent_collected_info, measure) = measure!(collect_rent_from_account( + &self.feature_set, + &self.rent_collector, + pubkey, + account + )); + time_collecting_rent_us += measure.as_us(); + // only store accounts where we collected rent // but get the hash for all these accounts even if collected rent is 0 (= not updated). // Also, there's another subtle side-effect from rewrites: this diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 81f76dd0d10341..c97941d2f2aeff 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -15,7 +15,7 @@ use { account::{Account, AccountSharedData, ReadableAccount, WritableAccount}, feature_set::{ self, include_loaded_accounts_data_size_in_fee_calculation, - remove_rounding_in_fee_calculation, + remove_rounding_in_fee_calculation, FeatureSet, }, fee::{FeeDetails, FeeStructure}, message::SanitizedMessage, @@ -23,7 +23,7 @@ use { nonce::State as NonceState, pubkey::Pubkey, rent::RentDue, - rent_collector::{RentCollector, RENT_EXEMPT_RENT_EPOCH}, + rent_collector::{CollectedInfo, RentCollector, RENT_EXEMPT_RENT_EPOCH}, rent_debits::RentDebits, saturating_add_assign, sysvar::{self, instructions::construct_instructions_data}, @@ -63,6 +63,36 @@ impl LoadedTransaction { } } +/// Collect rent from an account if rent is still enabled and regardless of +/// whether rent is enabled, set the rent epoch to u64::MAX if the account is +/// rent exempt. +pub fn collect_rent_from_account( + feature_set: &FeatureSet, + rent_collector: &RentCollector, + address: &Pubkey, + account: &mut AccountSharedData, +) -> CollectedInfo { + if !feature_set.is_active(&feature_set::disable_rent_fees_collection::id()) { + rent_collector.collect_from_existing_account(address, account) + } else { + // When rent fee collection is disabled, we won't collect rent for any account. If there + // are any rent paying accounts, their `rent_epoch` won't change either. However, if the + // account itself is rent-exempted but its `rent_epoch` is not u64::MAX, we will set its + // `rent_epoch` to u64::MAX. In such case, the behavior stays the same as before. + if account.rent_epoch() != RENT_EXEMPT_RENT_EPOCH + && rent_collector.get_rent_due( + account.lamports(), + account.data().len(), + account.rent_epoch(), + ) == RentDue::Exempt + { + account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH); + } + + CollectedInfo::default() + } +} + /// Check whether the payer_account is capable of paying the fee. The /// side effect is to subtract the fee amount from the payer_account /// balance of lamports. If the payer_acount is not able to pay the @@ -233,30 +263,15 @@ fn load_transaction_accounts( .get_account_shared_data(key) .map(|mut account| { if message.is_writable(i) { - if !feature_set - .is_active(&feature_set::disable_rent_fees_collection::id()) - { - let rent_due = rent_collector - .collect_from_existing_account(key, &mut account) - .rent_amount; - - (account.data().len(), account, rent_due) - } else { - // When rent fee collection is disabled, we won't collect rent for any account. If there - // are any rent paying accounts, their `rent_epoch` won't change either. However, if the - // account itself is rent-exempted but its `rent_epoch` is not u64::MAX, we will set its - // `rent_epoch` to u64::MAX. In such case, the behavior stays the same as before. - if account.rent_epoch() != RENT_EXEMPT_RENT_EPOCH - && rent_collector.get_rent_due( - account.lamports(), - account.data().len(), - account.rent_epoch(), - ) == RentDue::Exempt - { - account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH); - } - (account.data().len(), account, 0) - } + let rent_due = collect_rent_from_account( + &feature_set, + rent_collector, + key, + &mut account, + ) + .rent_amount; + + (account.data().len(), account, rent_due) } else { (account.data().len(), account, 0) } @@ -475,7 +490,7 @@ mod tests { prioritization_fee::{PrioritizationFeeDetails, PrioritizationFeeType}, }, solana_sdk::{ - account::{AccountSharedData, ReadableAccount, WritableAccount}, + account::{Account, AccountSharedData, ReadableAccount, WritableAccount}, bpf_loader_upgradeable, compute_budget::ComputeBudgetInstruction, epoch_schedule::EpochSchedule, @@ -2234,4 +2249,75 @@ mod tests { assert_eq!(result, vec![Err(TransactionError::InvalidWritableAccount)]); } + + #[test] + fn test_collect_rent_from_account() { + let feature_set = FeatureSet::all_enabled(); + let rent_collector = RentCollector { + epoch: 1, + ..RentCollector::default() + }; + + let address = Pubkey::new_unique(); + let min_exempt_balance = rent_collector.rent.minimum_balance(0); + let mut account = AccountSharedData::from(Account { + lamports: min_exempt_balance, + ..Account::default() + }); + + assert_eq!( + collect_rent_from_account(&feature_set, &rent_collector, &address, &mut account), + CollectedInfo::default() + ); + assert_eq!(account.rent_epoch(), RENT_EXEMPT_RENT_EPOCH); + } + + #[test] + fn test_collect_rent_from_account_rent_paying() { + let feature_set = FeatureSet::all_enabled(); + let rent_collector = RentCollector { + epoch: 1, + ..RentCollector::default() + }; + + let address = Pubkey::new_unique(); + let mut account = AccountSharedData::from(Account { + lamports: 1, + ..Account::default() + }); + + assert_eq!( + collect_rent_from_account(&feature_set, &rent_collector, &address, &mut account), + CollectedInfo::default() + ); + assert_eq!(account.rent_epoch(), 0); + assert_eq!(account.lamports(), 1); + } + + #[test] + fn test_collect_rent_from_account_rent_enabled() { + let feature_set = + all_features_except(Some(&[feature_set::disable_rent_fees_collection::id()])); + let rent_collector = RentCollector { + epoch: 1, + ..RentCollector::default() + }; + + let address = Pubkey::new_unique(); + let mut account = AccountSharedData::from(Account { + lamports: 1, + data: vec![0], + ..Account::default() + }); + + assert_eq!( + collect_rent_from_account(&feature_set, &rent_collector, &address, &mut account), + CollectedInfo { + rent_amount: 1, + account_data_len_reclaimed: 1 + } + ); + assert_eq!(account.rent_epoch(), 0); + assert_eq!(account.lamports(), 0); + } }