Fix bad rent in Bank::deposit as if since epoch 0 (#10468)

* Fix bad rent in Bank::deposit as if since epoch 0

* Remove redundant predicate

* Rename

* Start to add tests with some cleanup

* Forgot to add refactor code...

* Enchance test

* Really fix rent timing in deposit with robust test

* Simplify new behavior by disabling rent altogether

docs/src/implemented-proposals/rent.md

@@ -14,6 +14,14 @@ Currently, the rent cost is fixed at the genesis. However, it's anticipated to b
 
 There are two timings of collecting rent from accounts: \(1\) when referenced by a transaction, \(2\) periodically once an epoch. \(1\) includes the transaction to create the new account itself, and it happens during the normal transaction processing by the bank as part of the load phase. \(2\) exists to ensure to collect rents from stale accounts, which aren't referenced in recent epochs at all. \(2\) requires the whole scan of accounts and is spread over an epoch based on account address prefix to avoid load spikes due to this rent collection.
 
+On the contrary, rent collection isn't applied to accounts that are directly manipulated by any of protocol-level bookkeeping processes including:
+
+- The distribution of rent collection itself (Otherwise, it may cause recursive rent collection handling)
+- The distribution of staking rewards at the start of every epoch (To reduce as much as processing spike at the start of new epoch)
+- The distribution of transaction fee at the end of every epoch
+
+Even if those processes are out of scope of rent collection, all of manipulated accounts will eventually be handled by the \(2\) mechanism.
+
 ## Actual processing of collecting rent
 
 Rent is due for one epoch's worth of time, and accounts always have `Account::rent_epoch` of `current_epoch + 1`.

runtime/src/accounts.rs

@@ -139,8 +139,9 @@ impl Accounts {
                         let (account, rent) =
                             AccountsDB::load(storage, ancestors, accounts_index, key)
                                 .map(|(mut account, _)| {
-                                    if message.is_writable(i) && !account.executable {
-                                        let rent_due = rent_collector.update(&key, &mut account);
+                                    if message.is_writable(i) {
+                                        let rent_due = rent_collector
+                                            .collect_from_existing_account(&key, &mut account);
                                         (account, rent_due)
                                     } else {
                                         (account, 0)
@@ -735,8 +736,7 @@ impl Accounts {
                 );
                 if message.is_writable(i) {
                     if account.rent_epoch == 0 {
-                        account.rent_epoch = rent_collector.epoch;
-                        acc.2 += rent_collector.update(&key, account);
+                        acc.2 += rent_collector.collect_from_created_account(&key, account);
                     }
                     accounts.push((key, &*account));
                 }

runtime/src/bank.rs

@@ -1163,18 +1163,13 @@ impl Bank {
             .unwrap()
             .genesis_hash(&genesis_config.hash(), &self.fee_calculator);
 
-        self.hashes_per_tick = genesis_config.poh_config.hashes_per_tick;
-        self.ticks_per_slot = genesis_config.ticks_per_slot;
-        self.ns_per_slot = genesis_config.poh_config.target_tick_duration.as_nanos()
-            * genesis_config.ticks_per_slot as u128;
+        self.hashes_per_tick = genesis_config.hashes_per_tick();
+        self.ticks_per_slot = genesis_config.ticks_per_slot();
+        self.ns_per_slot = genesis_config.ns_per_slot();
         self.genesis_creation_time = genesis_config.creation_time;
         self.unused = genesis_config.unused;
         self.max_tick_height = (self.slot + 1) * self.ticks_per_slot;
-        self.slots_per_year = years_as_slots(
-            1.0,
-            &genesis_config.poh_config.target_tick_duration,
-            self.ticks_per_slot,
-        );
+        self.slots_per_year = genesis_config.slots_per_year();
 
         self.epoch_schedule = genesis_config.epoch_schedule;
 
@@ -2108,7 +2103,9 @@ impl Bank {
         // parallelize?
         let mut rent = 0;
         for (pubkey, mut account) in accounts {
-            rent += self.rent_collector.update(&pubkey, &mut account);
+            rent += self
+                .rent_collector
+                .collect_from_existing_account(&pubkey, &mut account);
             // Store all of them unconditionally to purge old AppendVec,
             // even if collected rent is 0 (= not updated).
             self.store_account(&pubkey, &account);
@@ -2545,10 +2542,25 @@ impl Bank {
 
     pub fn deposit(&self, pubkey: &Pubkey, lamports: u64) {
         let mut account = self.get_account(pubkey).unwrap_or_default();
-        self.collected_rent.fetch_add(
-            self.rent_collector.update(pubkey, &mut account),
-            Ordering::Relaxed,
-        );
+
+        let should_be_in_new_behavior = match self.operating_mode() {
+            OperatingMode::Development => true,
+            OperatingMode::Preview => self.epoch() >= Epoch::max_value(),
+            OperatingMode::Stable => self.epoch() >= Epoch::max_value(),
+        };
+
+        // don't collect rents if we're in the new behavior;
+        // in genral, it's not worthwhile to account for rents outside the runtime (transactions)
+        // there are too many and subtly nuanced modification codepaths
+        if !should_be_in_new_behavior {
+            // previously we're too much collecting rents as if it existed since epoch 0...
+            self.collected_rent.fetch_add(
+                self.rent_collector
+                    .collect_from_existing_account(pubkey, &mut account),
+                Ordering::Relaxed,
+            );
+        }
+
         account.lamports += lamports;
         self.store_account(pubkey, &account);
     }

runtime/src/rent_collector.rs

@@ -1,10 +1,10 @@
 //! calculate and collect rent from Accounts
 use solana_sdk::{
-    account::Account, clock::Epoch, epoch_schedule::EpochSchedule, incinerator, pubkey::Pubkey,
-    rent::Rent, sysvar,
+    account::Account, clock::Epoch, epoch_schedule::EpochSchedule, genesis_config::GenesisConfig,
+    incinerator, pubkey::Pubkey, rent::Rent, sysvar,
 };
 
-#[derive(Default, Serialize, Deserialize, Clone, PartialEq, Debug, AbiExample)]
+#[derive(Serialize, Deserialize, Clone, PartialEq, Debug, AbiExample)]
 pub struct RentCollector {
     pub epoch: Epoch,
     pub epoch_schedule: EpochSchedule,
@@ -12,6 +12,18 @@ pub struct RentCollector {
     pub rent: Rent,
 }
 
+impl Default for RentCollector {
+    fn default() -> Self {
+        Self {
+            epoch: Epoch::default(),
+            epoch_schedule: EpochSchedule::default(),
+            // derive default value using GenesisConfig::default()
+            slots_per_year: GenesisConfig::default().slots_per_year(),
+            rent: Rent::default(),
+        }
+    }
+}
+
 impl RentCollector {
     pub fn new(
         epoch: Epoch,
@@ -36,7 +48,8 @@ impl RentCollector {
     // updates this account's lamports and status and returns
     //  the account rent collected, if any
     //
-    pub fn update(&self, address: &Pubkey, account: &mut Account) -> u64 {
+    #[must_use = "add to Bank::collected_rent"]
+    pub fn collect_from_existing_account(&self, address: &Pubkey, account: &mut Account) -> u64 {
         if account.executable
             || account.rent_epoch > self.epoch
             || sysvar::check_id(&account.owner)
@@ -75,4 +88,48 @@ impl RentCollector {
             }
         }
     }
+
+    #[must_use = "add to Bank::collected_rent"]
+    pub fn collect_from_created_account(&self, address: &Pubkey, account: &mut Account) -> u64 {
+        // initialize rent_epoch as created at this epoch
+        account.rent_epoch = self.epoch;
+        self.collect_from_existing_account(address, account)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_collect_from_account_created_and_existing() {
+        let old_lamports = 1000;
+        let old_epoch = 1;
+        let new_epoch = 3;
+
+        let (mut created_account, mut existing_account) = {
+            let mut account = Account::default();
+            account.lamports = old_lamports;
+            account.rent_epoch = old_epoch;
+
+            (account.clone(), account)
+        };
+
+        let rent_collector = RentCollector::default().clone_with_epoch(new_epoch);
+
+        let collected =
+            rent_collector.collect_from_created_account(&Pubkey::new_rand(), &mut created_account);
+        assert!(created_account.lamports < old_lamports);
+        assert_eq!(created_account.lamports + collected, old_lamports);
+        assert_ne!(created_account.rent_epoch, old_epoch);
+
+        let collected = rent_collector
+            .collect_from_existing_account(&Pubkey::new_rand(), &mut existing_account);
+        assert!(existing_account.lamports < old_lamports);
+        assert_eq!(existing_account.lamports + collected, old_lamports);
+        assert_ne!(existing_account.rent_epoch, old_epoch);
+
+        assert!(created_account.lamports > existing_account.lamports);
+        assert_eq!(created_account.rent_epoch, existing_account.rent_epoch);
+    }
 }

sdk/src/genesis_config.rs

@@ -14,6 +14,7 @@ use crate::{
     shred_version::compute_shred_version,
     signature::{Keypair, Signer},
     system_program,
+    timing::years_as_slots,
 };
 use bincode::{deserialize, serialize};
 use chrono::{TimeZone, Utc};
@@ -183,6 +184,26 @@ impl GenesisConfig {
     pub fn add_rewards_pool(&mut self, pubkey: Pubkey, account: Account) {
         self.rewards_pools.insert(pubkey, account);
     }
+
+    pub fn hashes_per_tick(&self) -> Option<u64> {
+        self.poh_config.hashes_per_tick
+    }
+
+    pub fn ticks_per_slot(&self) -> u64 {
+        self.ticks_per_slot
+    }
+
+    pub fn ns_per_slot(&self) -> u128 {
+        self.poh_config.target_tick_duration.as_nanos() * self.ticks_per_slot() as u128
+    }
+
+    pub fn slots_per_year(&self) -> f64 {
+        years_as_slots(
+            1.0,
+            &self.poh_config.target_tick_duration,
+            self.ticks_per_slot(),
+        )
+    }
 }
 
 impl fmt::Display for GenesisConfig {