From 369e13b111e05a7d4c653c31fee9f1282ca6a8e5 Mon Sep 17 00:00:00 2001 From: Jack May Date: Wed, 10 Mar 2021 09:48:41 -0800 Subject: [PATCH] cleanup old runtime features (#15787) --- programs/bpf_loader/src/lib.rs | 78 ++++++++++------------------ programs/bpf_loader/src/syscalls.rs | 57 ++++++-------------- runtime/benches/message_processor.rs | 8 ++- runtime/src/message_processor.rs | 46 +++++----------- sdk/src/feature_set.rs | 20 ------- 5 files changed, 60 insertions(+), 149 deletions(-) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index c7c0e9aed..870adb4aa 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -30,7 +30,6 @@ use solana_sdk::{ bpf_loader_upgradeable::{self, UpgradeableLoaderState}, clock::Clock, entrypoint::SUCCESS, - feature_set::matching_buffer_upgrade_authorities, ic_logger_msg, ic_msg, instruction::InstructionError, keyed_account::{from_keyed_account, next_keyed_account, KeyedAccount}, @@ -303,27 +302,18 @@ fn process_loader_upgradeable_instruction( return Err(InstructionError::AccountAlreadyInitialized); } - if invoke_context.is_feature_active(&matching_buffer_upgrade_authorities::id()) { - let authority = next_keyed_account(account_iter)?; + let authority = next_keyed_account(account_iter)?; - buffer.set_state(&UpgradeableLoaderState::Buffer { - authority_address: Some(*authority.unsigned_key()), - })?; - } else { - let authority = next_keyed_account(account_iter) - .ok() - .map(|account| account.unsigned_key()); - buffer.set_state(&UpgradeableLoaderState::Buffer { - authority_address: authority.cloned(), - })?; - } + buffer.set_state(&UpgradeableLoaderState::Buffer { + authority_address: Some(*authority.unsigned_key()), + })?; } UpgradeableLoaderInstruction::Write { offset, bytes } => { let buffer = next_keyed_account(account_iter)?; let authority = next_keyed_account(account_iter)?; if let UpgradeableLoaderState::Buffer { authority_address } = buffer.state()? { - if authority_address == None { + if authority_address.is_none() { ic_logger_msg!(logger, "Buffer is immutable"); return Err(InstructionError::Immutable); // TODO better error code } @@ -354,19 +344,9 @@ fn process_loader_upgradeable_instruction( let rent = from_keyed_account::(next_keyed_account(account_iter)?)?; let clock = from_keyed_account::(next_keyed_account(account_iter)?)?; let system = next_keyed_account(account_iter)?; - let (upgrade_authority_address, upgrade_authority_signer) = - if invoke_context.is_feature_active(&matching_buffer_upgrade_authorities::id()) { - let authority = next_keyed_account(account_iter)?; - ( - Some(*authority.unsigned_key()), - authority.signer_key().is_none(), - ) - } else { - let authority = next_keyed_account(account_iter) - .ok() - .map(|account| account.unsigned_key()); - (authority.cloned(), false) - }; + let authority = next_keyed_account(account_iter)?; + let upgrade_authority_address = Some(*authority.unsigned_key()); + let upgrade_authority_signer = authority.signer_key().is_none(); // Verify Program account @@ -386,15 +366,13 @@ fn process_loader_upgradeable_instruction( // Verify Buffer account if let UpgradeableLoaderState::Buffer { authority_address } = buffer.state()? { - if invoke_context.is_feature_active(&matching_buffer_upgrade_authorities::id()) { - if authority_address != upgrade_authority_address { - ic_logger_msg!(logger, "Buffer and upgrade authority don't match"); - return Err(InstructionError::IncorrectAuthority); - } - if upgrade_authority_signer { - ic_logger_msg!(logger, "Upgrade authority did not sign"); - return Err(InstructionError::MissingRequiredSignature); - } + if authority_address != upgrade_authority_address { + ic_logger_msg!(logger, "Buffer and upgrade authority don't match"); + return Err(InstructionError::IncorrectAuthority); + } + if upgrade_authority_signer { + ic_logger_msg!(logger, "Upgrade authority did not sign"); + return Err(InstructionError::MissingRequiredSignature); } } else { ic_logger_msg!(logger, "Invalid Buffer account"); @@ -511,15 +489,13 @@ fn process_loader_upgradeable_instruction( // Verify Buffer account if let UpgradeableLoaderState::Buffer { authority_address } = buffer.state()? { - if invoke_context.is_feature_active(&matching_buffer_upgrade_authorities::id()) { - if authority_address != Some(*authority.unsigned_key()) { - ic_logger_msg!(logger, "Buffer and upgrade authority don't match"); - return Err(InstructionError::IncorrectAuthority); - } - if authority.signer_key().is_none() { - ic_logger_msg!(logger, "Upgrade authority did not sign"); - return Err(InstructionError::MissingRequiredSignature); - } + if authority_address != Some(*authority.unsigned_key()) { + ic_logger_msg!(logger, "Buffer and upgrade authority don't match"); + return Err(InstructionError::IncorrectAuthority); + } + if authority.signer_key().is_none() { + ic_logger_msg!(logger, "Upgrade authority did not sign"); + return Err(InstructionError::MissingRequiredSignature); } } else { ic_logger_msg!(logger, "Invalid Buffer account"); @@ -553,7 +529,7 @@ fn process_loader_upgradeable_instruction( upgrade_authority_address, } = programdata.state()? { - if upgrade_authority_address == None { + if upgrade_authority_address.is_none() { ic_logger_msg!(logger, "Program not upgradeable"); return Err(InstructionError::Immutable); } @@ -614,13 +590,11 @@ fn process_loader_upgradeable_instruction( match account.state()? { UpgradeableLoaderState::Buffer { authority_address } => { - if invoke_context.is_feature_active(&matching_buffer_upgrade_authorities::id()) - && new_authority == None - { + if new_authority.is_none() { ic_logger_msg!(logger, "Buffer authority is not optional"); return Err(InstructionError::IncorrectAuthority); } - if authority_address == None { + if authority_address.is_none() { ic_logger_msg!(logger, "Buffer is immutable"); return Err(InstructionError::Immutable); } @@ -640,7 +614,7 @@ fn process_loader_upgradeable_instruction( slot, upgrade_authority_address, } => { - if upgrade_authority_address == None { + if upgrade_authority_address.is_none() { ic_logger_msg!(logger, "Program not upgradeable"); return Err(InstructionError::Immutable); } diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index 417774cbe..f4a87849a 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -16,7 +16,7 @@ use solana_sdk::{ bpf_loader, bpf_loader_deprecated, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, entrypoint::{MAX_PERMITTED_DATA_INCREASE, SUCCESS}, - feature_set::{per_byte_logging_cost, ristretto_mul_syscall_enabled, use_loaded_executables}, + feature_set::ristretto_mul_syscall_enabled, hash::{Hasher, HASH_BYTES}, ic_msg, instruction::{AccountMeta, Instruction, InstructionError}, @@ -159,19 +159,13 @@ pub fn bind_syscall_context_objects<'a>( vm.bind_syscall_context_object(Box::new(SyscallAbort {}), None)?; vm.bind_syscall_context_object( Box::new(SyscallPanic { - compute_meter: if invoke_context.is_feature_active(&per_byte_logging_cost::id()) { - Some(invoke_context.get_compute_meter()) - } else { - None - }, + compute_meter: invoke_context.get_compute_meter(), loader_id, }), None, )?; vm.bind_syscall_context_object( Box::new(SyscallLog { - per_byte_cost: invoke_context.is_feature_active(&per_byte_logging_cost::id()), - cost: bpf_compute_budget.log_units, compute_meter: invoke_context.get_compute_meter(), logger: invoke_context.get_logger(), loader_id, @@ -413,7 +407,7 @@ impl SyscallObject for SyscallAbort { /// Causes the BPF program to be halted immediately /// Log a user's info message pub struct SyscallPanic<'a> { - compute_meter: Option>>, + compute_meter: Rc>, loader_id: &'a Pubkey, } impl<'a> SyscallObject for SyscallPanic<'a> { @@ -427,9 +421,7 @@ impl<'a> SyscallObject for SyscallPanic<'a> { memory_mapping: &MemoryMapping, result: &mut Result>, ) { - if let Some(ref mut compute_meter) = self.compute_meter { - question_mark!(compute_meter.consume(len), result); - } + question_mark!(self.compute_meter.consume(len), result); *result = translate_string_and_do( memory_mapping, file, @@ -442,8 +434,6 @@ impl<'a> SyscallObject for SyscallPanic<'a> { /// Log a user's info message pub struct SyscallLog<'a> { - per_byte_cost: bool, - cost: u64, compute_meter: Rc>, logger: Rc>, loader_id: &'a Pubkey, @@ -459,11 +449,7 @@ impl<'a> SyscallObject for SyscallLog<'a> { memory_mapping: &MemoryMapping, result: &mut Result>, ) { - if self.per_byte_cost { - question_mark!(self.compute_meter.consume(len), result); - } else { - question_mark!(self.compute_meter.consume(self.cost), result); - } + question_mark!(self.compute_meter.consume(len), result); question_mark!( translate_string_and_do( memory_mapping, @@ -1430,10 +1416,7 @@ where SyscallError::InstructionError(InstructionError::MissingAccount) })?; - if i == program_account_index - || (invoke_context.is_feature_active(&use_loaded_executables::id()) - && account.borrow().executable) - { + if i == program_account_index || account.borrow().executable { // Use the known executable accounts.push(Rc::new(account)); refs.push(None); @@ -1616,7 +1599,7 @@ fn call<'a>( // Construct executables let program_account = (**accounts.get(callee_program_id_index).ok_or_else(|| { - ic_msg!(invoke_context, "Unknown program {}", callee_program_id,); + ic_msg!(invoke_context, "Unknown program {}", callee_program_id); SyscallError::InstructionError(InstructionError::MissingAccount) })?) .clone(); @@ -1768,11 +1751,11 @@ mod tests { for (ok, start, length, value) in cases { if ok { assert_eq!( - translate(&memory_mapping, AccessType::Load, start, length,).unwrap(), + translate(&memory_mapping, AccessType::Load, start, length).unwrap(), value ) } else { - assert!(translate(&memory_mapping, AccessType::Load, start, length,).is_err()) + assert!(translate(&memory_mapping, AccessType::Load, start, length).is_err()) } } } @@ -1897,7 +1880,7 @@ mod tests { assert_eq!(data, translated_data); data[0] = 10; assert_eq!(data, translated_data); - assert!(translate_slice::(&memory_mapping, 96, u64::MAX, &bpf_loader::id(),).is_err()); + assert!(translate_slice::(&memory_mapping, 96, u64::MAX, &bpf_loader::id()).is_err()); // Pubkeys let mut data = vec![solana_sdk::pubkey::new_rand(); 5]; @@ -1989,7 +1972,7 @@ mod tests { remaining: string.len() as u64 - 1, })); let mut syscall_panic = SyscallPanic { - compute_meter: Some(compute_meter), + compute_meter, loader_id: &bpf_loader::id(), }; let mut result: Result> = Ok(0); @@ -2009,8 +1992,12 @@ mod tests { result ); + let compute_meter: Rc> = + Rc::new(RefCell::new(MockComputeMeter { + remaining: string.len() as u64, + })); let mut syscall_panic = SyscallPanic { - compute_meter: None, + compute_meter, loader_id: &bpf_loader::id(), }; let mut result: Result> = Ok(0); @@ -2032,13 +2019,11 @@ mod tests { let addr = string.as_ptr() as *const _ as u64; let compute_meter: Rc> = - Rc::new(RefCell::new(MockComputeMeter { remaining: 3 })); + Rc::new(RefCell::new(MockComputeMeter { remaining: 1000000 })); let log = Rc::new(RefCell::new(vec![])); let logger: Rc> = Rc::new(RefCell::new(MockLogger { log: log.clone() })); let mut syscall_sol_log = SyscallLog { - per_byte_cost: false, - cost: 1, compute_meter, logger, loader_id: &bpf_loader::id(), @@ -2100,12 +2085,6 @@ mod tests { &memory_mapping, &mut result, ); - assert_eq!( - Err(EbpfError::UserError(BpfError::SyscallError( - SyscallError::InstructionError(InstructionError::ComputationalBudgetExceeded) - ))), - result - ); let compute_meter: Rc> = Rc::new(RefCell::new(MockComputeMeter { @@ -2113,8 +2092,6 @@ mod tests { })); let logger: Rc> = Rc::new(RefCell::new(MockLogger { log })); let mut syscall_sol_log = SyscallLog { - per_byte_cost: true, - cost: 1, compute_meter, logger, loader_id: &bpf_loader::id(), diff --git a/runtime/benches/message_processor.rs b/runtime/benches/message_processor.rs index 3933cc65c..11fb05697 100644 --- a/runtime/benches/message_processor.rs +++ b/runtime/benches/message_processor.rs @@ -16,13 +16,12 @@ fn bench_verify_account_changes_data(bencher: &mut Bencher) { let pre = PreAccount::new( &pubkey::new_rand(), &AccountSharedData::new(0, BUFSIZE, &owner), - false, ); let post = AccountSharedData::new(0, BUFSIZE, &owner); assert_eq!( pre.verify( &owner, - Some(false), + false, &Rent::default(), &post, &mut ExecuteDetailsTimings::default() @@ -34,7 +33,7 @@ fn bench_verify_account_changes_data(bencher: &mut Bencher) { bencher.iter(|| { pre.verify( &owner, - Some(false), + false, &Rent::default(), &post, &mut ExecuteDetailsTimings::default(), @@ -53,12 +52,11 @@ fn bench_verify_account_changes_data(bencher: &mut Bencher) { let pre = PreAccount::new( &pubkey::new_rand(), &AccountSharedData::new(0, BUFSIZE, &owner), - false, ); bencher.iter(|| { pre.verify( &non_owner, - Some(false), + false, &Rent::default(), &post, &mut ExecuteDetailsTimings::default(), diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 315fa601d..b0cdd91fc 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -8,7 +8,7 @@ use solana_sdk::{ account::{AccountSharedData, ReadableAccount}, account_utils::StateMut, bpf_loader_upgradeable::{self, UpgradeableLoaderState}, - feature_set::{instructions_sysvar_enabled, track_writable_deescalation, FeatureSet}, + feature_set::{instructions_sysvar_enabled, FeatureSet}, ic_msg, instruction::{CompiledInstruction, Instruction, InstructionError}, keyed_account::{create_keyed_readonly_accounts, KeyedAccount}, @@ -82,15 +82,13 @@ impl ExecuteDetailsTimings { #[derive(Clone, Debug, Default)] pub struct PreAccount { key: Pubkey, - is_writable: bool, account: RefCell, changed: bool, } impl PreAccount { - pub fn new(key: &Pubkey, account: &AccountSharedData, is_writable: bool) -> Self { + pub fn new(key: &Pubkey, account: &AccountSharedData) -> Self { Self { key: *key, - is_writable, account: RefCell::new(account.clone()), changed: false, } @@ -99,19 +97,13 @@ impl PreAccount { pub fn verify( &self, program_id: &Pubkey, - is_writable: Option, + is_writable: bool, rent: &Rent, post: &AccountSharedData, timings: &mut ExecuteDetailsTimings, ) -> Result<(), InstructionError> { let pre = self.account.borrow(); - let is_writable = if let Some(is_writable) = is_writable { - is_writable - } else { - self.is_writable - }; - // Only the owner of the account may change owner and // only if the account is writable and // only if the account is not executable and @@ -331,8 +323,6 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { accounts: &[Rc>], caller_privileges: Option<&[bool]>, ) -> Result<(), InstructionError> { - let track_writable_deescalation = - self.is_feature_active(&track_writable_deescalation::id()); match self.program_ids.last() { Some(program_id) => MessageProcessor::verify_and_update( message, @@ -341,7 +331,6 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { accounts, program_id, &self.rent, - track_writable_deescalation, caller_privileges, &mut self.timings, ), @@ -870,9 +859,8 @@ impl MessageProcessor { { let mut work = |_unique_index: usize, account_index: usize| { let key = &message.account_keys[account_index]; - let is_writable = message.is_writable(account_index); let account = accounts[account_index].borrow(); - pre_accounts.push(PreAccount::new(key, &account, is_writable)); + pre_accounts.push(PreAccount::new(key, &account)); Ok(()) }; let _ = instruction.visit_each_account(&mut work); @@ -916,7 +904,7 @@ impl MessageProcessor { .map_err(|_| InstructionError::AccountBorrowOutstanding)?; pre_accounts[unique_index].verify( &program_id, - Some(message.is_writable(account_index)), + message.is_writable(account_index), rent, &account, timings, @@ -943,7 +931,6 @@ impl MessageProcessor { accounts: &[Rc>], program_id: &Pubkey, rent: &Rent, - track_writable_deescalation: bool, caller_privileges: Option<&[bool]>, timings: &mut ExecuteDetailsTimings, ) -> Result<(), InstructionError> { @@ -953,14 +940,10 @@ impl MessageProcessor { if account_index < message.account_keys.len() && account_index < accounts.len() { let key = &message.account_keys[account_index]; let account = &accounts[account_index]; - let is_writable = if track_writable_deescalation { - Some(if let Some(caller_privileges) = caller_privileges { - caller_privileges[account_index] - } else { - message.is_writable(account_index) - }) + let is_writable = if let Some(caller_privileges) = caller_privileges { + caller_privileges[account_index] } else { - None + message.is_writable(account_index) }; // Find the matching PreAccount for pre_account in pre_accounts.iter_mut() { @@ -1132,11 +1115,11 @@ mod tests { 1, &program_ids[i], )))); - pre_accounts.push(PreAccount::new(&keys[i], &accounts[i].borrow(), false)) + pre_accounts.push(PreAccount::new(&keys[i], &accounts[i].borrow())) } let account = AccountSharedData::new(1, 1, &solana_sdk::pubkey::Pubkey::default()); for program_id in program_ids.iter() { - pre_accounts.push(PreAccount::new(program_id, &account.clone(), false)); + pre_accounts.push(PreAccount::new(program_id, &account.clone())); } let mut invoke_context = ThisInvokeContext::new( @@ -1282,7 +1265,6 @@ mod tests { data: vec![], ..AccountSharedData::default() }, - false, ), post: AccountSharedData { owner: *owner, @@ -1322,7 +1304,7 @@ mod tests { pub fn verify(&self) -> Result<(), InstructionError> { self.pre.verify( &self.program_id, - Some(self.is_writable), + self.is_writable, &self.rent, &self.post, &mut ExecuteDetailsTimings::default(), @@ -2004,16 +1986,16 @@ mod tests { let mut program_account = AccountSharedData::new(1, 0, &native_loader::id()); program_account.executable = true; - let executable_preaccount = PreAccount::new(&callee_program_id, &program_account, true); + let executable_preaccount = PreAccount::new(&callee_program_id, &program_account); let executable_accounts = vec![(callee_program_id, RefCell::new(program_account.clone()))]; let owned_key = solana_sdk::pubkey::new_rand(); let owned_account = AccountSharedData::new(42, 1, &callee_program_id); - let owned_preaccount = PreAccount::new(&owned_key, &owned_account, true); + let owned_preaccount = PreAccount::new(&owned_key, &owned_account); let not_owned_key = solana_sdk::pubkey::new_rand(); let not_owned_account = AccountSharedData::new(84, 1, &solana_sdk::pubkey::new_rand()); - let not_owned_preaccount = PreAccount::new(¬_owned_key, ¬_owned_account, true); + let not_owned_preaccount = PreAccount::new(¬_owned_key, ¬_owned_account); #[allow(unused_mut)] let mut accounts = vec![ diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index b22313a61..4ba16a863 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -83,18 +83,10 @@ pub mod stake_program_v3 { solana_sdk::declare_id!("Ego6nTu7WsBcZBvVqJQKp6Yku2N3mrfG8oYCfaLZkAeK"); } -pub mod use_loaded_executables { - solana_sdk::declare_id!("3Jq7mE2chDpf6oeEDsuGK7orTYEgyQjCPvaRppTNdVGK"); -} - pub mod turbine_retransmit_peers_patch { solana_sdk::declare_id!("5Lu3JnWSFwRYpXzwDMkanWSk6XqSuF2i5fpnVhzB5CTc"); } -pub mod track_writable_deescalation { - solana_sdk::declare_id!("HVPSxqskEtRLRT2ZeEMmkmt9FWqoFX4vrN6f5VaadLED"); -} - pub mod require_custodian_for_locked_stake_authorize { solana_sdk::declare_id!("D4jsDcXaqdW8tDAWn8H4R25Cdns2YwLneujSL1zvjW6R"); } @@ -103,18 +95,10 @@ pub mod spl_token_v2_self_transfer_fix { solana_sdk::declare_id!("BL99GYhdjjcv6ys22C9wPgn2aTVERDbPHHo4NbS3hgp7"); } -pub mod matching_buffer_upgrade_authorities { - solana_sdk::declare_id!("B5PSjDEJvKJEUQSL7q94N7XCEoWJCYum8XfUg7yuugUU"); -} - pub mod warp_timestamp_again { solana_sdk::declare_id!("GvDsGDkH5gyzwpDhxNixx8vtx1kwYHH13RiNAPw27zXb"); } -pub mod per_byte_logging_cost { - solana_sdk::declare_id!("59dM4SV6dPEKXPfkrkhFkRdn4K6xwKxdNAPMyXG7J1wT"); -} - pub mod check_init_vote_data { solana_sdk::declare_id!("3ccR6QpxGYsAbWyfevEtBNGfWV4xBffxRj2tD6A9i39F"); } @@ -142,16 +126,12 @@ lazy_static! { (simple_capitalization::id(), "simple capitalization"), (bpf_loader_upgradeable_program::id(), "upgradeable bpf loader"), (stake_program_v3::id(), "solana_stake_program v3"), - (use_loaded_executables::id(), "use loaded executable accounts"), (turbine_retransmit_peers_patch::id(), "turbine retransmit peers patch #14631"), - (track_writable_deescalation::id(), "track account writable deescalation"), (require_custodian_for_locked_stake_authorize::id(), "require custodian to authorize withdrawer change for locked stake"), (spl_token_v2_self_transfer_fix::id(), "spl-token self-transfer fix"), - (matching_buffer_upgrade_authorities::id(), "Upgradeable buffer and program authorities must match"), (full_inflation::mainnet::certusone::enable::id(), "Full inflation enabled by Certus One"), (full_inflation::mainnet::certusone::vote::id(), "Community vote allowing Certus One to enable full inflation"), (warp_timestamp_again::id(), "warp timestamp again, adjust bounding to 25% fast 80% slow #15204"), - (per_byte_logging_cost::id(), "charge the compute budget per byte for logging"), (check_init_vote_data::id(), "check initialized Vote data"), (check_program_owner::id(), "limit programs to operating on accounts owned by itself") /*************** ADD NEW FEATURES HERE ***************/