From 5dd61b5db295bc6eba9947f6443ded51eb66fa56 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 21 Oct 2020 14:47:48 +0000 Subject: [PATCH] Port various rent fixes to runtime feature (#12842) (#13068) * Port various rent fixes to runtime feature * Fix CI * Use more consistent naming... (cherry picked from commit 608b81b4126b0beb265d76bf30543cb77ca3c541) Co-authored-by: Ryo Onodera --- runtime/src/accounts.rs | 21 ++++++++-- runtime/src/bank.rs | 58 ++++++++++++++-------------- runtime/src/feature_set.rs | 5 +++ runtime/src/rent_collector.rs | 72 +++++++++++++++-------------------- 4 files changed, 84 insertions(+), 72 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 2f6b5c4fd8..b81113c9bf 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -144,6 +144,8 @@ impl Accounts { let mut payer_index = None; let mut tx_rent: TransactionRent = 0; let mut accounts = 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 Self::is_non_loader_key(message, key, i) { if payer_index.is_none() { @@ -163,7 +165,11 @@ impl Accounts { .map(|(mut account, _)| { if message.is_writable(i) { let rent_due = rent_collector - .collect_from_existing_account(&key, &mut account); + .collect_from_existing_account( + &key, + &mut account, + rent_fix_enabled, + ); (account, rent_due) } else { (account, 0) @@ -721,6 +727,8 @@ impl Accounts { } /// Store the accounts into the DB + // allow(clippy) needed for various gating flags + #[allow(clippy::too_many_arguments)] pub fn store_accounts( &self, slot: Slot, @@ -731,6 +739,7 @@ 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, @@ -740,6 +749,7 @@ impl Accounts { rent_collector, last_blockhash_with_fee_calculator, fix_recent_blockhashes_sysvar_delay, + rent_fix_enabled, ); self.accounts_db.store(slot, &accounts_to_store); } @@ -767,6 +777,7 @@ 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, _hash_age_kind), (_, tx))) in loaded @@ -807,7 +818,11 @@ impl Accounts { ); if message.is_writable(i) { if account.rent_epoch == 0 { - acc.2 += rent_collector.collect_from_created_account(&key, account); + acc.2 += rent_collector.collect_from_created_account( + &key, + account, + rent_fix_enabled, + ); } accounts.push((key, &*account)); } @@ -1123,7 +1138,6 @@ mod tests { lamports_per_byte_year: 42, ..Rent::default() }, - ClusterType::Development, ); let min_balance = rent_collector.rent.minimum_balance(nonce::State::size()); let fee_calculator = FeeCalculator::new(min_balance); @@ -1801,6 +1815,7 @@ mod tests { &rent_collector, &(Hash::default(), FeeCalculator::default()), true, + true, ); assert_eq!(collected_accounts.len(), 2); assert!(collected_accounts diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 6b6302daa6..b775ebfef6 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -807,9 +807,7 @@ impl Bank { slots_per_year: parent.slots_per_year, epoch_schedule, collected_rent: AtomicU64::new(0), - rent_collector: parent - .rent_collector - .clone_with_epoch(epoch, parent.cluster_type()), + rent_collector: parent.rent_collector.clone_with_epoch(epoch), max_tick_height: (slot + 1) * parent.ticks_per_slot, block_height: parent.block_height + 1, fee_calculator: fee_rate_governor.create_fee_calculator(), @@ -927,9 +925,7 @@ impl Bank { fee_rate_governor: fields.fee_rate_governor, collected_rent: AtomicU64::new(fields.collected_rent), // clone()-ing is needed to consider a gated behavior in rent_collector - rent_collector: fields - .rent_collector - .clone_with_epoch(fields.epoch, genesis_config.cluster_type), + rent_collector: fields.rent_collector.clone_with_epoch(fields.epoch), epoch_schedule: fields.epoch_schedule, inflation: Arc::new(RwLock::new(fields.inflation)), stakes: RwLock::new(fields.stakes), @@ -1668,7 +1664,6 @@ impl Bank { &self.epoch_schedule, self.slots_per_year, &genesis_config.rent, - self.cluster_type(), ); // Add additional native programs specified in the genesis config @@ -2591,6 +2586,7 @@ 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); @@ -2834,9 +2830,11 @@ impl Bank { // parallelize? let mut rent = 0; for (pubkey, mut account) in accounts { - rent += self - .rent_collector - .collect_from_existing_account(&pubkey, &mut account); + rent += self.rent_collector.collect_from_existing_account( + &pubkey, + &mut account, + self.cumulative_rent_related_fixes_enabled(), + ); // Store all of them unconditionally to purge old AppendVec, // even if collected rent is 0 (= not updated). self.store_account(&pubkey, &account); @@ -2954,12 +2952,9 @@ impl Bank { self.get_epoch_and_slot_index(self.parent_slot()); let should_enable = match self.cluster_type() { - ClusterType::Development => true, - ClusterType::Devnet => true, - ClusterType::Testnet => current_epoch >= 97, ClusterType::MainnetBeta => { #[cfg(not(test))] - let should_enable = current_epoch >= Epoch::max_value(); + 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 @@ -2968,6 +2963,7 @@ impl Bank { should_enable } + _ => self.cumulative_rent_related_fixes_enabled(), }; let mut partitions = vec![]; @@ -3314,21 +3310,19 @@ impl Bank { pub fn deposit(&self, pubkey: &Pubkey, lamports: u64) -> u64 { let mut account = self.get_account(pubkey).unwrap_or_default(); - let should_be_in_new_behavior = match self.cluster_type() { - ClusterType::Development => true, - ClusterType::Devnet => true, - ClusterType::Testnet => self.epoch() >= 97, - ClusterType::MainnetBeta => self.epoch() >= Epoch::max_value(), - }; + 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 !should_be_in_new_behavior { + 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), + self.rent_collector.collect_from_existing_account( + pubkey, + &mut account, + rent_fix_enabled, + ), Relaxed, ); } @@ -3895,6 +3889,10 @@ 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() + } + // This is called from snapshot restore AND for each epoch boundary // The entire code path herein must be idempotent fn apply_feature_activations(&mut self, init_finish_or_warp: bool) { @@ -4100,8 +4098,9 @@ mod tests { use crate::{ accounts_index::{AccountMap, Ancestors}, genesis_utils::{ - create_genesis_config_with_leader, create_genesis_config_with_vote_accounts, - GenesisConfigInfo, ValidatorVoteKeypairs, BOOTSTRAP_VALIDATOR_LAMPORTS, + activate_all_features, create_genesis_config_with_leader, + create_genesis_config_with_vote_accounts, GenesisConfigInfo, ValidatorVoteKeypairs, + BOOTSTRAP_VALIDATOR_LAMPORTS, }, native_loader::NativeLoaderError, process_instruction::InvokeContext, @@ -5071,7 +5070,8 @@ mod tests { #[test] fn test_rent_eager_across_epoch_with_full_gap() { - let (genesis_config, _mint_keypair) = create_genesis_config(1); + let (mut genesis_config, _mint_keypair) = create_genesis_config(1); + activate_all_features(&mut genesis_config); let mut bank = Arc::new(Bank::new(&genesis_config)); assert_eq!(bank.rent_collection_partitions(), vec![(0, 0, 32)]); @@ -5093,7 +5093,8 @@ mod tests { #[test] fn test_rent_eager_across_epoch_with_half_gap() { - let (genesis_config, _mint_keypair) = create_genesis_config(1); + let (mut genesis_config, _mint_keypair) = create_genesis_config(1); + activate_all_features(&mut genesis_config); let mut bank = Arc::new(Bank::new(&genesis_config)); assert_eq!(bank.rent_collection_partitions(), vec![(0, 0, 32)]); @@ -5641,7 +5642,8 @@ mod tests { fn test_rent_eager_collect_rent_in_partition() { solana_logger::setup(); - let (genesis_config, _mint_keypair) = create_genesis_config(1); + let (mut genesis_config, _mint_keypair) = create_genesis_config(1); + activate_all_features(&mut genesis_config); let zero_lamport_pubkey = Pubkey::new_rand(); let rent_due_pubkey = Pubkey::new_rand(); diff --git a/runtime/src/feature_set.rs b/runtime/src/feature_set.rs index 979519e21d..42f4750b15 100644 --- a/runtime/src/feature_set.rs +++ b/runtime/src/feature_set.rs @@ -130,6 +130,11 @@ impl FeatureSet { pub fn is_active(&self, feature_id: &Pubkey) -> bool { self.active.contains(feature_id) } + + pub fn cumulative_rent_related_fixes_enabled(&self) -> bool { + self.is_active(&cumulative_rent_related_fixes::id()) + } + /// All features enabled, useful for testing pub fn all_enabled() -> Self { Self { diff --git a/runtime/src/rent_collector.rs b/runtime/src/rent_collector.rs index 7549104ff8..155bbd2fe5 100644 --- a/runtime/src/rent_collector.rs +++ b/runtime/src/rent_collector.rs @@ -1,13 +1,7 @@ //! calculate and collect rent from Accounts use solana_sdk::{ - account::Account, - clock::Epoch, - epoch_schedule::EpochSchedule, - genesis_config::{ClusterType, GenesisConfig}, - incinerator, - pubkey::Pubkey, - rent::Rent, - sysvar, + account::Account, clock::Epoch, epoch_schedule::EpochSchedule, genesis_config::GenesisConfig, + incinerator, pubkey::Pubkey, rent::Rent, sysvar, }; #[derive(Serialize, Deserialize, Clone, PartialEq, Debug, AbiExample)] @@ -16,11 +10,6 @@ pub struct RentCollector { pub epoch_schedule: EpochSchedule, pub slots_per_year: f64, pub rent: Rent, - // serde(skip) is needed not to break abi - // Also, wrap this with Option so that we can spot any uninitialized codepath (like - // snapshot restore) - #[serde(skip)] - pub cluster_type: Option, } impl Default for RentCollector { @@ -31,7 +20,6 @@ impl Default for RentCollector { // derive default value using GenesisConfig::default() slots_per_year: GenesisConfig::default().slots_per_year(), rent: Rent::default(), - cluster_type: Option::default(), } } } @@ -42,39 +30,32 @@ impl RentCollector { epoch_schedule: &EpochSchedule, slots_per_year: f64, rent: &Rent, - cluster_type: ClusterType, ) -> Self { Self { epoch, epoch_schedule: *epoch_schedule, slots_per_year, rent: *rent, - cluster_type: Some(cluster_type), } } - pub fn clone_with_epoch(&self, epoch: Epoch, cluster_type: ClusterType) -> Self { + pub fn clone_with_epoch(&self, epoch: Epoch) -> Self { Self { epoch, - cluster_type: Some(cluster_type), ..self.clone() } } - fn enable_new_behavior(&self) -> bool { - match self.cluster_type.unwrap() { - ClusterType::Development => true, - ClusterType::Devnet => true, - ClusterType::Testnet => self.epoch >= 97, - ClusterType::MainnetBeta => self.epoch >= Epoch::max_value(), - } - } - // updates this account's lamports and status and returns // 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) -> u64 { + pub fn collect_from_existing_account( + &self, + address: &Pubkey, + account: &mut Account, + rent_fix_enabled: bool, + ) -> u64 { if account.executable || account.rent_epoch > self.epoch || sysvar::check_id(&account.owner) @@ -100,7 +81,7 @@ impl RentCollector { if exempt || rent_due != 0 { if account.lamports > rent_due { account.rent_epoch = self.epoch - + if self.enable_new_behavior() && exempt { + + if rent_fix_enabled && exempt { // Rent isn't collected for the next epoch // Make sure to check exempt status later in curent epoch again 0 @@ -123,10 +104,15 @@ impl RentCollector { } #[must_use = "add to Bank::collected_rent"] - pub fn collect_from_created_account(&self, address: &Pubkey, account: &mut Account) -> u64 { + pub fn collect_from_created_account( + &self, + address: &Pubkey, + account: &mut Account, + enable_new_behavior: bool, + ) -> u64 { // initialize rent_epoch as created at this epoch account.rent_epoch = self.epoch; - self.collect_from_existing_account(address, account) + self.collect_from_existing_account(address, account, enable_new_behavior) } } @@ -148,19 +134,24 @@ mod tests { (account.clone(), account) }; - let rent_collector = - RentCollector::default().clone_with_epoch(new_epoch, ClusterType::Development); + 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(&Pubkey::new_rand(), &mut created_account); + let collected = rent_collector.collect_from_created_account( + &Pubkey::new_rand(), + &mut created_account, + true, + ); 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(&Pubkey::new_rand(), &mut existing_account); + let collected = rent_collector.collect_from_existing_account( + &Pubkey::new_rand(), + &mut existing_account, + true, + ); assert!(existing_account.lamports < old_lamports); assert_eq!(existing_account.lamports + collected, old_lamports); assert_ne!(existing_account.rent_epoch, old_epoch); @@ -183,11 +174,10 @@ mod tests { assert_eq!(account.rent_epoch, 0); // create a tested rent collector - let rent_collector = - RentCollector::default().clone_with_epoch(epoch, ClusterType::Development); + 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); + collected = rent_collector.collect_from_existing_account(&pubkey, &mut account, true); assert_eq!(account.lamports, huge_lamports); assert_eq!(collected, 0); @@ -195,7 +185,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); + collected = rent_collector.collect_from_existing_account(&pubkey, &mut account, true); assert_eq!(account.lamports, tiny_lamports - collected); assert_ne!(collected, 0); }