From ca60bc6ea2e7d12977dc88f617503791e3ffbda1 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Thu, 2 Sep 2021 10:17:50 -0600 Subject: [PATCH 1/6] Add feature --- sdk/src/feature_set.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index ef50407d2d18b2..fd3313e66f6349 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -191,6 +191,10 @@ pub mod stake_program_advance_activating_credits_observed { solana_sdk::declare_id!("SAdVFw3RZvzbo6DvySbSdBnHN4gkzSTH9dSxesyKKPj"); } +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 = [ @@ -234,6 +238,7 @@ lazy_static! { (instructions_sysvar_owned_by_sysvar::id(), "fix owner for instructions sysvar"), (close_upgradeable_program_accounts::id(), "enable closing upgradeable program accounts"), (stake_program_advance_activating_credits_observed::id(), "Enable advancing credits observed for activation epoch #19309"), + (demote_program_write_locks::id(), "demote program write locks to readonly #19593"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() From dd1302932794463b6931ac2da67c34c522309ea9 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Thu, 2 Sep 2021 16:15:03 -0600 Subject: [PATCH 2/6] Demote write lock on program ids --- cli-output/src/display.rs | 2 +- core/src/banking_stage.rs | 11 +- core/src/cost_model.rs | 15 ++- core/src/cost_tracker.rs | 11 +- ledger-tool/src/main.rs | 5 +- program-runtime/src/instruction_processor.rs | 23 +++- program-test/src/lib.rs | 7 +- programs/bpf_loader/src/syscalls.rs | 18 ++- rpc/src/transaction_status_service.rs | 3 +- runtime/src/accounts.rs | 117 ++++++++++++++++--- runtime/src/bank.rs | 29 ++++- runtime/src/message_processor.rs | 19 ++- sdk/benches/serialize_instructions.rs | 8 +- sdk/program/src/message/legacy.rs | 21 ++-- sdk/program/src/message/sanitized.rs | 14 ++- sdk/src/transaction/sanitized.rs | 4 +- transaction-status/src/parse_accounts.rs | 2 +- 17 files changed, 237 insertions(+), 72 deletions(-) diff --git a/cli-output/src/display.rs b/cli-output/src/display.rs index 3d1c3e74fcb494..f1d52c1f67dc64 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) { + 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/banking_stage.rs b/core/src/banking_stage.rs index 97bffd0931ff80..16ea95fab602b6 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -1049,6 +1049,7 @@ impl BankingStage { libsecp256k1_fail_on_bad_count: bool, cost_tracker: &Arc>, banking_stage_stats: &BankingStageStats, + demote_program_write_locks: bool, ) -> (Vec, Vec, Vec) { let mut retryable_transaction_packet_indexes: Vec = vec![]; @@ -1082,7 +1083,8 @@ impl BankingStage { verified_transactions_with_packet_indexes .into_iter() .filter_map(|(tx, tx_index)| { - let result = cost_tracker_readonly.would_transaction_fit(&tx); + let result = cost_tracker_readonly + .would_transaction_fit(&tx, demote_program_write_locks); if result.is_err() { debug!("transaction {:?} would exceed limit: {:?}", tx, result); retryable_transaction_packet_indexes.push(tx_index); @@ -1165,6 +1167,7 @@ impl BankingStage { bank.libsecp256k1_fail_on_bad_count(), cost_tracker, banking_stage_stats, + bank.demote_program_write_locks(), ); packet_conversion_time.stop(); inc_new_counter_info!("banking_stage-packet_conversion", 1); @@ -1201,7 +1204,10 @@ impl BankingStage { let mut cost_tracking_time = Measure::start("cost_tracking_time"); transactions.iter().enumerate().for_each(|(index, tx)| { if unprocessed_tx_indexes.iter().all(|&i| i != index) { - cost_tracker.write().unwrap().add_transaction_cost(tx); + cost_tracker + .write() + .unwrap() + .add_transaction_cost(tx, bank.demote_program_write_locks()); } }); cost_tracking_time.stop(); @@ -1268,6 +1274,7 @@ impl BankingStage { bank.libsecp256k1_fail_on_bad_count(), cost_tracker, banking_stage_stats, + bank.demote_program_write_locks(), ); unprocessed_packet_conversion_time.stop(); diff --git a/core/src/cost_model.rs b/core/src/cost_model.rs index fcaf6d719e1b50..ade82ea8e746b1 100644 --- a/core/src/cost_model.rs +++ b/core/src/cost_model.rs @@ -113,7 +113,11 @@ impl CostModel { ); } - pub fn calculate_cost(&mut self, transaction: &SanitizedTransaction) -> &TransactionCost { + pub fn calculate_cost( + &mut self, + transaction: &SanitizedTransaction, + demote_program_write_locks: bool, + ) -> &TransactionCost { self.transaction_cost.reset(); // calculate transaction exeution cost @@ -122,7 +126,7 @@ impl CostModel { // calculate account access cost let message = transaction.message(); message.account_keys_iter().enumerate().for_each(|(i, k)| { - let is_writable = message.is_writable(i); + let is_writable = message.is_writable(i, demote_program_write_locks); if is_writable { self.transaction_cost.writable_accounts.push(*k); @@ -353,7 +357,7 @@ mod tests { .unwrap(); let mut cost_model = CostModel::default(); - let tx_cost = cost_model.calculate_cost(&tx); + let tx_cost = cost_model.calculate_cost(&tx, /*demote_program_write_locks=*/ true); assert_eq!(2 + 2, tx_cost.writable_accounts.len()); assert_eq!(signer1.pubkey(), tx_cost.writable_accounts[0]); assert_eq!(signer2.pubkey(), tx_cost.writable_accounts[1]); @@ -395,7 +399,7 @@ mod tests { cost_model .upsert_instruction_cost(&system_program::id(), expected_execution_cost) .unwrap(); - let tx_cost = cost_model.calculate_cost(&tx); + let tx_cost = cost_model.calculate_cost(&tx, /*demote_program_write_locks=*/ true); assert_eq!(expected_account_cost, tx_cost.account_access_cost); assert_eq!(expected_execution_cost, tx_cost.execution_cost); assert_eq!(2, tx_cost.writable_accounts.len()); @@ -465,7 +469,8 @@ mod tests { } else { thread::spawn(move || { let mut cost_model = cost_model.write().unwrap(); - let tx_cost = cost_model.calculate_cost(&tx); + let tx_cost = cost_model + .calculate_cost(&tx, /*demote_program_write_locks=*/ true); assert_eq!(3, tx_cost.writable_accounts.len()); assert_eq!(expected_account_cost, tx_cost.account_access_cost); }) diff --git a/core/src/cost_tracker.rs b/core/src/cost_tracker.rs index 0d9f32b2807dbd..40a86133adb7d3 100644 --- a/core/src/cost_tracker.rs +++ b/core/src/cost_tracker.rs @@ -46,18 +46,23 @@ impl CostTracker { pub fn would_transaction_fit( &self, transaction: &SanitizedTransaction, + demote_program_write_locks: bool, ) -> Result<(), CostModelError> { let mut cost_model = self.cost_model.write().unwrap(); - let tx_cost = cost_model.calculate_cost(transaction); + let tx_cost = cost_model.calculate_cost(transaction, demote_program_write_locks); self.would_fit( &tx_cost.writable_accounts, &(tx_cost.account_access_cost + tx_cost.execution_cost), ) } - pub fn add_transaction_cost(&mut self, transaction: &SanitizedTransaction) { + pub fn add_transaction_cost( + &mut self, + transaction: &SanitizedTransaction, + demote_program_write_locks: bool, + ) { let mut cost_model = self.cost_model.write().unwrap(); - let tx_cost = cost_model.calculate_cost(transaction); + let tx_cost = cost_model.calculate_cost(transaction, demote_program_write_locks); let cost = tx_cost.account_access_cost + tx_cost.execution_cost; for account_key in tx_cost.writable_accounts.iter() { *self diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 67e27375e7c721..a5ed2ac8e3095f 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -792,7 +792,10 @@ fn compute_slot_cost(blockstore: &Blockstore, slot: Slot) -> Result<(), String> .for_each(|transaction| { num_programs += transaction.message().instructions().len(); - let tx_cost = cost_model.calculate_cost(&transaction); + let tx_cost = cost_model.calculate_cost( + &transaction, + true, // demote_program_write_locks + ); if cost_tracker.try_add(tx_cost).is_err() { println!( "Slot: {}, CostModel rejected transaction {:?}, stats {:?}!", diff --git a/program-runtime/src/instruction_processor.rs b/program-runtime/src/instruction_processor.rs index 7b5ed32b57242c..2c93de36599284 100644 --- a/program-runtime/src/instruction_processor.rs +++ b/program-runtime/src/instruction_processor.rs @@ -4,6 +4,7 @@ use solana_sdk::{ account::{AccountSharedData, ReadableAccount, WritableAccount}, account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, + feature_set::demote_program_write_locks, ic_logger_msg, ic_msg, instruction::{CompiledInstruction, Instruction, InstructionError}, keyed_account::{keyed_account_at_index, KeyedAccount}, @@ -347,6 +348,7 @@ impl InstructionProcessor { instruction: &'a CompiledInstruction, executable_accounts: &'a [(Pubkey, Rc>)], accounts: &'a [(Pubkey, Rc>)], + demote_sysvar_write_locks: bool, ) -> Vec<(bool, bool, &'a Pubkey, &'a RefCell)> { executable_accounts .iter() @@ -355,7 +357,7 @@ impl InstructionProcessor { let index = *index as usize; ( message.is_signer(index), - message.is_writable(index), + message.is_writable(index, demote_sysvar_write_locks), &accounts[index].0, &accounts[index].1 as &RefCell, ) @@ -600,6 +602,8 @@ impl InstructionProcessor { { let invoke_context = invoke_context.borrow(); + let demote_program_write_locks = + invoke_context.is_feature_active(&demote_program_write_locks::id()); let keyed_accounts = invoke_context.get_keyed_accounts()?; for (src_keyed_account_index, ((_key, account), dst_keyed_account_index)) in accounts .iter() @@ -608,7 +612,9 @@ impl InstructionProcessor { { let dst_keyed_account = &keyed_accounts[dst_keyed_account_index]; let src_keyed_account = account.borrow(); - if message.is_writable(src_keyed_account_index) && !src_keyed_account.executable() { + if message.is_writable(src_keyed_account_index, demote_program_write_locks) + && !src_keyed_account.executable() + { if dst_keyed_account.data_len()? != src_keyed_account.data().len() && dst_keyed_account.data_len()? != 0 { @@ -652,8 +658,15 @@ impl InstructionProcessor { invoke_context.verify_and_update(instruction, accounts, caller_write_privileges)?; // Construct keyed accounts - let keyed_accounts = - Self::create_keyed_accounts(message, instruction, executable_accounts, 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_program_write_locks, + ); // Invoke callee invoke_context.push(program_id, &keyed_accounts)?; @@ -671,7 +684,7 @@ impl InstructionProcessor { if result.is_ok() { // Verify the called program has not misbehaved let write_privileges: Vec = (0..message.account_keys.len()) - .map(|i| message.is_writable(i)) + .map(|i| message.is_writable(i, demote_program_write_locks)) .collect(); result = invoke_context.verify_and_update(instruction, accounts, &write_privileges); } diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs index 0db2d01c19c186..da1ef80d86ecb1 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -24,6 +24,7 @@ use { compute_budget::ComputeBudget, entrypoint::{ProgramResult, SUCCESS}, epoch_schedule::EpochSchedule, + feature_set::demote_program_write_locks, fee_calculator::{FeeCalculator, FeeRateGovernor}, genesis_config::{ClusterType, GenesisConfig}, hash::Hash, @@ -261,12 +262,14 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { } panic!("Program id {} wasn't found in account_infos", program_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)) + .map(|(i, _)| message.is_writable(i, demote_program_write_locks)) .collect::>(); stable_log::program_invoke(&logger, &program_id, invoke_context.invoke_depth()); @@ -337,7 +340,7 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { // Copy writeable account modifications back into the caller's AccountInfos for (i, (pubkey, account)) in accounts.iter().enumerate().take(message.account_keys.len()) { - if !message.is_writable(i) { + if !message.is_writable(i, demote_program_write_locks) { continue; } for account_info in account_infos { diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index 847fe820ca1447..ba1c1aeda7b58f 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -21,9 +21,9 @@ use solana_sdk::{ entrypoint::{MAX_PERMITTED_DATA_INCREASE, SUCCESS}, epoch_schedule::EpochSchedule, feature_set::{ - blake3_syscall_enabled, close_upgradeable_program_accounts, disable_fees_sysvar, - enforce_aligned_host_addrs, libsecp256k1_0_5_upgrade_enabled, mem_overlap_fix, - secp256k1_recover_syscall_enabled, + blake3_syscall_enabled, close_upgradeable_program_accounts, demote_program_write_locks, + disable_fees_sysvar, enforce_aligned_host_addrs, libsecp256k1_0_5_upgrade_enabled, + mem_overlap_fix, secp256k1_recover_syscall_enabled, }, hash::{Hasher, HASH_BYTES}, ic_msg, @@ -2293,7 +2293,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_program_write_locks, + ) = { let invoke_context = syscall.get_context()?; invoke_context @@ -2388,6 +2395,7 @@ fn call<'a>( accounts, account_refs, caller_write_privileges, + invoke_context.is_feature_active(&demote_program_write_locks::id()), ) }; @@ -2413,7 +2421,7 @@ fn call<'a>( for (i, ((_key, 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) && !account.executable() { + 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() { diff --git a/rpc/src/transaction_status_service.rs b/rpc/src/transaction_status_service.rs index 43a76dc51cb575..62630d3a17d412 100644 --- a/rpc/src/transaction_status_service.rs +++ b/rpc/src/transaction_status_service.rs @@ -113,7 +113,8 @@ impl TransactionStatusService { }) .expect("FeeCalculator must exist"); let fee = transaction.message().calculate_fee(&fee_calculator); - let tx_account_locks = transaction.get_account_locks(); + let tx_account_locks = + transaction.get_account_locks(bank.demote_program_write_locks()); let inner_instructions = inner_instructions.map(|inner_instructions| { inner_instructions diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 015640a3acea32..81a8a05ea6ad35 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -201,8 +201,9 @@ impl Accounts { fn construct_instructions_account( message: &SanitizedMessage, is_owned_by_sysvar: bool, + demote_program_write_locks: bool, ) -> AccountSharedData { - let mut data = message.serialize_instructions(); + let mut data = message.serialize_instructions(demote_program_write_locks); // add room for current instruction index. data.resize(data.len() + 2, 0); let owner = if is_owned_by_sysvar { @@ -240,6 +241,8 @@ impl Accounts { let mut account_deps = Vec::with_capacity(message.account_keys_len()); let mut rent_debits = RentDebits::default(); let rent_for_sysvars = feature_set.is_active(&feature_set::rent_for_sysvars::id()); + 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(i) { @@ -248,20 +251,21 @@ impl Accounts { } if solana_sdk::sysvar::instructions::check_id(key) { - if message.is_writable(i) { + if message.is_writable(i, demote_program_write_locks) { return Err(TransactionError::InvalidAccountIndex); } Self::construct_instructions_account( message, feature_set .is_active(&feature_set::instructions_sysvar_owned_by_sysvar::id()), + demote_program_write_locks, ) } else { let (account, rent) = self .accounts_db .load_with_fixed_root(ancestors, key) .map(|(mut account, _)| { - if message.is_writable(i) { + if message.is_writable(i, demote_program_write_locks) { let rent_due = rent_collector.collect_from_existing_account( key, &mut account, @@ -886,8 +890,11 @@ impl Accounts { pub fn lock_accounts<'a>( &self, txs: impl Iterator, + demote_program_write_locks: bool, ) -> Vec> { - let keys: Vec<_> = txs.map(|tx| tx.get_account_locks()).collect(); + let keys: Vec<_> = txs + .map(|tx| tx.get_account_locks(demote_program_write_locks)) + .collect(); let mut account_locks = &mut self.account_locks.lock().unwrap(); keys.into_iter() .map(|keys| self.lock_account(&mut account_locks, keys.writable, keys.readonly)) @@ -900,6 +907,7 @@ impl Accounts { &self, txs: impl Iterator, results: &[Result<()>], + demote_program_write_locks: bool, ) { let keys: Vec<_> = txs .zip(results) @@ -907,7 +915,7 @@ impl Accounts { Err(TransactionError::AccountInUse) => None, Err(TransactionError::SanitizeFailure) => None, Err(TransactionError::AccountLoadedTwice) => None, - _ => Some(tx.get_account_locks()), + _ => Some(tx.get_account_locks(demote_program_write_locks)), }) .collect(); let mut account_locks = self.account_locks.lock().unwrap(); @@ -930,6 +938,7 @@ impl Accounts { last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), rent_for_sysvars: bool, merge_nonce_error_into_system_error: bool, + demote_program_write_locks: bool, ) { let accounts_to_store = self.collect_accounts_to_store( txs, @@ -939,6 +948,7 @@ impl Accounts { last_blockhash_with_fee_calculator, rent_for_sysvars, merge_nonce_error_into_system_error, + demote_program_write_locks, ); self.accounts_db.store_cached(slot, &accounts_to_store); } @@ -964,6 +974,7 @@ impl Accounts { last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), rent_for_sysvars: 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() { @@ -1012,7 +1023,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_program_write_locks) && (res.is_ok() || (maybe_nonce_rollback.is_some() && (is_nonce_account || is_fee_payer))) { @@ -1731,6 +1742,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, @@ -1741,7 +1754,7 @@ mod tests { instructions, ); let tx = new_sanitized_tx(&[&keypair0], message, Hash::default()); - let results0 = accounts.lock_accounts([tx.clone()].iter()); + let results0 = accounts.lock_accounts([tx.clone()].iter(), demote_program_write_locks); assert!(results0[0].is_ok()); assert_eq!( @@ -1776,7 +1789,7 @@ mod tests { ); let tx1 = new_sanitized_tx(&[&keypair1], message, Hash::default()); let txs = vec![tx0, tx1]; - let results1 = accounts.lock_accounts(txs.iter()); + 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 @@ -1791,8 +1804,8 @@ mod tests { 2 ); - accounts.unlock_accounts([tx].iter(), &results0); - accounts.unlock_accounts(txs.iter(), &results1); + 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, @@ -1803,7 +1816,7 @@ mod tests { instructions, ); let tx = new_sanitized_tx(&[&keypair1], message, Hash::default()); - let results2 = accounts.lock_accounts([tx].iter()); + 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 @@ -1840,6 +1853,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])]; @@ -1872,13 +1887,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()); + 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); + accounts_clone.unlock_accounts(txs.iter(), &results, demote_program_write_locks); if exit_clone.clone().load(Ordering::Relaxed) { break; } @@ -1887,18 +1904,85 @@ 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()); + 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); + 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_for_tests( + Vec::new(), + &ClusterType::Development, + AccountSecondaryIndexes::default(), + false, + AccountShrinkThreshold::default(), + ); + 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 = new_sanitized_tx(&[&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(); @@ -1991,6 +2075,7 @@ mod tests { &(Hash::default(), FeeCalculator::default()), true, true, // merge_nonce_error_into_system_error + true, // demote_program_write_locks ); assert_eq!(collected_accounts.len(), 2); assert!(collected_accounts @@ -2370,6 +2455,7 @@ mod tests { &(next_blockhash, FeeCalculator::default()), true, true, // merge_nonce_error_into_system_error + true, // demote_program_write_locks ); assert_eq!(collected_accounts.len(), 2); assert_eq!( @@ -2487,6 +2573,7 @@ mod tests { &(next_blockhash, FeeCalculator::default()), true, 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 ed812e92c5b0c2..d3f1b1eda2f5ed 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2755,7 +2755,10 @@ impl Bank { .into_iter() .map(SanitizedTransaction::try_from) .collect::>>()?; - let lock_results = self.rc.accounts.lock_accounts(sanitized_txs.iter()); + let lock_results = self + .rc + .accounts + .lock_accounts(sanitized_txs.iter(), self.demote_program_write_locks()); Ok(TransactionBatch::new( lock_results, self, @@ -2775,7 +2778,10 @@ impl Bank { }) }) .collect::>>()?; - let lock_results = self.rc.accounts.lock_accounts(sanitized_txs.iter()); + let lock_results = self + .rc + .accounts + .lock_accounts(sanitized_txs.iter(), self.demote_program_write_locks()); Ok(TransactionBatch::new( lock_results, self, @@ -2788,7 +2794,10 @@ impl Bank { &'a self, txs: &'b [SanitizedTransaction], ) -> TransactionBatch<'a, 'b> { - let lock_results = self.rc.accounts.lock_accounts(txs.iter()); + let lock_results = self + .rc + .accounts + .lock_accounts(txs.iter(), self.demote_program_write_locks()); TransactionBatch::new(lock_results, self, Cow::Borrowed(txs)) } @@ -2863,9 +2872,11 @@ impl Bank { pub fn unlock_accounts(&self, batch: &mut TransactionBatch) { if batch.needs_unlock { batch.needs_unlock = false; - self.rc - .accounts - .unlock_accounts(batch.sanitized_transactions().iter(), batch.lock_results()) + self.rc.accounts.unlock_accounts( + batch.sanitized_transactions().iter(), + batch.lock_results(), + self.demote_program_write_locks(), + ) } } @@ -3624,6 +3635,7 @@ impl Bank { &self.last_blockhash_with_fee_calculator(), self.rent_for_sysvars(), self.merge_nonce_error_into_system_error(), + self.demote_program_write_locks(), ); let rent_debits = self.collect_rent(executed, loaded_txs); @@ -5376,6 +5388,11 @@ impl Bank { .is_active(&feature_set::stake_program_advance_activating_credits_observed::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( diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 06d1828cc75355..864a196a71b67f 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -10,7 +10,8 @@ use solana_sdk::{ account::{AccountSharedData, ReadableAccount, WritableAccount}, compute_budget::ComputeBudget, feature_set::{ - neon_evm_compute_budget, tx_wide_compute_cap, updated_verify_policy, FeatureSet, + demote_program_write_locks, neon_evm_compute_budget, tx_wide_compute_cap, + updated_verify_policy, FeatureSet, }, fee_calculator::FeeCalculator, hash::Hash, @@ -94,6 +95,7 @@ impl<'a> ThisInvokeContext<'a> { instruction, executable_accounts, accounts, + feature_set.is_active(&demote_program_write_locks::id()), ); let compute_meter = if feature_set.is_active(&tx_wide_compute_cap::id()) { compute_meter @@ -389,6 +391,7 @@ impl MessageProcessor { } /// Verify the results of an instruction + #[allow(clippy::too_many_arguments)] pub fn verify( message: &Message, instruction: &CompiledInstruction, @@ -399,6 +402,7 @@ impl MessageProcessor { timings: &mut ExecuteDetailsTimings, logger: Rc>, 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)?; @@ -419,7 +423,7 @@ impl MessageProcessor { pre_accounts[unique_index] .verify( program_id, - message.is_writable(account_index), + message.is_writable(account_index, demote_program_write_locks), rent, &account, timings, @@ -534,6 +538,7 @@ impl MessageProcessor { timings, invoke_context.get_logger(), invoke_context.is_feature_active(&updated_verify_policy::id()), + invoke_context.is_feature_active(&demote_program_write_locks::id()), )?; timings.accumulate(&invoke_context.timings); @@ -713,7 +718,7 @@ mod tests { ))), )); let write_privileges: Vec = (0..message.account_keys.len()) - .map(|i| message.is_writable(i)) + .map(|i| message.is_writable(i, /*demote_program_write_locks=*/ true)) .collect(); invoke_context .verify_and_update(&message.instructions[0], &these_accounts, &write_privileges) @@ -1193,6 +1198,8 @@ mod tests { metas.clone(), ); let message = Message::new(&[instruction], None); + 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 blockhash = Hash::default(); @@ -1210,7 +1217,7 @@ mod tests { Rc::new(RefCell::new(MockComputeMeter::default())), Rc::new(RefCell::new(Executors::default())), None, - Arc::new(FeatureSet::all_enabled()), + Arc::new(feature_set), Arc::new(Accounts::default_for_tests()), &ancestors, &blockhash, @@ -1222,7 +1229,7 @@ mod tests { .account_keys .iter() .enumerate() - .map(|(i, _)| message.is_writable(i)) + .map(|(i, _)| message.is_writable(i, demote_program_write_locks)) .collect::>(); accounts[0].1.borrow_mut().data_as_mut_slice()[0] = 1; assert_eq!( @@ -1282,7 +1289,7 @@ mod tests { .account_keys .iter() .enumerate() - .map(|(i, _)| message.is_writable(i)) + .map(|(i, _)| message.is_writable(i, demote_program_write_locks)) .collect::>(); assert_eq!( InstructionProcessor::process_cross_program_instruction( diff --git a/sdk/benches/serialize_instructions.rs b/sdk/benches/serialize_instructions.rs index 198e9ef0fae779..2d639bf01f7406 100644 --- a/sdk/benches/serialize_instructions.rs +++ b/sdk/benches/serialize_instructions.rs @@ -15,6 +15,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(); @@ -30,7 +32,7 @@ fn bench_manual_instruction_serialize(b: &mut Bencher) { SanitizedMessage::try_from(Message::new(&instructions, Some(&Pubkey::new_unique()))) .unwrap(); b.iter(|| { - test::black_box(message.serialize_instructions()); + test::black_box(message.serialize_instructions(DEMOTE_PROGRAM_WRITE_LOCKS)); }); } @@ -49,7 +51,7 @@ fn bench_manual_instruction_deserialize(b: &mut Bencher) { let message = SanitizedMessage::try_from(Message::new(&instructions, Some(&Pubkey::new_unique()))) .unwrap(); - let serialized = message.serialize_instructions(); + 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()); @@ -63,7 +65,7 @@ fn bench_manual_instruction_deserialize_single(b: &mut Bencher) { let message = SanitizedMessage::try_from(Message::new(&instructions, Some(&Pubkey::new_unique()))) .unwrap(); - let serialized = message.serialize_instructions(); + 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/legacy.rs b/sdk/program/src/message/legacy.rs index 7279eb5e5fa5a4..21742b837da0f3 100644 --- a/sdk/program/src/message/legacy.rs +++ b/sdk/program/src/message/legacy.rs @@ -353,7 +353,7 @@ impl Message { self.program_position(i).is_some() } - pub fn is_writable(&self, i: usize) -> 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 @@ -363,6 +363,7 @@ impl Message { let key = self.account_keys[i]; 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 { @@ -374,7 +375,7 @@ impl Message { 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_program_write_locks=*/ true) { writable_keys.push(key); } else { readonly_keys.push(key); @@ -412,7 +413,8 @@ 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_program_write_locks=*/ true); let mut meta_byte = 0; if is_signer { meta_byte |= 1 << Self::IS_SIGNER_BIT; @@ -866,12 +868,13 @@ mod tests { recent_blockhash: Hash::default(), instructions: vec![], }; - assert!(message.is_writable(0)); - assert!(!message.is_writable(1)); - assert!(!message.is_writable(2)); - assert!(message.is_writable(3)); - assert!(message.is_writable(4)); - assert!(!message.is_writable(5)); + 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] diff --git a/sdk/program/src/message/sanitized.rs b/sdk/program/src/message/sanitized.rs index 551bee22f82624..c25c3ad24cdccb 100644 --- a/sdk/program/src/message/sanitized.rs +++ b/sdk/program/src/message/sanitized.rs @@ -192,9 +192,12 @@ impl SanitizedMessage { /// Returns true if the account at the specified index is writable by the /// instructions in this message. - pub fn is_writable(&self, index: usize) -> bool { + pub fn is_writable(&self, index: usize, demote_program_write_locks: bool) -> bool { + if demote_program_write_locks && self.is_invoked(index) { + return false; + } match self { - Self::Legacy(message) => message.is_writable(index), + Self::Legacy(message) => message.is_writable(index, demote_program_write_locks), Self::V0(message) => message.is_writable(index), } } @@ -219,7 +222,7 @@ impl SanitizedMessage { // 67..69 - data len - u16 // 69..data_len - data #[allow(clippy::integer_arithmetic)] - pub fn serialize_instructions(&self) -> 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); @@ -234,7 +237,7 @@ impl SanitizedMessage { 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_program_write_locks); let mut account_meta = InstructionsSysvarAccountMeta::NONE; if is_signer { account_meta |= InstructionsSysvarAccountMeta::IS_SIGNER; @@ -440,9 +443,10 @@ mod tests { ), ]; + let demote_program_write_locks = true; let message = Message::new(&instructions, Some(&id1)); let sanitized_message = SanitizedMessage::try_from(message.clone()).unwrap(); - let serialized = sanitized_message.serialize_instructions(); + let serialized = sanitized_message.serialize_instructions(demote_program_write_locks); // assert that SanitizedMessage::serialize_instructions has the same behavior as the // deprecated Message::serialize_instructions method diff --git a/sdk/src/transaction/sanitized.rs b/sdk/src/transaction/sanitized.rs index 592468988d697b..d3ffc77af6da4d 100644 --- a/sdk/src/transaction/sanitized.rs +++ b/sdk/src/transaction/sanitized.rs @@ -125,7 +125,7 @@ impl SanitizedTransaction { } /// Return the list of accounts that must be locked during processing this transaction. - pub fn get_account_locks(&self) -> TransactionAccountLocks { + pub fn get_account_locks(&self, demote_program_write_locks: bool) -> TransactionAccountLocks { let message = &self.message; let num_readonly_accounts = message.num_readonly_accounts(); let num_writable_accounts = message @@ -138,7 +138,7 @@ impl SanitizedTransaction { }; for (i, key) in message.account_keys_iter().enumerate() { - if message.is_writable(i) { + if message.is_writable(i, demote_program_write_locks) { account_locks.writable.push(key); } else { account_locks.readonly.push(key); diff --git a/transaction-status/src/parse_accounts.rs b/transaction-status/src/parse_accounts.rs index ebef018e716695..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), + writable: message.is_writable(i, /*demote_program_write_locks=*/ true), signer: message.is_signer(i), }); } From 84dbfdac133c19082da4456943d15ec183928201 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Fri, 3 Sep 2021 00:24:17 -0600 Subject: [PATCH 3/6] Fixup bpf tests --- programs/bpf/tests/programs.rs | 96 ++-------------------------------- 1 file changed, 4 insertions(+), 92 deletions(-) diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 11ce9581d44831..cc337f48683572 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -1796,7 +1796,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(), @@ -1815,10 +1815,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) ); } @@ -2114,96 +2116,6 @@ fn test_program_bpf_upgrade_via_cpi() { assert_ne!(programdata, original_programdata); } -#[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_for_tests(&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), - ], - ); - - // 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() { From fa0882e3b04e88729ae6938b51173ded9532d762 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Fri, 3 Sep 2021 11:36:23 -0600 Subject: [PATCH 4/6] Update MappedMessage::is_writable --- sdk/program/src/message/mapped.rs | 56 +++++++++++++++++++++++++--- sdk/program/src/message/sanitized.rs | 11 ++---- 2 files changed, 54 insertions(+), 13 deletions(-) diff --git a/sdk/program/src/message/mapped.rs b/sdk/program/src/message/mapped.rs index 06be294df39989..0f30e35238db07 100644 --- a/sdk/program/src/message/mapped.rs +++ b/sdk/program/src/message/mapped.rs @@ -4,7 +4,7 @@ use { pubkey::Pubkey, sysvar, }, - std::collections::HashSet, + std::{collections::HashSet, convert::TryFrom}, }; /// Combination of a version #0 message and its mapped addresses @@ -96,20 +96,32 @@ impl MappedMessage { } /// Returns true if the account at the specified index was loaded as writable - pub fn is_writable(&self, key_index: usize) -> bool { + pub fn is_writable(&self, key_index: usize, demote_program_write_locks: bool) -> bool { if self.is_writable_index(key_index) { if let Some(key) = self.get_account_key(key_index) { - return !(sysvar::is_sysvar_id(key) || BUILTIN_PROGRAMS_KEYS.contains(key)); + return !(sysvar::is_sysvar_id(key) || BUILTIN_PROGRAMS_KEYS.contains(key) + || (demote_program_write_locks && self.is_key_called_as_program(key_index))); } } false } + + /// Returns true if the account at the specified index is called as a program by an instruction + pub fn is_key_called_as_program(&self, key_index: usize) -> bool { + if let Ok(key_index) = u8::try_from(key_index) { + self.message.instructions + .iter() + .any(|ix| ix.program_id_index == key_index) + } else { + false + } + } } #[cfg(test)] mod tests { use super::*; - use crate::{message::MessageHeader, system_program, sysvar}; + use crate::{instruction::CompiledInstruction, message::MessageHeader, system_program, sysvar}; use itertools::Itertools; fn create_test_mapped_message() -> (MappedMessage, [Pubkey; 6]) { @@ -236,10 +248,42 @@ mod tests { mapped_msg.message.account_keys[0] = sysvar::clock::id(); assert!(mapped_msg.is_writable_index(0)); - assert!(!mapped_msg.is_writable(0)); + assert!(!mapped_msg.is_writable(0, /*demote_program_write_locks=*/ true)); mapped_msg.message.account_keys[0] = system_program::id(); assert!(mapped_msg.is_writable_index(0)); - assert!(!mapped_msg.is_writable(0)); + assert!(!mapped_msg.is_writable(0, /*demote_program_write_locks=*/ true)); + } + + #[test] + fn test_demote_writable_program() { + let key0 = Pubkey::new_unique(); + let key1 = Pubkey::new_unique(); + let key2 = Pubkey::new_unique(); + let mapped_msg = MappedMessage { + message: v0::Message { + header: MessageHeader { + num_required_signatures: 1, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 0, + }, + account_keys: vec![key0], + instructions: vec![ + CompiledInstruction { + program_id_index: 2, + accounts: vec![1], + data: vec![], + } + ], + ..v0::Message::default() + }, + mapped_addresses: MappedAddresses { + writable: vec![key1, key2], + readonly: vec![], + }, + }; + + assert!(mapped_msg.is_writable_index(2)); + assert!(!mapped_msg.is_writable(2, /*demote_program_write_locks=*/ true)); } } diff --git a/sdk/program/src/message/sanitized.rs b/sdk/program/src/message/sanitized.rs index c25c3ad24cdccb..e33d188c963fa2 100644 --- a/sdk/program/src/message/sanitized.rs +++ b/sdk/program/src/message/sanitized.rs @@ -175,12 +175,9 @@ impl SanitizedMessage { /// Returns true if the account at the specified index is invoked as a /// program in this message. pub fn is_invoked(&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 + match self { + Self::Legacy(message) => message.is_key_called_as_program(key_index), + Self::V0(message) => message.is_key_called_as_program(key_index), } } @@ -198,7 +195,7 @@ impl SanitizedMessage { } match self { Self::Legacy(message) => message.is_writable(index, demote_program_write_locks), - Self::V0(message) => message.is_writable(index), + Self::V0(message) => message.is_writable(index, demote_program_write_locks), } } From e647fc55d978b367431e636ade7bd5b5bd36db68 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Fri, 3 Sep 2021 11:36:37 -0600 Subject: [PATCH 5/6] Comma nit --- runtime/src/accounts.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 81a8a05ea6ad35..5fade1b981eee9 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -1974,13 +1974,13 @@ mod tests { .lock() .unwrap() .write_locks - .contains(&keypair0.pubkey()),); + .contains(&keypair0.pubkey())); assert!(accounts .account_locks .lock() .unwrap() .write_locks - .contains(&keypair1.pubkey()),); + .contains(&keypair1.pubkey())); } #[test] From ce89201cbdac9be754daaf31c3305dd03f172819 Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Fri, 3 Sep 2021 13:40:40 -0600 Subject: [PATCH 6/6] Review comments --- program-runtime/src/instruction_processor.rs | 4 ++-- sdk/program/src/message/sanitized.rs | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/program-runtime/src/instruction_processor.rs b/program-runtime/src/instruction_processor.rs index 2c93de36599284..49c6381be1e4b2 100644 --- a/program-runtime/src/instruction_processor.rs +++ b/program-runtime/src/instruction_processor.rs @@ -348,7 +348,7 @@ impl InstructionProcessor { instruction: &'a CompiledInstruction, executable_accounts: &'a [(Pubkey, Rc>)], accounts: &'a [(Pubkey, Rc>)], - demote_sysvar_write_locks: bool, + demote_program_write_locks: bool, ) -> Vec<(bool, bool, &'a Pubkey, &'a RefCell)> { executable_accounts .iter() @@ -357,7 +357,7 @@ impl InstructionProcessor { let index = *index as usize; ( message.is_signer(index), - message.is_writable(index, demote_sysvar_write_locks), + message.is_writable(index, demote_program_write_locks), &accounts[index].0, &accounts[index].1 as &RefCell, ) diff --git a/sdk/program/src/message/sanitized.rs b/sdk/program/src/message/sanitized.rs index e33d188c963fa2..3b8d3d79755c99 100644 --- a/sdk/program/src/message/sanitized.rs +++ b/sdk/program/src/message/sanitized.rs @@ -190,9 +190,6 @@ impl SanitizedMessage { /// Returns true if the account at the specified index is writable by the /// instructions in this message. pub fn is_writable(&self, index: usize, demote_program_write_locks: bool) -> bool { - if demote_program_write_locks && self.is_invoked(index) { - return false; - } match self { Self::Legacy(message) => message.is_writable(index, demote_program_write_locks), Self::V0(message) => message.is_writable(index, demote_program_write_locks),