From fecbaf0d62c6e3126f943df2021fd713f9b72016 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Fri, 6 Dec 2024 10:12:00 -0800 Subject: [PATCH 1/4] add support for return data --- harness/src/fuzz/firedancer.rs | 1 + harness/src/fuzz/mollusk.rs | 1 + harness/src/lib.rs | 21 ++++++++++++++++++--- harness/src/result.rs | 25 +++++++++++++++++++++++++ 4 files changed, 45 insertions(+), 3 deletions(-) diff --git a/harness/src/fuzz/firedancer.rs b/harness/src/fuzz/firedancer.rs index 58cdba14..94f3a351 100644 --- a/harness/src/fuzz/firedancer.rs +++ b/harness/src/fuzz/firedancer.rs @@ -232,6 +232,7 @@ fn parse_fixture_effects( .compute_budget .compute_unit_limit .saturating_sub(effects.compute_units_available), + return_data: None, // TODO: Omitted for now. resulting_accounts, } } diff --git a/harness/src/fuzz/mollusk.rs b/harness/src/fuzz/mollusk.rs index 1154e2ba..477e92c9 100644 --- a/harness/src/fuzz/mollusk.rs +++ b/harness/src/fuzz/mollusk.rs @@ -94,6 +94,7 @@ impl From<&FuzzEffects> for InstructionResult { execution_time, program_result, raw_result, + return_data: None, // TODO: Omitted for now. resulting_accounts, } } diff --git a/harness/src/lib.rs b/harness/src/lib.rs index fdb1704f..28957924 100644 --- a/harness/src/lib.rs +++ b/harness/src/lib.rs @@ -46,9 +46,15 @@ use { solana_compute_budget::compute_budget::ComputeBudget, solana_program_runtime::invoke_context::{EnvironmentConfig, InvokeContext}, solana_sdk::{ - account::AccountSharedData, bpf_loader_upgradeable, feature_set::FeatureSet, - fee::FeeStructure, hash::Hash, instruction::Instruction, precompiles::get_precompile, - pubkey::Pubkey, transaction_context::TransactionContext, + account::AccountSharedData, + bpf_loader_upgradeable, + feature_set::FeatureSet, + fee::FeeStructure, + hash::Hash, + instruction::Instruction, + precompiles::get_precompile, + pubkey::Pubkey, + transaction_context::{TransactionContext, TransactionReturnData}, }, solana_timings::ExecuteTimings, std::sync::Arc, @@ -217,6 +223,14 @@ impl Mollusk { } }; + let return_data = match transaction_context.get_return_data() { + (program_id, data) if !data.is_empty() => Some(TransactionReturnData { + program_id: *program_id, + data: data.to_vec(), + }), + _ => None, + }; + let resulting_accounts: Vec<(Pubkey, AccountSharedData)> = accounts .iter() .map(|(pubkey, account)| { @@ -236,6 +250,7 @@ impl Mollusk { execution_time: timings.details.execute_us, program_result: invoke_result.clone().into(), raw_result: invoke_result, + return_data, resulting_accounts, } } diff --git a/harness/src/result.rs b/harness/src/result.rs index 0d0aeab5..bb9e82eb 100644 --- a/harness/src/result.rs +++ b/harness/src/result.rs @@ -5,6 +5,7 @@ use solana_sdk::{ instruction::InstructionError, program_error::ProgramError, pubkey::Pubkey, + transaction_context::TransactionReturnData, }; /// The result code of the program's execution. @@ -51,6 +52,8 @@ pub struct InstructionResult { pub program_result: ProgramResult, /// The raw result of the program's execution. pub raw_result: Result<(), InstructionError>, + /// The return data produced by the instruction, if any. + pub return_data: Option, /// The resulting accounts after executing the instruction. /// /// This includes all accounts provided to the processor, in the order @@ -66,6 +69,7 @@ impl Default for InstructionResult { execution_time: 0, program_result: ProgramResult::Success, raw_result: Ok(()), + return_data: None, resulting_accounts: vec![], } } @@ -111,6 +115,20 @@ impl InstructionResult { actual_result, check_result, ); } + CheckType::ReturnData(return_data) => { + let check_return_data = return_data; + let Some(actual_return_data) = &self.return_data else { + panic!( + "CHECK: return data: got None, expected {:?}", + check_return_data, + ); + }; + assert_eq!( + actual_return_data, check_return_data, + "CHECK: return_data: got {:?}, expected {:?}", + actual_return_data, check_return_data, + ); + } CheckType::ResultingAccount(account) => { let pubkey = account.pubkey; let resulting_account = self @@ -217,6 +235,8 @@ enum CheckType<'a> { ExecutionTime(u64), /// Check the result code of the program's execution. ProgramResult(ProgramResult), + /// Check the return data produced by executing the instruction. + ReturnData(TransactionReturnData), /// Check a resulting account after executing the instruction. ResultingAccount(AccountCheck<'a>), } @@ -255,6 +275,11 @@ impl<'a> Check<'a> { Check::new(CheckType::ProgramResult(ProgramResult::UnknownError(error))) } + /// Check the return data produced by executing the instruction. + pub fn return_data(return_data: TransactionReturnData) -> Self { + Check::new(CheckType::ReturnData(return_data)) + } + /// Check a resulting account after executing the instruction. pub fn account(pubkey: &Pubkey) -> AccountCheckBuilder { AccountCheckBuilder::new(pubkey) From 746b3623b8a1532cf47e8264fc7656599ac0d564 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Tue, 10 Dec 2024 00:51:33 -0800 Subject: [PATCH 2/4] uncomment return data test; should fail --- harness/src/result.rs | 1 + harness/tests/fd_test_vectors.rs | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/harness/src/result.rs b/harness/src/result.rs index bb9e82eb..49988a94 100644 --- a/harness/src/result.rs +++ b/harness/src/result.rs @@ -217,6 +217,7 @@ impl InstructionResult { b.resulting_accounts.len(), "resulting accounts length mismatch" ); + assert_eq!(self.return_data, b.return_data, "return data mismatch"); for (a, b) in self .resulting_accounts .iter() diff --git a/harness/tests/fd_test_vectors.rs b/harness/tests/fd_test_vectors.rs index 703a4307..87379494 100644 --- a/harness/tests/fd_test_vectors.rs +++ b/harness/tests/fd_test_vectors.rs @@ -106,10 +106,10 @@ fn test_load_firedancer_fixtures() { loaded_fixture.output.compute_units_available, generated_fixture.output.compute_units_available, ); - // assert_eq!( - // loaded_fixture.output.return_data, - // generated_fixture.output.return_data, - // ); + assert_eq!( + loaded_fixture.output.return_data, + generated_fixture.output.return_data, + ); } }); }); From 18a72544f3b69a300d49da1460534a9362783323 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Tue, 10 Dec 2024 01:02:27 -0800 Subject: [PATCH 3/4] add retdata to effects; no idea if this will pass --- fuzz/fixture/src/effects.rs | 8 +++++++- harness/src/fuzz/mollusk.rs | 5 ++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/fuzz/fixture/src/effects.rs b/fuzz/fixture/src/effects.rs index e1c54212..9e4abacc 100644 --- a/fuzz/fixture/src/effects.rs +++ b/fuzz/fixture/src/effects.rs @@ -2,7 +2,10 @@ use { super::proto::{AcctState as ProtoAccount, InstrEffects as ProtoEffects}, - solana_sdk::{account::AccountSharedData, keccak::Hasher, pubkey::Pubkey}, + solana_sdk::{ + account::AccountSharedData, keccak::Hasher, pubkey::Pubkey, + transaction_context::TransactionReturnData, + }, }; /// Represents the effects of a single instruction. @@ -14,6 +17,7 @@ pub struct Effects { pub execution_time: u64, // Program return code. Zero is success, errors are non-zero. pub program_result: u32, + pub return_data: Option, /// Resulting accounts with state, to be checked post-simulation. pub resulting_accounts: Vec<(Pubkey, AccountSharedData)>, } @@ -34,6 +38,7 @@ impl From for Effects { compute_units_consumed, execution_time, program_result, + return_data: None, // TODO resulting_accounts, } } @@ -45,6 +50,7 @@ impl From for ProtoEffects { compute_units_consumed, execution_time, program_result, + return_data: _, // TODO resulting_accounts, } = value; diff --git a/harness/src/fuzz/mollusk.rs b/harness/src/fuzz/mollusk.rs index 477e92c9..63c49ad9 100644 --- a/harness/src/fuzz/mollusk.rs +++ b/harness/src/fuzz/mollusk.rs @@ -56,6 +56,7 @@ impl From<&InstructionResult> for FuzzEffects { fn from(input: &InstructionResult) -> Self { let compute_units_consumed = input.compute_units_consumed; let execution_time = input.execution_time; + let return_data = input.return_data.clone(); let program_result = match &input.program_result { ProgramResult::Success => 0, @@ -69,6 +70,7 @@ impl From<&InstructionResult> for FuzzEffects { compute_units_consumed, execution_time, program_result, + return_data, resulting_accounts, } } @@ -78,6 +80,7 @@ impl From<&FuzzEffects> for InstructionResult { fn from(input: &FuzzEffects) -> Self { let compute_units_consumed = input.compute_units_consumed; let execution_time = input.execution_time; + let return_data = input.return_data.clone(); let raw_result = if input.program_result == 0 { Ok(()) @@ -94,7 +97,7 @@ impl From<&FuzzEffects> for InstructionResult { execution_time, program_result, raw_result, - return_data: None, // TODO: Omitted for now. + return_data, resulting_accounts, } } From 500e7cbaa34b6b77cd431fd077824cdb7677fcb4 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Tue, 10 Dec 2024 01:26:44 -0800 Subject: [PATCH 4/4] oh i understand everything now --- fuzz/fixture/proto/invoke.proto | 7 +++++-- fuzz/fixture/src/effects.rs | 13 ++++++------- harness/src/fuzz/firedancer.rs | 7 +++++-- harness/src/lib.rs | 20 ++++---------------- harness/src/result.rs | 16 +++++----------- 5 files changed, 25 insertions(+), 38 deletions(-) diff --git a/fuzz/fixture/proto/invoke.proto b/fuzz/fixture/proto/invoke.proto index e6872601..e2d14d61 100644 --- a/fuzz/fixture/proto/invoke.proto +++ b/fuzz/fixture/proto/invoke.proto @@ -64,14 +64,17 @@ message InstrEffects { // Program return code. Zero is success, errors are non-zero. uint32 program_result = 3; + // The instruction return data. + bytes return_data = 4; + // Copies of accounts that were provided to the instruction. May be in an // arbitrary order. The pubkey of each account is unique in this list. Each // account address must also be in the InstrContext. - repeated AcctState resulting_accounts = 4; + repeated AcctState resulting_accounts = 5; } // An instruction processing test fixture. message InstrFixture { InstrContext input = 1; InstrEffects output = 2; -} \ No newline at end of file +} diff --git a/fuzz/fixture/src/effects.rs b/fuzz/fixture/src/effects.rs index 9e4abacc..c5194d7c 100644 --- a/fuzz/fixture/src/effects.rs +++ b/fuzz/fixture/src/effects.rs @@ -2,10 +2,7 @@ use { super::proto::{AcctState as ProtoAccount, InstrEffects as ProtoEffects}, - solana_sdk::{ - account::AccountSharedData, keccak::Hasher, pubkey::Pubkey, - transaction_context::TransactionReturnData, - }, + solana_sdk::{account::AccountSharedData, keccak::Hasher, pubkey::Pubkey}, }; /// Represents the effects of a single instruction. @@ -17,7 +14,7 @@ pub struct Effects { pub execution_time: u64, // Program return code. Zero is success, errors are non-zero. pub program_result: u32, - pub return_data: Option, + pub return_data: Vec, /// Resulting accounts with state, to be checked post-simulation. pub resulting_accounts: Vec<(Pubkey, AccountSharedData)>, } @@ -28,6 +25,7 @@ impl From for Effects { compute_units_consumed, execution_time, program_result, + return_data, resulting_accounts, } = value; @@ -38,7 +36,7 @@ impl From for Effects { compute_units_consumed, execution_time, program_result, - return_data: None, // TODO + return_data, resulting_accounts, } } @@ -50,7 +48,7 @@ impl From for ProtoEffects { compute_units_consumed, execution_time, program_result, - return_data: _, // TODO + return_data, resulting_accounts, } = value; @@ -61,6 +59,7 @@ impl From for ProtoEffects { compute_units_consumed, execution_time, program_result, + return_data, resulting_accounts, } } diff --git a/harness/src/fuzz/firedancer.rs b/harness/src/fuzz/firedancer.rs index 94f3a351..5cc9bc41 100644 --- a/harness/src/fuzz/firedancer.rs +++ b/harness/src/fuzz/firedancer.rs @@ -169,6 +169,8 @@ fn build_fixture_effects(context: &FuzzContext, result: &InstructionResult) -> F } }; + let return_data = result.return_data.clone(); + let modified_accounts = context .accounts .iter() @@ -191,7 +193,7 @@ fn build_fixture_effects(context: &FuzzContext, result: &InstructionResult) -> F compute_units_available: context .compute_units_available .saturating_sub(result.compute_units_consumed), - return_data: Vec::new(), // TODO: Mollusk doesn't capture return data. + return_data, } } @@ -210,6 +212,7 @@ fn parse_fixture_effects( }; let program_result = raw_result.clone().into(); + let return_data = effects.return_data.clone(); let resulting_accounts = accounts .iter() @@ -232,7 +235,7 @@ fn parse_fixture_effects( .compute_budget .compute_unit_limit .saturating_sub(effects.compute_units_available), - return_data: None, // TODO: Omitted for now. + return_data, resulting_accounts, } } diff --git a/harness/src/lib.rs b/harness/src/lib.rs index 28957924..a66e366e 100644 --- a/harness/src/lib.rs +++ b/harness/src/lib.rs @@ -46,15 +46,9 @@ use { solana_compute_budget::compute_budget::ComputeBudget, solana_program_runtime::invoke_context::{EnvironmentConfig, InvokeContext}, solana_sdk::{ - account::AccountSharedData, - bpf_loader_upgradeable, - feature_set::FeatureSet, - fee::FeeStructure, - hash::Hash, - instruction::Instruction, - precompiles::get_precompile, - pubkey::Pubkey, - transaction_context::{TransactionContext, TransactionReturnData}, + account::AccountSharedData, bpf_loader_upgradeable, feature_set::FeatureSet, + fee::FeeStructure, hash::Hash, instruction::Instruction, precompiles::get_precompile, + pubkey::Pubkey, transaction_context::TransactionContext, }, solana_timings::ExecuteTimings, std::sync::Arc, @@ -223,13 +217,7 @@ impl Mollusk { } }; - let return_data = match transaction_context.get_return_data() { - (program_id, data) if !data.is_empty() => Some(TransactionReturnData { - program_id: *program_id, - data: data.to_vec(), - }), - _ => None, - }; + let return_data = transaction_context.get_return_data().1.to_vec(); let resulting_accounts: Vec<(Pubkey, AccountSharedData)> = accounts .iter() diff --git a/harness/src/result.rs b/harness/src/result.rs index 49988a94..803e41b2 100644 --- a/harness/src/result.rs +++ b/harness/src/result.rs @@ -5,7 +5,6 @@ use solana_sdk::{ instruction::InstructionError, program_error::ProgramError, pubkey::Pubkey, - transaction_context::TransactionReturnData, }; /// The result code of the program's execution. @@ -53,7 +52,7 @@ pub struct InstructionResult { /// The raw result of the program's execution. pub raw_result: Result<(), InstructionError>, /// The return data produced by the instruction, if any. - pub return_data: Option, + pub return_data: Vec, /// The resulting accounts after executing the instruction. /// /// This includes all accounts provided to the processor, in the order @@ -69,7 +68,7 @@ impl Default for InstructionResult { execution_time: 0, program_result: ProgramResult::Success, raw_result: Ok(()), - return_data: None, + return_data: vec![], resulting_accounts: vec![], } } @@ -117,12 +116,7 @@ impl InstructionResult { } CheckType::ReturnData(return_data) => { let check_return_data = return_data; - let Some(actual_return_data) = &self.return_data else { - panic!( - "CHECK: return data: got None, expected {:?}", - check_return_data, - ); - }; + let actual_return_data = &self.return_data; assert_eq!( actual_return_data, check_return_data, "CHECK: return_data: got {:?}, expected {:?}", @@ -237,7 +231,7 @@ enum CheckType<'a> { /// Check the result code of the program's execution. ProgramResult(ProgramResult), /// Check the return data produced by executing the instruction. - ReturnData(TransactionReturnData), + ReturnData(Vec), /// Check a resulting account after executing the instruction. ResultingAccount(AccountCheck<'a>), } @@ -277,7 +271,7 @@ impl<'a> Check<'a> { } /// Check the return data produced by executing the instruction. - pub fn return_data(return_data: TransactionReturnData) -> Self { + pub fn return_data(return_data: Vec) -> Self { Check::new(CheckType::ReturnData(return_data)) }