From d5c4d4d5c8c2d4e993ecfab54ae25dc8ab58d6eb Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Mon, 27 Sep 2021 13:02:57 -0600 Subject: [PATCH 1/4] Make helper associated fn --- runtime/src/accounts.rs | 9 +-------- sdk/program/src/message/legacy.rs | 7 +++++++ sdk/program/src/message/mapped.rs | 7 +++++++ sdk/program/src/message/sanitized.rs | 8 ++++++++ 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 97323bf0f62ecb..f32e769f0170e7 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -241,7 +241,6 @@ impl Accounts { 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()); - let is_upgradeable_loader_present = is_upgradeable_loader_present(message); for (i, key) in message.account_keys_iter().enumerate() { let account = if !message.is_non_loader_key(i) { @@ -280,7 +279,7 @@ impl Accounts { if bpf_loader_upgradeable::check_id(account.owner()) { if demote_program_write_locks && message.is_writable(i, demote_program_write_locks) - && !is_upgradeable_loader_present + && !message.is_upgradeable_loader_present() { error_counters.invalid_writable_account += 1; return Err(TransactionError::InvalidWritableAccount); @@ -1133,12 +1132,6 @@ pub fn prepare_if_nonce_account( false } -fn is_upgradeable_loader_present(message: &SanitizedMessage) -> bool { - message - .account_keys_iter() - .any(|&key| key == bpf_loader_upgradeable::id()) -} - pub fn create_test_accounts( accounts: &Accounts, pubkeys: &mut Vec, diff --git a/sdk/program/src/message/legacy.rs b/sdk/program/src/message/legacy.rs index 21742b837da0f3..456ea95808661d 100644 --- a/sdk/program/src/message/legacy.rs +++ b/sdk/program/src/message/legacy.rs @@ -503,6 +503,13 @@ impl Message { } false } + + /// Returns true if any account is the bpf upgradeable loader + pub fn is_upgradeable_loader_present(&self) -> bool { + self.account_keys + .iter() + .any(|&key| key == bpf_loader_upgradeable::id()) + } } #[cfg(test)] diff --git a/sdk/program/src/message/mapped.rs b/sdk/program/src/message/mapped.rs index 0f30e35238db07..3d8bec3885b63a 100644 --- a/sdk/program/src/message/mapped.rs +++ b/sdk/program/src/message/mapped.rs @@ -1,5 +1,6 @@ use { crate::{ + bpf_loader_upgradeable, message::{legacy::BUILTIN_PROGRAMS_KEYS, v0}, pubkey::Pubkey, sysvar, @@ -116,6 +117,12 @@ impl MappedMessage { false } } + + /// Returns true if any account is the bpf upgradeable loader + pub fn is_upgradeable_loader_present(&self) -> bool { + self.account_keys_iter() + .any(|&key| key == bpf_loader_upgradeable::id()) + } } #[cfg(test)] diff --git a/sdk/program/src/message/sanitized.rs b/sdk/program/src/message/sanitized.rs index 3b8d3d79755c99..d771f2ca082336 100644 --- a/sdk/program/src/message/sanitized.rs +++ b/sdk/program/src/message/sanitized.rs @@ -308,6 +308,14 @@ impl SanitizedMessage { .saturating_add(num_secp256k1_signatures), ) } + + /// Inspect all message keys for the bpf upgradeable loader + pub fn is_upgradeable_loader_present(&self) -> bool { + match self { + Self::Legacy(message) => message.is_upgradeable_loader_present(), + Self::V0(message) => message.is_upgradeable_loader_present(), + } + } } #[cfg(test)] From da7a9c69e119cc05d1f815b992c9b153303fa27e Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Mon, 27 Sep 2021 11:02:42 -0600 Subject: [PATCH 2/4] Add feature definition --- 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 44db6fc860866e..e5207fdf17bc49 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -223,6 +223,10 @@ pub mod do_support_realloc { solana_sdk::declare_id!("75m6ysz33AfLA5DDEzWM1obBrnPQRSsdVQ2nRmc8Vuu1"); } +pub mod restore_write_lock_when_upgradeable { + solana_sdk::declare_id!("3Tye2iVqQTxprFSJNpyz5W6SjKNQVfRUDR2s3oVYS6h6"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -274,6 +278,7 @@ lazy_static! { (sol_log_data_syscall_enabled::id(), "enable sol_log_data syscall"), (stakes_remove_delegation_if_inactive::id(), "remove delegations from stakes cache when inactive"), (do_support_realloc::id(), "support account data reallocation"), + (restore_write_lock_when_upgradeable::id(), "restore program-id write lock when upgradeable loader present"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() From 16f1b021a17ae56cbc079602bd55e6f529d55f3f Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Mon, 27 Sep 2021 14:14:37 -0600 Subject: [PATCH 3/4] Add handling to preserve program-id write lock when upgradeable loader is present; restore bpf upgrade-self test --- cli-output/src/display.rs | 2 +- core/src/banking_stage.rs | 10 +- core/src/cost_model.rs | 12 ++- core/src/cost_tracker.rs | 8 +- ledger-tool/src/main.rs | 2 +- program-runtime/src/instruction_processor.rs | 12 ++- program-test/src/lib.rs | 11 ++- programs/bpf/tests/programs.rs | 96 +++++++++++++++++++- programs/bpf_loader/src/syscalls.rs | 11 ++- rpc/src/transaction_status_service.rs | 4 +- runtime/src/accounts.rs | 76 +++++++++------- runtime/src/bank.rs | 27 +++--- runtime/src/message_processor.rs | 27 ++++-- sdk/benches/serialize_instructions.rs | 8 +- sdk/program/src/message/legacy.rs | 29 +++--- sdk/program/src/message/mapped.rs | 16 ++-- sdk/program/src/message/sanitized.rs | 14 +-- sdk/src/transaction/sanitized.rs | 7 +- transaction-status/src/parse_accounts.rs | 2 +- 19 files changed, 249 insertions(+), 125 deletions(-) diff --git a/cli-output/src/display.rs b/cli-output/src/display.rs index f1d52c1f67dc64..bf28c9fb163a3a 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_program_write_locks=*/ true) { + if message.is_writable(index, /*demote_program_write_lock_features=*/ 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 0de26fb6456b27..4745f46adf2e03 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -1091,7 +1091,7 @@ impl BankingStage { feature_set: &Arc, cost_tracker: &Arc>, banking_stage_stats: &BankingStageStats, - demote_program_write_locks: bool, + demote_program_write_lock_features: bool, votes_only: bool, ) -> (Vec, Vec, Vec) { let mut retryable_transaction_packet_indexes: Vec = vec![]; @@ -1126,7 +1126,7 @@ impl BankingStage { .into_iter() .filter_map(|(tx, tx_index)| { let result = cost_tracker_readonly - .would_transaction_fit(&tx, demote_program_write_locks); + .would_transaction_fit(&tx, demote_program_write_lock_features); if result.is_err() { debug!("transaction {:?} would exceed limit: {:?}", tx, result); retryable_transaction_packet_indexes.push(tx_index); @@ -1208,7 +1208,7 @@ impl BankingStage { &bank.feature_set, cost_tracker, banking_stage_stats, - bank.demote_program_write_locks(), + bank.demote_program_write_lock_features(), bank.vote_only_bank(), ); packet_conversion_time.stop(); @@ -1249,7 +1249,7 @@ impl BankingStage { cost_tracker .write() .unwrap() - .add_transaction_cost(tx, bank.demote_program_write_locks()); + .add_transaction_cost(tx, bank.demote_program_write_lock_features()); } }); cost_tracking_time.stop(); @@ -1315,7 +1315,7 @@ impl BankingStage { &bank.feature_set, cost_tracker, banking_stage_stats, - bank.demote_program_write_locks(), + bank.demote_program_write_lock_features(), bank.vote_only_bank(), ); unprocessed_packet_conversion_time.stop(); diff --git a/core/src/cost_model.rs b/core/src/cost_model.rs index ade82ea8e746b1..ac323851644627 100644 --- a/core/src/cost_model.rs +++ b/core/src/cost_model.rs @@ -116,7 +116,7 @@ impl CostModel { pub fn calculate_cost( &mut self, transaction: &SanitizedTransaction, - demote_program_write_locks: bool, + demote_program_write_lock_features: bool, ) -> &TransactionCost { self.transaction_cost.reset(); @@ -126,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, demote_program_write_locks); + let is_writable = message.is_writable(i, demote_program_write_lock_features); if is_writable { self.transaction_cost.writable_accounts.push(*k); @@ -357,7 +357,8 @@ mod tests { .unwrap(); let mut cost_model = CostModel::default(); - let tx_cost = cost_model.calculate_cost(&tx, /*demote_program_write_locks=*/ true); + let tx_cost = + cost_model.calculate_cost(&tx, /*demote_program_write_lock_features=*/ 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]); @@ -399,7 +400,8 @@ mod tests { cost_model .upsert_instruction_cost(&system_program::id(), expected_execution_cost) .unwrap(); - let tx_cost = cost_model.calculate_cost(&tx, /*demote_program_write_locks=*/ true); + let tx_cost = + cost_model.calculate_cost(&tx, /*demote_program_write_lock_features=*/ 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()); @@ -470,7 +472,7 @@ mod tests { thread::spawn(move || { let mut cost_model = cost_model.write().unwrap(); let tx_cost = cost_model - .calculate_cost(&tx, /*demote_program_write_locks=*/ true); + .calculate_cost(&tx, /*demote_program_write_lock_features=*/ 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 40a86133adb7d3..1d37b3a1e504fc 100644 --- a/core/src/cost_tracker.rs +++ b/core/src/cost_tracker.rs @@ -46,10 +46,10 @@ impl CostTracker { pub fn would_transaction_fit( &self, transaction: &SanitizedTransaction, - demote_program_write_locks: bool, + demote_program_write_lock_features: bool, ) -> Result<(), CostModelError> { let mut cost_model = self.cost_model.write().unwrap(); - let tx_cost = cost_model.calculate_cost(transaction, demote_program_write_locks); + let tx_cost = cost_model.calculate_cost(transaction, demote_program_write_lock_features); self.would_fit( &tx_cost.writable_accounts, &(tx_cost.account_access_cost + tx_cost.execution_cost), @@ -59,10 +59,10 @@ impl CostTracker { pub fn add_transaction_cost( &mut self, transaction: &SanitizedTransaction, - demote_program_write_locks: bool, + demote_program_write_lock_features: bool, ) { let mut cost_model = self.cost_model.write().unwrap(); - let tx_cost = cost_model.calculate_cost(transaction, demote_program_write_locks); + let tx_cost = cost_model.calculate_cost(transaction, demote_program_write_lock_features); 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 bcf3b80fbd2fbe..3f63e3746db063 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -794,7 +794,7 @@ fn compute_slot_cost(blockstore: &Blockstore, slot: Slot) -> Result<(), String> let tx_cost = cost_model.calculate_cost( &transaction, - true, // demote_program_write_locks + true, // demote_program_write_lock_features ); if cost_tracker.try_add(tx_cost).is_err() { println!( diff --git a/program-runtime/src/instruction_processor.rs b/program-runtime/src/instruction_processor.rs index 20692119239e64..b3efbf50823aad 100644 --- a/program-runtime/src/instruction_processor.rs +++ b/program-runtime/src/instruction_processor.rs @@ -4,7 +4,10 @@ use solana_sdk::{ account::{AccountSharedData, ReadableAccount, WritableAccount}, account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, - feature_set::{demote_program_write_locks, do_support_realloc, fix_write_privs}, + feature_set::{ + demote_program_write_locks, do_support_realloc, fix_write_privs, + restore_write_lock_when_upgradeable, + }, ic_msg, instruction::{Instruction, InstructionError}, message::Message, @@ -618,10 +621,11 @@ impl InstructionProcessor { ); if result.is_ok() { // Verify the called program has not misbehaved - let demote_program_write_locks = - invoke_context.is_feature_active(&demote_program_write_locks::id()); + let demote_program_write_lock_features = invoke_context + .is_feature_active(&demote_program_write_locks::id()) + && invoke_context.is_feature_active(&restore_write_lock_when_upgradeable::id()); let write_privileges: Vec = (0..message.account_keys.len()) - .map(|i| message.is_writable(i, demote_program_write_locks)) + .map(|i| message.is_writable(i, demote_program_write_lock_features)) .collect(); result = invoke_context.verify_and_update(instruction, account_indices, &write_privileges); diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs index 69b97f5ecf6010..18864491c664b1 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -24,7 +24,7 @@ use { compute_budget::ComputeBudget, entrypoint::{ProgramResult, SUCCESS}, epoch_schedule::EpochSchedule, - feature_set::demote_program_write_locks, + feature_set::{demote_program_write_locks, restore_write_lock_when_upgradeable}, fee_calculator::{FeeCalculator, FeeRateGovernor}, genesis_config::{ClusterType, GenesisConfig}, hash::Hash, @@ -262,14 +262,15 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { let message = Message::new(&[instruction.clone()], None); let program_id_index = message.instructions[0].program_id_index as usize; let program_id = message.account_keys[program_id_index]; - let demote_program_write_locks = - invoke_context.is_feature_active(&demote_program_write_locks::id()); + let demote_program_write_lock_features = invoke_context + .is_feature_active(&demote_program_write_locks::id()) + && invoke_context.is_feature_active(&restore_write_lock_when_upgradeable::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_program_write_locks)) + .map(|(i, _)| message.is_writable(i, demote_program_write_lock_features)) .collect::>(); stable_log::program_invoke(&logger, &program_id, invoke_context.invoke_depth()); @@ -295,7 +296,7 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { account.set_executable(account_info.executable); account.set_rent_epoch(account_info.rent_epoch); } - let account_info = if message.is_writable(i, demote_program_write_locks) { + let account_info = if message.is_writable(i, demote_program_write_lock_features) { Some(account_info) } else { None diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index c99dfacff63baf..2be7c2114044cf 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -1867,7 +1867,7 @@ fn test_program_bpf_upgrade_and_invoke_in_same_tx() { "solana_bpf_rust_panic", ); - // Attempt to invoke, then upgrade the program in same tx + // Invoke, then upgrade the program, and then invoke again in same tx let message = Message::new( &[ invoke_instruction.clone(), @@ -1886,12 +1886,10 @@ 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(1, InstructionError::InvalidArgument) + TransactionError::InstructionError(2, InstructionError::ProgramFailedToComplete) ); } @@ -2187,6 +2185,96 @@ 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_readonly(noop_program_id, false), + AccountMeta::new_readonly(noop_program_id, false), + AccountMeta::new_readonly(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() { diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index d97ce1f5c8196a..0a2ebb54f90b10 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -22,8 +22,8 @@ use solana_sdk::{ allow_native_ids, blake3_syscall_enabled, check_seed_length, close_upgradeable_program_accounts, demote_program_write_locks, disable_fees_sysvar, do_support_realloc, libsecp256k1_0_5_upgrade_enabled, mem_overlap_fix, - return_data_syscall_enabled, secp256k1_recover_syscall_enabled, - sol_log_data_syscall_enabled, + restore_write_lock_when_upgradeable, return_data_syscall_enabled, + secp256k1_recover_syscall_enabled, sol_log_data_syscall_enabled, }, hash::{Hasher, HASH_BYTES}, ic_msg, @@ -2079,8 +2079,9 @@ fn get_translated_accounts<'a, T, F>( where F: Fn(&T, &mut dyn InvokeContext) -> Result, EbpfError>, { - let demote_program_write_locks = - invoke_context.is_feature_active(&demote_program_write_locks::id()); + let demote_program_write_lock_features = invoke_context + .is_feature_active(&demote_program_write_locks::id()) + && invoke_context.is_feature_active(&restore_write_lock_when_upgradeable::id()); let mut account_indices = Vec::with_capacity(message.account_keys.len()); let mut accounts = Vec::with_capacity(message.account_keys.len()); for (i, account_key) in message.account_keys.iter().enumerate() { @@ -2106,7 +2107,7 @@ where account.set_executable(caller_account.executable); account.set_rent_epoch(caller_account.rent_epoch); } - let caller_account = if message.is_writable(i, demote_program_write_locks) { + let caller_account = if message.is_writable(i, demote_program_write_lock_features) { Some(caller_account) } else { None diff --git a/rpc/src/transaction_status_service.rs b/rpc/src/transaction_status_service.rs index 62630d3a17d412..6d10467795bf88 100644 --- a/rpc/src/transaction_status_service.rs +++ b/rpc/src/transaction_status_service.rs @@ -113,8 +113,8 @@ impl TransactionStatusService { }) .expect("FeeCalculator must exist"); let fee = transaction.message().calculate_fee(&fee_calculator); - let tx_account_locks = - transaction.get_account_locks(bank.demote_program_write_locks()); + let tx_account_locks = transaction + .get_account_locks(bank.demote_program_write_lock_features()); let inner_instructions = inner_instructions.map(|inner_instructions| { inner_instructions diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index f32e769f0170e7..4851488958a817 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -199,9 +199,9 @@ impl Accounts { fn construct_instructions_account( message: &SanitizedMessage, is_owned_by_sysvar: bool, - demote_program_write_locks: bool, + demote_program_write_lock_features: bool, ) -> AccountSharedData { - let mut data = message.serialize_instructions(demote_program_write_locks); + let mut data = message.serialize_instructions(demote_program_write_lock_features); // add room for current instruction index. data.resize(data.len() + 2, 0); let owner = if is_owned_by_sysvar { @@ -239,8 +239,9 @@ 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()); + let demote_program_write_lock_features = feature_set + .is_active(&feature_set::demote_program_write_locks::id()) + && feature_set.is_active(&feature_set::restore_write_lock_when_upgradeable::id()); for (i, key) in message.account_keys_iter().enumerate() { let account = if !message.is_non_loader_key(i) { @@ -256,14 +257,14 @@ impl Accounts { message, feature_set .is_active(&feature_set::instructions_sysvar_owned_by_sysvar::id()), - demote_program_write_locks, + demote_program_write_lock_features, ) } else { let (account, rent) = self .accounts_db .load_with_fixed_root(ancestors, key) .map(|(mut account, _)| { - if message.is_writable(i, demote_program_write_locks) { + if message.is_writable(i, demote_program_write_lock_features) { let rent_due = rent_collector.collect_from_existing_account( key, &mut account, @@ -277,8 +278,8 @@ impl Accounts { .unwrap_or_default(); if bpf_loader_upgradeable::check_id(account.owner()) { - if demote_program_write_locks - && message.is_writable(i, demote_program_write_locks) + if demote_program_write_lock_features + && message.is_writable(i, demote_program_write_lock_features) && !message.is_upgradeable_loader_present() { error_counters.invalid_writable_account += 1; @@ -307,8 +308,8 @@ impl Accounts { } } } else if account.executable() - && demote_program_write_locks - && message.is_writable(i, demote_program_write_locks) + && demote_program_write_lock_features + && message.is_writable(i, demote_program_write_lock_features) { error_counters.invalid_writable_account += 1; return Err(TransactionError::InvalidWritableAccount); @@ -916,10 +917,10 @@ impl Accounts { pub fn lock_accounts<'a>( &self, txs: impl Iterator, - demote_program_write_locks: bool, + demote_program_write_lock_features: bool, ) -> Vec> { let keys: Vec<_> = txs - .map(|tx| tx.get_account_locks(demote_program_write_locks)) + .map(|tx| tx.get_account_locks(demote_program_write_lock_features)) .collect(); let mut account_locks = &mut self.account_locks.lock().unwrap(); keys.into_iter() @@ -933,7 +934,7 @@ impl Accounts { &self, txs: impl Iterator, results: &[Result<()>], - demote_program_write_locks: bool, + demote_program_write_lock_features: bool, ) { let keys: Vec<_> = txs .zip(results) @@ -941,7 +942,7 @@ impl Accounts { Err(TransactionError::AccountInUse) => None, Err(TransactionError::SanitizeFailure) => None, Err(TransactionError::AccountLoadedTwice) => None, - _ => Some(tx.get_account_locks(demote_program_write_locks)), + _ => Some(tx.get_account_locks(demote_program_write_lock_features)), }) .collect(); let mut account_locks = self.account_locks.lock().unwrap(); @@ -964,7 +965,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, + demote_program_write_lock_features: bool, ) { let accounts_to_store = self.collect_accounts_to_store( txs, @@ -974,7 +975,7 @@ impl Accounts { last_blockhash_with_fee_calculator, rent_for_sysvars, merge_nonce_error_into_system_error, - demote_program_write_locks, + demote_program_write_lock_features, ); self.accounts_db.store_cached(slot, &accounts_to_store); } @@ -1000,7 +1001,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, + demote_program_write_lock_features: 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() { @@ -1049,7 +1050,7 @@ impl Accounts { fee_payer_index = Some(i); } let is_fee_payer = Some(i) == fee_payer_index; - if message.is_writable(i, demote_program_write_locks) + if message.is_writable(i, demote_program_write_lock_features) && (res.is_ok() || (maybe_nonce_rollback.is_some() && (is_nonce_account || is_fee_payer))) { @@ -2070,7 +2071,7 @@ 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 demote_program_write_lock_features = true; let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; let message = Message::new_with_compiled_instructions( @@ -2082,7 +2083,8 @@ mod tests { instructions, ); let tx = new_sanitized_tx(&[&keypair0], message, Hash::default()); - let results0 = accounts.lock_accounts([tx.clone()].iter(), demote_program_write_locks); + let results0 = + accounts.lock_accounts([tx.clone()].iter(), demote_program_write_lock_features); assert!(results0[0].is_ok()); assert_eq!( @@ -2117,7 +2119,7 @@ mod tests { ); let tx1 = new_sanitized_tx(&[&keypair1], message, Hash::default()); let txs = vec![tx0, tx1]; - let results1 = accounts.lock_accounts(txs.iter(), demote_program_write_locks); + let results1 = accounts.lock_accounts(txs.iter(), demote_program_write_lock_features); 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 @@ -2132,8 +2134,8 @@ mod tests { 2 ); - accounts.unlock_accounts([tx].iter(), &results0, demote_program_write_locks); - accounts.unlock_accounts(txs.iter(), &results1, demote_program_write_locks); + accounts.unlock_accounts([tx].iter(), &results0, demote_program_write_lock_features); + accounts.unlock_accounts(txs.iter(), &results1, demote_program_write_lock_features); let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; let message = Message::new_with_compiled_instructions( 1, @@ -2144,7 +2146,7 @@ mod tests { instructions, ); let tx = new_sanitized_tx(&[&keypair1], message, Hash::default()); - let results2 = accounts.lock_accounts([tx].iter(), demote_program_write_locks); + let results2 = accounts.lock_accounts([tx].iter(), demote_program_write_lock_features); assert!(results2[0].is_ok()); // Now keypair1 account can be locked as writable // Check that read-only lock with zero references is deleted @@ -2181,7 +2183,7 @@ 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 demote_program_write_lock_features = true; let accounts_arc = Arc::new(accounts); @@ -2217,13 +2219,17 @@ mod tests { let txs = vec![writable_tx.clone()]; let results = accounts_clone .clone() - .lock_accounts(txs.iter(), demote_program_write_locks); + .lock_accounts(txs.iter(), demote_program_write_lock_features); for result in results.iter() { if result.is_ok() { counter_clone.clone().fetch_add(1, Ordering::SeqCst); } } - accounts_clone.unlock_accounts(txs.iter(), &results, demote_program_write_locks); + accounts_clone.unlock_accounts( + txs.iter(), + &results, + demote_program_write_lock_features, + ); if exit_clone.clone().load(Ordering::Relaxed) { break; } @@ -2234,20 +2240,20 @@ mod tests { let txs = vec![readonly_tx.clone()]; let results = accounts_arc .clone() - .lock_accounts(txs.iter(), demote_program_write_locks); + .lock_accounts(txs.iter(), demote_program_write_lock_features); 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, demote_program_write_locks); + accounts_arc.unlock_accounts(txs.iter(), &results, demote_program_write_lock_features); thread::sleep(time::Duration::from_millis(50)); } exit.store(true, Ordering::Relaxed); } #[test] - fn test_demote_program_write_locks() { + fn test_demote_program_write_lock_features() { let keypair0 = Keypair::new(); let keypair1 = Keypair::new(); let keypair2 = Keypair::new(); @@ -2270,7 +2276,7 @@ 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 demote_program_write_lock_features = true; let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; let message = Message::new_with_compiled_instructions( @@ -2282,7 +2288,7 @@ mod tests { instructions, ); let tx = new_sanitized_tx(&[&keypair0], message, Hash::default()); - let results0 = accounts.lock_accounts([tx].iter(), demote_program_write_locks); + let results0 = accounts.lock_accounts([tx].iter(), demote_program_write_lock_features); assert!(results0[0].is_ok()); // Instruction program-id account demoted to readonly @@ -2398,7 +2404,7 @@ mod tests { &(Hash::default(), FeeCalculator::default()), true, true, // merge_nonce_error_into_system_error - true, // demote_program_write_locks + true, // demote_program_write_lock_features ); assert_eq!(collected_accounts.len(), 2); assert!(collected_accounts @@ -2776,7 +2782,7 @@ mod tests { &(next_blockhash, FeeCalculator::default()), true, true, // merge_nonce_error_into_system_error - true, // demote_program_write_locks + true, // demote_program_write_lock_features ); assert_eq!(collected_accounts.len(), 2); assert_eq!( @@ -2892,7 +2898,7 @@ mod tests { &(next_blockhash, FeeCalculator::default()), true, true, // merge_nonce_error_into_system_error - true, // demote_program_write_locks + true, // demote_program_write_lock_features ); 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 6afc5d02600577..9e0b28f28537d2 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2782,10 +2782,10 @@ impl Bank { .into_iter() .map(SanitizedTransaction::try_from) .collect::>>()?; - let lock_results = self - .rc - .accounts - .lock_accounts(sanitized_txs.iter(), self.demote_program_write_locks()); + let lock_results = self.rc.accounts.lock_accounts( + sanitized_txs.iter(), + self.demote_program_write_lock_features(), + ); Ok(TransactionBatch::new( lock_results, self, @@ -2805,10 +2805,10 @@ impl Bank { }) }) .collect::>>()?; - let lock_results = self - .rc - .accounts - .lock_accounts(sanitized_txs.iter(), self.demote_program_write_locks()); + let lock_results = self.rc.accounts.lock_accounts( + sanitized_txs.iter(), + self.demote_program_write_lock_features(), + ); Ok(TransactionBatch::new( lock_results, self, @@ -2824,7 +2824,7 @@ impl Bank { let lock_results = self .rc .accounts - .lock_accounts(txs.iter(), self.demote_program_write_locks()); + .lock_accounts(txs.iter(), self.demote_program_write_lock_features()); TransactionBatch::new(lock_results, self, Cow::Borrowed(txs)) } @@ -2909,7 +2909,7 @@ impl Bank { self.rc.accounts.unlock_accounts( batch.sanitized_transactions().iter(), batch.lock_results(), - self.demote_program_write_locks(), + self.demote_program_write_lock_features(), ) } } @@ -3654,7 +3654,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(), + self.demote_program_write_lock_features(), ); let rent_debits = self.collect_rent(executed, loaded_txs); @@ -5410,9 +5410,12 @@ impl Bank { .is_active(&feature_set::stake_program_advance_activating_credits_observed::id()) } - pub fn demote_program_write_locks(&self) -> bool { + pub fn demote_program_write_lock_features(&self) -> bool { self.feature_set .is_active(&feature_set::demote_program_write_locks::id()) + && self + .feature_set + .is_active(&feature_set::restore_write_lock_when_upgradeable::id()) } pub fn stakes_remove_delegation_if_inactive_enabled(&self) -> bool { diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 3e7c2d66665b68..5a3a2eda402c28 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -11,7 +11,7 @@ use solana_sdk::{ compute_budget::ComputeBudget, feature_set::{ demote_program_write_locks, do_support_realloc, neon_evm_compute_budget, - tx_wide_compute_cap, FeatureSet, + restore_write_lock_when_upgradeable, tx_wide_compute_cap, FeatureSet, }, fee_calculator::FeeCalculator, hash::Hash, @@ -159,9 +159,12 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { } // Create the KeyedAccounts that will be passed to the program - let demote_program_write_locks = self + let demote_program_write_lock_features = self .feature_set - .is_active(&demote_program_write_locks::id()); + .is_active(&demote_program_write_locks::id()) + && self + .feature_set + .is_active(&restore_write_lock_when_upgradeable::id()); let keyed_accounts = program_indices .iter() .map(|account_index| { @@ -181,7 +184,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { }; ( message.is_signer(index_in_instruction), - message.is_writable(index_in_instruction, demote_program_write_locks), + message.is_writable(index_in_instruction, demote_program_write_lock_features), &self.accounts[account_index].0, &self.accounts[account_index].1 as &RefCell, ) @@ -206,7 +209,9 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { program_indices: &[usize], ) -> Result<(), InstructionError> { let program_id = instruction.program_id(&message.account_keys); - let demote_program_write_locks = self.is_feature_active(&demote_program_write_locks::id()); + let demote_program_write_lock_features = self + .is_feature_active(&demote_program_write_locks::id()) + && self.is_feature_active(&restore_write_lock_when_upgradeable::id()); let do_support_realloc = self.feature_set.is_active(&do_support_realloc::id()); // Verify all executable accounts have zero outstanding refs @@ -231,7 +236,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { self.pre_accounts[unique_index] .verify( program_id, - message.is_writable(account_index, demote_program_write_locks), + message.is_writable(account_index, demote_program_write_lock_features), &self.rent, &account, &mut self.timings, @@ -702,7 +707,7 @@ mod tests { None, ); let write_privileges: Vec = (0..message.account_keys.len()) - .map(|i| message.is_writable(i, /*demote_program_write_locks=*/ true)) + .map(|i| message.is_writable(i, /*demote_program_write_lock_features=*/ true)) .collect(); // modify account owned by the program @@ -1199,7 +1204,9 @@ mod tests { let message = Message::new(&[callee_instruction], None); let feature_set = FeatureSet::all_enabled(); - let demote_program_write_locks = feature_set.is_active(&demote_program_write_locks::id()); + let demote_program_write_lock_features = feature_set + .is_active(&demote_program_write_locks::id()) + && feature_set.is_active(&restore_write_lock_when_upgradeable::id()); let ancestors = Ancestors::default(); let blockhash = Hash::default(); @@ -1234,7 +1241,7 @@ mod tests { .account_keys .iter() .enumerate() - .map(|(i, _)| message.is_writable(i, demote_program_write_locks)) + .map(|(i, _)| message.is_writable(i, demote_program_write_lock_features)) .collect::>(); accounts[0].1.borrow_mut().data_as_mut_slice()[0] = 1; assert_eq!( @@ -1313,7 +1320,7 @@ mod tests { .account_keys .iter() .enumerate() - .map(|(i, _)| message.is_writable(i, demote_program_write_locks)) + .map(|(i, _)| message.is_writable(i, demote_program_write_lock_features)) .collect::>(); assert_eq!( InstructionProcessor::process_cross_program_instruction( diff --git a/sdk/benches/serialize_instructions.rs b/sdk/benches/serialize_instructions.rs index 2d639bf01f7406..17b2d91af1a667 100644 --- a/sdk/benches/serialize_instructions.rs +++ b/sdk/benches/serialize_instructions.rs @@ -15,7 +15,7 @@ fn make_instructions() -> Vec { vec![inst; 4] } -const DEMOTE_PROGRAM_WRITE_LOCKS: bool = true; +const DEMOTE_PROGRAM_WRITE_LOCK_FEATURES: bool = true; #[bench] fn bench_bincode_instruction_serialize(b: &mut Bencher) { @@ -32,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(DEMOTE_PROGRAM_WRITE_LOCKS)); + test::black_box(message.serialize_instructions(DEMOTE_PROGRAM_WRITE_LOCK_FEATURES)); }); } @@ -51,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(DEMOTE_PROGRAM_WRITE_LOCKS); + let serialized = message.serialize_instructions(DEMOTE_PROGRAM_WRITE_LOCK_FEATURES); b.iter(|| { for i in 0..instructions.len() { test::black_box(instructions::load_instruction_at(i, &serialized).unwrap()); @@ -65,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(DEMOTE_PROGRAM_WRITE_LOCKS); + let serialized = message.serialize_instructions(DEMOTE_PROGRAM_WRITE_LOCK_FEATURES); 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 456ea95808661d..7667a4d565f7dd 100644 --- a/sdk/program/src/message/legacy.rs +++ b/sdk/program/src/message/legacy.rs @@ -353,7 +353,10 @@ impl Message { self.program_position(i).is_some() } - pub fn is_writable(&self, i: usize, demote_program_write_locks: bool) -> bool { + pub fn is_writable(&self, i: usize, demote_program_write_lock_features: bool) -> bool { + let demote_program_id = demote_program_write_lock_features + && self.is_key_called_as_program(i) + && !self.is_upgradeable_loader_present(); (i < (self.header.num_required_signatures - self.header.num_readonly_signed_accounts) as usize || (i >= self.header.num_required_signatures as usize @@ -363,7 +366,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)) + && !demote_program_id } pub fn is_signer(&self, i: usize) -> bool { @@ -375,7 +378,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, /*demote_program_write_locks=*/ true) { + if self.is_writable(i, /*demote_program_write_lock_features=*/ true) { writable_keys.push(key); } else { readonly_keys.push(key); @@ -413,8 +416,10 @@ 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_program_write_locks=*/ true); + let is_writable = self.is_writable( + account_index, + /*demote_program_write_lock_features=*/ true, + ); let mut meta_byte = 0; if is_signer { meta_byte |= 1 << Self::IS_SIGNER_BIT; @@ -875,13 +880,13 @@ mod tests { recent_blockhash: Hash::default(), instructions: vec![], }; - 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)); + let demote_program_write_lock_features = true; + assert!(message.is_writable(0, demote_program_write_lock_features)); + assert!(!message.is_writable(1, demote_program_write_lock_features)); + assert!(!message.is_writable(2, demote_program_write_lock_features)); + assert!(message.is_writable(3, demote_program_write_lock_features)); + assert!(message.is_writable(4, demote_program_write_lock_features)); + assert!(!message.is_writable(5, demote_program_write_lock_features)); } #[test] diff --git a/sdk/program/src/message/mapped.rs b/sdk/program/src/message/mapped.rs index 3d8bec3885b63a..54f76658ae47d6 100644 --- a/sdk/program/src/message/mapped.rs +++ b/sdk/program/src/message/mapped.rs @@ -97,11 +97,15 @@ impl MappedMessage { } /// Returns true if the account at the specified index was loaded as writable - pub fn is_writable(&self, key_index: usize, demote_program_write_locks: bool) -> bool { + pub fn is_writable(&self, key_index: usize, demote_program_write_lock_features: 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) - || (demote_program_write_locks && self.is_key_called_as_program(key_index))); + let demote_program_id = demote_program_write_lock_features + && self.is_key_called_as_program(key_index) + && !self.is_upgradeable_loader_present(); + return !(sysvar::is_sysvar_id(key) + || BUILTIN_PROGRAMS_KEYS.contains(key) + || demote_program_id); } } false @@ -255,11 +259,11 @@ mod tests { mapped_msg.message.account_keys[0] = sysvar::clock::id(); assert!(mapped_msg.is_writable_index(0)); - assert!(!mapped_msg.is_writable(0, /*demote_program_write_locks=*/ true)); + assert!(!mapped_msg.is_writable(0, /*demote_program_write_lock_features=*/ true)); mapped_msg.message.account_keys[0] = system_program::id(); assert!(mapped_msg.is_writable_index(0)); - assert!(!mapped_msg.is_writable(0, /*demote_program_write_locks=*/ true)); + assert!(!mapped_msg.is_writable(0, /*demote_program_write_lock_features=*/ true)); } #[test] @@ -291,6 +295,6 @@ mod tests { }; assert!(mapped_msg.is_writable_index(2)); - assert!(!mapped_msg.is_writable(2, /*demote_program_write_locks=*/ true)); + assert!(!mapped_msg.is_writable(2, /*demote_program_write_lock_features=*/ true)); } } diff --git a/sdk/program/src/message/sanitized.rs b/sdk/program/src/message/sanitized.rs index d771f2ca082336..42e2fdb5878f9f 100644 --- a/sdk/program/src/message/sanitized.rs +++ b/sdk/program/src/message/sanitized.rs @@ -189,10 +189,10 @@ 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 { + pub fn is_writable(&self, index: usize, demote_program_write_lock_features: bool) -> bool { match self { - Self::Legacy(message) => message.is_writable(index, demote_program_write_locks), - Self::V0(message) => message.is_writable(index, demote_program_write_locks), + Self::Legacy(message) => message.is_writable(index, demote_program_write_lock_features), + Self::V0(message) => message.is_writable(index, demote_program_write_lock_features), } } @@ -216,7 +216,7 @@ impl SanitizedMessage { // 67..69 - data len - u16 // 69..data_len - data #[allow(clippy::integer_arithmetic)] - pub fn serialize_instructions(&self, demote_program_write_locks: bool) -> Vec { + pub fn serialize_instructions(&self, demote_program_write_lock_features: 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); @@ -231,7 +231,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, demote_program_write_locks); + let is_writable = self.is_writable(account_index, demote_program_write_lock_features); let mut account_meta = InstructionsSysvarAccountMeta::NONE; if is_signer { account_meta |= InstructionsSysvarAccountMeta::IS_SIGNER; @@ -445,10 +445,10 @@ mod tests { ), ]; - let demote_program_write_locks = true; + let demote_program_write_lock_features = true; let message = Message::new(&instructions, Some(&id1)); let sanitized_message = SanitizedMessage::try_from(message.clone()).unwrap(); - let serialized = sanitized_message.serialize_instructions(demote_program_write_locks); + let serialized = sanitized_message.serialize_instructions(demote_program_write_lock_features); // 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 dfa128ba0cabb5..4c20063cb24a9a 100644 --- a/sdk/src/transaction/sanitized.rs +++ b/sdk/src/transaction/sanitized.rs @@ -126,7 +126,10 @@ impl SanitizedTransaction { } /// Return the list of accounts that must be locked during processing this transaction. - pub fn get_account_locks(&self, demote_program_write_locks: bool) -> TransactionAccountLocks { + pub fn get_account_locks( + &self, + demote_program_write_lock_features: bool, + ) -> TransactionAccountLocks { let message = &self.message; let num_readonly_accounts = message.num_readonly_accounts(); let num_writable_accounts = message @@ -139,7 +142,7 @@ impl SanitizedTransaction { }; for (i, key) in message.account_keys_iter().enumerate() { - if message.is_writable(i, demote_program_write_locks) { + if message.is_writable(i, demote_program_write_lock_features) { 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 197643450b6eab..38e98ab0d22344 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_program_write_locks=*/ true), + writable: message.is_writable(i, /*demote_program_write_lock_features=*/ true), signer: message.is_signer(i), }); } From 46957bc45e6cf8c912f71b7071a5a57ad028843a Mon Sep 17 00:00:00 2001 From: Tyera Eulberg Date: Tue, 28 Sep 2021 03:46:30 -0600 Subject: [PATCH 4/4] Use single feature --- cli-output/src/display.rs | 2 +- core/src/banking_stage.rs | 10 +-- core/src/cost_model.rs | 12 ++-- core/src/cost_tracker.rs | 8 +-- ledger-tool/src/main.rs | 2 +- program-runtime/src/instruction_processor.rs | 12 ++-- program-test/src/lib.rs | 11 ++- programs/bpf_loader/src/syscalls.rs | 11 ++- rpc/src/transaction_status_service.rs | 4 +- runtime/src/accounts.rs | 76 +++++++++----------- runtime/src/bank.rs | 27 ++++--- runtime/src/message_processor.rs | 27 +++---- sdk/benches/serialize_instructions.rs | 8 +-- sdk/program/src/message/legacy.rs | 26 ++++--- sdk/program/src/message/mapped.rs | 10 +-- sdk/program/src/message/sanitized.rs | 14 ++-- sdk/src/feature_set.rs | 7 +- sdk/src/transaction/sanitized.rs | 7 +- transaction-status/src/parse_accounts.rs | 2 +- 19 files changed, 121 insertions(+), 155 deletions(-) diff --git a/cli-output/src/display.rs b/cli-output/src/display.rs index bf28c9fb163a3a..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, /*demote_program_write_lock_features=*/ 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/banking_stage.rs b/core/src/banking_stage.rs index 4745f46adf2e03..0de26fb6456b27 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -1091,7 +1091,7 @@ impl BankingStage { feature_set: &Arc, cost_tracker: &Arc>, banking_stage_stats: &BankingStageStats, - demote_program_write_lock_features: bool, + demote_program_write_locks: bool, votes_only: bool, ) -> (Vec, Vec, Vec) { let mut retryable_transaction_packet_indexes: Vec = vec![]; @@ -1126,7 +1126,7 @@ impl BankingStage { .into_iter() .filter_map(|(tx, tx_index)| { let result = cost_tracker_readonly - .would_transaction_fit(&tx, demote_program_write_lock_features); + .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); @@ -1208,7 +1208,7 @@ impl BankingStage { &bank.feature_set, cost_tracker, banking_stage_stats, - bank.demote_program_write_lock_features(), + bank.demote_program_write_locks(), bank.vote_only_bank(), ); packet_conversion_time.stop(); @@ -1249,7 +1249,7 @@ impl BankingStage { cost_tracker .write() .unwrap() - .add_transaction_cost(tx, bank.demote_program_write_lock_features()); + .add_transaction_cost(tx, bank.demote_program_write_locks()); } }); cost_tracking_time.stop(); @@ -1315,7 +1315,7 @@ impl BankingStage { &bank.feature_set, cost_tracker, banking_stage_stats, - bank.demote_program_write_lock_features(), + bank.demote_program_write_locks(), bank.vote_only_bank(), ); unprocessed_packet_conversion_time.stop(); diff --git a/core/src/cost_model.rs b/core/src/cost_model.rs index ac323851644627..ade82ea8e746b1 100644 --- a/core/src/cost_model.rs +++ b/core/src/cost_model.rs @@ -116,7 +116,7 @@ impl CostModel { pub fn calculate_cost( &mut self, transaction: &SanitizedTransaction, - demote_program_write_lock_features: bool, + demote_program_write_locks: bool, ) -> &TransactionCost { self.transaction_cost.reset(); @@ -126,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, demote_program_write_lock_features); + let is_writable = message.is_writable(i, demote_program_write_locks); if is_writable { self.transaction_cost.writable_accounts.push(*k); @@ -357,8 +357,7 @@ mod tests { .unwrap(); let mut cost_model = CostModel::default(); - let tx_cost = - cost_model.calculate_cost(&tx, /*demote_program_write_lock_features=*/ true); + 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]); @@ -400,8 +399,7 @@ mod tests { cost_model .upsert_instruction_cost(&system_program::id(), expected_execution_cost) .unwrap(); - let tx_cost = - cost_model.calculate_cost(&tx, /*demote_program_write_lock_features=*/ true); + 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()); @@ -472,7 +470,7 @@ mod tests { thread::spawn(move || { let mut cost_model = cost_model.write().unwrap(); let tx_cost = cost_model - .calculate_cost(&tx, /*demote_program_write_lock_features=*/ true); + .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 1d37b3a1e504fc..40a86133adb7d3 100644 --- a/core/src/cost_tracker.rs +++ b/core/src/cost_tracker.rs @@ -46,10 +46,10 @@ impl CostTracker { pub fn would_transaction_fit( &self, transaction: &SanitizedTransaction, - demote_program_write_lock_features: bool, + demote_program_write_locks: bool, ) -> Result<(), CostModelError> { let mut cost_model = self.cost_model.write().unwrap(); - let tx_cost = cost_model.calculate_cost(transaction, demote_program_write_lock_features); + 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), @@ -59,10 +59,10 @@ impl CostTracker { pub fn add_transaction_cost( &mut self, transaction: &SanitizedTransaction, - demote_program_write_lock_features: bool, + demote_program_write_locks: bool, ) { let mut cost_model = self.cost_model.write().unwrap(); - let tx_cost = cost_model.calculate_cost(transaction, demote_program_write_lock_features); + 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 3f63e3746db063..bcf3b80fbd2fbe 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -794,7 +794,7 @@ fn compute_slot_cost(blockstore: &Blockstore, slot: Slot) -> Result<(), String> let tx_cost = cost_model.calculate_cost( &transaction, - true, // demote_program_write_lock_features + true, // demote_program_write_locks ); if cost_tracker.try_add(tx_cost).is_err() { println!( diff --git a/program-runtime/src/instruction_processor.rs b/program-runtime/src/instruction_processor.rs index b3efbf50823aad..20692119239e64 100644 --- a/program-runtime/src/instruction_processor.rs +++ b/program-runtime/src/instruction_processor.rs @@ -4,10 +4,7 @@ use solana_sdk::{ account::{AccountSharedData, ReadableAccount, WritableAccount}, account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, - feature_set::{ - demote_program_write_locks, do_support_realloc, fix_write_privs, - restore_write_lock_when_upgradeable, - }, + feature_set::{demote_program_write_locks, do_support_realloc, fix_write_privs}, ic_msg, instruction::{Instruction, InstructionError}, message::Message, @@ -621,11 +618,10 @@ impl InstructionProcessor { ); if result.is_ok() { // Verify the called program has not misbehaved - let demote_program_write_lock_features = invoke_context - .is_feature_active(&demote_program_write_locks::id()) - && invoke_context.is_feature_active(&restore_write_lock_when_upgradeable::id()); + let demote_program_write_locks = + invoke_context.is_feature_active(&demote_program_write_locks::id()); let write_privileges: Vec = (0..message.account_keys.len()) - .map(|i| message.is_writable(i, demote_program_write_lock_features)) + .map(|i| message.is_writable(i, demote_program_write_locks)) .collect(); result = invoke_context.verify_and_update(instruction, account_indices, &write_privileges); diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs index 18864491c664b1..69b97f5ecf6010 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -24,7 +24,7 @@ use { compute_budget::ComputeBudget, entrypoint::{ProgramResult, SUCCESS}, epoch_schedule::EpochSchedule, - feature_set::{demote_program_write_locks, restore_write_lock_when_upgradeable}, + feature_set::demote_program_write_locks, fee_calculator::{FeeCalculator, FeeRateGovernor}, genesis_config::{ClusterType, GenesisConfig}, hash::Hash, @@ -262,15 +262,14 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { let message = Message::new(&[instruction.clone()], None); let program_id_index = message.instructions[0].program_id_index as usize; let program_id = message.account_keys[program_id_index]; - let demote_program_write_lock_features = invoke_context - .is_feature_active(&demote_program_write_locks::id()) - && invoke_context.is_feature_active(&restore_write_lock_when_upgradeable::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_program_write_lock_features)) + .map(|(i, _)| message.is_writable(i, demote_program_write_locks)) .collect::>(); stable_log::program_invoke(&logger, &program_id, invoke_context.invoke_depth()); @@ -296,7 +295,7 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { account.set_executable(account_info.executable); account.set_rent_epoch(account_info.rent_epoch); } - let account_info = if message.is_writable(i, demote_program_write_lock_features) { + let account_info = if message.is_writable(i, demote_program_write_locks) { Some(account_info) } else { None diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index 0a2ebb54f90b10..d97ce1f5c8196a 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -22,8 +22,8 @@ use solana_sdk::{ allow_native_ids, blake3_syscall_enabled, check_seed_length, close_upgradeable_program_accounts, demote_program_write_locks, disable_fees_sysvar, do_support_realloc, libsecp256k1_0_5_upgrade_enabled, mem_overlap_fix, - restore_write_lock_when_upgradeable, return_data_syscall_enabled, - secp256k1_recover_syscall_enabled, sol_log_data_syscall_enabled, + return_data_syscall_enabled, secp256k1_recover_syscall_enabled, + sol_log_data_syscall_enabled, }, hash::{Hasher, HASH_BYTES}, ic_msg, @@ -2079,9 +2079,8 @@ fn get_translated_accounts<'a, T, F>( where F: Fn(&T, &mut dyn InvokeContext) -> Result, EbpfError>, { - let demote_program_write_lock_features = invoke_context - .is_feature_active(&demote_program_write_locks::id()) - && invoke_context.is_feature_active(&restore_write_lock_when_upgradeable::id()); + let demote_program_write_locks = + invoke_context.is_feature_active(&demote_program_write_locks::id()); let mut account_indices = Vec::with_capacity(message.account_keys.len()); let mut accounts = Vec::with_capacity(message.account_keys.len()); for (i, account_key) in message.account_keys.iter().enumerate() { @@ -2107,7 +2106,7 @@ where account.set_executable(caller_account.executable); account.set_rent_epoch(caller_account.rent_epoch); } - let caller_account = if message.is_writable(i, demote_program_write_lock_features) { + let caller_account = if message.is_writable(i, demote_program_write_locks) { Some(caller_account) } else { None diff --git a/rpc/src/transaction_status_service.rs b/rpc/src/transaction_status_service.rs index 6d10467795bf88..62630d3a17d412 100644 --- a/rpc/src/transaction_status_service.rs +++ b/rpc/src/transaction_status_service.rs @@ -113,8 +113,8 @@ impl TransactionStatusService { }) .expect("FeeCalculator must exist"); let fee = transaction.message().calculate_fee(&fee_calculator); - let tx_account_locks = transaction - .get_account_locks(bank.demote_program_write_lock_features()); + 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 4851488958a817..f32e769f0170e7 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -199,9 +199,9 @@ impl Accounts { fn construct_instructions_account( message: &SanitizedMessage, is_owned_by_sysvar: bool, - demote_program_write_lock_features: bool, + demote_program_write_locks: bool, ) -> AccountSharedData { - let mut data = message.serialize_instructions(demote_program_write_lock_features); + 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 { @@ -239,9 +239,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_lock_features = feature_set - .is_active(&feature_set::demote_program_write_locks::id()) - && feature_set.is_active(&feature_set::restore_write_lock_when_upgradeable::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) { @@ -257,14 +256,14 @@ impl Accounts { message, feature_set .is_active(&feature_set::instructions_sysvar_owned_by_sysvar::id()), - demote_program_write_lock_features, + 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, demote_program_write_lock_features) { + if message.is_writable(i, demote_program_write_locks) { let rent_due = rent_collector.collect_from_existing_account( key, &mut account, @@ -278,8 +277,8 @@ impl Accounts { .unwrap_or_default(); if bpf_loader_upgradeable::check_id(account.owner()) { - if demote_program_write_lock_features - && message.is_writable(i, demote_program_write_lock_features) + if demote_program_write_locks + && message.is_writable(i, demote_program_write_locks) && !message.is_upgradeable_loader_present() { error_counters.invalid_writable_account += 1; @@ -308,8 +307,8 @@ impl Accounts { } } } else if account.executable() - && demote_program_write_lock_features - && message.is_writable(i, demote_program_write_lock_features) + && demote_program_write_locks + && message.is_writable(i, demote_program_write_locks) { error_counters.invalid_writable_account += 1; return Err(TransactionError::InvalidWritableAccount); @@ -917,10 +916,10 @@ impl Accounts { pub fn lock_accounts<'a>( &self, txs: impl Iterator, - demote_program_write_lock_features: bool, + demote_program_write_locks: bool, ) -> Vec> { let keys: Vec<_> = txs - .map(|tx| tx.get_account_locks(demote_program_write_lock_features)) + .map(|tx| tx.get_account_locks(demote_program_write_locks)) .collect(); let mut account_locks = &mut self.account_locks.lock().unwrap(); keys.into_iter() @@ -934,7 +933,7 @@ impl Accounts { &self, txs: impl Iterator, results: &[Result<()>], - demote_program_write_lock_features: bool, + demote_program_write_locks: bool, ) { let keys: Vec<_> = txs .zip(results) @@ -942,7 +941,7 @@ impl Accounts { Err(TransactionError::AccountInUse) => None, Err(TransactionError::SanitizeFailure) => None, Err(TransactionError::AccountLoadedTwice) => None, - _ => Some(tx.get_account_locks(demote_program_write_lock_features)), + _ => Some(tx.get_account_locks(demote_program_write_locks)), }) .collect(); let mut account_locks = self.account_locks.lock().unwrap(); @@ -965,7 +964,7 @@ impl Accounts { last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), rent_for_sysvars: bool, merge_nonce_error_into_system_error: bool, - demote_program_write_lock_features: bool, + demote_program_write_locks: bool, ) { let accounts_to_store = self.collect_accounts_to_store( txs, @@ -975,7 +974,7 @@ impl Accounts { last_blockhash_with_fee_calculator, rent_for_sysvars, merge_nonce_error_into_system_error, - demote_program_write_lock_features, + demote_program_write_locks, ); self.accounts_db.store_cached(slot, &accounts_to_store); } @@ -1001,7 +1000,7 @@ impl Accounts { last_blockhash_with_fee_calculator: &(Hash, FeeCalculator), rent_for_sysvars: bool, merge_nonce_error_into_system_error: bool, - demote_program_write_lock_features: 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() { @@ -1050,7 +1049,7 @@ impl Accounts { fee_payer_index = Some(i); } let is_fee_payer = Some(i) == fee_payer_index; - if message.is_writable(i, demote_program_write_lock_features) + if message.is_writable(i, demote_program_write_locks) && (res.is_ok() || (maybe_nonce_rollback.is_some() && (is_nonce_account || is_fee_payer))) { @@ -2071,7 +2070,7 @@ mod tests { accounts.store_slow_uncached(0, &keypair2.pubkey(), &account2); accounts.store_slow_uncached(0, &keypair3.pubkey(), &account3); - let demote_program_write_lock_features = true; + let demote_program_write_locks = true; let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; let message = Message::new_with_compiled_instructions( @@ -2083,8 +2082,7 @@ mod tests { instructions, ); let tx = new_sanitized_tx(&[&keypair0], message, Hash::default()); - let results0 = - accounts.lock_accounts([tx.clone()].iter(), demote_program_write_lock_features); + let results0 = accounts.lock_accounts([tx.clone()].iter(), demote_program_write_locks); assert!(results0[0].is_ok()); assert_eq!( @@ -2119,7 +2117,7 @@ mod tests { ); let tx1 = new_sanitized_tx(&[&keypair1], message, Hash::default()); let txs = vec![tx0, tx1]; - let results1 = accounts.lock_accounts(txs.iter(), demote_program_write_lock_features); + 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 @@ -2134,8 +2132,8 @@ mod tests { 2 ); - accounts.unlock_accounts([tx].iter(), &results0, demote_program_write_lock_features); - accounts.unlock_accounts(txs.iter(), &results1, demote_program_write_lock_features); + 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, @@ -2146,7 +2144,7 @@ mod tests { instructions, ); let tx = new_sanitized_tx(&[&keypair1], message, Hash::default()); - let results2 = accounts.lock_accounts([tx].iter(), demote_program_write_lock_features); + 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 @@ -2183,7 +2181,7 @@ mod tests { accounts.store_slow_uncached(0, &keypair1.pubkey(), &account1); accounts.store_slow_uncached(0, &keypair2.pubkey(), &account2); - let demote_program_write_lock_features = true; + let demote_program_write_locks = true; let accounts_arc = Arc::new(accounts); @@ -2219,17 +2217,13 @@ mod tests { let txs = vec![writable_tx.clone()]; let results = accounts_clone .clone() - .lock_accounts(txs.iter(), demote_program_write_lock_features); + .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, - demote_program_write_lock_features, - ); + accounts_clone.unlock_accounts(txs.iter(), &results, demote_program_write_locks); if exit_clone.clone().load(Ordering::Relaxed) { break; } @@ -2240,20 +2234,20 @@ mod tests { let txs = vec![readonly_tx.clone()]; let results = accounts_arc .clone() - .lock_accounts(txs.iter(), demote_program_write_lock_features); + .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, demote_program_write_lock_features); + 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_lock_features() { + fn test_demote_program_write_locks() { let keypair0 = Keypair::new(); let keypair1 = Keypair::new(); let keypair2 = Keypair::new(); @@ -2276,7 +2270,7 @@ mod tests { accounts.store_slow_uncached(0, &keypair2.pubkey(), &account2); accounts.store_slow_uncached(0, &keypair3.pubkey(), &account3); - let demote_program_write_lock_features = true; + let demote_program_write_locks = true; let instructions = vec![CompiledInstruction::new(2, &(), vec![0, 1])]; let message = Message::new_with_compiled_instructions( @@ -2288,7 +2282,7 @@ mod tests { instructions, ); let tx = new_sanitized_tx(&[&keypair0], message, Hash::default()); - let results0 = accounts.lock_accounts([tx].iter(), demote_program_write_lock_features); + let results0 = accounts.lock_accounts([tx].iter(), demote_program_write_locks); assert!(results0[0].is_ok()); // Instruction program-id account demoted to readonly @@ -2404,7 +2398,7 @@ mod tests { &(Hash::default(), FeeCalculator::default()), true, true, // merge_nonce_error_into_system_error - true, // demote_program_write_lock_features + true, // demote_program_write_locks ); assert_eq!(collected_accounts.len(), 2); assert!(collected_accounts @@ -2782,7 +2776,7 @@ mod tests { &(next_blockhash, FeeCalculator::default()), true, true, // merge_nonce_error_into_system_error - true, // demote_program_write_lock_features + true, // demote_program_write_locks ); assert_eq!(collected_accounts.len(), 2); assert_eq!( @@ -2898,7 +2892,7 @@ mod tests { &(next_blockhash, FeeCalculator::default()), true, true, // merge_nonce_error_into_system_error - true, // demote_program_write_lock_features + 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 9e0b28f28537d2..6afc5d02600577 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -2782,10 +2782,10 @@ impl Bank { .into_iter() .map(SanitizedTransaction::try_from) .collect::>>()?; - let lock_results = self.rc.accounts.lock_accounts( - sanitized_txs.iter(), - self.demote_program_write_lock_features(), - ); + let lock_results = self + .rc + .accounts + .lock_accounts(sanitized_txs.iter(), self.demote_program_write_locks()); Ok(TransactionBatch::new( lock_results, self, @@ -2805,10 +2805,10 @@ impl Bank { }) }) .collect::>>()?; - let lock_results = self.rc.accounts.lock_accounts( - sanitized_txs.iter(), - self.demote_program_write_lock_features(), - ); + let lock_results = self + .rc + .accounts + .lock_accounts(sanitized_txs.iter(), self.demote_program_write_locks()); Ok(TransactionBatch::new( lock_results, self, @@ -2824,7 +2824,7 @@ impl Bank { let lock_results = self .rc .accounts - .lock_accounts(txs.iter(), self.demote_program_write_lock_features()); + .lock_accounts(txs.iter(), self.demote_program_write_locks()); TransactionBatch::new(lock_results, self, Cow::Borrowed(txs)) } @@ -2909,7 +2909,7 @@ impl Bank { self.rc.accounts.unlock_accounts( batch.sanitized_transactions().iter(), batch.lock_results(), - self.demote_program_write_lock_features(), + self.demote_program_write_locks(), ) } } @@ -3654,7 +3654,7 @@ impl Bank { &self.last_blockhash_with_fee_calculator(), self.rent_for_sysvars(), self.merge_nonce_error_into_system_error(), - self.demote_program_write_lock_features(), + self.demote_program_write_locks(), ); let rent_debits = self.collect_rent(executed, loaded_txs); @@ -5410,12 +5410,9 @@ impl Bank { .is_active(&feature_set::stake_program_advance_activating_credits_observed::id()) } - pub fn demote_program_write_lock_features(&self) -> bool { + pub fn demote_program_write_locks(&self) -> bool { self.feature_set .is_active(&feature_set::demote_program_write_locks::id()) - && self - .feature_set - .is_active(&feature_set::restore_write_lock_when_upgradeable::id()) } pub fn stakes_remove_delegation_if_inactive_enabled(&self) -> bool { diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 5a3a2eda402c28..3e7c2d66665b68 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -11,7 +11,7 @@ use solana_sdk::{ compute_budget::ComputeBudget, feature_set::{ demote_program_write_locks, do_support_realloc, neon_evm_compute_budget, - restore_write_lock_when_upgradeable, tx_wide_compute_cap, FeatureSet, + tx_wide_compute_cap, FeatureSet, }, fee_calculator::FeeCalculator, hash::Hash, @@ -159,12 +159,9 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { } // Create the KeyedAccounts that will be passed to the program - let demote_program_write_lock_features = self + let demote_program_write_locks = self .feature_set - .is_active(&demote_program_write_locks::id()) - && self - .feature_set - .is_active(&restore_write_lock_when_upgradeable::id()); + .is_active(&demote_program_write_locks::id()); let keyed_accounts = program_indices .iter() .map(|account_index| { @@ -184,7 +181,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { }; ( message.is_signer(index_in_instruction), - message.is_writable(index_in_instruction, demote_program_write_lock_features), + message.is_writable(index_in_instruction, demote_program_write_locks), &self.accounts[account_index].0, &self.accounts[account_index].1 as &RefCell, ) @@ -209,9 +206,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { program_indices: &[usize], ) -> Result<(), InstructionError> { let program_id = instruction.program_id(&message.account_keys); - let demote_program_write_lock_features = self - .is_feature_active(&demote_program_write_locks::id()) - && self.is_feature_active(&restore_write_lock_when_upgradeable::id()); + let demote_program_write_locks = self.is_feature_active(&demote_program_write_locks::id()); let do_support_realloc = self.feature_set.is_active(&do_support_realloc::id()); // Verify all executable accounts have zero outstanding refs @@ -236,7 +231,7 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { self.pre_accounts[unique_index] .verify( program_id, - message.is_writable(account_index, demote_program_write_lock_features), + message.is_writable(account_index, demote_program_write_locks), &self.rent, &account, &mut self.timings, @@ -707,7 +702,7 @@ mod tests { None, ); let write_privileges: Vec = (0..message.account_keys.len()) - .map(|i| message.is_writable(i, /*demote_program_write_lock_features=*/ true)) + .map(|i| message.is_writable(i, /*demote_program_write_locks=*/ true)) .collect(); // modify account owned by the program @@ -1204,9 +1199,7 @@ mod tests { let message = Message::new(&[callee_instruction], None); let feature_set = FeatureSet::all_enabled(); - let demote_program_write_lock_features = feature_set - .is_active(&demote_program_write_locks::id()) - && feature_set.is_active(&restore_write_lock_when_upgradeable::id()); + let demote_program_write_locks = feature_set.is_active(&demote_program_write_locks::id()); let ancestors = Ancestors::default(); let blockhash = Hash::default(); @@ -1241,7 +1234,7 @@ mod tests { .account_keys .iter() .enumerate() - .map(|(i, _)| message.is_writable(i, demote_program_write_lock_features)) + .map(|(i, _)| message.is_writable(i, demote_program_write_locks)) .collect::>(); accounts[0].1.borrow_mut().data_as_mut_slice()[0] = 1; assert_eq!( @@ -1320,7 +1313,7 @@ mod tests { .account_keys .iter() .enumerate() - .map(|(i, _)| message.is_writable(i, demote_program_write_lock_features)) + .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 17b2d91af1a667..2d639bf01f7406 100644 --- a/sdk/benches/serialize_instructions.rs +++ b/sdk/benches/serialize_instructions.rs @@ -15,7 +15,7 @@ fn make_instructions() -> Vec { vec![inst; 4] } -const DEMOTE_PROGRAM_WRITE_LOCK_FEATURES: bool = true; +const DEMOTE_PROGRAM_WRITE_LOCKS: bool = true; #[bench] fn bench_bincode_instruction_serialize(b: &mut Bencher) { @@ -32,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(DEMOTE_PROGRAM_WRITE_LOCK_FEATURES)); + test::black_box(message.serialize_instructions(DEMOTE_PROGRAM_WRITE_LOCKS)); }); } @@ -51,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(DEMOTE_PROGRAM_WRITE_LOCK_FEATURES); + 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()); @@ -65,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(DEMOTE_PROGRAM_WRITE_LOCK_FEATURES); + 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 7667a4d565f7dd..bbbb49d2d2bc63 100644 --- a/sdk/program/src/message/legacy.rs +++ b/sdk/program/src/message/legacy.rs @@ -353,8 +353,8 @@ impl Message { self.program_position(i).is_some() } - pub fn is_writable(&self, i: usize, demote_program_write_lock_features: bool) -> bool { - let demote_program_id = demote_program_write_lock_features + pub fn is_writable(&self, i: usize, demote_program_write_locks: bool) -> bool { + let demote_program_id = demote_program_write_locks && self.is_key_called_as_program(i) && !self.is_upgradeable_loader_present(); (i < (self.header.num_required_signatures - self.header.num_readonly_signed_accounts) @@ -378,7 +378,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, /*demote_program_write_lock_features=*/ true) { + if self.is_writable(i, /*demote_program_write_locks=*/ true) { writable_keys.push(key); } else { readonly_keys.push(key); @@ -416,10 +416,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, - /*demote_program_write_lock_features=*/ true, - ); + 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; @@ -880,13 +878,13 @@ mod tests { recent_blockhash: Hash::default(), instructions: vec![], }; - let demote_program_write_lock_features = true; - assert!(message.is_writable(0, demote_program_write_lock_features)); - assert!(!message.is_writable(1, demote_program_write_lock_features)); - assert!(!message.is_writable(2, demote_program_write_lock_features)); - assert!(message.is_writable(3, demote_program_write_lock_features)); - assert!(message.is_writable(4, demote_program_write_lock_features)); - assert!(!message.is_writable(5, demote_program_write_lock_features)); + 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/mapped.rs b/sdk/program/src/message/mapped.rs index 54f76658ae47d6..7c599fb6d9d2bf 100644 --- a/sdk/program/src/message/mapped.rs +++ b/sdk/program/src/message/mapped.rs @@ -97,10 +97,10 @@ impl MappedMessage { } /// Returns true if the account at the specified index was loaded as writable - pub fn is_writable(&self, key_index: usize, demote_program_write_lock_features: bool) -> 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) { - let demote_program_id = demote_program_write_lock_features + let demote_program_id = demote_program_write_locks && self.is_key_called_as_program(key_index) && !self.is_upgradeable_loader_present(); return !(sysvar::is_sysvar_id(key) @@ -259,11 +259,11 @@ mod tests { mapped_msg.message.account_keys[0] = sysvar::clock::id(); assert!(mapped_msg.is_writable_index(0)); - assert!(!mapped_msg.is_writable(0, /*demote_program_write_lock_features=*/ true)); + 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, /*demote_program_write_lock_features=*/ true)); + assert!(!mapped_msg.is_writable(0, /*demote_program_write_locks=*/ true)); } #[test] @@ -295,6 +295,6 @@ mod tests { }; assert!(mapped_msg.is_writable_index(2)); - assert!(!mapped_msg.is_writable(2, /*demote_program_write_lock_features=*/ true)); + 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 42e2fdb5878f9f..d771f2ca082336 100644 --- a/sdk/program/src/message/sanitized.rs +++ b/sdk/program/src/message/sanitized.rs @@ -189,10 +189,10 @@ 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_lock_features: bool) -> bool { + pub fn is_writable(&self, index: usize, demote_program_write_locks: bool) -> bool { match self { - Self::Legacy(message) => message.is_writable(index, demote_program_write_lock_features), - Self::V0(message) => message.is_writable(index, demote_program_write_lock_features), + Self::Legacy(message) => message.is_writable(index, demote_program_write_locks), + Self::V0(message) => message.is_writable(index, demote_program_write_locks), } } @@ -216,7 +216,7 @@ impl SanitizedMessage { // 67..69 - data len - u16 // 69..data_len - data #[allow(clippy::integer_arithmetic)] - pub fn serialize_instructions(&self, demote_program_write_lock_features: 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); @@ -231,7 +231,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, demote_program_write_lock_features); + 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; @@ -445,10 +445,10 @@ mod tests { ), ]; - let demote_program_write_lock_features = true; + 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(demote_program_write_lock_features); + 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/feature_set.rs b/sdk/src/feature_set.rs index e5207fdf17bc49..502d0b4728af6e 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -223,10 +223,6 @@ pub mod do_support_realloc { solana_sdk::declare_id!("75m6ysz33AfLA5DDEzWM1obBrnPQRSsdVQ2nRmc8Vuu1"); } -pub mod restore_write_lock_when_upgradeable { - solana_sdk::declare_id!("3Tye2iVqQTxprFSJNpyz5W6SjKNQVfRUDR2s3oVYS6h6"); -} - lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -268,7 +264,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"), + (demote_program_write_locks::id(), "demote program write locks to readonly, except when upgradeable loader present #19593 #20265"), (ed25519_program_enabled::id(), "enable builtin ed25519 signature verify program"), (allow_native_ids::id(), "allow native program ids in program derived addresses"), (check_seed_length::id(), "Check program address seed lengths"), @@ -278,7 +274,6 @@ lazy_static! { (sol_log_data_syscall_enabled::id(), "enable sol_log_data syscall"), (stakes_remove_delegation_if_inactive::id(), "remove delegations from stakes cache when inactive"), (do_support_realloc::id(), "support account data reallocation"), - (restore_write_lock_when_upgradeable::id(), "restore program-id write lock when upgradeable loader present"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/sdk/src/transaction/sanitized.rs b/sdk/src/transaction/sanitized.rs index 4c20063cb24a9a..dfa128ba0cabb5 100644 --- a/sdk/src/transaction/sanitized.rs +++ b/sdk/src/transaction/sanitized.rs @@ -126,10 +126,7 @@ impl SanitizedTransaction { } /// Return the list of accounts that must be locked during processing this transaction. - pub fn get_account_locks( - &self, - demote_program_write_lock_features: bool, - ) -> 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 @@ -142,7 +139,7 @@ impl SanitizedTransaction { }; for (i, key) in message.account_keys_iter().enumerate() { - if message.is_writable(i, demote_program_write_lock_features) { + 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 38e98ab0d22344..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_program_write_lock_features=*/ true), + writable: message.is_writable(i, /*demote_program_write_locks=*/ true), signer: message.is_signer(i), }); }