diff --git a/runtime/benches/is_zeroed.rs b/runtime/benches/is_zeroed.rs deleted file mode 100644 index fe99997966..0000000000 --- a/runtime/benches/is_zeroed.rs +++ /dev/null @@ -1,34 +0,0 @@ -#![feature(test)] - -extern crate test; - -use solana_runtime::message_processor::is_zeroed; -use test::Bencher; - -const BUFSIZE: usize = 1024 * 1024 + 127; -static BUF0: [u8; BUFSIZE] = [0; BUFSIZE]; -static BUF1: [u8; BUFSIZE] = [1; BUFSIZE]; - -#[bench] -fn bench_is_zeroed(bencher: &mut Bencher) { - bencher.iter(|| { - is_zeroed(&BUF0); - }); -} - -#[bench] -fn bench_is_zeroed_not(bencher: &mut Bencher) { - bencher.iter(|| { - is_zeroed(&BUF1); - }); -} - -#[bench] -fn bench_is_zeroed_by_iter(bencher: &mut Bencher) { - bencher.iter(|| BUF0.iter().all(|item| *item == 0)); -} - -#[bench] -fn bench_is_zeroed_not_by_iter(bencher: &mut Bencher) { - bencher.iter(|| BUF1.iter().all(|item| *item == 0)); -} diff --git a/runtime/benches/message_processor.rs b/runtime/benches/message_processor.rs index 494929b34d..212501ac16 100644 --- a/runtime/benches/message_processor.rs +++ b/runtime/benches/message_processor.rs @@ -2,7 +2,9 @@ extern crate test; +use log::*; use solana_runtime::message_processor::*; +use solana_sdk::{account::Account, pubkey::Pubkey}; use test::Bencher; #[bench] @@ -12,3 +14,59 @@ fn bench_has_duplicates(bencher: &mut Bencher) { assert!(!has_duplicates(&data)); }) } + +#[bench] +fn bench_verify_instruction_data(bencher: &mut Bencher) { + solana_logger::setup(); + + let owner = Pubkey::new_rand(); + let non_owner = Pubkey::new_rand(); + let pre = Account::new(0, BUFSIZE, &owner); + let post = Account::new(0, BUFSIZE, &owner); + assert_eq!(verify_instruction(true, &owner, &pre, &post), Ok(())); + + bencher.iter(|| pre.data == post.data); + let summary = bencher.bench(|_bencher| {}).unwrap(); + info!("data compare {} ns/iter", summary.median); + + // this one should be faster + bencher.iter(|| { + verify_instruction(true, &owner, &pre, &post).unwrap(); + }); + let summary = bencher.bench(|_bencher| {}).unwrap(); + info!("data no change by owner: {} ns/iter", summary.median); + + bencher.iter(|| { + verify_instruction(true, &non_owner, &pre, &post).unwrap(); + }); + let summary = bencher.bench(|_bencher| {}).unwrap(); + info!("data no change by non owner: {} ns/iter", summary.median); +} + +const BUFSIZE: usize = 1024 * 1024 + 127; +static BUF0: [u8; BUFSIZE] = [0; BUFSIZE]; +static BUF1: [u8; BUFSIZE] = [1; BUFSIZE]; + +#[bench] +fn bench_is_zeroed(bencher: &mut Bencher) { + bencher.iter(|| { + is_zeroed(&BUF0); + }); +} + +#[bench] +fn bench_is_zeroed_not(bencher: &mut Bencher) { + bencher.iter(|| { + is_zeroed(&BUF1); + }); +} + +#[bench] +fn bench_is_zeroed_by_iter(bencher: &mut Bencher) { + bencher.iter(|| BUF0.iter().all(|item| *item == 0)); +} + +#[bench] +fn bench_is_zeroed_not_by_iter(bencher: &mut Bencher) { + bencher.iter(|| BUF1.iter().all(|item| *item == 0)); +} diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 5ecbccca0f..077dcb3a09 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -58,7 +58,7 @@ fn get_subset_unchecked_mut<'a, T>( .collect()) } -fn verify_instruction( +pub fn verify_instruction( is_debitable: bool, program_id: &Pubkey, pre: &Account, @@ -70,26 +70,22 @@ fn verify_instruction( // only if the account is credit-debit and // only if the data is zero-initialized or empty if pre.owner != post.owner - && (!is_debitable - // line coverage used to get branch coverage - || *program_id != pre.owner - // line coverage used to get branch coverage + && (!is_debitable // line coverage used to get branch coverage + || *program_id != pre.owner // line coverage used to get branch coverage || !is_zeroed(&post.data)) { return Err(InstructionError::ModifiedProgramId); } // An account not assigned to the program cannot have its balance decrease. - if *program_id != pre.owner - // line coverage used to get branch coverage + if *program_id != pre.owner // line coverage used to get branch coverage && pre.lamports > post.lamports { return Err(InstructionError::ExternalAccountLamportSpend); } // The balance of credit-only accounts may only increase. - if !is_debitable - // line coverage used to get branch coverage + if !is_debitable // line coverage used to get branch coverage && pre.lamports > post.lamports { return Err(InstructionError::CreditOnlyLamportSpend); @@ -98,32 +94,50 @@ fn verify_instruction( // Only the system program can change the size of the data // and only if the system program owns the account if pre.data.len() != post.data.len() - && (!system_program::check_id(program_id) - // line coverage used to get branch coverage + && (!system_program::check_id(program_id) // line coverage used to get branch coverage || !system_program::check_id(&pre.owner)) { return Err(InstructionError::AccountDataSizeChanged); } - // Verify data... - if pre.data != post.data { - // Credit-only account data may not change. - if !is_debitable { - return Err(InstructionError::CreditOnlyDataModified); - } - // For accounts not assigned to the program, the data may not change. - if *program_id != pre.owner { - return Err(InstructionError::ExternalAccountDataModified); + enum DataChanged { + Unchecked, + Checked(bool), + }; + + // Verify data, remember answer because comparing + // a megabyte costs us multiple microseconds... + let mut data_changed = DataChanged::Unchecked; + let mut data_changed = || -> bool { + match data_changed { + DataChanged::Unchecked => { + let changed = pre.data != post.data; + data_changed = DataChanged::Checked(changed); + changed + } + DataChanged::Checked(changed) => changed, } + }; + + // For accounts not assigned to the program, the data may not change. + if *program_id != pre.owner // line coverage used to get branch coverage + && data_changed() + { + return Err(InstructionError::ExternalAccountDataModified); + } + + // Credit-only account data may not change. + if !is_debitable // line coverage used to get branch coverage + && data_changed() + { + return Err(InstructionError::CreditOnlyDataModified); } // executable is one-way (false->true) and // only system or the account owner may modify. if pre.executable != post.executable - && (!is_debitable - // line coverage used to get branch coverage - || pre.executable - // line coverage used to get branch coverage + && (!is_debitable // line coverage used to get branch coverage + || pre.executable // line coverage used to get branch coverage || *program_id != pre.owner) { return Err(InstructionError::ExecutableModified); @@ -589,7 +603,6 @@ mod tests { verify_instruction(is_debitable, &program_id, &pre, &post) }; - let system_program_id = system_program::id(); let mallory_program_id = Pubkey::new_rand(); assert_eq!( @@ -600,13 +613,13 @@ mod tests { assert_eq!( change_data(&mallory_program_id, true), Err(InstructionError::ExternalAccountDataModified), - "malicious Mallory should not be able to change the account data" + "non-owner mallory should not be able to change the account data" ); assert_eq!( - change_data(&system_program_id, false), + change_data(&alice_program_id, false), Err(InstructionError::CreditOnlyDataModified), - "system program should not be able to change the data if credit-only" + "alice isn't allowed to touch a CO account" ); } @@ -641,7 +654,7 @@ mod tests { assert_eq!( verify_instruction(true, &alice_program_id, &pre, &post), Ok(()), - "alice should be able to deduct lamports and the account to bob if the data is zeroed", + "alice should be able to deduct lamports and give the account to bob if the data is zeroed", ); }