From 7f1368e7925d388ec0c6978dfdf282106b0f44ba Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 8 Mar 2021 12:34:08 +0900 Subject: [PATCH] Remove old feature: cumulative_rent_related_fixes (bp #15754) (#15757) * Remove old feature: cumulative_rent_related_fixes (#15754) (cherry picked from commit 8b0c6db8714461341ea0b3eccf92501b35d79f00) # Conflicts: # runtime/src/accounts.rs # runtime/src/bank.rs # sdk/src/feature_set.rs * Fix conflicts * Remove stale comment Co-authored-by: Ryo Onodera --- ledger-tool/src/main.rs | 28 +++++---------- runtime/src/accounts.rs | 20 ++--------- runtime/src/bank.rs | 66 ++++++----------------------------- runtime/src/rent_collector.rs | 36 ++++++------------- sdk/src/feature_set.rs | 9 ----- 5 files changed, 31 insertions(+), 128 deletions(-) diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 29032eac09..484bb1bcfb 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -2198,26 +2198,14 @@ fn main() { feature_account_balance, ), ); - if base_bank - .get_account(&feature_set::cumulative_rent_related_fixes::id()) - .is_some() - { - // steal some lamports from the pretty old feature not to affect - // capitalizaion, which doesn't affect inflation behavior! - base_bank.store_account( - &feature_set::cumulative_rent_related_fixes::id(), - &Account::default(), - ); - } else { - let old_cap = base_bank.set_capitalization(); - let new_cap = base_bank.capitalization(); - warn!( - "Skewing capitalization a bit to enable simple capitalization as \ - requested: increasing {} from {} to {}", - feature_account_balance, old_cap, new_cap, - ); - assert_eq!(old_cap + feature_account_balance, new_cap); - } + let old_cap = base_bank.set_capitalization(); + let new_cap = base_bank.capitalization(); + warn!( + "Skewing capitalization a bit to enable simple capitalization as \ + requested: increasing {} from {} to {}", + feature_account_balance, old_cap, new_cap, + ); + assert_eq!(old_cap + feature_account_balance, new_cap); } else { warn!("Already simple_capitalization is activated (or scheduled)"); } diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index a98cf97af0..8d93c7bd8d 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -171,7 +171,6 @@ impl Accounts { let mut tx_rent: TransactionRent = 0; let mut accounts = Vec::with_capacity(message.account_keys.len()); let mut account_deps = Vec::with_capacity(message.account_keys.len()); - let rent_fix_enabled = feature_set.cumulative_rent_related_fixes_enabled(); for (i, key) in message.account_keys.iter().enumerate() { let account = if message.is_non_loader_key(key, i) { @@ -192,11 +191,8 @@ impl Accounts { .load(ancestors, key) .map(|(mut account, _)| { if message.is_writable(i) { - let rent_due = rent_collector.collect_from_existing_account( - &key, - &mut account, - rent_fix_enabled, - ); + let rent_due = rent_collector + .collect_from_existing_account(&key, &mut account); (account, rent_due) } else { (account, 0) @@ -877,7 +873,6 @@ impl Accounts { rent_collector: &RentCollector, last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), fix_recent_blockhashes_sysvar_delay: bool, - rent_fix_enabled: bool, ) { let accounts_to_store = self.collect_accounts_to_store( txs, @@ -887,7 +882,6 @@ impl Accounts { rent_collector, last_blockhash_with_fee_calculator, fix_recent_blockhashes_sysvar_delay, - rent_fix_enabled, ); self.accounts_db.store_cached(slot, &accounts_to_store); } @@ -912,7 +906,6 @@ impl Accounts { rent_collector: &RentCollector, last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), fix_recent_blockhashes_sysvar_delay: bool, - rent_fix_enabled: bool, ) -> Vec<(&'a Pubkey, &'a Account)> { let mut accounts = Vec::with_capacity(loaded.len()); for (i, ((raccs, _nonce_rollback), (_, tx))) in loaded @@ -981,11 +974,7 @@ impl Accounts { } } if account.rent_epoch == 0 { - acc.3 += rent_collector.collect_from_created_account( - &key, - account, - rent_fix_enabled, - ); + acc.3 += rent_collector.collect_from_created_account(&key, account); } accounts.push((key, &*account)); } @@ -1967,7 +1956,6 @@ mod tests { &rent_collector, &(Hash::default(), FeeCalculator::default()), true, - true, ); assert_eq!(collected_accounts.len(), 2); assert!(collected_accounts @@ -2334,7 +2322,6 @@ mod tests { &rent_collector, &(next_blockhash, FeeCalculator::default()), true, - true, ); assert_eq!(collected_accounts.len(), 2); assert_eq!( @@ -2445,7 +2432,6 @@ mod tests { &rent_collector, &(next_blockhash, FeeCalculator::default()), true, - true, ); assert_eq!(collected_accounts.len(), 1); let collected_nonce_account = collected_accounts diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index aba2a96c05..5042be8442 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -3175,7 +3175,6 @@ impl Bank { &self.rent_collector, &self.last_blockhash_with_fee_calculator(), self.fix_recent_blockhashes_sysvar_delay(), - self.cumulative_rent_related_fixes_enabled(), ); self.collect_rent(executed, loaded_accounts); @@ -3424,11 +3423,9 @@ impl Bank { // parallelize? let mut rent = 0; for (pubkey, mut account) in accounts { - rent += self.rent_collector.collect_from_existing_account( - &pubkey, - &mut account, - self.cumulative_rent_related_fixes_enabled(), - ); + 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); @@ -3547,30 +3544,9 @@ impl Bank { let (parent_epoch, mut parent_slot_index) = self.get_epoch_and_slot_index(self.parent_slot()); - let should_enable = match self.cluster_type() { - ClusterType::MainnetBeta => { - #[cfg(not(test))] - let should_enable = self.cumulative_rent_related_fixes_enabled(); - - // needed for test_rent_eager_across_epoch_with_gap_under_multi_epoch_cycle, - // which depends on ClusterType::MainnetBeta - #[cfg(test)] - let should_enable = true; - - should_enable - } - _ => self.cumulative_rent_related_fixes_enabled(), - }; - let mut partitions = vec![]; if parent_epoch < current_epoch { - // this needs to be gated because this potentially can change the behavior - // (= bank hash) at each start of epochs - let slot_skipped = if should_enable { - (self.slot() - self.parent_slot()) > 1 - } else { - current_slot_index > 0 - }; + let slot_skipped = (self.slot() - self.parent_slot()) > 1; if slot_skipped { // Generate special partitions because there are skipped slots // exactly at the epoch transition. @@ -3584,9 +3560,7 @@ impl Bank { parent_epoch, )); - // this needs to be gated because this potentially can change the behavior - // (= bank hash) at each start of epochs - if should_enable && current_slot_index > 0 { + if current_slot_index > 0 { // ... for current epoch partitions.push(self.partition_from_slot_indexes_with_gapped_epochs( 0, @@ -3950,25 +3924,9 @@ impl Bank { } pub fn deposit(&self, pubkey: &Pubkey, lamports: u64) -> u64 { + // This doesn't collect rents intentionally. + // Rents should only be applied to actual TXes let mut account = self.get_account(pubkey).unwrap_or_default(); - - let rent_fix_enabled = self.cumulative_rent_related_fixes_enabled(); - - // 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 !rent_fix_enabled { - // 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, - rent_fix_enabled, - ), - Relaxed, - ); - } - account.lamports += lamports; self.store_account(pubkey, &account); account.lamports @@ -4639,10 +4597,6 @@ impl Bank { .is_active(&feature_set::no_overflow_rent_distribution::id()) } - pub fn cumulative_rent_related_fixes_enabled(&self) -> bool { - self.feature_set.cumulative_rent_related_fixes_enabled() - } - pub fn stake_program_v2_enabled(&self) -> bool { self.feature_set .is_active(&feature_set::stake_program_v2::id()) @@ -10228,19 +10182,19 @@ pub(crate) mod tests { if bank.slot == 32 { assert_eq!( bank.hash().to_string(), - "4syPxVrVFUpksTre5BB5w7qd3BxSU4WzUT6R2fjFgMJ2" + "7AkMgAb2v4tuoiSf3NnVgaBxSvp7XidbrSwsPEn4ENTp" ); } if bank.slot == 64 { assert_eq!( bank.hash().to_string(), - "4GKgnCxQs6AJxcqYQkxa8oF8gEp13bfRNCm2uzCceA26" + "2JzWWRBtQgdXboaACBRXNNKsHeBtn57uYmqH1AgGUkdG" ); } if bank.slot == 128 { assert_eq!( bank.hash().to_string(), - "9YwXsk2qpM7bZLnWGdtqCmDEygiu1KpEcr4zWWBTUKw6" + "FQnVhDVjhCyfBxFb3bdm3CLiuCePvWuW5TGDsLBZnKAo" ); break; } diff --git a/runtime/src/rent_collector.rs b/runtime/src/rent_collector.rs index 462a129237..2834725403 100644 --- a/runtime/src/rent_collector.rs +++ b/runtime/src/rent_collector.rs @@ -50,12 +50,7 @@ impl RentCollector { // the account rent collected, if any // #[must_use = "add to Bank::collected_rent"] - pub fn collect_from_existing_account( - &self, - address: &Pubkey, - account: &mut Account, - rent_fix_enabled: bool, - ) -> u64 { + 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) @@ -81,7 +76,7 @@ impl RentCollector { if exempt || rent_due != 0 { if account.lamports > rent_due { account.rent_epoch = self.epoch - + if rent_fix_enabled && exempt { + + if exempt { // Rent isn't collected for the next epoch // Make sure to check exempt status later in curent epoch again 0 @@ -104,15 +99,10 @@ impl RentCollector { } #[must_use = "add to Bank::collected_rent"] - pub fn collect_from_created_account( - &self, - address: &Pubkey, - account: &mut Account, - enable_new_behavior: bool, - ) -> u64 { + 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, enable_new_behavior) + self.collect_from_existing_account(address, account) } } @@ -139,21 +129,15 @@ mod tests { let rent_collector = RentCollector::default().clone_with_epoch(new_epoch); // collect rent on a newly-created account - let collected = rent_collector.collect_from_created_account( - &solana_sdk::pubkey::new_rand(), - &mut created_account, - true, - ); + let collected = rent_collector + .collect_from_created_account(&solana_sdk::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); // collect rent on a already-existing account - let collected = rent_collector.collect_from_existing_account( - &solana_sdk::pubkey::new_rand(), - &mut existing_account, - true, - ); + let collected = rent_collector + .collect_from_existing_account(&solana_sdk::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); @@ -179,7 +163,7 @@ mod tests { let rent_collector = RentCollector::default().clone_with_epoch(epoch); // first mark account as being collected while being rent-exempt - collected = rent_collector.collect_from_existing_account(&pubkey, &mut account, true); + collected = rent_collector.collect_from_existing_account(&pubkey, &mut account); assert_eq!(account.lamports, huge_lamports); assert_eq!(collected, 0); @@ -187,7 +171,7 @@ mod tests { account.lamports = tiny_lamports; // ... and trigger another rent collection on the same epoch and check that rent is working - collected = rent_collector.collect_from_existing_account(&pubkey, &mut account, true); + collected = rent_collector.collect_from_existing_account(&pubkey, &mut account); assert_eq!(account.lamports, tiny_lamports - collected); assert_ne!(collected, 0); } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 2df5e23583..44b319efb7 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -75,10 +75,6 @@ pub mod max_program_call_depth_64 { solana_sdk::declare_id!("YCKSgA6XmjtkQrHBQjpyNrX6EMhJPcYcLWMVgWn36iv"); } -pub mod cumulative_rent_related_fixes { - solana_sdk::declare_id!("FtjnuAtJTWwX3Kx9m24LduNEhzaGuuPfDW6e14SX2Fy5"); -} - pub mod sol_log_compute_units_syscall { solana_sdk::declare_id!("BHuZqHAj7JdZc68wVgZZcy51jZykvgrx4zptR44RyChe"); } @@ -196,7 +192,6 @@ lazy_static! { (ristretto_mul_syscall_enabled::id(), "ristretto multiply syscall"), (max_invoke_depth_4::id(), "max invoke call depth 4"), (max_program_call_depth_64::id(), "max program call depth 64"), - (cumulative_rent_related_fixes::id(), "rent fixes (#10206, #10468, #11342)"), (sol_log_compute_units_syscall::id(), "sol_log_compute_units syscall (#13243)"), (pubkey_log_syscall_enabled::id(), "pubkey log syscall"), (pull_request_ping_pong_check::id(), "ping-pong packet check #12794"), @@ -285,10 +280,6 @@ impl FeatureSet { self.active.get(feature_id).copied() } - pub fn cumulative_rent_related_fixes_enabled(&self) -> bool { - self.is_active(&cumulative_rent_related_fixes::id()) - } - /// List of enabled features that trigger full inflation pub fn full_inflation_features_enabled(&self) -> HashSet { let mut hash_set = FULL_INFLATION_FEATURE_PAIRS