From d90d5ee9b6cab305b9e03b0ce12e9d09e695abb0 Mon Sep 17 00:00:00 2001 From: Brooks Prumo Date: Sat, 8 Jan 2022 09:03:46 -0600 Subject: [PATCH] Refactor Rent::due() with RentDue enum (#22346) --- cli/src/cluster_query.rs | 2 +- runtime/src/accounts_db.rs | 7 ++- runtime/src/bank.rs | 16 +++--- runtime/src/rent_collector.rs | 87 +++++++++++++++++--------------- sdk/program/src/rent.rs | 94 +++++++++++++++++------------------ sdk/src/account.rs | 28 +++++++++++ 6 files changed, 137 insertions(+), 97 deletions(-) diff --git a/cli/src/cluster_query.rs b/cli/src/cluster_query.rs index 66bc8e24a8..f72f651ed9 100644 --- a/cli/src/cluster_query.rs +++ b/cli/src/cluster_query.rs @@ -2128,7 +2128,7 @@ pub fn process_calculate_rent( timing::years_as_slots(1.0, &seconds_per_tick, clock::DEFAULT_TICKS_PER_SLOT); let slots_per_epoch = epoch_schedule.slots_per_epoch as f64; let years_per_epoch = slots_per_epoch / slots_per_year; - let (lamports_per_epoch, _) = rent.due(0, data_length, years_per_epoch); + let lamports_per_epoch = rent.due(0, data_length, years_per_epoch).lamports(); let cli_rent_calculation = CliRentCalculation { lamports_per_byte_year: rent.lamports_per_byte_year, lamports_per_epoch, diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index e0dba85124..60ca2b4bb3 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -6704,10 +6704,9 @@ impl AccountsDb { accounts_data_len += stored_account.data().len() as u64; } - if !rent_collector.should_collect_rent(&pubkey, &stored_account, false) || { - let (_rent_due, exempt) = rent_collector.get_rent_due(&stored_account); - exempt - } { + if !rent_collector.should_collect_rent(&pubkey, &stored_account, false) + || rent_collector.get_rent_due(&stored_account).is_exempt() + { num_accounts_rent_exempt += 1; } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index e36e098704..87f4c291d9 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -6293,7 +6293,7 @@ impl Bank { } if !rent_collector.should_collect_rent(pubkey, account, false) - || rent_collector.get_rent_due(account).1 + || rent_collector.get_rent_due(account).is_exempt() { total_accounts_stats.num_rent_exempt_accounts += 1; } else { @@ -7461,11 +7461,15 @@ pub(crate) mod tests { .get_slots_in_epoch(epoch + 1) }) .sum(); - let (generic_rent_due_for_system_account, _) = bank.rent_collector.rent.due( - bank.get_minimum_balance_for_rent_exemption(0) - 1, - 0, - slots_elapsed as f64 / bank.rent_collector.slots_per_year, - ); + let generic_rent_due_for_system_account = bank + .rent_collector + .rent + .due( + bank.get_minimum_balance_for_rent_exemption(0) - 1, + 0, + slots_elapsed as f64 / bank.rent_collector.slots_per_year, + ) + .lamports(); store_accounts_for_rent_test( &bank, diff --git a/runtime/src/rent_collector.rs b/runtime/src/rent_collector.rs index 2e51302b5c..976f3d8142 100644 --- a/runtime/src/rent_collector.rs +++ b/runtime/src/rent_collector.rs @@ -6,7 +6,7 @@ use solana_sdk::{ genesis_config::GenesisConfig, incinerator, pubkey::Pubkey, - rent::Rent, + rent::{Rent, RentDue}, sysvar, }; @@ -66,7 +66,7 @@ impl RentCollector { /// given an account that 'should_collect_rent' /// returns (amount rent due, is_exempt_from_rent) - pub fn get_rent_due(&self, account: &impl ReadableAccount) -> (u64, bool) { + pub fn get_rent_due(&self, account: &impl ReadableAccount) -> RentDue { let slots_elapsed: u64 = (account.rent_epoch()..=self.epoch) .map(|epoch| self.epoch_schedule.get_slots_in_epoch(epoch + 1)) .sum(); @@ -82,9 +82,9 @@ impl RentCollector { .due(account.lamports(), account.data().len(), years_elapsed) } - // updates this account's lamports and status and returns - // the account rent collected, if any - // This is NOT thread safe at some level. If we try to collect from the same account in parallel, we may collect twice. + // Updates the account's lamports and status, and returns the amount of rent collected, if any. + // This is NOT thread safe at some level. If we try to collect from the same account in + // parallel, we may collect twice. #[must_use = "add to Bank::collected_rent"] pub fn collect_from_existing_account( &self, @@ -93,42 +93,35 @@ impl RentCollector { rent_for_sysvars: bool, filler_account_suffix: Option<&Pubkey>, ) -> u64 { - if !self.should_collect_rent(address, account, rent_for_sysvars) - || account.rent_epoch() > self.epoch - || crate::accounts_db::AccountsDb::is_filler_account_helper( - address, - filler_account_suffix, - ) + if self.can_skip_rent_collection(address, account, rent_for_sysvars, filler_account_suffix) { - 0 - } else { - let (rent_due, exempt) = self.get_rent_due(account); - - if exempt || rent_due != 0 { - if account.lamports() > rent_due { - account.set_rent_epoch( - self.epoch - + if exempt { - // Rent isn't collected for the next epoch - // Make sure to check exempt status later in current epoch again - 0 - } else { - // Rent is collected for next epoch - 1 - }, - ); - let _ = account.checked_sub_lamports(rent_due); // will not fail. We check above. - rent_due - } else { - let rent_charged = account.lamports(); - *account = AccountSharedData::default(); - rent_charged - } - } else { - // maybe collect rent later, leave account alone - 0 - } + return 0; } + + let rent_due = self.get_rent_due(account); + if let RentDue::Paying(0) = rent_due { + // maybe collect rent later, leave account alone + return 0; + } + + let epoch_increment = match rent_due { + // Rent isn't collected for the next epoch + // Make sure to check exempt status again later in current epoch + RentDue::Exempt => 0, + // Rent is collected for next epoch + RentDue::Paying(_) => 1, + }; + account.set_rent_epoch(self.epoch + epoch_increment); + + let begin_lamports = account.lamports(); + account.saturating_sub_lamports(rent_due.lamports()); + let end_lamports = account.lamports(); + + if end_lamports == 0 { + *account = AccountSharedData::default(); + } + + begin_lamports - end_lamports } #[must_use = "add to Bank::collected_rent"] @@ -142,6 +135,22 @@ impl RentCollector { account.set_rent_epoch(self.epoch); self.collect_from_existing_account(address, account, rent_for_sysvars, None) } + + /// Performs easy checks to see if rent collection can be skipped + fn can_skip_rent_collection( + &self, + address: &Pubkey, + account: &mut AccountSharedData, + rent_for_sysvars: bool, + filler_account_suffix: Option<&Pubkey>, + ) -> bool { + !self.should_collect_rent(address, account, rent_for_sysvars) + || account.rent_epoch() > self.epoch + || crate::accounts_db::AccountsDb::is_filler_account_helper( + address, + filler_account_suffix, + ) + } } #[cfg(test)] diff --git a/sdk/program/src/rent.rs b/sdk/program/src/rent.rs index 7d47ffe2fc..502eef43ad 100644 --- a/sdk/program/src/rent.rs +++ b/sdk/program/src/rent.rs @@ -64,16 +64,13 @@ impl Rent { } /// rent due on account's data_len with balance - pub fn due(&self, balance: u64, data_len: usize, years_elapsed: f64) -> (u64, bool) { + pub fn due(&self, balance: u64, data_len: usize, years_elapsed: f64) -> RentDue { if self.is_exempt(balance, data_len) { - (0, true) + RentDue::Exempt } else { - ( - ((self.lamports_per_byte_year * (data_len as u64 + ACCOUNT_STORAGE_OVERHEAD)) - as f64 - * years_elapsed) as u64, - false, - ) + let actual_data_len = data_len as u64 + ACCOUNT_STORAGE_OVERHEAD; + let lamports_per_year = self.lamports_per_byte_year * actual_data_len; + RentDue::Paying((lamports_per_year as f64 * years_elapsed) as u64) } } @@ -96,6 +93,33 @@ impl Rent { } } +/// Enumerate return values from `Rent::due()` +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +pub enum RentDue { + /// Used to indicate the account is rent exempt + Exempt, + /// The account owes rent, and the amount is the field + Paying(u64), +} + +impl RentDue { + /// Return the lamports due for rent + pub fn lamports(&self) -> u64 { + match self { + RentDue::Exempt => 0, + RentDue::Paying(x) => *x, + } + } + + /// Return 'true' if rent exempt + pub fn is_exempt(&self) -> bool { + match self { + RentDue::Exempt => true, + RentDue::Paying(_) => false, + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -106,11 +130,10 @@ mod tests { assert_eq!( default_rent.due(0, 2, 1.2), - ( + RentDue::Paying( (((2 + ACCOUNT_STORAGE_OVERHEAD) * DEFAULT_LAMPORTS_PER_BYTE_YEAR) as f64 * 1.2) - as u64, - DEFAULT_LAMPORTS_PER_BYTE_YEAR == 0 - ) + as u64 + ), ); assert_eq!( default_rent.due( @@ -119,7 +142,7 @@ mod tests { 2, 1.2 ), - (0, true) + RentDue::Exempt, ); let custom_rent = Rent { @@ -130,10 +153,9 @@ mod tests { assert_eq!( custom_rent.due(0, 2, 1.2), - ( + RentDue::Paying( (((2 + ACCOUNT_STORAGE_OVERHEAD) * custom_rent.lamports_per_byte_year) as f64 * 1.2) as u64, - false ) ); @@ -144,43 +166,21 @@ mod tests { 2, 1.2 ), - (0, true) + RentDue::Exempt ); } - #[ignore] #[test] - #[should_panic] - fn show_rent_model() { - use crate::{clock::*, sysvar::Sysvar}; + fn test_rent_due_lamports() { + assert_eq!(RentDue::Exempt.lamports(), 0); - const SECONDS_PER_YEAR: f64 = 365.242_199 * 24.0 * 60.0 * 60.0; - const SLOTS_PER_YEAR: f64 = SECONDS_PER_YEAR / DEFAULT_S_PER_SLOT; + let amount = 123; + assert_eq!(RentDue::Paying(amount).lamports(), amount); + } - let rent = Rent::default(); - panic!( - "\n\n\ - ==================================================\n\ - empty account, no data:\n\ - \t{} lamports per epoch, {} lamports to be rent_exempt\n\n\ - stake_history, which is {}kB of data:\n\ - \t{} lamports per epoch, {} lamports to be rent_exempt\n\ - ==================================================\n\n", - rent.due( - 0, - 0, - (1.0 / SLOTS_PER_YEAR) * DEFAULT_SLOTS_PER_EPOCH as f64, - ) - .0, - rent.minimum_balance(0), - crate::sysvar::stake_history::StakeHistory::size_of() / 1024, - rent.due( - 0, - crate::sysvar::stake_history::StakeHistory::size_of(), - (1.0 / SLOTS_PER_YEAR) * DEFAULT_SLOTS_PER_EPOCH as f64, - ) - .0, - rent.minimum_balance(crate::sysvar::stake_history::StakeHistory::size_of()), - ); + #[test] + fn test_rent_due_is_exempt() { + assert!(RentDue::Exempt.is_exempt()); + assert!(!RentDue::Paying(0).is_exempt()); } } diff --git a/sdk/src/account.rs b/sdk/src/account.rs index 2e8e2fc34a..87322f37a4 100644 --- a/sdk/src/account.rs +++ b/sdk/src/account.rs @@ -103,6 +103,12 @@ pub trait WritableAccount: ReadableAccount { ); Ok(()) } + fn saturating_add_lamports(&mut self, lamports: u64) { + self.set_lamports(self.lamports().saturating_add(lamports)) + } + fn saturating_sub_lamports(&mut self, lamports: u64) { + self.set_lamports(self.lamports().saturating_sub(lamports)) + } fn data_mut(&mut self) -> &mut Vec; fn data_as_mut_slice(&mut self) -> &mut [u8]; fn set_owner(&mut self, owner: Pubkey); @@ -800,6 +806,28 @@ pub mod tests { account2.checked_sub_lamports(u64::MAX).unwrap(); } + #[test] + fn test_account_saturating_add_lamports() { + let key = Pubkey::new_unique(); + let (mut account, _) = make_two_accounts(&key); + + let remaining = 22; + account.set_lamports(u64::MAX - remaining); + account.saturating_add_lamports(remaining * 2); + assert_eq!(account.lamports(), u64::MAX); + } + + #[test] + fn test_account_saturating_sub_lamports() { + let key = Pubkey::new_unique(); + let (mut account, _) = make_two_accounts(&key); + + let remaining = 33; + account.set_lamports(remaining); + account.saturating_sub_lamports(remaining * 2); + assert_eq!(account.lamports(), 0); + } + #[test] #[allow(clippy::redundant_clone)] fn test_account_shared_data_all_fields() {