From 54c68ea83f845cc6fdc26d1d0428ac9a8a0e8315 Mon Sep 17 00:00:00 2001 From: sakridge Date: Tue, 30 Mar 2021 10:05:09 -0700 Subject: [PATCH] Drop write lock on sysvars (#15497) * Drop write lock on sysvars * adds env var for demoting sysvar write lock demotion * moves demote logic to is_writable * feature gates sysvar write lock demotion * adds builtins to write lock demotion * adds system program id to builtins * adds Feature111... * adds an abi-freeze test * mvines set of builtin program keys Co-authored-by: Michael Vines * update tests * adds bpf loader keys * Add test sysvar * Plumb demote_sysvar to is_writable * more plumbing of demote_sysvar_write_locks to is_writable * patches test_program_bpf_instruction_introspection * hard codes demote_sysvar_write_locks to false for serialization/encoding methods * Revert "hard codes demote_sysvar_write_locks to false for serialization/encoding methods" This reverts commit ae3e2d2e777437bddd753933097a210dcbc1b1fc. * change the hardcoded ones to demote_sysvar_write_locks=true * Use data_as_mut_slice Co-authored-by: behzad nouri Co-authored-by: Michael Vines --- cli-output/src/display.rs | 2 +- core/src/transaction_status_service.rs | 5 +- program-test/src/lib.rs | 5 +- programs/bpf/tests/programs.rs | 4 +- programs/bpf_loader/src/syscalls.rs | 17 ++++- runtime/src/accounts.rs | 96 ++++++++++++++++++------ runtime/src/bank.rs | 84 ++++++++++++++++++++- runtime/src/message_processor.rs | 57 ++++++++++---- sdk/benches/serialize_instructions.rs | 12 ++- sdk/program/src/message.rs | 96 +++++++++++++++++++----- sdk/src/feature_set.rs | 5 ++ transaction-status/src/parse_accounts.rs | 2 +- 12 files changed, 316 insertions(+), 69 deletions(-) diff --git a/cli-output/src/display.rs b/cli-output/src/display.rs index fdcf7e9e1..d3f6465da 100644 --- a/cli-output/src/display.rs +++ b/cli-output/src/display.rs @@ -133,7 +133,7 @@ fn format_account_mode(message: &Message, index: usize) -> String { } else { "-" }, - if message.is_writable(index) { + if message.is_writable(index, /*demote_sysvar_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 1de4db037..babf80350 100644 --- a/core/src/transaction_status_service.rs +++ b/core/src/transaction_status_service.rs @@ -106,8 +106,9 @@ impl TransactionStatusService { }) .expect("FeeCalculator must exist"); let fee = fee_calculator.calculate_fee(transaction.message()); - let (writable_keys, readonly_keys) = - transaction.message.get_account_keys_by_lock_type(); + let (writable_keys, readonly_keys) = transaction + .message + .get_account_keys_by_lock_type(bank.demote_sysvar_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 3a791e49e..4ee39c776 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -30,6 +30,7 @@ use { solana_sdk::{ account::{Account, AccountSharedData, ReadableAccount}, clock::Slot, + feature_set::demote_sysvar_write_locks, genesis_config::{ClusterType, GenesisConfig}, keyed_account::KeyedAccount, process_instruction::{ @@ -244,12 +245,14 @@ impl 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()); // 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)) + .map(|(i, _)| message.is_writable(i, demote_sysvar_write_locks)) .collect::>(); stable_log::program_invoke(&logger, &program_id, invoke_context.invoke_depth()); diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 77768ff40..4854345b4 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -1323,7 +1323,9 @@ fn test_program_bpf_instruction_introspection() { let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); assert_eq!( result.unwrap_err().unwrap(), - TransactionError::InvalidAccountIndex + // sysvar write locks are demoted to read only. So this will no longer + // cause InvalidAccountIndex error. + TransactionError::InstructionError(0, InstructionError::ProgramFailedToComplete), ); // No accounts, should error diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index 6aea17e69..42722d16d 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -16,7 +16,10 @@ use solana_sdk::{ bpf_loader, bpf_loader_deprecated, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, entrypoint::{MAX_PERMITTED_DATA_INCREASE, SUCCESS}, - feature_set::{cpi_data_cost, cpi_share_ro_and_exec_accounts, ristretto_mul_syscall_enabled}, + feature_set::{ + cpi_data_cost, cpi_share_ro_and_exec_accounts, demote_sysvar_write_locks, + ristretto_mul_syscall_enabled, + }, hash::{Hasher, HASH_BYTES}, ic_msg, instruction::{AccountMeta, Instruction, InstructionError}, @@ -1571,7 +1574,14 @@ fn call<'a>( signers_seeds_len: u64, memory_mapping: &MemoryMapping, ) -> Result> { - let (message, executables, accounts, account_refs, caller_write_privileges) = { + let ( + message, + executables, + accounts, + account_refs, + caller_write_privileges, + demote_sysvar_write_locks, + ) = { let invoke_context = syscall.get_context()?; invoke_context @@ -1653,6 +1663,7 @@ fn call<'a>( accounts, account_refs, caller_write_privileges, + invoke_context.is_feature_active(&demote_sysvar_write_locks::id()), ) }; @@ -1678,7 +1689,7 @@ fn call<'a>( for (i, (account, account_ref)) in accounts.iter().zip(account_refs).enumerate() { let account = account.borrow(); if let Some(account_ref) = account_ref { - if message.is_writable(i) && !account.executable { + if message.is_writable(i, demote_sysvar_write_locks) && !account.executable { *account_ref.lamports = account.lamports; *account_ref.owner = account.owner; if account_ref.data.len() != account.data().len() { diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 963b198a1..9bc03db43 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -163,8 +163,11 @@ impl Accounts { false } - fn construct_instructions_account(message: &Message) -> AccountSharedData { - let mut data = message.serialize_instructions(); + fn construct_instructions_account( + message: &Message, + demote_sysvar_write_locks: bool, + ) -> AccountSharedData { + let mut data = message.serialize_instructions(demote_sysvar_write_locks); // add room for current instruction index. data.resize(data.len() + 2, 0); AccountSharedData::from(Account { @@ -193,6 +196,8 @@ 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()); for (i, key) in message.account_keys.iter().enumerate() { let account = if message.is_non_loader_key(key, i) { @@ -203,16 +208,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) { + if message.is_writable(i, demote_sysvar_write_locks) { return Err(TransactionError::InvalidAccountIndex); } - Self::construct_instructions_account(message) + Self::construct_instructions_account(message, demote_sysvar_write_locks) } else { let (account, rent) = self .accounts_db .load(ancestors, key) .map(|(mut account, _)| { - if message.is_writable(i) { + if message.is_writable(i, demote_sysvar_write_locks) { let rent_due = rent_collector .collect_from_existing_account(&key, &mut account); (account, rent_due) @@ -753,13 +758,21 @@ impl Accounts { Ok(()) } - fn unlock_account(&self, tx: &Transaction, result: &Result<()>, locks: &mut AccountLocks) { + fn unlock_account( + &self, + tx: &Transaction, + result: &Result<()>, + locks: &mut AccountLocks, + demote_sysvar_write_locks: bool, + ) { match result { Err(TransactionError::AccountInUse) => (), Err(TransactionError::SanitizeFailure) => (), Err(TransactionError::AccountLoadedTwice) => (), _ => { - let (writable_keys, readonly_keys) = &tx.message().get_account_keys_by_lock_type(); + let (writable_keys, readonly_keys) = &tx + .message() + .get_account_keys_by_lock_type(demote_sysvar_write_locks); for k in writable_keys { locks.unlock_write(k); } @@ -792,6 +805,7 @@ impl Accounts { &self, txs: &[Transaction], txs_iteration_order: Option<&[usize]>, + demote_sysvar_write_locks: bool, ) -> Vec> { use solana_sdk::sanitize::Sanitize; let keys: Vec> = OrderedIterator::new(txs, txs_iteration_order) @@ -802,7 +816,9 @@ impl Accounts { return Err(TransactionError::AccountLoadedTwice); } - Ok(tx.message().get_account_keys_by_lock_type()) + Ok(tx + .message() + .get_account_keys_by_lock_type(demote_sysvar_write_locks)) }) .collect(); let mut account_locks = &mut self.account_locks.lock().unwrap(); @@ -822,13 +838,16 @@ impl Accounts { txs: &[Transaction], txs_iteration_order: Option<&[usize]>, results: &[Result<()>], + demote_sysvar_write_locks: bool, ) { let mut account_locks = self.account_locks.lock().unwrap(); debug!("bank unlock accounts"); OrderedIterator::new(txs, txs_iteration_order) .zip(results.iter()) - .for_each(|((_, tx), result)| self.unlock_account(tx, result, &mut account_locks)); + .for_each(|((_, tx), result)| { + self.unlock_account(tx, result, &mut account_locks, demote_sysvar_write_locks) + }); } /// Store the accounts into the DB @@ -844,6 +863,7 @@ impl Accounts { rent_collector: &RentCollector, last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), fix_recent_blockhashes_sysvar_delay: bool, + demote_sysvar_write_locks: bool, ) { let accounts_to_store = self.collect_accounts_to_store( txs, @@ -853,6 +873,7 @@ impl Accounts { rent_collector, last_blockhash_with_fee_calculator, fix_recent_blockhashes_sysvar_delay, + demote_sysvar_write_locks, ); self.accounts_db.store_cached(slot, &accounts_to_store); } @@ -877,6 +898,7 @@ impl Accounts { rent_collector: &RentCollector, last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), fix_recent_blockhashes_sysvar_delay: bool, + demote_sysvar_write_locks: bool, ) -> Vec<(&'a Pubkey, &'a AccountSharedData)> { let mut accounts = Vec::with_capacity(loaded.len()); for (i, ((raccs, _nonce_rollback), (_, tx))) in loaded @@ -927,7 +949,7 @@ impl Accounts { fee_payer_index = Some(i); } let is_fee_payer = Some(i) == fee_payer_index; - if message.is_writable(i) + if message.is_writable(i, demote_sysvar_write_locks) && (res.is_ok() || (maybe_nonce_rollback.is_some() && (is_nonce_account || is_fee_payer))) { @@ -1665,7 +1687,11 @@ mod tests { instructions, ); let tx = Transaction::new(&[&keypair0], message, Hash::default()); - let results0 = accounts.lock_accounts(&[tx.clone()], None); + let results0 = accounts.lock_accounts( + &[tx.clone()], + None, // txs_iteration_order + true, // demote_sysvar_write_locks + ); assert!(results0[0].is_ok()); assert_eq!( @@ -1700,7 +1726,10 @@ mod tests { ); let tx1 = Transaction::new(&[&keypair1], message, Hash::default()); let txs = vec![tx0, tx1]; - let results1 = accounts.lock_accounts(&txs, None); + let results1 = accounts.lock_accounts( + &txs, None, // txs_iteration_order + true, // demote_sysvar_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 @@ -1715,9 +1744,16 @@ mod tests { 2 ); - accounts.unlock_accounts(&[tx], None, &results0); - accounts.unlock_accounts(&txs, None, &results1); - + accounts.unlock_accounts( + &[tx], + None, // txs_iteration_order + &results0, + true, // demote_sysvar_write_locks + ); + accounts.unlock_accounts( + &txs, None, // txs_iteration_order + &results1, true, // demote_sysvar_write_locks + ); let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; let message = Message::new_with_compiled_instructions( 1, @@ -1728,8 +1764,11 @@ mod tests { instructions, ); let tx = Transaction::new(&[&keypair1], message, Hash::default()); - let results2 = accounts.lock_accounts(&[tx], None); - + let results2 = accounts.lock_accounts( + &[tx], + None, // txs_iteration_order + true, // demote_sysvar_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 @@ -1793,13 +1832,19 @@ mod tests { let exit_clone = exit_clone.clone(); loop { let txs = vec![writable_tx.clone()]; - let results = accounts_clone.clone().lock_accounts(&txs, None); + let results = accounts_clone.clone().lock_accounts( + &txs, None, // txs_iteration_order + true, // demote_sysvar_write_locks + ); for result in results.iter() { if result.is_ok() { counter_clone.clone().fetch_add(1, Ordering::SeqCst); } } - accounts_clone.unlock_accounts(&txs, None, &results); + accounts_clone.unlock_accounts( + &txs, None, // txs_iteration_order + &results, true, // demote_sysvar_write_locks + ); if exit_clone.clone().load(Ordering::Relaxed) { break; } @@ -1808,13 +1853,19 @@ mod tests { let counter_clone = counter; for _ in 0..5 { let txs = vec![readonly_tx.clone()]; - let results = accounts_arc.clone().lock_accounts(&txs, None); + let results = accounts_arc.clone().lock_accounts( + &txs, None, // txs_iteration_order + true, // demote_sysvar_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, None, &results); + accounts_arc.unlock_accounts( + &txs, None, // txs_iteration_order + &results, true, // demote_sysvar_write_locks + ); thread::sleep(time::Duration::from_millis(50)); } exit.store(true, Ordering::Relaxed); @@ -1902,6 +1953,7 @@ mod tests { &rent_collector, &(Hash::default(), FeeCalculator::default()), true, + true, // demote_sysvar_write_locks ); assert_eq!(collected_accounts.len(), 2); assert!(collected_accounts @@ -2267,6 +2319,7 @@ mod tests { &rent_collector, &(next_blockhash, FeeCalculator::default()), true, + true, // demote_sysvar_write_locks ); assert_eq!(collected_accounts.len(), 2); assert_eq!( @@ -2378,6 +2431,7 @@ mod tests { &rent_collector, &(next_blockhash, FeeCalculator::default()), true, + true, // demote_sysvar_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 8bda4cad9..19a8d2b0c 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2411,15 +2411,21 @@ impl Bank { .map_or(Ok(()), |sig| self.get_signature_status(sig).unwrap()) } + 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: &'b [Transaction], iteration_order: Option>, ) -> TransactionBatch<'a, 'b> { - let results = self - .rc - .accounts - .lock_accounts(txs, iteration_order.as_deref()); + let results = self.rc.accounts.lock_accounts( + txs, + iteration_order.as_deref(), + self.demote_sysvar_write_locks(), + ); TransactionBatch::new(results, &self, txs, iteration_order) } @@ -2484,6 +2490,7 @@ impl Bank { batch.transactions(), batch.iteration_order(), batch.lock_results(), + self.demote_sysvar_write_locks(), ) } } @@ -3210,6 +3217,7 @@ impl Bank { &self.rent_collector, &self.last_blockhash_with_fee_calculator(), self.fix_recent_blockhashes_sysvar_delay(), + self.demote_sysvar_write_locks(), ); self.collect_rent(executed, loaded_accounts); @@ -12262,4 +12270,72 @@ pub(crate) mod tests { vec![pubkeys_balances[3], pubkeys_balances[1]] ); } + + #[test] + fn test_transfer_sysvar() { + solana_logger::setup(); + let GenesisConfigInfo { + genesis_config, + mint_keypair, + .. + } = create_genesis_config_with_leader( + 1_000_000_000_000_000, + &Pubkey::new_unique(), + bootstrap_validator_stake_lamports(), + ); + let mut bank = Bank::new(&genesis_config); + + fn mock_ix_processor( + _pubkey: &Pubkey, + ka: &[KeyedAccount], + _data: &[u8], + _invoke_context: &mut dyn InvokeContext, + ) -> std::result::Result<(), InstructionError> { + use solana_sdk::account::WritableAccount; + let mut data = ka[1].try_account_ref_mut()?; + data.data_as_mut_slice()[0] = 5; + Ok(()) + } + + let program_id = solana_sdk::pubkey::new_rand(); + bank.add_builtin("mock_program1", program_id, mock_ix_processor); + + let blockhash = bank.last_blockhash(); + let blockhash_sysvar = sysvar::recent_blockhashes::id(); + let orig_lamports = bank + .get_account(&sysvar::recent_blockhashes::id()) + .unwrap() + .lamports; + info!("{:?}", bank.get_account(&sysvar::recent_blockhashes::id())); + let tx = system_transaction::transfer(&mint_keypair, &blockhash_sysvar, 10, blockhash); + assert_eq!( + bank.process_transaction(&tx), + Err(TransactionError::InstructionError( + 0, + InstructionError::ReadonlyLamportChange + )) + ); + assert_eq!( + bank.get_account(&sysvar::recent_blockhashes::id()) + .unwrap() + .lamports, + orig_lamports + ); + info!("{:?}", bank.get_account(&sysvar::recent_blockhashes::id())); + + let accounts = vec![ + AccountMeta::new(mint_keypair.pubkey(), true), + AccountMeta::new(blockhash_sysvar, false), + ]; + let ix = Instruction::new_with_bincode(program_id, &0, accounts); + let message = Message::new(&[ix], Some(&mint_keypair.pubkey())); + let tx = Transaction::new(&[&mint_keypair], message, blockhash); + assert_eq!( + bank.process_transaction(&tx), + Err(TransactionError::InstructionError( + 0, + InstructionError::ReadonlyDataModified + )) + ); + } } diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index ead89b43d..c5d032aa0 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -8,7 +8,10 @@ use solana_sdk::{ account::{AccountSharedData, ReadableAccount, WritableAccount}, account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, - feature_set::{cpi_share_ro_and_exec_accounts, instructions_sysvar_enabled, FeatureSet}, + feature_set::{ + cpi_share_ro_and_exec_accounts, demote_sysvar_write_locks, instructions_sysvar_enabled, + FeatureSet, + }, ic_msg, instruction::{CompiledInstruction, Instruction, InstructionError}, keyed_account::{create_keyed_readonly_accounts, KeyedAccount}, @@ -336,6 +339,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { &self.rent, caller_write_privileges, &mut self.timings, + self.feature_set.is_active(&demote_sysvar_write_locks::id()), ), None => Err(InstructionError::GenericError), // Should never happen } @@ -527,6 +531,7 @@ impl MessageProcessor { instruction: &'a CompiledInstruction, executable_accounts: &'a [(Pubkey, Rc>)], accounts: &'a [Rc>], + demote_sysvar_write_locks: bool, ) -> Vec> { let mut keyed_accounts = create_keyed_readonly_accounts(&executable_accounts); let mut keyed_accounts2: Vec<_> = instruction @@ -537,7 +542,7 @@ impl MessageProcessor { let index = index as usize; let key = &message.account_keys[index]; let account = &accounts[index]; - if message.is_writable(index) { + if message.is_writable(index, demote_sysvar_write_locks) { KeyedAccount::new(key, is_signer, account) } else { KeyedAccount::new_readonly(key, is_signer, account) @@ -682,7 +687,14 @@ impl MessageProcessor { ) -> Result<(), InstructionError> { let invoke_context = RefCell::new(invoke_context); - let (message, executables, accounts, account_refs, caller_write_privileges) = { + let ( + message, + executables, + accounts, + account_refs, + caller_write_privileges, + demote_sysvar_write_locks, + ) = { let invoke_context = invoke_context.borrow(); let caller_program_id = invoke_context.get_caller()?; @@ -774,6 +786,7 @@ impl MessageProcessor { accounts, account_refs, caller_write_privileges, + invoke_context.is_feature_active(&demote_sysvar_write_locks::id()), ) }; @@ -792,7 +805,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) && !account.executable { + if message.is_writable(i, demote_sysvar_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() @@ -835,10 +848,16 @@ 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 keyed_accounts = - Self::create_keyed_accounts(message, instruction, executable_accounts, accounts); + let keyed_accounts = Self::create_keyed_accounts( + message, + instruction, + executable_accounts, + accounts, + demote_sysvar_write_locks, + ); // Invoke callee invoke_context.push(program_id)?; @@ -908,6 +927,7 @@ impl MessageProcessor { accounts: &[Rc>], rent: &Rent, timings: &mut ExecuteDetailsTimings, + demote_sysvar_write_locks: bool, ) -> Result<(), InstructionError> { // Verify all executable accounts have zero outstanding refs Self::verify_account_references(executable_accounts)?; @@ -926,7 +946,7 @@ impl MessageProcessor { let account = accounts[account_index].borrow(); pre_accounts[unique_index].verify( &program_id, - message.is_writable(account_index), + message.is_writable(account_index, demote_sysvar_write_locks), rent, &account, timings, @@ -955,6 +975,7 @@ impl MessageProcessor { rent: &Rent, caller_write_privileges: Option<&[bool]>, timings: &mut ExecuteDetailsTimings, + demote_sysvar_write_locks: bool, ) -> Result<(), InstructionError> { // Verify the per-account instruction results let (mut pre_sum, mut post_sum) = (0_u128, 0_u128); @@ -965,7 +986,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) + message.is_writable(account_index, demote_sysvar_write_locks) }; // Find the matching PreAccount for pre_account in pre_accounts.iter_mut() { @@ -1019,6 +1040,7 @@ impl MessageProcessor { feature_set: Arc, bpf_compute_budget: BpfComputeBudget, timings: &mut ExecuteDetailsTimings, + demote_sysvar_write_locks: bool, ) -> Result<(), InstructionError> { // Fixup the special instructions key if present // before the account pre-values are taken care of @@ -1050,8 +1072,13 @@ impl MessageProcessor { instruction_recorder, feature_set, ); - let keyed_accounts = - Self::create_keyed_accounts(message, instruction, executable_accounts, accounts); + let keyed_accounts = Self::create_keyed_accounts( + message, + instruction, + executable_accounts, + accounts, + demote_sysvar_write_locks, + ); self.process_instruction( program_id, &keyed_accounts, @@ -1066,6 +1093,7 @@ impl MessageProcessor { accounts, &rent_collector.rent, timings, + demote_sysvar_write_locks, )?; timings.accumulate(&invoke_context.timings); @@ -1092,6 +1120,7 @@ impl MessageProcessor { bpf_compute_budget: BpfComputeBudget, timings: &mut ExecuteDetailsTimings, ) -> 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() @@ -1110,6 +1139,7 @@ impl MessageProcessor { feature_set.clone(), bpf_compute_budget, timings, + demote_sysvar_write_locks, ) .map_err(|err| TransactionError::InstructionError(instruction_index as u8, err))?; } @@ -2071,12 +2101,13 @@ 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)) + .map(|(i, _)| message.is_writable(i, demote_sysvar_write_locks)) .collect::>(); assert_eq!( MessageProcessor::process_cross_program_instruction( @@ -2111,7 +2142,7 @@ mod tests { .account_keys .iter() .enumerate() - .map(|(i, _)| message.is_writable(i)) + .map(|(i, _)| message.is_writable(i, demote_sysvar_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 dae549c98..1367c0474 100644 --- a/sdk/benches/serialize_instructions.rs +++ b/sdk/benches/serialize_instructions.rs @@ -27,7 +27,9 @@ 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()); + test::black_box(message.serialize_instructions( + true, // demote_sysvar_write_locks + )); }); } @@ -44,7 +46,9 @@ 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(); + let serialized = message.serialize_instructions( + true, // demote_sysvar_write_locks + ); b.iter(|| { for i in 0..instructions.len() { test::black_box(instructions::load_instruction_at(i, &serialized).unwrap()); @@ -56,7 +60,9 @@ 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(); + let serialized = message.serialize_instructions( + true, // demote_sysvar_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 8bc4e5199..d14b43b38 100644 --- a/sdk/program/src/message.rs +++ b/sdk/program/src/message.rs @@ -6,13 +6,34 @@ use crate::serialize_utils::{ append_slice, append_u16, append_u8, read_pubkey, read_slice, read_u16, read_u8, }; use crate::{ + bpf_loader, bpf_loader_deprecated, bpf_loader_upgradeable, hash::Hash, instruction::{AccountMeta, CompiledInstruction, Instruction}, pubkey::Pubkey, - short_vec, system_instruction, + short_vec, system_instruction, system_program, sysvar, }; use itertools::Itertools; -use std::convert::TryFrom; +use lazy_static::lazy_static; +use std::{convert::TryFrom, str::FromStr}; + +lazy_static! { + // Copied keys over since direct references create cyclical dependency. + static ref BUILTIN_PROGRAMS_KEYS: [Pubkey; 10] = { + let parse = |s| Pubkey::from_str(s).unwrap(); + [ + parse("Config1111111111111111111111111111111111111"), + parse("Feature111111111111111111111111111111111111"), + parse("NativeLoader1111111111111111111111111111111"), + parse("Stake11111111111111111111111111111111111111"), + parse("StakeConfig11111111111111111111111111111111"), + parse("Vote111111111111111111111111111111111111111"), + system_program::id(), + bpf_loader::id(), + bpf_loader_deprecated::id(), + bpf_loader_upgradeable::id(), + ] + }; +} fn position(keys: &[Pubkey], key: &Pubkey) -> u8 { keys.iter().position(|k| k == key).unwrap() as u8 @@ -326,23 +347,31 @@ impl Message { self.program_position(i).is_some() } - pub fn is_writable(&self, i: usize) -> bool { - i < (self.header.num_required_signatures - self.header.num_readonly_signed_accounts) + pub fn is_writable(&self, i: usize, demote_sysvar_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 && i < self.account_keys.len() - - self.header.num_readonly_unsigned_accounts as usize) + - 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)) + } } pub fn is_signer(&self, i: usize) -> bool { i < self.header.num_required_signatures as usize } - pub fn get_account_keys_by_lock_type(&self) -> (Vec<&Pubkey>, Vec<&Pubkey>) { + pub fn get_account_keys_by_lock_type( + &self, + demote_sysvar_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) { + if self.is_writable(i, demote_sysvar_write_locks) { writable_keys.push(key); } else { readonly_keys.push(key); @@ -364,7 +393,7 @@ impl Message { // 36..64 - program_id // 33..34 - data len - u16 // 35..data_len - data - pub fn serialize_instructions(&self) -> Vec { + pub fn serialize_instructions(&self, demote_sysvar_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); @@ -379,7 +408,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); + let is_writable = self.is_writable(account_index, demote_sysvar_write_locks); let mut meta_byte = 0; if is_signer { meta_byte |= 1 << Self::IS_SIGNER_BIT; @@ -459,7 +488,8 @@ impl Message { #[cfg(test)] mod tests { use super::*; - use crate::instruction::AccountMeta; + use crate::{hash, instruction::AccountMeta}; + use std::collections::HashSet; #[test] fn test_message_unique_program_ids() { @@ -471,6 +501,27 @@ mod tests { assert_eq!(program_ids, vec![program_id0]); } + #[test] + fn test_builtin_program_keys() { + let keys: HashSet = BUILTIN_PROGRAMS_KEYS.iter().copied().collect(); + assert_eq!(keys.len(), 10); + for k in keys { + let k = format!("{}", k); + assert!(k.ends_with("11111111111111111111111")); + } + } + + #[test] + fn test_builtin_program_keys_abi_freeze() { + // Once the feature is flipped on, we can't further modify + // BUILTIN_PROGRAMS_KEYS without the risk of breaking consensus. + let builtins = format!("{:?}", *BUILTIN_PROGRAMS_KEYS); + assert_eq!( + format!("{}", hash::hash(builtins.as_bytes())), + "ACqmMkYbo9eqK6QrRSrB3HLyR6uHhLf31SCfGUAJjiWj" + ); + } + #[test] fn test_message_unique_program_ids_not_adjacent() { let program_id0 = Pubkey::default(); @@ -796,12 +847,13 @@ mod tests { recent_blockhash: Hash::default(), instructions: vec![], }; - assert_eq!(message.is_writable(0), true); - assert_eq!(message.is_writable(1), false); - assert_eq!(message.is_writable(2), false); - assert_eq!(message.is_writable(3), true); - assert_eq!(message.is_writable(4), true); - assert_eq!(message.is_writable(5), false); + 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); } #[test] @@ -829,7 +881,9 @@ mod tests { Some(&id1), ); assert_eq!( - message.get_account_keys_by_lock_type(), + message.get_account_keys_by_lock_type( + true, // demote_sysvar_write_locks + ), (vec![&id1, &id0], vec![&id3, &id2, &program_id]) ); } @@ -859,7 +913,9 @@ mod tests { ]; let message = Message::new(&instructions, Some(&id1)); - let serialized = message.serialize_instructions(); + let serialized = message.serialize_instructions( + true, // demote_sysvar_write_locks + ); for (i, instruction) in instructions.iter().enumerate() { assert_eq!( Message::deserialize_instruction(i, &serialized).unwrap(), @@ -880,7 +936,9 @@ mod tests { ]; let message = Message::new(&instructions, Some(&id1)); - let serialized = message.serialize_instructions(); + let serialized = message.serialize_instructions( + true, // demote_sysvar_write_locks + ); 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 8ead147e6..c7a64fdd5 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -123,6 +123,10 @@ pub mod upgradeable_close_instruction { solana_sdk::declare_id!("FsPaByos3gA9bUEhp3EimQpQPCoSvCEigHod496NmABQ"); } +pub mod demote_sysvar_write_locks { + solana_sdk::declare_id!("86LJYRuq2zgtHuL3FccR6hqFJQMQkFoun4knAxcPiF1P"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -154,6 +158,7 @@ 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"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/transaction-status/src/parse_accounts.rs b/transaction-status/src/parse_accounts.rs index ebef018e7..20bd74a56 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), + writable: message.is_writable(i, /*demote_sysvar_write_locks=*/ true), signer: message.is_signer(i), }); }