Refactor Rent::due() with RentDue enum (#22346)

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,

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) || {
+                if !rent_collector.should_collect_rent(&pubkey, &stored_account, false)
-                    let (_rent_due, exempt) = rent_collector.get_rent_due(&stored_account);
+                    || rent_collector.get_rent_due(&stored_account).is_exempt()
-                    exempt
+                {
-                } {
                     num_accounts_rent_exempt += 1;
                 }
 

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(
+        let generic_rent_due_for_system_account = bank
-            bank.get_minimum_balance_for_rent_exemption(0) - 1,
+            .rent_collector
-            0,
+            .rent
-            slots_elapsed as f64 / bank.rent_collector.slots_per_year,
+            .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,

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
+    // Updates the account's lamports and status, and returns the amount of rent collected, if any.
-    //  the account rent collected, if any
+    // This is NOT thread safe at some level. If we try to collect from the same account in
-    // This is NOT thread safe at some level. If we try to collect from the same account in parallel, we may collect twice.
+    // 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)
+        if self.can_skip_rent_collection(address, account, rent_for_sysvars, filler_account_suffix)
-            || account.rent_epoch() > self.epoch
-            || crate::accounts_db::AccountsDb::is_filler_account_helper(
-                address,
-                filler_account_suffix,
-            )
         {
-            0
+            return 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
-            }
         }
+
+        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)]

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 {
-            (
+            let actual_data_len = data_len as u64 + ACCOUNT_STORAGE_OVERHEAD;
-                ((self.lamports_per_byte_year * (data_len as u64 + ACCOUNT_STORAGE_OVERHEAD))
+            let lamports_per_year = self.lamports_per_byte_year * actual_data_len;
-                    as f64
+            RentDue::Paying((lamports_per_year as f64 * years_elapsed) as u64)
-                    * years_elapsed) as u64,
-                false,
-            )
         }
     }
 
@@ -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,
+                    as u64
-                DEFAULT_LAMPORTS_PER_BYTE_YEAR == 0
+            ),
-            )
         );
         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 test_rent_due_lamports() {
-    fn show_rent_model() {
+        assert_eq!(RentDue::Exempt.lamports(), 0);
-        use crate::{clock::*, sysvar::Sysvar};
 
-        const SECONDS_PER_YEAR: f64 = 365.242_199 * 24.0 * 60.0 * 60.0;
+        let amount = 123;
-        const SLOTS_PER_YEAR: f64 = SECONDS_PER_YEAR / DEFAULT_S_PER_SLOT;
+        assert_eq!(RentDue::Paying(amount).lamports(), amount);
+    }
 
-        let rent = Rent::default();
+    #[test]
-        panic!(
+    fn test_rent_due_is_exempt() {
-            "\n\n\
+        assert!(RentDue::Exempt.is_exempt());
-             ==================================================\n\
+        assert!(!RentDue::Paying(0).is_exempt());
-             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()),
-        );
     }
 }

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<u8>;
     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() {