From ee92535cfd87174a6ecb50d54e01facc0f7ecfb8 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Fri, 8 Nov 2024 16:57:09 +0100 Subject: [PATCH] v2.1: Fix - SVM account_loader tests (backport of #3448) (#3520) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix - SVM account_loader tests (#3448) * Fixes test_load_transaction_accounts_program_account_executable_bypass. * Fixes test_load_transaction_accounts_data_sizes(). * Removes test_load_transaction_accounts_program_account_not_found_but_loaded(). (cherry picked from commit 74035490023c48bce51734cd4d74542ebcd38b32) Co-authored-by: Alexander Meißner --- Cargo.lock | 1 + svm/Cargo.toml | 1 + svm/src/account_loader.rs | 280 ++++++++++++++++++-------------------- 3 files changed, 133 insertions(+), 149 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 593950d228dc33..04f2c5bb5c1466 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8395,6 +8395,7 @@ dependencies = [ "solana-type-overrides", "solana-version", "solana-vote", + "solana_rbpf", "spl-token-2022", "test-case", "thiserror", diff --git a/svm/Cargo.toml b/svm/Cargo.toml index a6edc55fc1fca2..0366710646407e 100644 --- a/svm/Cargo.toml +++ b/svm/Cargo.toml @@ -81,6 +81,7 @@ solana-svm = { path = ".", features = ["dev-context-only-utils"] } solana-svm-conformance = { workspace = true } solana-transaction-status = { workspace = true } solana-version = { workspace = true } +solana_rbpf = { workspace = true } spl-token-2022 = { workspace = true, features = ["no-entrypoint"] } test-case = { workspace = true } tokio = { workspace = true, features = ["full"] } diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 2e47957d7f31dd..9cc7eb6ecf37b6 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -572,11 +572,14 @@ mod tests { solana_compute_budget::{compute_budget::ComputeBudget, compute_budget_limits}, solana_feature_set::FeatureSet, solana_program_runtime::loaded_programs::{ - ProgramCacheEntry, ProgramCacheEntryOwner, ProgramCacheForTxBatch, + ProgramCacheEntry, ProgramCacheEntryOwner, ProgramCacheEntryType, + ProgramCacheForTxBatch, }, + solana_rbpf::program::BuiltinProgram, solana_sdk::{ account::{Account, AccountSharedData, ReadableAccount, WritableAccount}, - bpf_loader, bpf_loader_upgradeable, + bpf_loader, + bpf_loader_upgradeable::{self, UpgradeableLoaderState}, epoch_schedule::EpochSchedule, hash::Hash, instruction::{AccountMeta, CompiledInstruction, Instruction}, @@ -597,7 +600,7 @@ mod tests { transaction::{Result, SanitizedTransaction, Transaction, TransactionError}, transaction_context::{TransactionAccount, TransactionContext}, }, - std::{borrow::Cow, cell::RefCell, collections::HashMap, sync::Arc}, + std::{borrow::Cow, cell::RefCell, collections::HashMap, fs::File, io::Read, sync::Arc}, }; #[derive(Default)] @@ -1356,52 +1359,6 @@ mod tests { ); } - #[test] - fn test_load_transaction_accounts_program_account_not_found_but_loaded() { - let key1 = Keypair::new(); - let key2 = Keypair::new(); - - let message = Message { - account_keys: vec![key1.pubkey(), key2.pubkey()], - header: MessageHeader::default(), - instructions: vec![CompiledInstruction { - program_id_index: 1, - accounts: vec![0], - data: vec![], - }], - recent_blockhash: Hash::default(), - }; - - let sanitized_message = new_unchecked_sanitized_message(message); - let mut mock_bank = TestCallbacks::default(); - let mut account_data = AccountSharedData::default(); - account_data.set_lamports(200); - mock_bank.accounts_map.insert(key1.pubkey(), account_data); - - let mut error_metrics = TransactionErrorMetrics::default(); - let mut loaded_programs = ProgramCacheForTxBatch::default(); - loaded_programs.replenish(key2.pubkey(), Arc::new(ProgramCacheEntry::default())); - - let sanitized_transaction = SanitizedTransaction::new_for_tests( - sanitized_message, - vec![Signature::new_unique()], - false, - ); - let result = load_transaction_accounts( - &mock_bank, - sanitized_transaction.message(), - LoadedTransactionAccount::default(), - &ComputeBudgetLimits::default(), - &mut error_metrics, - None, - &FeatureSet::default(), - &RentCollector::default(), - &loaded_programs, - ); - - assert_eq!(result.err(), Some(TransactionError::AccountNotFound)); - } - #[test] fn test_load_transaction_accounts_program_account_executable_bypass() { // currently, the account loader retrieves read-only non-instruction accounts from the program cache @@ -1437,10 +1394,7 @@ mod tests { .insert(bpf_loader::id(), loader_data.clone()); mock_bank .accounts_map - .insert(native_loader::id(), loader_data); - - let mut error_metrics = TransactionErrorMetrics::default(); - let mut loaded_programs = ProgramCacheForTxBatch::default(); + .insert(native_loader::id(), loader_data.clone()); let transaction = SanitizedTransaction::from_transaction_for_tests(Transaction::new_signed_with_payer( @@ -1454,32 +1408,17 @@ mod tests { Hash::default(), )); - let result = load_transaction_accounts( - &mock_bank, - transaction.message(), - LoadedTransactionAccount { - account: account_data.clone(), - ..LoadedTransactionAccount::default() - }, - &ComputeBudgetLimits::default(), - &mut error_metrics, - None, - &FeatureSet::default(), - &RentCollector::default(), - &loaded_programs, - ); - - // without cache, program is invalid - assert_eq!( - result.err(), - Some(TransactionError::InvalidProgramForExecution) - ); - + let mut loaded_programs = ProgramCacheForTxBatch::default(); loaded_programs.replenish( program_keypair.pubkey(), - Arc::new(ProgramCacheEntry::default()), + Arc::new(ProgramCacheEntry::new_tombstone( + 0, + ProgramCacheEntryOwner::LoaderV2, + ProgramCacheEntryType::Closed, + )), ); + let mut error_metrics = TransactionErrorMetrics::default(); let result = load_transaction_accounts( &mock_bank, transaction.message(), @@ -1495,9 +1434,9 @@ mod tests { &loaded_programs, ); - // with cache, executable flag is bypassed + // Executable flag is bypassed let mut cached_program = AccountSharedData::default(); - cached_program.set_owner(native_loader::id()); + cached_program.set_owner(bpf_loader::id()); cached_program.set_executable(true); assert_eq!( @@ -1506,6 +1445,7 @@ mod tests { accounts: vec![ (account_keypair.pubkey(), account_data.clone()), (program_keypair.pubkey(), cached_program), + (bpf_loader::id(), loader_data), ], program_indices: vec![vec![1]], rent: 0, @@ -2415,6 +2355,42 @@ mod tests { fn test_load_transaction_accounts_data_sizes() { let mut mock_bank = TestCallbacks::default(); + let program1_keypair = Keypair::new(); + let program1 = program1_keypair.pubkey(); + let program2 = Pubkey::new_unique(); + let programdata2 = Pubkey::new_unique(); + use solana_sdk::account_utils::StateMut; + + let program2_size = std::mem::size_of::() as u32; + let mut program2_account = AccountSharedData::default(); + program2_account.set_owner(bpf_loader_upgradeable::id()); + program2_account.set_executable(true); + program2_account.set_data(vec![0; program2_size as usize]); + program2_account + .set_state(&UpgradeableLoaderState::Program { + programdata_address: programdata2, + }) + .unwrap(); + mock_bank.accounts_map.insert(program2, program2_account); + let mut programdata2_account = AccountSharedData::default(); + programdata2_account.set_owner(bpf_loader_upgradeable::id()); + programdata2_account.set_data(vec![0; program2_size as usize]); + programdata2_account + .set_state(&UpgradeableLoaderState::ProgramData { + slot: 0, + upgrade_authority_address: None, + }) + .unwrap(); + let mut programdata = programdata2_account.data().to_vec(); + let mut file = + File::open("tests/example-programs/hello-solana/hello_solana_program.so").unwrap(); + file.read_to_end(&mut programdata).unwrap(); + let programdata2_size = programdata.len() as u32; + programdata2_account.set_data(programdata); + mock_bank + .accounts_map + .insert(programdata2, programdata2_account); + let mut next_size = 1; let mut make_account = |pubkey, owner, executable| { let size = next_size; @@ -2435,22 +2411,6 @@ mod tests { (size as u32, account) }; - let (native_loader_size, _) = make_account(native_loader::id(), native_loader::id(), true); - let (bpf_loader_size, _) = make_account(bpf_loader::id(), native_loader::id(), true); - let (upgradeable_loader_size, _) = - make_account(bpf_loader_upgradeable::id(), native_loader::id(), true); - - let program1_keypair = Keypair::new(); - let program1 = program1_keypair.pubkey(); - let (program1_size, _) = make_account(program1, bpf_loader::id(), true); - - let program2 = Pubkey::new_unique(); - let (program2_size, _) = make_account(program2, bpf_loader_upgradeable::id(), true); - - let programdata2 = Pubkey::new_unique(); - let (programdata2_size, _) = - make_account(programdata2, bpf_loader_upgradeable::id(), false); - let fee_payer_keypair = Keypair::new(); let fee_payer = fee_payer_keypair.pubkey(); let (fee_payer_size, fee_payer_account) = @@ -2462,7 +2422,30 @@ mod tests { let account2 = Pubkey::new_unique(); let (account2_size, _) = make_account(account2, program2, false); - let test_transaction_data_size_with_cache = |transaction, cache, expected_size| { + let (native_loader_size, _) = make_account(native_loader::id(), native_loader::id(), true); + let (bpf_loader_size, _) = make_account(bpf_loader::id(), native_loader::id(), true); + let (upgradeable_loader_size, _) = + make_account(bpf_loader_upgradeable::id(), native_loader::id(), true); + + let (_program1_size, _) = make_account(program1, bpf_loader::id(), true); + + let mut program_cache = ProgramCacheForTxBatch::default(); + let program1_entry = ProgramCacheEntry { + account_size: 0, + account_owner: ProgramCacheEntryOwner::LoaderV2, + program: ProgramCacheEntryType::Closed, + ..ProgramCacheEntry::default() + }; + program_cache.replenish(program1, Arc::new(program1_entry)); + let program2_entry = ProgramCacheEntry { + account_size: (program2_size + programdata2_size) as usize, + account_owner: ProgramCacheEntryOwner::LoaderV3, + program: ProgramCacheEntryType::Unloaded(Arc::new(BuiltinProgram::new_mock())), + ..ProgramCacheEntry::default() + }; + program_cache.replenish(program2, Arc::new(program2_entry)); + + let test_transaction_data_size = |transaction, expected_size| { let loaded_transaction_accounts = load_transaction_accounts( &mock_bank, &transaction, @@ -2476,7 +2459,7 @@ mod tests { None, &FeatureSet::default(), &RentCollector::default(), - &cache, + &program_cache, ) .unwrap(); @@ -2486,7 +2469,7 @@ mod tests { ); }; - let test_data_size_with_cache = |instructions: Vec<_>, cache, expected_size| { + let test_data_size = |instructions: Vec<_>, expected_size| { let transaction = SanitizedTransaction::from_transaction_for_tests( Transaction::new_signed_with_payer( &instructions, @@ -2496,21 +2479,13 @@ mod tests { ), ); - test_transaction_data_size_with_cache(transaction, cache, expected_size) + test_transaction_data_size(transaction, expected_size) }; for account_meta in [AccountMeta::new, AccountMeta::new_readonly] { - let test_data_size = |instructions, expected_size| { - test_data_size_with_cache( - instructions, - ProgramCacheForTxBatch::default(), - expected_size, - ) - }; - // one program plus loader let ixns = vec![Instruction::new_with_bytes(program1, &[], vec![])]; - test_data_size(ixns, program1_size + bpf_loader_size + fee_payer_size); + test_data_size(ixns, bpf_loader_size + fee_payer_size); // two programs, two loaders, two accounts let ixns = vec![ @@ -2521,8 +2496,8 @@ mod tests { ixns, account1_size + account2_size - + program1_size + program2_size + + programdata2_size + bpf_loader_size + upgradeable_loader_size + fee_payer_size, @@ -2534,17 +2509,17 @@ mod tests { &[], vec![account_meta(account2, false)], )]; - test_data_size( - ixns, - account2_size + program1_size + bpf_loader_size + fee_payer_size, - ); + test_data_size(ixns, account2_size + bpf_loader_size + fee_payer_size); // program and loader counted once let ixns = vec![ - Instruction::new_with_bytes(program1, &[], vec![]), - Instruction::new_with_bytes(program1, &[], vec![]), + Instruction::new_with_bytes(program2, &[], vec![]), + Instruction::new_with_bytes(program2, &[], vec![]), ]; - test_data_size(ixns, program1_size + bpf_loader_size + fee_payer_size); + test_data_size( + ixns, + upgradeable_loader_size + program2_size + programdata2_size + fee_payer_size, + ); // native loader not counted if loader let ixns = vec![Instruction::new_with_bytes(bpf_loader::id(), &[], vec![])]; @@ -2576,11 +2551,24 @@ mod tests { // loader counted twice if included in instruction let ixns = vec![Instruction::new_with_bytes( - program1, + program2, &[], - vec![account_meta(bpf_loader::id(), false)], + vec![account_meta(bpf_loader_upgradeable::id(), false)], )]; - test_data_size(ixns, program1_size + bpf_loader_size * 2 + fee_payer_size); + test_data_size( + ixns, + upgradeable_loader_size * 2 + program2_size + programdata2_size + fee_payer_size, + ); + + // loader counted twice even if included first + let ixns = vec![ + Instruction::new_with_bytes(bpf_loader_upgradeable::id(), &[], vec![]), + Instruction::new_with_bytes(program2, &[], vec![]), + ]; + test_data_size( + ixns, + upgradeable_loader_size * 2 + program2_size + programdata2_size + fee_payer_size, + ); // cover that case with multiple loaders to be sure let ixns = vec![ @@ -2602,43 +2590,25 @@ mod tests { test_data_size( ixns, account1_size - + program1_size + program2_size + + programdata2_size + bpf_loader_size * 2 + upgradeable_loader_size * 2 + fee_payer_size, ); - // loader counted twice even if included first - let ixns = vec![ - Instruction::new_with_bytes(bpf_loader::id(), &[], vec![]), - Instruction::new_with_bytes(program1, &[], vec![]), - ]; - test_data_size(ixns, program1_size + bpf_loader_size * 2 + fee_payer_size); - // fee-payer counted once let ixns = vec![Instruction::new_with_bytes( program1, &[], vec![account_meta(fee_payer, false)], )]; - test_data_size(ixns, program1_size + bpf_loader_size + fee_payer_size); - - // edge cases involving program cache - let mut program_cache = ProgramCacheForTxBatch::default(); - - let program2_entry = ProgramCacheEntry { - account_size: (program2_size + programdata2_size) as usize, - account_owner: ProgramCacheEntryOwner::LoaderV3, - ..ProgramCacheEntry::default() - }; - program_cache.replenish(program2, Arc::new(program2_entry)); + test_data_size(ixns, bpf_loader_size + fee_payer_size); // normal function call uses the combined cache size let ixns = vec![Instruction::new_with_bytes(program2, &[], vec![])]; - test_data_size_with_cache( + test_data_size( ixns, - program_cache.clone(), program2_size + programdata2_size + upgradeable_loader_size + fee_payer_size, ); @@ -2648,22 +2618,28 @@ mod tests { &[], vec![account_meta(program2, false)], )]; - test_data_size_with_cache( + test_data_size( ixns, - program_cache.clone(), program2_size + upgradeable_loader_size + fee_payer_size, ); - // programdata as instruction account double-counts it + // programdata as readonly instruction account double-counts it let ixns = vec![Instruction::new_with_bytes( program2, &[], vec![account_meta(programdata2, false)], )]; - test_data_size_with_cache( + let factor = if ixns[0].accounts[0].is_writable { + 1 + } else { + 2 + }; + test_data_size( ixns, - program_cache.clone(), - program2_size + programdata2_size * 2 + upgradeable_loader_size + fee_payer_size, + program2_size + + programdata2_size * factor + + upgradeable_loader_size + + fee_payer_size, ); // both as instruction accounts, for completeness @@ -2675,17 +2651,23 @@ mod tests { account_meta(programdata2, false), ], )]; - test_data_size_with_cache( + let factor = if ixns[0].accounts[0].is_writable { + 0 + } else { + 1 + }; + test_data_size( ixns, - program_cache.clone(), - program2_size + programdata2_size + upgradeable_loader_size + fee_payer_size, + program2_size + + programdata2_size * factor + + upgradeable_loader_size + + fee_payer_size, ); // writable program bypasses the cache let tx = new_unchecked_sanitized_transaction_with_writable_program(program2, fee_payer); - test_transaction_data_size_with_cache( + test_transaction_data_size( tx, - program_cache.clone(), program2_size + upgradeable_loader_size + fee_payer_size, );