Skip to content

Commit 55c05c5

Browse files
LichtsoLichtso
authored
Fix - FailedVerification and Closed tombstones (solana-labs#419)
* Only the verifier can cause FailedVerification, everything else is Closed * Removes the environments parameter from load_program_accounts(). * cargo fmt * Simplify invocation of deployed program * Attempt to invoke a program before it is deployed * Attempt to invoke a buffer before it is used in a deployment * Escalates Option return value of load_program_accounts() to load_program_with_pubkey(). * Review feedback
1 parent 562254e commit 55c05c5

File tree

5 files changed

+245
-213
lines changed

5 files changed

+245
-213
lines changed

‎ledger-tool/src/program.rs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -522,7 +522,11 @@ pub fn program(ledger_path: &Path, matches: &ArgMatches<'_>) {
522522
let mut loaded_programs =
523523
bank.new_program_cache_for_tx_batch_for_slot(bank.slot() + DELAY_VISIBILITY_SLOT_OFFSET);
524524
for key in cached_account_keys {
525-
loaded_programs.replenish(key, bank.load_program(&key, false, bank.epoch()));
525+
loaded_programs.replenish(
526+
key,
527+
bank.load_program(&key, false, bank.epoch())
528+
.expect("Couldn't find program account"),
529+
);
526530
debug!("Loaded program {}", key);
527531
}
528532
invoke_context.programs_loaded_for_tx_batch = &loaded_programs;

‎program-runtime/src/loaded_programs.rs‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -63,11 +63,11 @@ pub trait ForkGraph {
6363
/// Actual payload of [LoadedProgram].
6464
#[derive(Default)]
6565
pub enum LoadedProgramType {
66-
/// Tombstone for programs which did not pass the verifier.
67-
///
68-
/// These can potentially come back alive if the environment changes.
66+
/// Tombstone for programs which currently do not pass the verifier but could if the feature set changed.
6967
FailedVerification(ProgramRuntimeEnvironment),
70-
/// Tombstone for programs which were explicitly undeployed / closed.
68+
/// Tombstone for programs that were either explicitly closed or never deployed.
69+
///
70+
/// It's also used for accounts belonging to program loaders, that don't actually contain program code (e.g. buffer accounts for LoaderV3 programs).
7171
#[default]
7272
Closed,
7373
/// Tombstone for programs which have recently been modified but the new version is not visible yet.
@@ -776,12 +776,17 @@ impl<FG: ForkGraph> ProgramCache<FG> {
776776
Ok(index) => {
777777
let existing = slot_versions.get_mut(index).unwrap();
778778
match (&existing.program, &entry.program) {
779+
// Add test for Closed => Loaded transition in same slot
779780
(LoadedProgramType::Builtin(_), LoadedProgramType::Builtin(_))
781+
| (LoadedProgramType::Closed, LoadedProgramType::LegacyV0(_))
782+
| (LoadedProgramType::Closed, LoadedProgramType::LegacyV1(_))
783+
| (LoadedProgramType::Closed, LoadedProgramType::Typed(_))
780784
| (LoadedProgramType::Unloaded(_), LoadedProgramType::LegacyV0(_))
781785
| (LoadedProgramType::Unloaded(_), LoadedProgramType::LegacyV1(_))
782786
| (LoadedProgramType::Unloaded(_), LoadedProgramType::Typed(_)) => {}
783787
#[cfg(test)]
784-
(LoadedProgramType::Unloaded(_), LoadedProgramType::TestLoaded(_)) => {}
788+
(LoadedProgramType::Closed, LoadedProgramType::TestLoaded(_))
789+
| (LoadedProgramType::Unloaded(_), LoadedProgramType::TestLoaded(_)) => {}
785790
_ => {
786791
// Something is wrong, I can feel it ...
787792
error!("ProgramCache::assign_program() failed key={:?} existing={:?} entry={:?}", key, slot_versions, entry);
@@ -1680,7 +1685,6 @@ mod tests {
16801685
#[test_matrix(
16811686
(
16821687
LoadedProgramType::FailedVerification(Arc::new(BuiltinProgram::new_mock())),
1683-
LoadedProgramType::Closed,
16841688
LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock())),
16851689
),
16861690
(
@@ -1692,7 +1696,10 @@ mod tests {
16921696
)
16931697
)]
16941698
#[test_matrix(
1695-
(LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())),),
1699+
(
1700+
LoadedProgramType::Closed,
1701+
LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())),
1702+
),
16961703
(
16971704
LoadedProgramType::FailedVerification(Arc::new(BuiltinProgram::new_mock())),
16981705
LoadedProgramType::Closed,
@@ -1739,6 +1746,10 @@ mod tests {
17391746
);
17401747
}
17411748

1749+
#[test_case(
1750+
LoadedProgramType::Closed,
1751+
LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock()))
1752+
)]
17421753
#[test_case(
17431754
LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())),
17441755
LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock()))

‎runtime/src/bank.rs‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1253,16 +1253,19 @@ impl Bank {
12531253
{
12541254
let effective_epoch = program_cache.latest_root_epoch.saturating_add(1);
12551255
drop(program_cache);
1256-
let recompiled = new.load_program(&key, false, effective_epoch);
1257-
recompiled
1258-
.tx_usage_counter
1259-
.fetch_add(program_to_recompile.tx_usage_counter.load(Relaxed), Relaxed);
1260-
recompiled
1261-
.ix_usage_counter
1262-
.fetch_add(program_to_recompile.ix_usage_counter.load(Relaxed), Relaxed);
1263-
let mut program_cache =
1264-
new.transaction_processor.program_cache.write().unwrap();
1265-
program_cache.assign_program(key, recompiled);
1256+
if let Some(recompiled) = new.load_program(&key, false, effective_epoch) {
1257+
recompiled.tx_usage_counter.fetch_add(
1258+
program_to_recompile.tx_usage_counter.load(Relaxed),
1259+
Relaxed,
1260+
);
1261+
recompiled.ix_usage_counter.fetch_add(
1262+
program_to_recompile.ix_usage_counter.load(Relaxed),
1263+
Relaxed,
1264+
);
1265+
let mut program_cache =
1266+
new.transaction_processor.program_cache.write().unwrap();
1267+
program_cache.assign_program(key, recompiled);
1268+
}
12661269
}
12671270
} else if new.epoch() != program_cache.latest_root_epoch
12681271
|| slot_index.saturating_add(slots_in_recompilation_phase) >= slots_in_epoch
@@ -6886,7 +6889,7 @@ impl Bank {
68866889
pubkey: &Pubkey,
68876890
reload: bool,
68886891
effective_epoch: Epoch,
6889-
) -> Arc<LoadedProgram> {
6892+
) -> Option<Arc<LoadedProgram>> {
68906893
self.transaction_processor
68916894
.load_program_with_pubkey(self, pubkey, reload, effective_epoch)
68926895
}

‎runtime/src/bank/tests.rs‎

Lines changed: 52 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,7 @@ use {
3939
compute_budget::ComputeBudget,
4040
compute_budget_processor::{self, MAX_COMPUTE_UNIT_LIMIT},
4141
declare_process_instruction,
42-
invoke_context::mock_process_instruction,
43-
loaded_programs::{
44-
LoadedProgram, LoadedProgramType, LoadedProgramsForTxBatch,
45-
DELAY_VISIBILITY_SLOT_OFFSET,
46-
},
42+
loaded_programs::{LoadedProgram, LoadedProgramType, LoadedProgramsForTxBatch},
4743
prioritization_fee::{PrioritizationFeeDetails, PrioritizationFeeType},
4844
timings::ExecuteTimings,
4945
},
@@ -7141,7 +7137,7 @@ fn test_bank_load_program() {
71417137
programdata_account.set_rent_epoch(1);
71427138
bank.store_account_and_update_capitalization(&key1, &program_account);
71437139
bank.store_account_and_update_capitalization(&programdata_key, &programdata_account);
7144-
let program = bank.load_program(&key1, false, bank.epoch());
7140+
let program = bank.load_program(&key1, false, bank.epoch()).unwrap();
71457141
assert_matches!(program.program, LoadedProgramType::LegacyV1(_));
71467142
assert_eq!(
71477143
program.account_size,
@@ -7167,6 +7163,26 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
71677163
);
71687164
let upgrade_authority_keypair = Keypair::new();
71697165

7166+
// Invoke not yet deployed program
7167+
let instruction = Instruction::new_with_bytes(program_keypair.pubkey(), &[], Vec::new());
7168+
let invocation_message = Message::new(&[instruction], Some(&mint_keypair.pubkey()));
7169+
let binding = mint_keypair.insecure_clone();
7170+
let transaction = Transaction::new(
7171+
&[&binding],
7172+
invocation_message.clone(),
7173+
bank.last_blockhash(),
7174+
);
7175+
assert_eq!(
7176+
bank.process_transaction(&transaction),
7177+
Err(TransactionError::ProgramAccountNotFound),
7178+
);
7179+
{
7180+
// Make sure it is not in the cache because the account owner is not a loader
7181+
let program_cache = bank.transaction_processor.program_cache.read().unwrap();
7182+
let slot_versions = program_cache.get_slot_versions_for_tests(&program_keypair.pubkey());
7183+
assert!(slot_versions.is_empty());
7184+
}
7185+
71707186
// Load program file
71717187
let mut file = File::open("../programs/bpf_loader/test_elfs/out/noop_aligned.so")
71727188
.expect("file open failed");
@@ -7214,6 +7230,28 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
72147230
&bpf_loader_upgradeable::id(),
72157231
);
72167232

7233+
// Test buffer invocation
7234+
bank.store_account(&buffer_address, &buffer_account);
7235+
let instruction = Instruction::new_with_bytes(buffer_address, &[], Vec::new());
7236+
let message = Message::new(&[instruction], Some(&mint_keypair.pubkey()));
7237+
let transaction = Transaction::new(&[&binding], message, bank.last_blockhash());
7238+
assert_eq!(
7239+
bank.process_transaction(&transaction),
7240+
Err(TransactionError::InstructionError(
7241+
0,
7242+
InstructionError::InvalidAccountData,
7243+
)),
7244+
);
7245+
{
7246+
let program_cache = bank.transaction_processor.program_cache.read().unwrap();
7247+
let slot_versions = program_cache.get_slot_versions_for_tests(&buffer_address);
7248+
assert_eq!(slot_versions.len(), 1);
7249+
assert!(matches!(
7250+
slot_versions[0].program,
7251+
LoadedProgramType::Closed,
7252+
));
7253+
}
7254+
72177255
// Test successful deploy
72187256
let payer_base_balance = LAMPORTS_PER_SOL;
72197257
let deploy_fees = {
@@ -7231,7 +7269,6 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
72317269
&system_program::id(),
72327270
),
72337271
);
7234-
bank.store_account(&buffer_address, &buffer_account);
72357272
bank.store_account(&program_keypair.pubkey(), &AccountSharedData::default());
72367273
bank.store_account(&programdata_address, &AccountSharedData::default());
72377274
let message = Message::new(
@@ -7296,30 +7333,15 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() {
72967333
assert_eq!(*elf.get(i).unwrap(), *byte);
72977334
}
72987335

7299-
let loaded_program = bank.load_program(&program_keypair.pubkey(), false, bank.epoch());
7336+
// Advance the bank so that the program becomes effective
7337+
goto_end_of_slot(bank.clone());
7338+
let bank = bank_client
7339+
.advance_slot(1, bank_forks.as_ref(), &mint_keypair.pubkey())
7340+
.unwrap();
73007341

7301-
// Invoke deployed program
7302-
mock_process_instruction(
7303-
&bpf_loader_upgradeable::id(),
7304-
vec![0, 1],
7305-
&[],
7306-
vec![
7307-
(programdata_address, post_programdata_account),
7308-
(program_keypair.pubkey(), post_program_account),
7309-
],
7310-
Vec::new(),
7311-
Ok(()),
7312-
solana_bpf_loader_program::Entrypoint::vm,
7313-
|invoke_context| {
7314-
invoke_context
7315-
.programs_modified_by_tx
7316-
.set_slot_for_tests(bank.slot() + DELAY_VISIBILITY_SLOT_OFFSET);
7317-
invoke_context
7318-
.programs_modified_by_tx
7319-
.replenish(program_keypair.pubkey(), loaded_program.clone());
7320-
},
7321-
|_invoke_context| {},
7322-
);
7342+
// Invoke the deployed program
7343+
let transaction = Transaction::new(&[&binding], invocation_message, bank.last_blockhash());
7344+
assert!(bank.process_transaction(&transaction).is_ok());
73237345

73247346
// Test initialized program account
73257347
bank.clear_signatures();

0 commit comments

Comments
 (0)