Add a feature to disable rent collection (solana-labs#33945)

* add a feature to disable rent collection * fix a test * fix a test * rekey * should collect rent * Update runtime/src/bank/fee_distribution.rs Co-authored-by: Brooks <[email protected]> * expand tests to cover both rent collection disabled and enabled * feedbacks * reviews - move should collect rent check out of rent collector into bank * enforce rent_epoch to u64:max when rent collection is disabled * review feedbacks and fix a test 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 too. * revise comments * update rent_epoch for rent exempted account * rebase * set rent_epoch in rent collection for rent exempted account * revert test change * don't assert --------- Co-authored-by: HaoranYi <[email protected]> Co-authored-by: Brooks <[email protected]>
anza-xyz · Nov 16, 2023 · 60fdd85 · 60fdd85
1 parent 87d20ae
commit 60fdd85
Show file tree

Hide file tree

Showing 5 changed files with 106 additions and 31 deletions.
diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs
@@ -50,6 +50,7 @@ use {
             State as NonceState,
         },
         pubkey::Pubkey,
+        rent::RentDue,
         saturating_add_assign,
         slot_hashes::SlotHashes,
         sysvar::{self, instructions::construct_instructions_data},
@@ -314,6 +315,7 @@ impl Accounts {
         reward_interval: RewardInterval,
         program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
         loaded_programs: &LoadedProgramsForTxBatch,
+        should_collect_rent: bool,
     ) -> Result<LoadedTransaction> {
         let in_reward_interval = reward_interval == RewardInterval::InsideInterval;
 
@@ -382,15 +384,31 @@ impl Accounts {
                             .load_with_fixed_root(ancestors, key)
                             .map(|(mut account, _)| {
                                 if message.is_writable(i) {
-                                    let rent_due = rent_collector
-                                        .collect_from_existing_account(
-                                            key,
-                                            &mut account,
-                                            self.accounts_db.filler_account_suffix.as_ref(),
-                                            set_exempt_rent_epoch_max,
-                                        )
-                                        .rent_amount;
-                                    (account.data().len(), account, rent_due)
+                                    if should_collect_rent {
+                                        let rent_due = rent_collector
+                                            .collect_from_existing_account(
+                                                key,
+                                                &mut account,
+                                                self.accounts_db.filler_account_suffix.as_ref(),
+                                                set_exempt_rent_epoch_max,
+                                            )
+                                            .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 set_exempt_rent_epoch_max
+                                            && (account.rent_epoch() != RENT_EXEMPT_RENT_EPOCH
+                                                && rent_collector.get_rent_due(&account)
+                                                    == RentDue::Exempt)
+                                        {
+                                            account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH);
+                                        }
+                                        (account.data().len(), account, 0)
+                                    }
                                 } else {
                                     (account.data().len(), account, 0)
                                 }
@@ -641,6 +659,7 @@ impl Accounts {
         in_reward_interval: RewardInterval,
         program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
         loaded_programs: &LoadedProgramsForTxBatch,
+        should_collect_rent: bool,
     ) -> Vec<TransactionLoadResult> {
         txs.iter()
             .zip(lock_results)
@@ -675,6 +694,7 @@ impl Accounts {
                         in_reward_interval,
                         program_accounts,
                         loaded_programs,
+                        should_collect_rent,
                     ) {
                         Ok(loaded_transaction) => loaded_transaction,
                         Err(e) => return (Err(e), None),
@@ -1498,6 +1518,7 @@ mod tests {
             RewardInterval::OutsideInterval,
             &HashMap::new(),
             &LoadedProgramsForTxBatch::default(),
+            true,
         )
     }
 
@@ -3097,6 +3118,7 @@ mod tests {
             RewardInterval::OutsideInterval,
             &HashMap::new(),
             &LoadedProgramsForTxBatch::default(),
+            true,
         )
     }
 

diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs
@@ -89,7 +89,7 @@ use {
         epoch_accounts_hash::EpochAccountsHash,
         nonce_info::{NonceInfo, NoncePartial},
         partitioned_rewards::PartitionedEpochRewardsConfig,
-        rent_collector::{CollectedInfo, RentCollector},
+        rent_collector::{CollectedInfo, RentCollector, RENT_EXEMPT_RENT_EPOCH},
         rent_debits::RentDebits,
         sorted_storages::SortedStorages,
         stake_rewards::{RewardInfo, StakeReward},
@@ -160,6 +160,7 @@ use {
         packet::PACKET_DATA_SIZE,
         precompiles::get_precompiles,
         pubkey::Pubkey,
+        rent::RentDue,
         saturating_add_assign,
         signature::{Keypair, Signature},
         slot_hashes::SlotHashes,
@@ -5255,6 +5256,7 @@ impl Bank {
             self.get_reward_interval(),
             &program_accounts_map,
             &programs_loaded_for_tx_batch.borrow(),
+            self.should_collect_rent(),
         );
         load_time.stop();
 
@@ -5890,6 +5892,13 @@ impl Bank {
             .is_active(&feature_set::skip_rent_rewrites::id())
     }
 
+    /// true if rent fees should be collected (i.e. disable_rent_fees_collection is NOT enabled)
+    fn should_collect_rent(&self) -> bool {
+        !self
+            .feature_set
+            .is_active(&feature_set::disable_rent_fees_collection::id())
+    }
+
     /// Collect rent from `accounts`
     ///
     /// This fn is called inside a parallel loop from `collect_rent_in_partition()`.  Avoid adding
@@ -5923,15 +5932,29 @@ impl Bank {
             .is_active(&solana_sdk::feature_set::set_exempt_rent_epoch_max::id());
         let mut skipped_rewrites = Vec::default();
         for (pubkey, account, _loaded_slot) in accounts.iter_mut() {
-            let (rent_collected_info, measure) =
-                measure!(self.rent_collector.collect_from_existing_account(
-                    pubkey,
-                    account,
-                    self.rc.accounts.accounts_db.filler_account_suffix.as_ref(),
-                    set_exempt_rent_epoch_max,
-                ));
-            time_collecting_rent_us += measure.as_us();
-
+            let rent_collected_info = if self.should_collect_rent() {
+                let (rent_collected_info, measure) =
+                    measure!(self.rent_collector.collect_from_existing_account(
+                        pubkey,
+                        account,
+                        self.rc.accounts.accounts_db.filler_account_suffix.as_ref(),
+                        set_exempt_rent_epoch_max,
+                    ));
+                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 set_exempt_rent_epoch_max
+                    && (account.rent_epoch() != RENT_EXEMPT_RENT_EPOCH
+                        && self.rent_collector.get_rent_due(account) == RentDue::Exempt)
+                {
+                    account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH);
+                }
+                CollectedInfo::default()
+            };
             // 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/runtime/src/bank/fee_distribution.rs b/runtime/src/bank/fee_distribution.rs
@@ -273,6 +273,13 @@ impl Bank {
     pub(super) fn distribute_rent_fees(&self) {
         let total_rent_collected = self.collected_rent.load(Relaxed);
 
+        if !self.should_collect_rent() {
+            if total_rent_collected != 0 {
+                warn!("Rent fees collection is disabled, yet total rent collect was non zero! Total rent collected: {total_rent_collected}");
+            }
+            return;
+        }
+
         let (burned_portion, rent_to_be_distributed) = self
             .rent_collector
             .rent

diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs
@@ -1578,13 +1578,17 @@ impl Bank {
     }
 }
 
-#[test]
-fn test_rent_eager_collect_rent_in_partition() {
+#[test_case(true; "enable rent fees collection")]
+#[test_case(false; "disable rent fees collection")]
+fn test_rent_eager_collect_rent_in_partition(should_collect_rent: bool) {
     solana_logger::setup();
 
     let (mut genesis_config, _mint_keypair) = create_genesis_config(1_000_000);
     for feature_id in FeatureSet::default().inactive {
-        if feature_id != solana_sdk::feature_set::set_exempt_rent_epoch_max::id() {
+        if feature_id != solana_sdk::feature_set::set_exempt_rent_epoch_max::id()
+            && (should_collect_rent
+                || feature_id != solana_sdk::feature_set::disable_rent_fees_collection::id())
+        {
             activate_feature(&mut genesis_config, feature_id);
         }
     }
@@ -1598,7 +1602,11 @@ fn test_rent_eager_collect_rent_in_partition() {
     let large_lamports = 123_456_789;
     // genesis_config.epoch_schedule.slots_per_epoch == 432_000 and is unsuitable for this test
     let some_slot = MINIMUM_SLOTS_PER_EPOCH; // chosen to cause epoch to be +1
-    let rent_collected = 1; // this is a function of 'some_slot'
+    let rent_collected = if bank.should_collect_rent() {
+        1 /* this is a function of 'some_slot' */
+    } else {
+        0
+    };
 
     bank.store_account(
         &zero_lamport_pubkey,
@@ -1648,9 +1656,9 @@ fn test_rent_eager_collect_rent_in_partition() {
         bank.get_account(&rent_due_pubkey).unwrap().lamports(),
         little_lamports - rent_collected
     );
-    assert_eq!(
-        bank.get_account(&rent_due_pubkey).unwrap().rent_epoch(),
-        current_epoch + 1
+    assert!(
+        bank.get_account(&rent_due_pubkey).unwrap().rent_epoch() == current_epoch + 1
+            || !bank.should_collect_rent()
     );
     assert_eq!(
         bank.get_account(&rent_exempt_pubkey).unwrap().lamports(),
@@ -10877,6 +10885,7 @@ fn test_rent_state_list_len() {
         RewardInterval::OutsideInterval,
         &HashMap::new(),
         &LoadedProgramsForTxBatch::default(),
+        true,
     );
 
     let compute_budget = bank.runtime_config.compute_budget.unwrap_or_else(|| {
@@ -11462,14 +11471,22 @@ fn test_get_rent_paying_pubkeys() {
 }
 
 /// Ensure that accounts data size is updated correctly by rent collection
-#[test]
-fn test_accounts_data_size_and_rent_collection() {
+#[test_case(true; "enable rent fees collection")]
+#[test_case(false; "disable rent fees collection")]
+fn test_accounts_data_size_and_rent_collection(should_collect_rent: bool) {
     for set_exempt_rent_epoch_max in [false, true] {
         let GenesisConfigInfo {
             mut genesis_config, ..
         } = genesis_utils::create_genesis_config(100 * LAMPORTS_PER_SOL);
         genesis_config.rent = Rent::default();
-        activate_all_features(&mut genesis_config);
+        for feature_id in FeatureSet::default().inactive {
+            if should_collect_rent
+                || feature_id != solana_sdk::feature_set::disable_rent_fees_collection::id()
+            {
+                activate_feature(&mut genesis_config, feature_id);
+            }
+        }
+
         let bank = Arc::new(Bank::new_for_tests(&genesis_config));
         let slot = bank.slot() + bank.slot_count_per_normal_epoch();
         let bank = Arc::new(Bank::new_from_parent(bank, &Pubkey::default(), slot));
@@ -11497,17 +11514,18 @@ fn test_accounts_data_size_and_rent_collection() {
         }
 
         // Collect rent for real
+        let should_collect_rent = bank.should_collect_rent();
         let accounts_data_size_delta_before_collecting_rent = bank.load_accounts_data_size_delta();
         bank.collect_rent_eagerly();
         let accounts_data_size_delta_after_collecting_rent = bank.load_accounts_data_size_delta();
 
         let accounts_data_size_delta_delta = accounts_data_size_delta_after_collecting_rent
             - accounts_data_size_delta_before_collecting_rent;
-        assert!(accounts_data_size_delta_delta < 0);
+        assert!(!should_collect_rent || accounts_data_size_delta_delta < 0);
         let reclaimed_data_size = accounts_data_size_delta_delta.saturating_neg() as usize;
 
         // Ensure the account is reclaimed by rent collection
-        assert_eq!(reclaimed_data_size, data_size,);
+        assert!(!should_collect_rent || reclaimed_data_size == data_size);
     }
 }
 

diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs
@@ -724,6 +724,10 @@ pub mod validate_fee_collector_account {
     solana_sdk::declare_id!("prpFrMtgNmzaNzkPJg9o753fVvbHKqNrNTm76foJ2wm");
 }
 
+pub mod disable_rent_fees_collection {
+    solana_sdk::declare_id!("CJzY83ggJHqPGDq8VisV3U91jDJLuEaALZooBrXtnnLU");
+}
+
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
@@ -900,6 +904,7 @@ lazy_static! {
         (update_hashes_per_tick5::id(), "Update desired hashes per tick to 9.2M"),
         (update_hashes_per_tick6::id(), "Update desired hashes per tick to 10M"),
         (validate_fee_collector_account::id(), "validate fee collector account #33888"),
+        (disable_rent_fees_collection::id(), "Disable rent fees collection #33945"),
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()