optimize verify_instruction (#6539)

runtime/benches/is_zeroed.rs

@@ -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));
-}

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));
+}

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
+        && (!is_debitable // line coverage used to get branch coverage
-            // line coverage used to get branch coverage
+            || *program_id != pre.owner // 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
+    if *program_id != pre.owner // line coverage used to get branch coverage
-        // 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
+    if !is_debitable // line coverage used to get branch coverage
-        // 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)
+        && (!system_program::check_id(program_id) // line coverage used to get branch coverage
-            // line coverage used to get branch coverage
             || !system_program::check_id(&pre.owner))
     {
         return Err(InstructionError::AccountDataSizeChanged);
     }
 
-    // Verify data...
+    enum DataChanged {
-    if pre.data != post.data {
+        Unchecked,
-        // Credit-only account data may not change.
+        Checked(bool),
-        if !is_debitable {
+    };
-            return Err(InstructionError::CreditOnlyDataModified);
+
-        }
+    // Verify data, remember answer because comparing
-        // For accounts not assigned to the program, the data may not change.
+    //   a megabyte costs us multiple microseconds...
-        if *program_id != pre.owner {
+    let mut data_changed = DataChanged::Unchecked;
-            return Err(InstructionError::ExternalAccountDataModified);
+    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
+        && (!is_debitable // line coverage used to get branch coverage
-            // line coverage used to get branch coverage
+            || pre.executable // 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",
         );
     }