Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Commit

Permalink
Fix - LoadedProgramType::Closed (#31922)
Browse files Browse the repository at this point in the history 
* Makes Bank::load_program() return correct tombstones.

* Removes early TX failure caused by closed and invalid programs.

* Adjusts the feature gate of simplify_writable_program_account_check.

(cherry picked from commit 3f13cd3)

# Conflicts:
#	runtime/src/accounts.rs
  • Loading branch information
@Lichtso @mergify
Lichtso authored and mergify[bot] committed Jun 8, 2023
1 parent bf789b6 commit 3ec2ffa
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 96 deletions.
20 changes: 2 additions & 18 deletions ledger-tool/src/program.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ use {
},
solana_program_runtime::{
invoke_context::InvokeContext,
loaded_programs::{
LoadProgramMetrics, LoadedProgram, LoadedProgramType, DELAY_VISIBILITY_SLOT_OFFSET,
},
loaded_programs::{LoadProgramMetrics, LoadedProgramType, DELAY_VISIBILITY_SLOT_OFFSET},
with_mock_invoke_context,
},
solana_rbpf::{
Expand Down Expand Up @@ -540,22 +538,8 @@ pub fn program(ledger_path: &Path, matches: &ArgMatches<'_>) {
let mut loaded_programs =
LoadedProgramsForTxBatch::new(bank.slot() + DELAY_VISIBILITY_SLOT_OFFSET);
for key in cached_account_keys {
let program = bank.load_program(&key).unwrap_or_else(|err| {
// Create a tombstone for the program in the cache
debug!("Failed to load program {}, error {:?}", key, err);
Arc::new(LoadedProgram::new_tombstone(
0,
LoadedProgramType::FailedVerification(
bank.loaded_programs_cache
.read()
.unwrap()
.program_runtime_environment_v1
.clone(),
),
))
});
loaded_programs.replenish(key, bank.load_program(&key));
debug!("Loaded program {}", key);
loaded_programs.replenish(key, program);
}
invoke_context.programs_loaded_for_tx_batch = &loaded_programs;

Expand Down
106 changes: 63 additions & 43 deletions runtime/src/accounts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,15 @@ use {
solana_address_lookup_table_program::{error::AddressLookupError, state::AddressLookupTable},
solana_program_runtime::{
compute_budget::{self, ComputeBudget},
loaded_programs::{LoadedProgram, LoadedProgramType, LoadedProgramsForTxBatch},
loaded_programs::LoadedProgramsForTxBatch,
},
solana_sdk::{
account::{Account, AccountSharedData, ReadableAccount, WritableAccount},
account_utils::StateMut,
bpf_loader_upgradeable,
bpf_loader_upgradeable::{self, UpgradeableLoaderState},
clock::{BankId, Slot},
feature_set::{
self, add_set_tx_loaded_accounts_data_size_instruction,
delay_visibility_of_program_deployment, enable_request_heap_frame_ix,
self, add_set_tx_loaded_accounts_data_size_instruction, enable_request_heap_frame_ix,
include_loaded_accounts_data_size_in_fee_calculation,
remove_congestion_multiplier_from_fee_calculation, remove_deprecated_request_unit_ix,
simplify_writable_program_account_check, use_default_units_in_fee_calculation,
Expand Down Expand Up @@ -292,27 +291,8 @@ impl Accounts {

fn account_shared_data_from_program(
key: &Pubkey,
feature_set: &FeatureSet,
program: &LoadedProgram,
program_accounts: &HashMap<Pubkey, (&Pubkey, u64)>,
) -> Result<AccountSharedData> {
// Check for tombstone
let result = match &program.program {
LoadedProgramType::FailedVerification(_) | LoadedProgramType::Closed => {
Err(TransactionError::InvalidProgramForExecution)
}
LoadedProgramType::DelayVisibility => {
debug_assert!(feature_set.is_active(&delay_visibility_of_program_deployment::id()));
Err(TransactionError::InvalidProgramForExecution)
}
_ => Ok(()),
};
if feature_set.is_active(&simplify_writable_program_account_check::id()) {
// Currently CPI only fails if an execution is actually attempted. With this check it
// would also fail if a transaction just references an invalid program. So the checking
// of the result is being feature gated.
result?;
}
// It's an executable program account. The program is already loaded in the cache.
// So the account data is not needed. Return a dummy AccountSharedData with meta
// information.
Expand Down Expand Up @@ -384,23 +364,21 @@ impl Accounts {
account_overrides.and_then(|overrides| overrides.get(key))
{
(account_override.data().len(), account_override.clone(), 0)
} else if let Some(program) = (!instruction_account && !message.is_writable(i))
.then_some(())
.and_then(|_| loaded_programs.find(key))
} else if let Some(program) = (feature_set
.is_active(&simplify_writable_program_account_check::id())
&& !instruction_account
&& !message.is_writable(i))
.then_some(())
.and_then(|_| loaded_programs.find(key))
{
// This condition block does special handling for accounts that are passed
// as instruction account to any of the instructions in the transaction.
// It's been noticed that some programs are reading other program accounts
// (that are passed to the program as instruction accounts). So such accounts
// are needed to be loaded even though corresponding compiled program may
// already be present in the cache.
Self::account_shared_data_from_program(
key,
feature_set,
program.as_ref(),
program_accounts,
)
.map(|program_account| (program.account_size, program_account, 0))?
Self::account_shared_data_from_program(key, program_accounts)
.map(|program_account| (program.account_size, program_account, 0))?
} else {
self.accounts_db
.load_with_fixed_root(ancestors, key)
Expand Down Expand Up @@ -456,17 +434,52 @@ impl Accounts {
validated_fee_payer = true;
}

if bpf_loader_upgradeable::check_id(account.owner()) {
if !feature_set.is_active(&simplify_writable_program_account_check::id())
&& message.is_writable(i)
&& !message.is_upgradeable_loader_present()
{
if !feature_set.is_active(&simplify_writable_program_account_check::id()) {
if bpf_loader_upgradeable::check_id(account.owner()) {
if message.is_writable(i) && !message.is_upgradeable_loader_present() {
error_counters.invalid_writable_account += 1;
return Err(TransactionError::InvalidWritableAccount);
}

if account.executable() {
// The upgradeable loader requires the derived ProgramData account
if let Ok(UpgradeableLoaderState::Program {
programdata_address,
}) = account.state()
{
if self
.accounts_db
.load_with_fixed_root(ancestors, &programdata_address)
.is_none()
{
error_counters.account_not_found += 1;
return Err(TransactionError::ProgramAccountNotFound);
}
} else {
error_counters.invalid_program_for_execution += 1;
return Err(TransactionError::InvalidProgramForExecution);
}
}
} else if account.executable() && message.is_writable(i) {
error_counters.invalid_writable_account += 1;
return Err(TransactionError::InvalidWritableAccount);
}
<<<<<<< HEAD
} else if account.executable() && message.is_writable(i) {
error_counters.invalid_writable_account += 1;
return Err(TransactionError::InvalidWritableAccount);
=======
}

if in_reward_interval
&& message.is_writable(i)
&& solana_stake_program::check_id(account.owner())
{
error_counters.program_execution_temporarily_restricted += 1;
return Err(TransactionError::ProgramExecutionTemporarilyRestricted {
account_index: i as u8,
});
>>>>>>> 3f13cd353 (Fix - LoadedProgramType::Closed (#31922))
}

tx_rent += rent;
Expand Down Expand Up @@ -1478,7 +1491,6 @@ mod tests {
},
solana_sdk::{
account::{AccountSharedData, WritableAccount},
bpf_loader_upgradeable::UpgradeableLoaderState,
compute_budget::ComputeBudgetInstruction,
epoch_schedule::EpochSchedule,
genesis_config::ClusterType,
Expand Down Expand Up @@ -2477,8 +2489,12 @@ mod tests {
instructions,
);
let tx = Transaction::new(&[&keypair], message.clone(), Hash::default());
let loaded_accounts =
load_accounts_with_excluded_features(tx, &accounts, &mut error_counters, None);
let loaded_accounts = load_accounts_with_excluded_features(
tx,
&accounts,
&mut error_counters,
Some(&[simplify_writable_program_account_check::id()]),
);

assert_eq!(error_counters.invalid_writable_account, 1);
assert_eq!(loaded_accounts.len(), 1);
Expand All @@ -2491,8 +2507,12 @@ mod tests {
message.account_keys = vec![key0, key1, key2]; // revert key change
message.header.num_readonly_unsigned_accounts = 2; // mark both executables as readonly
let tx = Transaction::new(&[&keypair], message, Hash::default());
let loaded_accounts =
load_accounts_with_excluded_features(tx, &accounts, &mut error_counters, None);
let loaded_accounts = load_accounts_with_excluded_features(
tx,
&accounts,
&mut error_counters,
Some(&[simplify_writable_program_account_check::id()]),
);

assert_eq!(error_counters.invalid_writable_account, 1);
assert_eq!(loaded_accounts.len(), 1);
Expand Down
55 changes: 26 additions & 29 deletions runtime/src/bank.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4128,11 +4128,14 @@ impl Bank {
}
}

pub fn load_program(&self, pubkey: &Pubkey) -> Result<Arc<LoadedProgram>> {
pub fn load_program(&self, pubkey: &Pubkey) -> Arc<LoadedProgram> {
let program = if let Some(program) = self.get_account_with_fixed_root(pubkey) {
program
} else {
return Err(TransactionError::ProgramAccountNotFound);
return Arc::new(LoadedProgram::new_tombstone(
self.slot,
LoadedProgramType::Closed,
));
};
let mut transaction_accounts = vec![(*pubkey, program)];
let is_upgradeable_loader =
Expand All @@ -4147,10 +4150,16 @@ impl Bank {
{
transaction_accounts.push((programdata_address, programdata_account));
} else {
return Err(TransactionError::ProgramAccountNotFound);
return Arc::new(LoadedProgram::new_tombstone(
self.slot,
LoadedProgramType::Closed,
));
}
} else {
return Err(TransactionError::ProgramAccountNotFound);
return Arc::new(LoadedProgram::new_tombstone(
self.slot,
LoadedProgramType::Closed,
));
}
}
let mut transaction_context = TransactionContext::new(
Expand All @@ -4159,24 +4168,20 @@ impl Bank {
1,
1,
);
let instruction_context = transaction_context
.get_next_instruction_context()
.map_err(|err| TransactionError::InstructionError(0, err))?;
let instruction_context = transaction_context.get_next_instruction_context().unwrap();
instruction_context.configure(if is_upgradeable_loader { &[0, 1] } else { &[0] }, &[], &[]);
transaction_context
.push()
.map_err(|err| TransactionError::InstructionError(0, err))?;
transaction_context.push().unwrap();
let instruction_context = transaction_context
.get_current_instruction_context()
.map_err(|err| TransactionError::InstructionError(0, err))?;
.unwrap();
let program = instruction_context
.try_borrow_program_account(&transaction_context, 0)
.map_err(|err| TransactionError::InstructionError(0, err))?;
.unwrap();
let programdata = if is_upgradeable_loader {
Some(
instruction_context
.try_borrow_program_account(&transaction_context, 1)
.map_err(|err| TransactionError::InstructionError(0, err))?,
.unwrap(),
)
} else {
None
Expand All @@ -4192,14 +4197,19 @@ impl Bank {
None, // log_collector
&program,
programdata.as_ref().unwrap_or(&program),
program_runtime_environment_v1,
program_runtime_environment_v1.clone(),
)
.map(|(loaded_program, metrics)| {
let mut timings = ExecuteDetailsTimings::default();
metrics.submit_datapoint(&mut timings);
loaded_program
})
.map_err(|err| TransactionError::InstructionError(0, err))
.unwrap_or_else(|_| {
Arc::new(LoadedProgram::new_tombstone(
self.slot,
LoadedProgramType::FailedVerification(program_runtime_environment_v1),
))
})
}

pub fn clear_program_cache(&self) {
Expand Down Expand Up @@ -4445,20 +4455,7 @@ impl Bank {
let missing_programs: Vec<(Pubkey, Arc<LoadedProgram>)> = missing_programs
.iter()
.map(|(key, count)| {
let program = self.load_program(key).unwrap_or_else(|err| {
// Create a tombstone for the program in the cache
debug!("Failed to load program {}, error {:?}", key, err);
Arc::new(LoadedProgram::new_tombstone(
self.slot,
LoadedProgramType::FailedVerification(
self.loaded_programs_cache
.read()
.unwrap()
.program_runtime_environment_v1
.clone(),
),
))
});
let program = self.load_program(key);
program.tx_usage_counter.store(*count, Ordering::Relaxed);
(*key, program)
})
Expand Down
9 changes: 3 additions & 6 deletions runtime/src/bank/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7397,8 +7397,6 @@ fn test_bank_load_program() {
bank.store_account_and_update_capitalization(&key1, &program_account);
bank.store_account_and_update_capitalization(&programdata_key, &programdata_account);
let program = bank.load_program(&key1);
assert!(program.is_ok());
let program = program.unwrap();
assert!(matches!(program.program, LoadedProgramType::LegacyV1(_)));
assert_eq!(
program.account_size,
Expand All @@ -7412,7 +7410,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
let mut bank = Bank::new_for_tests(&genesis_config);
bank.feature_set = Arc::new(FeatureSet::all_enabled());
let bank = Arc::new(bank);
let bank_client = BankClient::new_shared(&bank);
let mut bank_client = BankClient::new_shared(&bank);

// Setup keypairs and addresses
let payer_keypair = Keypair::new();
Expand Down Expand Up @@ -7553,9 +7551,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
assert_eq!(*elf.get(i).unwrap(), *byte);
}

let loaded_program = bank
.load_program(&program_keypair.pubkey())
.expect("Failed to load the program");
let loaded_program = bank.load_program(&program_keypair.pubkey());

// Invoke deployed program
mock_process_instruction(
Expand Down Expand Up @@ -7583,6 +7579,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
// Test initialized program account
bank.clear_signatures();
bank.store_account(&buffer_address, &buffer_account);
let bank = bank_client.advance_slot(1, &mint_keypair.pubkey()).unwrap();
let message = Message::new(
&[Instruction::new_with_bincode(
bpf_loader_upgradeable::id(),
Expand Down

0 comments on commit 3ec2ffa

Please sign in to comment.