Skip to content

Commit

Permalink
refactor: collect rent from account (anza-xyz#1527)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jstarry
jstarry authored May 31, 2024
1 parent b4352f3 commit c997030
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 52 deletions.
36 changes: 11 additions & 25 deletions runtime/src/bank.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down
140 changes: 113 additions & 27 deletions svm/src/account_loader.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ 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,
native_loader,
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},
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -233,30 +263,15 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
.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)
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
}

0 comments on commit c997030

Please sign in to comment.