diff --git a/cli-output/src/display.rs b/cli-output/src/display.rs index ec4f8b4a41030a..9d383010488155 100644 --- a/cli-output/src/display.rs +++ b/cli-output/src/display.rs @@ -140,7 +140,7 @@ fn format_account_mode(message: &Message, index: usize) -> String { } else { "-" }, - if message.is_writable(index, /*demote_sysvar_write_locks=*/ true) { + if message.is_writable(index, /*demote_program_write_locks=*/ true) { "w" // comment for consistent rust fmt (no joking; lol) } else { "-" diff --git a/core/src/transaction_status_service.rs b/core/src/transaction_status_service.rs index 6c412e16f45867..4136451768c943 100644 --- a/core/src/transaction_status_service.rs +++ b/core/src/transaction_status_service.rs @@ -111,7 +111,7 @@ impl TransactionStatusService { let fee = fee_calculator.calculate_fee(transaction.message()); let (writable_keys, readonly_keys) = transaction .message - .get_account_keys_by_lock_type(bank.demote_sysvar_write_locks()); + .get_account_keys_by_lock_type(bank.demote_program_write_locks()); let inner_instructions = inner_instructions.map(|inner_instructions| { inner_instructions diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs index 3b1158e0697722..234f0f46bc3a7c 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -19,7 +19,7 @@ use { clock::{Clock, Slot}, entrypoint::{ProgramResult, SUCCESS}, epoch_schedule::EpochSchedule, - feature_set::demote_sysvar_write_locks, + feature_set::demote_program_write_locks, fee_calculator::{FeeCalculator, FeeRateGovernor}, genesis_config::{ClusterType, GenesisConfig}, hash::Hash, @@ -256,14 +256,14 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { } panic!("Program id {} wasn't found in account_infos", program_id); }; - let demote_sysvar_write_locks = - invoke_context.is_feature_active(&demote_sysvar_write_locks::id()); + let demote_program_write_locks = + invoke_context.is_feature_active(&demote_program_write_locks::id()); // TODO don't have the caller's keyed_accounts so can't validate writer or signer escalation or deescalation yet let caller_privileges = message .account_keys .iter() .enumerate() - .map(|(i, _)| message.is_writable(i, demote_sysvar_write_locks)) + .map(|(i, _)| message.is_writable(i, demote_program_write_locks)) .collect::>(); stable_log::program_invoke(&logger, &program_id, invoke_context.invoke_depth()); @@ -334,7 +334,7 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { // Copy writeable account modifications back into the caller's AccountInfos for (i, account_pubkey) in message.account_keys.iter().enumerate() { - if !message.is_writable(i, true) { + if !message.is_writable(i, demote_program_write_locks) { continue; } diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 1d02da6d8262a5..1dcb229e9a550d 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -1773,7 +1773,7 @@ fn test_program_bpf_upgrade_and_invoke_in_same_tx() { "solana_bpf_rust_panic", ); - // Invoke, then upgrade the program, and then invoke again in same tx + // Attempt to invoke, then upgrade the program in same tx let message = Message::new( &[ invoke_instruction.clone(), @@ -1792,10 +1792,12 @@ fn test_program_bpf_upgrade_and_invoke_in_same_tx() { message.clone(), bank.last_blockhash(), ); + // program_id is automatically demoted to readonly, preventing the upgrade, which requires + // writeability let (result, _) = process_transaction_and_record_inner(&bank, tx); assert_eq!( result.unwrap_err(), - TransactionError::InstructionError(2, InstructionError::ProgramFailedToComplete) + TransactionError::InstructionError(1, InstructionError::InvalidArgument) ); } @@ -2076,97 +2078,6 @@ fn test_program_bpf_upgrade_via_cpi() { ); } -#[cfg(feature = "bpf_rust")] -#[test] -fn test_program_bpf_upgrade_self_via_cpi() { - solana_logger::setup(); - - let GenesisConfigInfo { - genesis_config, - mint_keypair, - .. - } = create_genesis_config(50); - let mut bank = Bank::new(&genesis_config); - let (name, id, entrypoint) = solana_bpf_loader_program!(); - bank.add_builtin(&name, id, entrypoint); - let (name, id, entrypoint) = solana_bpf_loader_upgradeable_program!(); - bank.add_builtin(&name, id, entrypoint); - let bank = Arc::new(bank); - let bank_client = BankClient::new_shared(&bank); - let noop_program_id = load_bpf_program( - &bank_client, - &bpf_loader::id(), - &mint_keypair, - "solana_bpf_rust_noop", - ); - - // Deploy upgradeable program - let buffer_keypair = Keypair::new(); - let program_keypair = Keypair::new(); - let program_id = program_keypair.pubkey(); - let authority_keypair = Keypair::new(); - load_upgradeable_bpf_program( - &bank_client, - &mint_keypair, - &buffer_keypair, - &program_keypair, - &authority_keypair, - "solana_bpf_rust_invoke_and_return", - ); - - let mut invoke_instruction = Instruction::new_with_bytes( - program_id, - &[0], - vec![ - AccountMeta::new(noop_program_id, false), - AccountMeta::new(noop_program_id, false), - AccountMeta::new(clock::id(), false), - AccountMeta::new(fees::id(), false), - ], - ); - - // Call the upgraded program - invoke_instruction.data[0] += 1; - let result = - bank_client.send_and_confirm_instruction(&mint_keypair, invoke_instruction.clone()); - assert!(result.is_ok()); - - // Prepare for upgrade - let buffer_keypair = Keypair::new(); - load_upgradeable_buffer( - &bank_client, - &mint_keypair, - &buffer_keypair, - &authority_keypair, - "solana_bpf_rust_panic", - ); - - // Invoke, then upgrade the program, and then invoke again in same tx - let message = Message::new( - &[ - invoke_instruction.clone(), - bpf_loader_upgradeable::upgrade( - &program_id, - &buffer_keypair.pubkey(), - &authority_keypair.pubkey(), - &mint_keypair.pubkey(), - ), - invoke_instruction, - ], - Some(&mint_keypair.pubkey()), - ); - let tx = Transaction::new( - &[&mint_keypair, &authority_keypair], - message.clone(), - bank.last_blockhash(), - ); - let (result, _) = process_transaction_and_record_inner(&bank, tx); - assert_eq!( - result.unwrap_err(), - TransactionError::InstructionError(2, InstructionError::ProgramFailedToComplete) - ); -} - #[cfg(feature = "bpf_rust")] #[test] fn test_program_bpf_set_upgrade_authority_via_cpi() { diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index 8b8b227db7afda..9f353bee3d7396 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -19,7 +19,7 @@ use solana_sdk::{ entrypoint::{MAX_PERMITTED_DATA_INCREASE, SUCCESS}, epoch_schedule::EpochSchedule, feature_set::{ - cpi_data_cost, cpi_share_ro_and_exec_accounts, demote_sysvar_write_locks, + cpi_data_cost, cpi_share_ro_and_exec_accounts, demote_program_write_locks, enforce_aligned_host_addrs, keccak256_syscall_enabled, memory_ops_syscalls, set_upgrade_authority_via_cpi_enabled, sysvar_via_syscall, update_data_on_realloc, }, @@ -2146,7 +2146,7 @@ fn call<'a>( accounts, account_refs, caller_write_privileges, - demote_sysvar_write_locks, + demote_program_write_locks, ) = { let invoke_context = syscall.get_context()?; @@ -2237,7 +2237,7 @@ fn call<'a>( accounts, account_refs, caller_write_privileges, - invoke_context.is_feature_active(&demote_sysvar_write_locks::id()), + invoke_context.is_feature_active(&demote_program_write_locks::id()), ) }; @@ -2263,9 +2263,9 @@ fn call<'a>( for (i, (account, account_ref)) in accounts.iter().zip(account_refs).enumerate() { let account = account.borrow(); if let Some(mut account_ref) = account_ref { - if message.is_writable(i, demote_sysvar_write_locks) && !account.executable { - *account_ref.lamports = account.lamports; - *account_ref.owner = account.owner; + if message.is_writable(i, demote_program_write_locks) && !account.executable() { + *account_ref.lamports = account.lamports(); + *account_ref.owner = *account.owner(); if account_ref.data.len() != account.data().len() { if !account_ref.data.is_empty() { // Only support for `CreateAccount` at this time. diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 16e9f991904f1c..b81efa72194cb1 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -172,9 +172,9 @@ impl Accounts { fn construct_instructions_account( message: &Message, - demote_sysvar_write_locks: bool, + demote_program_write_locks: bool, ) -> AccountSharedData { - let mut data = message.serialize_instructions(demote_sysvar_write_locks); + let mut data = message.serialize_instructions(demote_program_write_locks); // add room for current instruction index. data.resize(data.len() + 2, 0); AccountSharedData::from(Account { @@ -203,9 +203,9 @@ 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 demote_sysvar_write_locks = - feature_set.is_active(&feature_set::demote_sysvar_write_locks::id()); let mut rent_debits = RentDebits::default(); + let demote_program_write_locks = + feature_set.is_active(&feature_set::demote_program_write_locks::id()); for (i, key) in message.account_keys.iter().enumerate() { let account = if message.is_non_loader_key(key, i) { @@ -216,16 +216,16 @@ impl Accounts { if solana_sdk::sysvar::instructions::check_id(key) && feature_set.is_active(&feature_set::instructions_sysvar_enabled::id()) { - if message.is_writable(i, demote_sysvar_write_locks) { + if message.is_writable(i, demote_program_write_locks) { return Err(TransactionError::InvalidAccountIndex); } - Self::construct_instructions_account(message, demote_sysvar_write_locks) + Self::construct_instructions_account(message, demote_program_write_locks) } else { let (account, rent) = self .accounts_db .load(ancestors, key) .map(|(mut account, _)| { - if message.is_writable(i, demote_sysvar_write_locks) { + if message.is_writable(i, demote_program_write_locks) { let rent_due = rent_collector .collect_from_existing_account(&key, &mut account); (account, rent_due) @@ -776,7 +776,7 @@ impl Accounts { tx: &Transaction, result: &Result<()>, locks: &mut AccountLocks, - demote_sysvar_write_locks: bool, + demote_program_write_locks: bool, ) { match result { Err(TransactionError::AccountInUse) => (), @@ -785,7 +785,7 @@ impl Accounts { _ => { let (writable_keys, readonly_keys) = &tx .message() - .get_account_keys_by_lock_type(demote_sysvar_write_locks); + .get_account_keys_by_lock_type(demote_program_write_locks); for k in writable_keys { locks.unlock_write(k); } @@ -817,7 +817,7 @@ impl Accounts { pub fn lock_accounts<'a>( &self, txs: impl Iterator, - demote_sysvar_write_locks: bool, + demote_program_write_locks: bool, ) -> Vec> { use solana_sdk::sanitize::Sanitize; let keys: Vec> = txs @@ -830,7 +830,7 @@ impl Accounts { Ok(tx .message() - .get_account_keys_by_lock_type(demote_sysvar_write_locks)) + .get_account_keys_by_lock_type(demote_program_write_locks)) }) .collect(); let mut account_locks = &mut self.account_locks.lock().unwrap(); @@ -849,7 +849,7 @@ impl Accounts { &self, txs: impl Iterator, results: &[Result<()>], - demote_sysvar_write_locks: bool, + demote_program_write_locks: bool, ) { let mut account_locks = self.account_locks.lock().unwrap(); debug!("bank unlock accounts"); @@ -858,7 +858,7 @@ impl Accounts { tx, lock_result, &mut account_locks, - demote_sysvar_write_locks, + demote_program_write_locks, ); } } @@ -875,8 +875,8 @@ impl Accounts { rent_collector: &RentCollector, last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), fix_recent_blockhashes_sysvar_delay: bool, - demote_sysvar_write_locks: bool, merge_nonce_error_into_system_error: bool, + demote_program_write_locks: bool, ) { let accounts_to_store = self.collect_accounts_to_store( txs, @@ -885,8 +885,8 @@ impl Accounts { rent_collector, last_blockhash_with_fee_calculator, fix_recent_blockhashes_sysvar_delay, - demote_sysvar_write_locks, merge_nonce_error_into_system_error, + demote_program_write_locks, ); self.accounts_db.store_cached(slot, &accounts_to_store); } @@ -902,6 +902,7 @@ impl Accounts { self.accounts_db.add_root(slot) } + #[allow(clippy::too_many_arguments)] fn collect_accounts_to_store<'a>( &self, txs: impl Iterator, @@ -910,8 +911,8 @@ impl Accounts { rent_collector: &RentCollector, last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), fix_recent_blockhashes_sysvar_delay: bool, - demote_sysvar_write_locks: bool, merge_nonce_error_into_system_error: bool, + demote_program_write_locks: 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() { @@ -964,7 +965,7 @@ impl Accounts { fee_payer_index = Some(i); } let is_fee_payer = Some(i) == fee_payer_index; - if message.is_writable(i, demote_sysvar_write_locks) + if message.is_writable(i, demote_program_write_locks) && (res.is_ok() || (maybe_nonce_rollback.is_some() && (is_nonce_account || is_fee_payer))) { @@ -1724,6 +1725,8 @@ mod tests { accounts.store_slow_uncached(0, &keypair2.pubkey(), &account2); accounts.store_slow_uncached(0, &keypair3.pubkey(), &account3); + let demote_program_write_locks = true; + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; let message = Message::new_with_compiled_instructions( 1, @@ -1734,10 +1737,7 @@ mod tests { instructions, ); let tx = Transaction::new(&[&keypair0], message, Hash::default()); - let results0 = accounts.lock_accounts( - [tx.clone()].iter(), - true, // demote_sysvar_write_locks - ); + let results0 = accounts.lock_accounts([tx.clone()].iter(), demote_program_write_locks); assert!(results0[0].is_ok()); assert_eq!( @@ -1772,10 +1772,7 @@ mod tests { ); let tx1 = Transaction::new(&[&keypair1], message, Hash::default()); let txs = vec![tx0, tx1]; - let results1 = accounts.lock_accounts( - txs.iter(), - true, // demote_sysvar_write_locks - ); + let results1 = accounts.lock_accounts(txs.iter(), demote_program_write_locks); assert!(results1[0].is_ok()); // Read-only account (keypair1) can be referenced multiple times assert!(results1[1].is_err()); // Read-only account (keypair1) cannot also be locked as writable @@ -1790,16 +1787,8 @@ mod tests { 2 ); - accounts.unlock_accounts( - [tx].iter(), - &results0, - true, // demote_sysvar_write_locks - ); - accounts.unlock_accounts( - txs.iter(), - &results1, - true, // demote_sysvar_write_locks - ); + accounts.unlock_accounts([tx].iter(), &results0, demote_program_write_locks); + accounts.unlock_accounts(txs.iter(), &results1, demote_program_write_locks); let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; let message = Message::new_with_compiled_instructions( 1, @@ -1810,10 +1799,7 @@ mod tests { instructions, ); let tx = Transaction::new(&[&keypair1], message, Hash::default()); - let results2 = accounts.lock_accounts( - [tx].iter(), - true, // demote_sysvar_write_locks - ); + let results2 = accounts.lock_accounts([tx].iter(), demote_program_write_locks); assert!(results2[0].is_ok()); // Now keypair1 account can be locked as writable // Check that read-only lock with zero references is deleted @@ -1849,6 +1835,8 @@ mod tests { accounts.store_slow_uncached(0, &keypair1.pubkey(), &account1); accounts.store_slow_uncached(0, &keypair2.pubkey(), &account2); + let demote_program_write_locks = true; + let accounts_arc = Arc::new(accounts); let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; @@ -1881,20 +1869,15 @@ mod tests { let exit_clone = exit_clone.clone(); loop { let txs = vec![writable_tx.clone()]; - let results = accounts_clone.clone().lock_accounts( - txs.iter(), - true, // demote_sysvar_write_locks - ); + let results = accounts_clone + .clone() + .lock_accounts(txs.iter(), demote_program_write_locks); for result in results.iter() { if result.is_ok() { counter_clone.clone().fetch_add(1, Ordering::SeqCst); } } - accounts_clone.unlock_accounts( - txs.iter(), - &results, - true, // demote_sysvar_write_locks - ); + accounts_clone.unlock_accounts(txs.iter(), &results, demote_program_write_locks); if exit_clone.clone().load(Ordering::Relaxed) { break; } @@ -1903,25 +1886,84 @@ mod tests { let counter_clone = counter; for _ in 0..5 { let txs = vec![readonly_tx.clone()]; - let results = accounts_arc.clone().lock_accounts( - txs.iter(), - true, // demote_sysvar_write_locks - ); + let results = accounts_arc + .clone() + .lock_accounts(txs.iter(), demote_program_write_locks); if results[0].is_ok() { let counter_value = counter_clone.clone().load(Ordering::SeqCst); thread::sleep(time::Duration::from_millis(50)); assert_eq!(counter_value, counter_clone.clone().load(Ordering::SeqCst)); } - accounts_arc.unlock_accounts( - txs.iter(), - &results, - true, // demote_sysvar_write_locks - ); + accounts_arc.unlock_accounts(txs.iter(), &results, demote_program_write_locks); thread::sleep(time::Duration::from_millis(50)); } exit.store(true, Ordering::Relaxed); } + #[test] + fn test_demote_program_write_locks() { + let keypair0 = Keypair::new(); + let keypair1 = Keypair::new(); + let keypair2 = Keypair::new(); + let keypair3 = Keypair::new(); + + let account0 = AccountSharedData::new(1, 0, &Pubkey::default()); + let account1 = AccountSharedData::new(2, 0, &Pubkey::default()); + let account2 = AccountSharedData::new(3, 0, &Pubkey::default()); + let account3 = AccountSharedData::new(4, 0, &Pubkey::default()); + + let accounts = Accounts::new_with_config( + Vec::new(), + &ClusterType::Development, + AccountSecondaryIndexes::default(), + false, + ); + accounts.store_slow_uncached(0, &keypair0.pubkey(), &account0); + accounts.store_slow_uncached(0, &keypair1.pubkey(), &account1); + accounts.store_slow_uncached(0, &keypair2.pubkey(), &account2); + accounts.store_slow_uncached(0, &keypair3.pubkey(), &account3); + + let demote_program_write_locks = true; + + let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; + let message = Message::new_with_compiled_instructions( + 1, + 0, + 0, // All accounts marked as writable + vec![keypair0.pubkey(), keypair1.pubkey(), native_loader::id()], + Hash::default(), + instructions, + ); + let tx = Transaction::new(&[&keypair0], message, Hash::default()); + let results0 = accounts.lock_accounts([tx].iter(), demote_program_write_locks); + + assert!(results0[0].is_ok()); + // Instruction program-id account demoted to readonly + assert_eq!( + *accounts + .account_locks + .lock() + .unwrap() + .readonly_locks + .get(&native_loader::id()) + .unwrap(), + 1 + ); + // Non-program accounts remain writable + assert!(accounts + .account_locks + .lock() + .unwrap() + .write_locks + .contains(&keypair0.pubkey())); + assert!(accounts + .account_locks + .lock() + .unwrap() + .write_locks + .contains(&keypair1.pubkey())); + } + #[test] fn test_collect_accounts_to_store() { let keypair0 = Keypair::new(); @@ -2009,8 +2051,8 @@ mod tests { &rent_collector, &(Hash::default(), FeeCalculator::default()), true, - true, // demote_sysvar_write_locks true, // merge_nonce_error_into_system_error + true, // demote_program_write_locks ); assert_eq!(collected_accounts.len(), 2); assert!(collected_accounts @@ -2395,8 +2437,8 @@ mod tests { &rent_collector, &(next_blockhash, FeeCalculator::default()), true, - true, // demote_sysvar_write_locks true, // merge_nonce_error_into_system_error + true, // demote_program_write_locks ); assert_eq!(collected_accounts.len(), 2); assert_eq!( @@ -2512,8 +2554,8 @@ mod tests { &rent_collector, &(next_blockhash, FeeCalculator::default()), true, - true, // demote_sysvar_write_locks true, // merge_nonce_error_into_system_error + true, // demote_program_write_locks ); 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 197129652dcf20..867737bf2fc797 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2468,11 +2468,6 @@ impl Bank { tick_height % self.ticks_per_slot == 0 } - pub fn demote_sysvar_write_locks(&self) -> bool { - self.feature_set - .is_active(&feature_set::demote_sysvar_write_locks::id()) - } - pub fn prepare_batch<'a, 'b>( &'a self, txs: impl Iterator, @@ -2480,9 +2475,9 @@ impl Bank { let hashed_txs: Vec = txs.map(HashedTransaction::from).collect(); let lock_results = self.rc.accounts.lock_accounts( hashed_txs.as_transactions_iter(), - self.demote_sysvar_write_locks(), + self.demote_program_write_locks(), ); - TransactionBatch::new(lock_results, &self, Cow::Owned(hashed_txs)) + TransactionBatch::new(lock_results, self, Cow::Owned(hashed_txs)) } pub fn prepare_hashed_batch<'a, 'b>( @@ -2491,9 +2486,9 @@ impl Bank { ) -> TransactionBatch<'a, 'b> { let lock_results = self.rc.accounts.lock_accounts( hashed_txs.as_transactions_iter(), - self.demote_sysvar_write_locks(), + self.demote_program_write_locks(), ); - TransactionBatch::new(lock_results, &self, Cow::Borrowed(hashed_txs)) + TransactionBatch::new(lock_results, self, Cow::Borrowed(hashed_txs)) } pub(crate) fn prepare_simulation_batch<'a, 'b>( @@ -2563,7 +2558,7 @@ impl Bank { self.rc.accounts.unlock_accounts( batch.transactions_iter(), batch.lock_results(), - self.demote_sysvar_write_locks(), + self.demote_program_write_locks(), ) } } @@ -3303,8 +3298,8 @@ impl Bank { &self.rent_collector, &self.last_blockhash_with_fee_calculator(), self.fix_recent_blockhashes_sysvar_delay(), - self.demote_sysvar_write_locks(), self.merge_nonce_error_into_system_error(), + self.demote_program_write_locks(), ); let rent_debits = self.collect_rent(executed, loaded_accounts); @@ -4853,6 +4848,11 @@ impl Bank { .is_active(&feature_set::merge_nonce_error_into_system_error::id()) } + pub fn demote_program_write_locks(&self) -> bool { + self.feature_set + .is_active(&feature_set::demote_program_write_locks::id()) + } + // Check if the wallclock time from bank creation to now has exceeded the allotted // time for transaction processing pub fn should_bank_still_be_processing_txs( @@ -7502,13 +7502,14 @@ pub(crate) mod tests { assert_eq!(bank.get_balance(&sysvar_pubkey), 1); bank.transfer(500, &mint_keypair, &normal_pubkey).unwrap(); - bank.transfer(500, &mint_keypair, &sysvar_pubkey).unwrap(); + bank.transfer(500, &mint_keypair, &sysvar_pubkey) + .unwrap_err(); assert_eq!(bank.get_balance(&normal_pubkey), 500); - assert_eq!(bank.get_balance(&sysvar_pubkey), 501); + assert_eq!(bank.get_balance(&sysvar_pubkey), 1); let bank = Arc::new(new_from_parent(&bank)); assert_eq!(bank.get_balance(&normal_pubkey), 500); - assert_eq!(bank.get_balance(&sysvar_pubkey), 501); + assert_eq!(bank.get_balance(&sysvar_pubkey), 1); } #[test] diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 21edde69acdb6d..b3d9a82131c99e 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -9,7 +9,7 @@ use solana_sdk::{ account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, feature_set::{ - cpi_share_ro_and_exec_accounts, demote_sysvar_write_locks, instructions_sysvar_enabled, + cpi_share_ro_and_exec_accounts, demote_program_write_locks, instructions_sysvar_enabled, updated_verify_policy, FeatureSet, }, ic_msg, @@ -359,7 +359,8 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { &self.rent, caller_write_privileges, &mut self.timings, - self.feature_set.is_active(&demote_sysvar_write_locks::id()), + self.feature_set + .is_active(&demote_program_write_locks::id()), self.feature_set.is_active(&updated_verify_policy::id()), ), None => Err(InstructionError::GenericError), // Should never happen @@ -573,7 +574,7 @@ impl MessageProcessor { instruction: &'a CompiledInstruction, executable_accounts: &'a [(Pubkey, Rc>)], accounts: &'a [Rc>], - demote_sysvar_write_locks: bool, + demote_program_write_locks: bool, ) -> Vec> { let mut keyed_accounts = create_keyed_readonly_accounts(&executable_accounts); let mut keyed_accounts2: Vec<_> = instruction @@ -584,7 +585,7 @@ impl MessageProcessor { let index = index as usize; let key = &message.account_keys[index]; let account = &accounts[index]; - if message.is_writable(index, demote_sysvar_write_locks) { + if message.is_writable(index, demote_program_write_locks) { KeyedAccount::new(key, is_signer, account) } else { KeyedAccount::new_readonly(key, is_signer, account) @@ -735,7 +736,7 @@ impl MessageProcessor { accounts, account_refs, caller_write_privileges, - demote_sysvar_write_locks, + demote_program_write_locks, ) = { let invoke_context = invoke_context.borrow(); @@ -828,7 +829,7 @@ impl MessageProcessor { accounts, account_refs, caller_write_privileges, - invoke_context.is_feature_active(&demote_sysvar_write_locks::id()), + invoke_context.is_feature_active(&demote_program_write_locks::id()), ) }; @@ -847,7 +848,7 @@ impl MessageProcessor { let invoke_context = invoke_context.borrow(); for (i, (account, account_ref)) in accounts.iter().zip(account_refs).enumerate() { let account = account.borrow(); - if message.is_writable(i, demote_sysvar_write_locks) && !account.executable { + if message.is_writable(i, demote_program_write_locks) && !account.executable { account_ref.try_account_ref_mut()?.lamports = account.lamports; account_ref.try_account_ref_mut()?.owner = account.owner; if account_ref.data_len()? != account.data().len() @@ -890,15 +891,15 @@ impl MessageProcessor { accounts, Some(caller_write_privileges), )?; - let demote_sysvar_write_locks = - invoke_context.is_feature_active(&demote_sysvar_write_locks::id()); // Construct keyed accounts + let demote_program_write_locks = + invoke_context.is_feature_active(&demote_program_write_locks::id()); let keyed_accounts = Self::create_keyed_accounts( message, instruction, executable_accounts, accounts, - demote_sysvar_write_locks, + demote_program_write_locks, ); // Invoke callee @@ -961,6 +962,7 @@ impl MessageProcessor { } /// Verify the results of an instruction + #[allow(clippy::too_many_arguments)] pub fn verify( message: &Message, instruction: &CompiledInstruction, @@ -969,8 +971,8 @@ impl MessageProcessor { accounts: &[Rc>], rent: &Rent, timings: &mut ExecuteDetailsTimings, - demote_sysvar_write_locks: bool, updated_verify_policy: bool, + demote_program_write_locks: bool, ) -> Result<(), InstructionError> { // Verify all executable accounts have zero outstanding refs Self::verify_account_references(executable_accounts)?; @@ -989,7 +991,7 @@ impl MessageProcessor { let account = accounts[account_index].borrow(); pre_accounts[unique_index].verify( &program_id, - message.is_writable(account_index, demote_sysvar_write_locks), + message.is_writable(account_index, demote_program_write_locks), rent, &account, timings, @@ -1020,7 +1022,7 @@ impl MessageProcessor { rent: &Rent, caller_write_privileges: Option<&[bool]>, timings: &mut ExecuteDetailsTimings, - demote_sysvar_write_locks: bool, + demote_program_write_locks: bool, updated_verify_policy: bool, ) -> Result<(), InstructionError> { // Verify the per-account instruction results @@ -1032,7 +1034,7 @@ impl MessageProcessor { let is_writable = if let Some(caller_write_privileges) = caller_write_privileges { caller_write_privileges[account_index] } else { - message.is_writable(account_index, demote_sysvar_write_locks) + message.is_writable(account_index, demote_program_write_locks) }; // Find the matching PreAccount for pre_account in pre_accounts.iter_mut() { @@ -1093,7 +1095,6 @@ impl MessageProcessor { feature_set: Arc, bpf_compute_budget: BpfComputeBudget, timings: &mut ExecuteDetailsTimings, - demote_sysvar_write_locks: bool, account_db: Arc, ancestors: &Ancestors, ) -> Result<(), InstructionError> { @@ -1129,12 +1130,14 @@ impl MessageProcessor { account_db, ancestors, ); + let demote_program_write_locks = + invoke_context.is_feature_active(&demote_program_write_locks::id()); let keyed_accounts = Self::create_keyed_accounts( message, instruction, executable_accounts, accounts, - demote_sysvar_write_locks, + demote_program_write_locks, ); self.process_instruction( program_id, @@ -1150,8 +1153,8 @@ impl MessageProcessor { accounts, &rent_collector.rent, timings, - demote_sysvar_write_locks, invoke_context.is_feature_active(&updated_verify_policy::id()), + demote_program_write_locks, )?; timings.accumulate(&invoke_context.timings); @@ -1180,7 +1183,6 @@ impl MessageProcessor { account_db: Arc, ancestors: &Ancestors, ) -> Result<(), TransactionError> { - let demote_sysvar_write_locks = feature_set.is_active(&demote_sysvar_write_locks::id()); for (instruction_index, instruction) in message.instructions.iter().enumerate() { let instruction_recorder = instruction_recorders .as_ref() @@ -1199,7 +1201,6 @@ impl MessageProcessor { feature_set.clone(), bpf_compute_budget, timings, - demote_sysvar_write_locks, account_db.clone(), ancestors, ) @@ -2168,6 +2169,9 @@ mod tests { ]; let programs: Vec<(_, ProcessInstructionWithContext)> = vec![(callee_program_id, mock_process_instruction)]; + let feature_set = FeatureSet::all_enabled(); + let demote_program_write_locks = feature_set.is_active(&demote_program_write_locks::id()); + let ancestors = Ancestors::default(); let mut invoke_context = ThisInvokeContext::new( &caller_program_id, @@ -2184,7 +2188,7 @@ mod tests { BpfComputeBudget::default(), Rc::new(RefCell::new(Executors::default())), None, - Arc::new(FeatureSet::all_enabled()), + Arc::new(feature_set), Arc::new(Accounts::default()), &ancestors, ); @@ -2200,13 +2204,12 @@ mod tests { &MockInstruction::NoopSuccess, metas.clone(), ); - let demote_sysvar_write_locks = true; let message = Message::new(&[instruction], None); let caller_write_privileges = message .account_keys .iter() .enumerate() - .map(|(i, _)| message.is_writable(i, demote_sysvar_write_locks)) + .map(|(i, _)| message.is_writable(i, demote_program_write_locks)) .collect::>(); assert_eq!( MessageProcessor::process_cross_program_instruction( @@ -2241,7 +2244,7 @@ mod tests { .account_keys .iter() .enumerate() - .map(|(i, _)| message.is_writable(i, demote_sysvar_write_locks)) + .map(|(i, _)| message.is_writable(i, demote_program_write_locks)) .collect::>(); assert_eq!( MessageProcessor::process_cross_program_instruction( diff --git a/sdk/benches/serialize_instructions.rs b/sdk/benches/serialize_instructions.rs index 1367c0474bfae4..201089a850caae 100644 --- a/sdk/benches/serialize_instructions.rs +++ b/sdk/benches/serialize_instructions.rs @@ -14,6 +14,8 @@ fn make_instructions() -> Vec { vec![inst; 4] } +const DEMOTE_PROGRAM_WRITE_LOCKS: bool = true; + #[bench] fn bench_bincode_instruction_serialize(b: &mut Bencher) { let instructions = make_instructions(); @@ -27,9 +29,7 @@ fn bench_manual_instruction_serialize(b: &mut Bencher) { let instructions = make_instructions(); let message = Message::new(&instructions, None); b.iter(|| { - test::black_box(message.serialize_instructions( - true, // demote_sysvar_write_locks - )); + test::black_box(message.serialize_instructions(DEMOTE_PROGRAM_WRITE_LOCKS)); }); } @@ -46,9 +46,7 @@ fn bench_bincode_instruction_deserialize(b: &mut Bencher) { fn bench_manual_instruction_deserialize(b: &mut Bencher) { let instructions = make_instructions(); let message = Message::new(&instructions, None); - let serialized = message.serialize_instructions( - true, // demote_sysvar_write_locks - ); + let serialized = message.serialize_instructions(DEMOTE_PROGRAM_WRITE_LOCKS); b.iter(|| { for i in 0..instructions.len() { test::black_box(instructions::load_instruction_at(i, &serialized).unwrap()); @@ -60,9 +58,7 @@ fn bench_manual_instruction_deserialize(b: &mut Bencher) { fn bench_manual_instruction_deserialize_single(b: &mut Bencher) { let instructions = make_instructions(); let message = Message::new(&instructions, None); - let serialized = message.serialize_instructions( - true, // demote_sysvar_write_locks - ); + let serialized = message.serialize_instructions(DEMOTE_PROGRAM_WRITE_LOCKS); b.iter(|| { test::black_box(instructions::load_instruction_at(3, &serialized).unwrap()); }); diff --git a/sdk/program/src/message.rs b/sdk/program/src/message.rs index 0ffe6963bf5094..a2113337bdcdbb 100644 --- a/sdk/program/src/message.rs +++ b/sdk/program/src/message.rs @@ -337,6 +337,16 @@ impl Message { .collect() } + pub fn is_key_called_as_program(&self, key_index: usize) -> bool { + if let Ok(key_index) = u8::try_from(key_index) { + self.instructions + .iter() + .any(|ix| ix.program_id_index == key_index) + } else { + false + } + } + pub fn is_key_passed_to_program(&self, index: usize) -> bool { if let Ok(index) = u8::try_from(index) { for ix in self.instructions.iter() { @@ -363,7 +373,7 @@ impl Message { self.program_position(i).is_some() } - pub fn is_writable(&self, i: usize, demote_sysvar_write_locks: bool) -> bool { + pub fn is_writable(&self, i: usize, demote_program_write_locks: bool) -> bool { (i < (self.header.num_required_signatures - self.header.num_readonly_signed_accounts) as usize || (i >= self.header.num_required_signatures as usize @@ -371,9 +381,9 @@ impl Message { - self.header.num_readonly_unsigned_accounts as usize)) && !{ let key = self.account_keys[i]; - demote_sysvar_write_locks - && (sysvar::is_sysvar_id(&key) || BUILTIN_PROGRAMS_KEYS.contains(&key)) + sysvar::is_sysvar_id(&key) || BUILTIN_PROGRAMS_KEYS.contains(&key) } + && !(demote_program_write_locks && self.is_key_called_as_program(i)) } pub fn is_signer(&self, i: usize) -> bool { @@ -382,12 +392,12 @@ impl Message { pub fn get_account_keys_by_lock_type( &self, - demote_sysvar_write_locks: bool, + demote_program_write_locks: bool, ) -> (Vec<&Pubkey>, Vec<&Pubkey>) { let mut writable_keys = vec![]; let mut readonly_keys = vec![]; for (i, key) in self.account_keys.iter().enumerate() { - if self.is_writable(i, demote_sysvar_write_locks) { + if self.is_writable(i, demote_program_write_locks) { writable_keys.push(key); } else { readonly_keys.push(key); @@ -409,7 +419,7 @@ impl Message { // 35..67 - program_id // 67..69 - data len - u16 // 69..data_len - data - pub fn serialize_instructions(&self, demote_sysvar_write_locks: bool) -> Vec { + pub fn serialize_instructions(&self, demote_program_write_locks: bool) -> Vec { // 64 bytes is a reasonable guess, calculating exactly is slower in benchmarks let mut data = Vec::with_capacity(self.instructions.len() * (32 * 2)); append_u16(&mut data, self.instructions.len() as u16); @@ -424,7 +434,7 @@ impl Message { for account_index in &instruction.accounts { let account_index = *account_index as usize; let is_signer = self.is_signer(account_index); - let is_writable = self.is_writable(account_index, demote_sysvar_write_locks); + let is_writable = self.is_writable(account_index, demote_program_write_locks); let mut meta_byte = 0; if is_signer { meta_byte |= 1 << Self::IS_SIGNER_BIT; @@ -863,13 +873,13 @@ mod tests { recent_blockhash: Hash::default(), instructions: vec![], }; - let demote_sysvar_write_locks = true; - assert_eq!(message.is_writable(0, demote_sysvar_write_locks), true); - assert_eq!(message.is_writable(1, demote_sysvar_write_locks), false); - assert_eq!(message.is_writable(2, demote_sysvar_write_locks), false); - assert_eq!(message.is_writable(3, demote_sysvar_write_locks), true); - assert_eq!(message.is_writable(4, demote_sysvar_write_locks), true); - assert_eq!(message.is_writable(5, demote_sysvar_write_locks), false); + let demote_program_write_locks = true; + assert!(message.is_writable(0, demote_program_write_locks)); + assert!(!message.is_writable(1, demote_program_write_locks)); + assert!(!message.is_writable(2, demote_program_write_locks)); + assert!(message.is_writable(3, demote_program_write_locks)); + assert!(message.is_writable(4, demote_program_write_locks)); + assert!(!message.is_writable(5, demote_program_write_locks)); } #[test] @@ -897,9 +907,7 @@ mod tests { Some(&id1), ); assert_eq!( - message.get_account_keys_by_lock_type( - true, // demote_sysvar_write_locks - ), + message.get_account_keys_by_lock_type(/*demote_program_write_locks=*/ true), (vec![&id1, &id0], vec![&id3, &id2, &program_id]) ); } @@ -929,9 +937,7 @@ mod tests { ]; let message = Message::new(&instructions, Some(&id1)); - let serialized = message.serialize_instructions( - true, // demote_sysvar_write_locks - ); + let serialized = message.serialize_instructions(/*demote_program_write_locks=*/ true); for (i, instruction) in instructions.iter().enumerate() { assert_eq!( Message::deserialize_instruction(i, &serialized).unwrap(), @@ -952,9 +958,7 @@ mod tests { ]; let message = Message::new(&instructions, Some(&id1)); - let serialized = message.serialize_instructions( - true, // demote_sysvar_write_locks - ); + let serialized = message.serialize_instructions(/*demote_program_write_locks=*/ true); assert_eq!( Message::deserialize_instruction(instructions.len(), &serialized).unwrap_err(), SanitizeError::IndexOutOfBounds, diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 010e0c1847cc22..56c1a57bc63612 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -107,10 +107,6 @@ pub mod upgradeable_close_instruction { solana_sdk::declare_id!("FsPaByos3gA9bUEhp3EimQpQPCoSvCEigHod496NmABQ"); } -pub mod demote_sysvar_write_locks { - solana_sdk::declare_id!("86LJYRuq2zgtHuL3FccR6hqFJQMQkFoun4knAxcPiF1P"); -} - pub mod sysvar_via_syscall { solana_sdk::declare_id!("7411E6gFQLDhQkdRjmpXwM1hzHMMoYQUjHicmvGPC1Nf"); } @@ -170,6 +166,10 @@ pub mod spl_token_v2_set_authority_fix { solana_sdk::declare_id!("FToKNBYyiF4ky9s8WsmLBXHCht17Ek7RXaLZGHzzQhJ1"); } +pub mod demote_program_write_locks { + solana_sdk::declare_id!("3E3jV7v9VcdJL8iYZUMax9DiDno8j7EWUVbhm9RtShj2"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -195,7 +195,6 @@ lazy_static! { (require_stake_for_gossip::id(), "require stakes for propagating crds values through gossip #15561"), (cpi_data_cost::id(), "charge the compute budget for data passed via CPI"), (upgradeable_close_instruction::id(), "close upgradeable buffer accounts"), - (demote_sysvar_write_locks::id(), "demote builtins and sysvar write locks to readonly #15497"), (sysvar_via_syscall::id(), "provide sysvars via syscalls"), (check_duplicates_by_hash::id(), "use transaction message hash for duplicate check"), (enforce_aligned_host_addrs::id(), "enforce aligned host addresses"), @@ -211,6 +210,7 @@ lazy_static! { (libsecp256k1_0_5_upgrade_enabled::id(), "upgrade libsecp256k1 to v0.5.0"), (merge_nonce_error_into_system_error::id(), "merge NonceError into SystemError"), (spl_token_v2_set_authority_fix::id(), "spl-token set_authority fix"), + (demote_program_write_locks::id(), "demote program write locks to readonly #19593"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/transaction-status/src/parse_accounts.rs b/transaction-status/src/parse_accounts.rs index 20bd74a56b13d5..197643450b6eab 100644 --- a/transaction-status/src/parse_accounts.rs +++ b/transaction-status/src/parse_accounts.rs @@ -13,7 +13,7 @@ pub fn parse_accounts(message: &Message) -> Vec { for (i, account_key) in message.account_keys.iter().enumerate() { accounts.push(ParsedAccount { pubkey: account_key.to_string(), - writable: message.is_writable(i, /*demote_sysvar_write_locks=*/ true), + writable: message.is_writable(i, /*demote_program_write_locks=*/ true), signer: message.is_signer(i), }); }