diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 5a1ffe8016..7bb85a7e89 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -1133,6 +1133,7 @@ mod tests { programs: vec![], accounts: vec![], sysvars: vec![], + disabled_features: vec![].into_iter().collect(), }; assert_eq!( Err(InstructionError::ProgramFailedToComplete), diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 10ea2aa322..522ec611ca 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -210,6 +210,8 @@ impl Accounts { let mut account_deps = Vec::with_capacity(message.account_keys.len()); let mut key_check = MessageProgramIdsCache::new(message); let mut rent_debits = RentDebits::default(); + let rent_for_sysvars = feature_set.is_active(&feature_set::rent_for_sysvars::id()); + for (i, key) in message.account_keys.iter().enumerate() { let account = if key_check.is_non_loader_key(key, i) { if payer_index.is_none() { @@ -229,8 +231,11 @@ 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); + let rent_due = rent_collector.collect_from_existing_account( + key, + &mut account, + rent_for_sysvars, + ); (account, rent_due) } else { (account, 0) @@ -916,6 +921,7 @@ impl Accounts { rent_collector: &RentCollector, last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), fix_recent_blockhashes_sysvar_delay: bool, + rent_for_sysvars: bool, ) { let accounts_to_store = self.collect_accounts_to_store( txs, @@ -924,6 +930,7 @@ impl Accounts { rent_collector, last_blockhash_with_fee_calculator, fix_recent_blockhashes_sysvar_delay, + rent_for_sysvars, ); self.accounts_db.store_cached(slot, &accounts_to_store); } @@ -948,6 +955,7 @@ impl Accounts { rent_collector: &RentCollector, last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), fix_recent_blockhashes_sysvar_delay: bool, + rent_for_sysvars: bool, ) -> Vec<(&'a Pubkey, &'a AccountSharedData)> { let mut accounts = Vec::with_capacity(loaded.len()); for (i, ((raccs, _nonce_rollback), tx)) in loaded.iter_mut().zip(txs).enumerate() { @@ -1009,7 +1017,11 @@ impl Accounts { } } if account.rent_epoch() == INITIAL_RENT_EPOCH { - let rent = rent_collector.collect_from_created_account(key, account); + let rent = rent_collector.collect_from_created_account( + key, + account, + rent_for_sysvars, + ); loaded_transaction.rent += rent; loaded_transaction .rent_debits @@ -2005,6 +2017,7 @@ mod tests { &rent_collector, &(Hash::default(), FeeCalculator::default()), true, + true, ); assert_eq!(collected_accounts.len(), 2); assert!(collected_accounts @@ -2381,6 +2394,7 @@ mod tests { &rent_collector, &(next_blockhash, FeeCalculator::default()), true, + true, ); assert_eq!(collected_accounts.len(), 2); assert_eq!( @@ -2497,6 +2511,7 @@ 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 1e9fa91359..a2c60deac6 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -1517,8 +1517,33 @@ impl Bank { where F: Fn(&Option) -> AccountSharedData, { - let old_account = self.get_sysvar_account_with_fixed_root(pubkey); - let new_account = updater(&old_account); + let old_account = if !self.rent_for_sysvars() { + // This old behavior is being retired for simpler reasoning for the benefits of all. + // Specifically, get_sysvar_account_with_fixed_root() doesn't work nicely with eager + // rent collection, which becomes significant for sysvars after rent_for_sysvars + // activation. That's because get_sysvar_account_with_fixed_root() invocations by both + // update_slot_history() and update_recent_blockhashes() ignores any updates + // by eager rent collection in this slot. + // Also, it turned out that get_sysvar_account_with_fixed_root()'s special + // behavior (idempotent) isn't needed to begin with, because we're fairly certain that + // we don't call new_from_parent() with same child slot multiple times in the + // production code (except after proper handling of duplicate slot dumping)... + self.get_sysvar_account_with_fixed_root(pubkey) + } else { + self.get_account_with_fixed_root(pubkey) + }; + let mut new_account = updater(&old_account); + + if self.rent_for_sysvars() { + // When new sysvar comes into existence (with RENT_UNADJUSTED_INITIAL_BALANCE lamports), + // this code ensures that the sysvar's balance is adjusted to be rent-exempt. + // Note that all of existing sysvar balances must be adjusted immediately (i.e. reset) upon + // the `rent_for_sysvars` feature activation (ref: reset_all_sysvar_balances). + // + // More generally, this code always re-calculates for possible sysvar data size change, + // although there is no such sysvars currently. + self.adjust_sysvar_balance_for_rent(&mut new_account); + } self.store_account_and_update_capitalization(pubkey, &new_account); } @@ -1527,9 +1552,23 @@ impl Bank { &self, old_account: &Option, ) -> InheritableAccountFields { + const RENT_UNADJUSTED_INITIAL_BALANCE: u64 = 1; + ( - old_account.as_ref().map(|a| a.lamports()).unwrap_or(1), - INITIAL_RENT_EPOCH, + old_account + .as_ref() + .map(|a| a.lamports()) + .unwrap_or(RENT_UNADJUSTED_INITIAL_BALANCE), + if !self.rent_for_sysvars() { + INITIAL_RENT_EPOCH + } else { + // start to inherit rent_epoch updated by rent collection to be consistent with + // other normal accounts + old_account + .as_ref() + .map(|a| a.rent_epoch()) + .unwrap_or(INITIAL_RENT_EPOCH) + }, ) } @@ -3411,6 +3450,7 @@ impl Bank { &self.rent_collector, &self.last_blockhash_with_fee_calculator(), self.fix_recent_blockhashes_sysvar_delay(), + self.rent_for_sysvars(), ); let rent_debits = self.collect_rent(executed, loaded_txs); @@ -3682,12 +3722,15 @@ impl Bank { let account_count = accounts.len(); // parallelize? + let rent_for_sysvars = self.rent_for_sysvars(); let mut total_rent = 0; let mut rent_debits = RentDebits::default(); for (pubkey, mut account) in accounts { - let rent = self - .rent_collector - .collect_from_existing_account(&pubkey, &mut account); + let rent = self.rent_collector.collect_from_existing_account( + &pubkey, + &mut account, + rent_for_sysvars, + ); total_rent += rent; // Store all of them unconditionally to purge old AppendVec, // even if collected rent is 0 (= not updated). @@ -4186,16 +4229,31 @@ impl Bank { if let Some(old_account) = self.get_account_with_fixed_root(pubkey) { match new_account.lamports().cmp(&old_account.lamports()) { std::cmp::Ordering::Greater => { - self.capitalization - .fetch_add(new_account.lamports() - old_account.lamports(), Relaxed); + let increased = new_account.lamports() - old_account.lamports(); + trace!( + "store_account_and_update_capitalization: increased: {} {}", + pubkey, + increased + ); + self.capitalization.fetch_add(increased, Relaxed); } std::cmp::Ordering::Less => { - self.capitalization - .fetch_sub(old_account.lamports() - new_account.lamports(), Relaxed); + let decreased = old_account.lamports() - new_account.lamports(); + trace!( + "store_account_and_update_capitalization: decreased: {} {}", + pubkey, + decreased + ); + self.capitalization.fetch_sub(decreased, Relaxed); } std::cmp::Ordering::Equal => {} } } else { + trace!( + "store_account_and_update_capitalization: created: {} {}", + pubkey, + new_account.lamports() + ); self.capitalization .fetch_add(new_account.lamports(), Relaxed); } @@ -4339,6 +4397,8 @@ impl Bank { // multiple times with the same parent_slot in the case of forking. // // Generally, all of sysvar update granularity should be slot boundaries. + // + // This behavior is deprecated... See comment in update_sysvar_account() for details fn get_sysvar_account_with_fixed_root(&self, pubkey: &Pubkey) -> Option { let mut ancestors = self.ancestors.clone(); ancestors.remove(&self.slot()); @@ -5097,6 +5157,14 @@ impl Bank { self.rewrite_stakes(); } + if new_feature_activations.contains(&feature_set::rent_for_sysvars::id()) { + // when this feature is activated, immediately all of existing sysvars are susceptible + // to rent collection and account data removal due to insufficient balance due to only + // having 1 lamport. + // so before any is accessed, reset the balance to be rent-exempt here at the same + // timing when perpetual balance adjustment is started in update_sysvar_account(). + self.reset_all_sysvar_balances(); + } if !debug_do_not_add_builtins { self.ensure_feature_builtins(init_finish_or_warp, &new_feature_activations); @@ -5105,6 +5173,41 @@ impl Bank { self.ensure_no_storage_rewards_pool(); } + fn reset_all_sysvar_balances(&self) { + for sysvar_id in &[ + sysvar::clock::id(), + sysvar::epoch_schedule::id(), + sysvar::fees::id(), + sysvar::recent_blockhashes::id(), + sysvar::rent::id(), + sysvar::rewards::id(), + sysvar::slot_hashes::id(), + sysvar::slot_history::id(), + sysvar::stake_history::id(), + ] { + if let Some(mut account) = self.get_account(sysvar_id) { + let (old_data_len, old_lamports) = (account.data().len(), account.lamports()); + self.adjust_sysvar_balance_for_rent(&mut account); + info!( + "reset_all_sysvar_balances (slot: {}): {} ({} bytes) is reset from {} to {}", + self.slot(), + sysvar_id, + old_data_len, + old_lamports, + account.lamports() + ); + self.store_account_and_update_capitalization(sysvar_id, &account); + } + } + } + + fn adjust_sysvar_balance_for_rent(&self, account: &mut AccountSharedData) { + account.set_lamports( + self.get_minimum_balance_for_rent_exemption(account.data().len()) + .max(account.lamports()), + ); + } + // Compute the active feature set based on the current bank state, and return the set of newly activated features fn compute_active_feature_set(&mut self, allow_new_activations: bool) -> HashSet { let mut active = self.feature_set.active.clone(); @@ -5284,6 +5387,11 @@ impl Bank { .is_active(&feature_set::consistent_recent_blockhashes_sysvar::id()), } } + + fn rent_for_sysvars(&self) -> bool { + self.feature_set + .is_active(&feature_set::rent_for_sysvars::id()) + } } impl Drop for Bank { @@ -5896,6 +6004,19 @@ pub(crate) mod tests { assert_eq!(bank.capitalization(), bank.calculate_capitalization(true)); } + fn assert_capitalization_diff_with_new_bank( + bank: &Bank, + updater: impl Fn() -> Bank, + asserter: impl Fn(u64, u64), + ) -> Bank { + let old = bank.capitalization(); + let bank = updater(); + let new = bank.capitalization(); + asserter(old, new); + assert_eq!(bank.capitalization(), bank.calculate_capitalization(true)); + bank + } + #[test] fn test_store_account_and_update_capitalization_missing() { let (genesis_config, _mint_keypair) = create_genesis_config(0); @@ -6183,7 +6304,9 @@ pub(crate) mod tests { let current_capitalization = bank.capitalization.load(Relaxed); - let sysvar_and_native_proram_delta = 1; + // only slot history is newly created + let sysvar_and_native_proram_delta = + min_rent_excempt_balance_for_sysvars(&bank, &[sysvar::slot_history::id()]); assert_eq!( previous_capitalization - (current_capitalization - sysvar_and_native_proram_delta), burned_portion @@ -7095,6 +7218,11 @@ pub(crate) mod tests { .map(|(slot, _)| *slot) .collect::>() } + + fn first_slot_in_next_epoch(&self) -> Slot { + self.epoch_schedule() + .get_first_slot_in_epoch(self.epoch() + 1) + } } #[test] @@ -8666,7 +8794,7 @@ pub(crate) mod tests { let (mut genesis_config, _mint_keypair) = create_genesis_config(500); let expected_previous_slot = 3; - let expected_next_slot = expected_previous_slot + 1; + let mut expected_next_slot = expected_previous_slot + 1; // First, initialize the clock sysvar activate_all_features(&mut genesis_config); @@ -8697,7 +8825,10 @@ pub(crate) mod tests { assert_eq!(dummy_rent_epoch, current_account.rent_epoch()); }, |old, new| { - assert_eq!(old + 1, new); + assert_eq!( + old + min_rent_excempt_balance_for_sysvars(&bank1, &[sysvar::clock::id()]), + new + ); }, ); @@ -8705,7 +8836,7 @@ pub(crate) mod tests { &bank1, || { bank1.update_sysvar_account(&dummy_clock_id, |optional_account| { - assert!(optional_account.is_none()); + assert!(optional_account.is_some()); create_account( &Clock { @@ -8746,7 +8877,9 @@ pub(crate) mod tests { expected_next_slot, from_account::(¤t_account).unwrap().slot ); - assert_eq!(INITIAL_RENT_EPOCH, current_account.rent_epoch()); + // inherit_specially_retained_account_fields() now starts to inherit rent_epoch too + // with rent_for_sysvars + assert_eq!(dummy_rent_epoch, current_account.rent_epoch()); }, |old, new| { // if existing, capitalization shouldn't change @@ -8754,8 +8887,9 @@ pub(crate) mod tests { }, ); - // Updating again should give bank1's sysvar to the closure not bank2's. - // Thus, assert with same expected_next_slot as previously + // Updating again should give bank2's sysvar to the closure not bank1's. + // Thus, increment expected_next_slot accordingly + expected_next_slot += 1; assert_capitalization_diff( &bank2, || { @@ -11608,6 +11742,265 @@ pub(crate) mod tests { bank.store_account(vote_pubkey, &vote_account); } + fn min_rent_excempt_balance_for_sysvars(bank: &Bank, sysvar_ids: &[Pubkey]) -> u64 { + sysvar_ids + .iter() + .map(|sysvar_id| { + trace!("min_rent_excempt_balance_for_sysvars: {}", sysvar_id); + bank.get_minimum_balance_for_rent_exemption( + bank.get_account(sysvar_id).unwrap().data().len(), + ) + }) + .sum() + } + + fn expected_cap_delta_after_sysvar_reset(bank: &Bank, sysvar_ids: &[Pubkey]) -> u64 { + min_rent_excempt_balance_for_sysvars(bank, sysvar_ids) - sysvar_ids.len() as u64 + } + + #[test] + fn test_adjust_sysvar_balance_for_rent() { + let (genesis_config, _mint_keypair) = create_genesis_config(0); + let bank = Bank::new(&genesis_config); + let mut smaller_sample_sysvar = bank.get_account(&sysvar::clock::id()).unwrap(); + assert_eq!(smaller_sample_sysvar.lamports(), 1); + bank.adjust_sysvar_balance_for_rent(&mut smaller_sample_sysvar); + assert_eq!( + smaller_sample_sysvar.lamports(), + bank.get_minimum_balance_for_rent_exemption(smaller_sample_sysvar.data().len()), + ); + + let mut bigger_sample_sysvar = AccountSharedData::new( + 1, + smaller_sample_sysvar.data().len() + 1, + &Pubkey::default(), + ); + bank.adjust_sysvar_balance_for_rent(&mut bigger_sample_sysvar); + assert!(smaller_sample_sysvar.lamports() < bigger_sample_sysvar.lamports()); + + // excess lamports shouldn't be reduced by adjust_sysvar_balance_for_rent() + let excess_lamports = smaller_sample_sysvar.lamports() + 999; + smaller_sample_sysvar.set_lamports(excess_lamports); + bank.adjust_sysvar_balance_for_rent(&mut smaller_sample_sysvar); + assert_eq!(smaller_sample_sysvar.lamports(), excess_lamports); + } + + // this test can be removed after rent_for_sysvars activation on mainnet-beta + #[test] + fn test_no_deletion_due_to_rent_upon_rent_for_sysvar_activation() { + solana_logger::setup(); + + let (mut genesis_config, _mint_keypair) = create_genesis_config(0); + let feature_balance = + std::cmp::max(genesis_config.rent.minimum_balance(Feature::size_of()), 1); + + // activate all features but rent_for_sysvars + activate_all_features(&mut genesis_config); + genesis_config + .accounts + .remove(&feature_set::rent_for_sysvars::id()); + let bank0 = Bank::new(&genesis_config); + let bank1 = Arc::new(new_from_parent(&Arc::new(bank0))); + + // schedule activation of simple capitalization + bank1.store_account_and_update_capitalization( + &feature_set::rent_for_sysvars::id(), + &feature::create_account(&Feature { activated_at: None }, feature_balance), + ); + + let bank2 = + Bank::new_from_parent(&bank1, &Pubkey::default(), bank1.first_slot_in_next_epoch()); + assert_eq!(bank2.get_program_accounts(&sysvar::id()).unwrap().len(), 8); + + // force rent collection for sysvars + bank2.collect_rent_in_partition((0, 0, 1)); // all range + + // no sysvar should be deleted due to rent + assert_eq!(bank2.get_program_accounts(&sysvar::id()).unwrap().len(), 8); + } + + // this test can be removed after rent_for_sysvars activation on mainnet-beta + #[test] + fn test_rent_for_sysvars_adjustment_minimum_genesis_set() { + solana_logger::setup(); + + let (mut genesis_config, _mint_keypair) = create_genesis_config(0); + let feature_balance = + std::cmp::max(genesis_config.rent.minimum_balance(Feature::size_of()), 1); + + // inhibit deprecated rewards sysvar creation altogether + genesis_config.accounts.insert( + feature_set::deprecate_rewards_sysvar::id(), + Account::from(feature::create_account( + &Feature { + activated_at: Some(0), + }, + feature_balance, + )), + ); + + let bank0 = Bank::new(&genesis_config); + let bank1 = Arc::new(new_from_parent(&Arc::new(bank0))); + + // schedule activation of rent_for_sysvars + bank1.store_account_and_update_capitalization( + &feature_set::rent_for_sysvars::id(), + &feature::create_account(&Feature { activated_at: None }, feature_balance), + ); + + { + let sysvars = bank1.get_program_accounts(&sysvar::id()).unwrap(); + assert_eq!(sysvars.len(), 8); + assert!(sysvars + .iter() + .map(|(_pubkey, account)| account.lamports()) + .all(|lamports| lamports == 1)); + } + + // 8 sysvars should be reset by reset_all_sysvar_balances() + let bank2 = assert_capitalization_diff_with_new_bank( + &bank1, + || Bank::new_from_parent(&bank1, &Pubkey::default(), bank1.first_slot_in_next_epoch()), + |old, new| { + assert_eq!( + old + expected_cap_delta_after_sysvar_reset( + &bank1, + &[ + sysvar::clock::id(), + sysvar::epoch_schedule::id(), + sysvar::fees::id(), + sysvar::recent_blockhashes::id(), + sysvar::rent::id(), + sysvar::slot_hashes::id(), + sysvar::slot_history::id(), + sysvar::stake_history::id(), + ] + ), + new + ) + }, + ); + + { + let sysvars = bank2.get_program_accounts(&sysvar::id()).unwrap(); + assert_eq!(sysvars.len(), 8); + assert!(sysvars + .iter() + .map(|(_pubkey, account)| account.lamports()) + .all(|lamports| lamports > 1)); + } + } + + // this test can be removed after rent_for_sysvars activation on mainnet-beta + #[test] + fn test_rent_for_sysvars_adjustment_full_set() { + solana_logger::setup(); + + let (mut genesis_config, _mint_keypair) = create_genesis_config(0); + let feature_balance = + std::cmp::max(genesis_config.rent.minimum_balance(Feature::size_of()), 1); + + // activate all features but rent_for_sysvars + activate_all_features(&mut genesis_config); + genesis_config + .accounts + .remove(&feature_set::rent_for_sysvars::id()); + // intentionally create deprecated rewards sysvar creation + genesis_config + .accounts + .remove(&feature_set::deprecate_rewards_sysvar::id()); + + // intentionally create bogus native programs + #[allow(clippy::unnecessary_wraps)] + fn mock_process_instruction( + _program_id: &Pubkey, + _data: &[u8], + _invoke_context: &mut dyn InvokeContext, + ) -> std::result::Result<(), solana_sdk::instruction::InstructionError> { + Ok(()) + } + let builtins = Builtins { + genesis_builtins: vec![ + Builtin::new( + "mock bpf", + solana_sdk::bpf_loader::id(), + mock_process_instruction, + ), + Builtin::new( + "mock bpf", + solana_sdk::bpf_loader_deprecated::id(), + mock_process_instruction, + ), + ], + feature_builtins: (vec![]), + }; + + let bank0 = Arc::new(Bank::new_with_paths( + &genesis_config, + Vec::new(), + &[], + None, + Some(&builtins), + AccountSecondaryIndexes::default(), + false, + AccountShrinkThreshold::default(), + false, + )); + // move to next epoch to create now deprecated rewards sysvar intentionally + let bank1 = Arc::new(Bank::new_from_parent( + &bank0, + &Pubkey::default(), + bank0.first_slot_in_next_epoch(), + )); + + // schedule activation of simple capitalization + bank1.store_account_and_update_capitalization( + &feature_set::rent_for_sysvars::id(), + &feature::create_account(&Feature { activated_at: None }, feature_balance), + ); + { + let sysvars = bank1.get_program_accounts(&sysvar::id()).unwrap(); + assert_eq!(sysvars.len(), 9); + assert!(sysvars + .iter() + .map(|(_pubkey, account)| account.lamports()) + .all(|lamports| lamports == 1)); + } + + // 9 sysvars should be reset by reset_all_sysvar_balances() + let bank2 = assert_capitalization_diff_with_new_bank( + &bank1, + || Bank::new_from_parent(&bank1, &Pubkey::default(), bank1.first_slot_in_next_epoch()), + |old, new| { + assert_eq!( + old + expected_cap_delta_after_sysvar_reset( + &bank1, + &[ + sysvar::clock::id(), + sysvar::epoch_schedule::id(), + sysvar::fees::id(), + sysvar::recent_blockhashes::id(), + sysvar::rent::id(), + sysvar::rewards::id(), + sysvar::slot_hashes::id(), + sysvar::slot_history::id(), + sysvar::stake_history::id(), + ] + ), + new + ) + }, + ); + { + let sysvars = bank2.get_program_accounts(&sysvar::id()).unwrap(); + assert_eq!(sysvars.len(), 9); + assert!(sysvars + .iter() + .map(|(_pubkey, account)| account.lamports()) + .all(|lamports| lamports > 1)); + } + } + #[test] fn test_update_clock_timestamp() { let leader_pubkey = solana_sdk::pubkey::new_rand(); diff --git a/runtime/src/rent_collector.rs b/runtime/src/rent_collector.rs index 98d609d843..744bd7ee0a 100644 --- a/runtime/src/rent_collector.rs +++ b/runtime/src/rent_collector.rs @@ -60,10 +60,11 @@ impl RentCollector { &self, address: &Pubkey, account: &mut AccountSharedData, + rent_for_sysvars: bool, ) -> u64 { if account.executable() // executable accounts must be rent-exempt balance || account.rent_epoch() > self.epoch - || sysvar::check_id(account.owner()) + || (!rent_for_sysvars && sysvar::check_id(account.owner())) || *address == incinerator::id() { 0 @@ -115,10 +116,11 @@ impl RentCollector { &self, address: &Pubkey, account: &mut AccountSharedData, + rent_for_sysvars: bool, ) -> u64 { // initialize rent_epoch as created at this epoch account.set_rent_epoch(self.epoch); - self.collect_from_existing_account(address, account) + self.collect_from_existing_account(address, account, rent_for_sysvars) } } @@ -146,15 +148,21 @@ 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); + let collected = rent_collector.collect_from_created_account( + &solana_sdk::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(&solana_sdk::pubkey::new_rand(), &mut existing_account); + let collected = rent_collector.collect_from_existing_account( + &solana_sdk::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); @@ -180,7 +188,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); + collected = rent_collector.collect_from_existing_account(&pubkey, &mut account, true); assert_eq!(account.lamports(), huge_lamports); assert_eq!(collected, 0); @@ -188,8 +196,33 @@ mod tests { account.set_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); } + + #[test] + fn test_rent_exempt_sysvar() { + let tiny_lamports = 1; + let mut account = AccountSharedData::default(); + account.set_owner(sysvar::id()); + account.set_lamports(tiny_lamports); + + let pubkey = solana_sdk::pubkey::new_rand(); + + assert_eq!(account.rent_epoch(), 0); + + let epoch = 3; + let rent_collector = RentCollector::default().clone_with_epoch(epoch); + + // old behavior: sysvars are special-cased + let collected = rent_collector.collect_from_existing_account(&pubkey, &mut account, false); + assert_eq!(account.lamports(), tiny_lamports); + assert_eq!(collected, 0); + + // new behavior: sysvars are NOT special-cased + let collected = rent_collector.collect_from_existing_account(&pubkey, &mut account, true); + assert_eq!(account.lamports(), 0); + assert_eq!(collected, 1); + } } diff --git a/runtime/src/system_instruction_processor.rs b/runtime/src/system_instruction_processor.rs index a8402bfc94..2d7ba74613 100644 --- a/runtime/src/system_instruction_processor.rs +++ b/runtime/src/system_instruction_processor.rs @@ -120,8 +120,13 @@ fn assign( return Err(InstructionError::MissingRequiredSignature); } - // guard against sysvars being made - if sysvar::check_id(owner) { + // bpf programs are allowed to do this; so this is inconsistent... + // Thus, we're starting to remove this restriction from system instruction + // processor for consistency and fewer special casing by piggybacking onto + // the related feature gate.. + let rent_for_sysvars = invoke_context.is_feature_active(&feature_set::rent_for_sysvars::id()); + if !rent_for_sysvars && sysvar::check_id(owner) { + // guard against sysvars being made ic_msg!(invoke_context, "Assign: cannot assign to sysvar, {}", owner); return Err(SystemError::InvalidProgramId.into()); } @@ -890,7 +895,7 @@ mod tests { } #[test] - fn test_create_sysvar_invalid_id() { + fn test_create_sysvar_invalid_id_with_feature() { // Attempt to create system account in account already owned by another program let from = solana_sdk::pubkey::new_rand(); let from_account = AccountSharedData::new_ref(100, 0, &system_program::id()); @@ -912,7 +917,36 @@ mod tests { &signers, &MockInvokeContext::new(vec![]), ); + assert_eq!(result, Ok(())); + } + #[test] + fn test_create_sysvar_invalid_id_without_feature() { + // Attempt to create system account in account already owned by another program + let from = solana_sdk::pubkey::new_rand(); + let from_account = AccountSharedData::new_ref(100, 0, &system_program::id()); + + let to = solana_sdk::pubkey::new_rand(); + let to_account = AccountSharedData::new_ref(0, 0, &system_program::id()); + + let signers = [from, to].iter().cloned().collect::>(); + let to_address = to.into(); + + let result = create_account( + &KeyedAccount::new(&from, true, &from_account), + &KeyedAccount::new(&to, false, &to_account), + &to_address, + 50, + 2, + &sysvar::id(), + &signers, + &MockInvokeContext { + disabled_features: vec![feature_set::rent_for_sysvars::id()] + .into_iter() + .collect(), + ..MockInvokeContext::new(vec![]) + }, + ); assert_eq!(result, Err(SystemError::InvalidProgramId.into())); } @@ -1025,7 +1059,7 @@ mod tests { } #[test] - fn test_assign_to_sysvar() { + fn test_assign_to_sysvar_with_feature() { let new_owner = sysvar::id(); let from = solana_sdk::pubkey::new_rand(); @@ -1039,6 +1073,30 @@ mod tests { &[from].iter().cloned().collect::>(), &MockInvokeContext::new(vec![]), ), + Ok(()) + ); + } + + #[test] + fn test_assign_to_sysvar_without_feature() { + let new_owner = sysvar::id(); + + let from = solana_sdk::pubkey::new_rand(); + let mut from_account = AccountSharedData::new(100, 0, &system_program::id()); + + assert_eq!( + assign( + &mut from_account, + &from.into(), + &new_owner, + &[from].iter().cloned().collect::>(), + &MockInvokeContext { + disabled_features: vec![feature_set::rent_for_sysvars::id()] + .into_iter() + .collect(), + ..MockInvokeContext::new(vec![]) + }, + ), Err(SystemError::InvalidProgramId.into()) ); } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 4cd330c2b4..9ea436d0d5 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -163,6 +163,10 @@ pub mod neon_evm_compute_budget { solana_sdk::declare_id!("GLrVvDPkQi5PMYUrsYWT9doZhSHr1BVZXqj5DbFps3rS"); } +pub mod rent_for_sysvars { + solana_sdk::declare_id!("BKCPBQQBZqggVnFso5nQ8rQ4RwwogYwjuUt9biBjxwNF"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -203,6 +207,7 @@ lazy_static! { (vote_stake_checked_instructions::id(), "vote/state program checked instructions #18345"), (updated_verify_policy::id(), "Update verify policy"), (neon_evm_compute_budget::id(), "bump neon_evm's compute budget"), + (rent_for_sysvars::id(), "collect rent from accounts owned by sysvars"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/sdk/src/process_instruction.rs b/sdk/src/process_instruction.rs index 33b0ea3a32..d740c87e8d 100644 --- a/sdk/src/process_instruction.rs +++ b/sdk/src/process_instruction.rs @@ -5,7 +5,7 @@ use solana_sdk::{ pubkey::Pubkey, sysvar::Sysvar, }; -use std::{cell::RefCell, fmt::Debug, rc::Rc, sync::Arc}; +use std::{cell::RefCell, collections::HashSet, fmt::Debug, rc::Rc, sync::Arc}; /// Prototype of a native loader entry point /// @@ -334,6 +334,7 @@ pub struct MockInvokeContext<'a> { pub programs: Vec<(Pubkey, ProcessInstructionWithContext)>, pub accounts: Vec<(Pubkey, Rc>)>, pub sysvars: Vec<(Pubkey, Option>>)>, + pub disabled_features: HashSet, } impl<'a> MockInvokeContext<'a> { pub fn new(keyed_accounts: Vec>) -> Self { @@ -348,6 +349,7 @@ impl<'a> MockInvokeContext<'a> { programs: vec![], accounts: vec![], sysvars: vec![], + disabled_features: HashSet::default(), }; invoke_context .invoke_stack @@ -441,8 +443,8 @@ impl<'a> InvokeContext for MockInvokeContext<'a> { None } fn record_instruction(&self, _instruction: &Instruction) {} - fn is_feature_active(&self, _feature_id: &Pubkey) -> bool { - true + fn is_feature_active(&self, feature_id: &Pubkey) -> bool { + !self.disabled_features.contains(feature_id) } fn get_account(&self, pubkey: &Pubkey) -> Option>> { for (key, account) in self.accounts.iter() {