Fewer unsafe hacks for AppendVec (#3801)

* storage account changes

* cleanup

* checks

* comments

* clippy

* tests

* woot!

* comments

* benches

runtime/benches/append_vec.rs

@@ -15,8 +15,8 @@ fn append_vec_append(bencher: &mut Bencher) {
     let path = get_append_vec_path("bench_append");
     let vec = AppendVec::new(&path.path, true, 64 * 1024);
     bencher.iter(|| {
-        let account = create_test_account(0);
+        let (meta, account) = create_test_account(0);
-        if vec.append_account(&account).is_none() {
+        if vec.append_account(meta, &account).is_none() {
             vec.reset();
         }
     });
@@ -26,8 +26,8 @@ fn add_test_accounts(vec: &AppendVec, size: usize) -> Vec<(usize, usize)> {
     (0..size)
         .into_iter()
         .filter_map(|sample| {
-            let account = create_test_account(sample);
+            let (meta, account) = create_test_account(sample);
-            vec.append_account(&account).map(|pos| (sample, pos))
+            vec.append_account(meta, &account).map(|pos| (sample, pos))
         })
         .collect()
 }
@@ -40,9 +40,9 @@ fn append_vec_sequential_read(bencher: &mut Bencher) {
     let mut indexes = add_test_accounts(&vec, size);
     bencher.iter(|| {
         let (sample, pos) = indexes.pop().unwrap();
-        let account = vec.get_account(pos);
+        let (account, _next) = vec.get_account(pos).unwrap();
-        let test = create_test_account(sample);
+        let (_meta, test) = create_test_account(sample);
-        assert_eq!(*account, test);
+        assert_eq!(account.data, test.data.as_slice());
         indexes.push((sample, pos));
     });
 }
@@ -55,9 +55,9 @@ fn append_vec_random_read(bencher: &mut Bencher) {
     bencher.iter(|| {
         let random_index: usize = thread_rng().gen_range(0, indexes.len());
         let (sample, pos) = &indexes[random_index];
-        let account = vec.get_account(*pos);
+        let (account, _next) = vec.get_account(*pos).unwrap();
-        let test = create_test_account(*sample);
+        let (_meta, test) = create_test_account(*sample);
-        assert_eq!(*account, test);
+        assert_eq!(account.data, test.data.as_slice());
     });
 }
 
@@ -70,8 +70,8 @@ fn append_vec_concurrent_append_read(bencher: &mut Bencher) {
     let indexes1 = indexes.clone();
     spawn(move || loop {
         let sample = indexes1.lock().unwrap().len();
-        let account = create_test_account(sample);
+        let (meta, account) = create_test_account(sample);
-        if let Some(pos) = vec1.append_account(&account) {
+        if let Some(pos) = vec1.append_account(meta, &account) {
             indexes1.lock().unwrap().push((sample, pos))
         } else {
             break;
@@ -84,9 +84,9 @@ fn append_vec_concurrent_append_read(bencher: &mut Bencher) {
         let len = indexes.lock().unwrap().len();
         let random_index: usize = thread_rng().gen_range(0, len);
         let (sample, pos) = indexes.lock().unwrap().get(random_index).unwrap().clone();
-        let account = vec.get_account(pos);
+        let (account, _next) = vec.get_account(pos).unwrap();
-        let test = create_test_account(sample);
+        let (_meta, test) = create_test_account(sample);
-        assert_eq!(*account, test);
+        assert_eq!(account.data, test.data.as_slice());
     });
 }
 
@@ -106,14 +106,14 @@ fn append_vec_concurrent_read_append(bencher: &mut Bencher) {
             .get(random_index % len)
             .unwrap()
             .clone();
-        let account = vec1.get_account(pos);
+        let (account, _next) = vec1.get_account(pos).unwrap();
-        let test = create_test_account(sample);
+        let (_meta, test) = create_test_account(sample);
-        assert_eq!(*account, test);
+        assert_eq!(account.data, test.data.as_slice());
     });
     bencher.iter(|| {
         let sample: usize = thread_rng().gen_range(0, 256);
-        let account = create_test_account(sample);
+        let (meta, account) = create_test_account(sample);
-        if let Some(pos) = vec.append_account(&account) {
+        if let Some(pos) = vec.append_account(meta, &account) {
             indexes.lock().unwrap().push((sample, pos))
         }
     });

runtime/src/accounts.rs

@@ -19,7 +19,7 @@
 //! commit for each fork entry would be indexed.
 
 use crate::accounts_index::{AccountsIndex, Fork};
-use crate::append_vec::{AppendVec, StoredAccount};
+use crate::append_vec::{AppendVec, StorageMeta, StoredAccount};
 use crate::message_processor::has_duplicates;
 use bincode::serialize;
 use hashbrown::{HashMap, HashSet};
@@ -29,7 +29,7 @@ use rayon::prelude::*;
 use solana_metrics::counter::Counter;
 use solana_sdk::account::Account;
 use solana_sdk::fee_calculator::FeeCalculator;
-use solana_sdk::hash::{hash, Hash, Hasher};
+use solana_sdk::hash::{Hash, Hasher};
 use solana_sdk::native_loader;
 use solana_sdk::pubkey::Pubkey;
 use solana_sdk::signature::{Keypair, KeypairUtil};
@@ -278,14 +278,21 @@ impl AccountsDB {
             .collect()
     }
 
+    fn hash_account(stored_account: &StoredAccount) -> Hash {
+        let mut hasher = Hasher::default();
+        hasher.hash(&serialize(&stored_account.balance).unwrap());
+        hasher.hash(stored_account.data);
+        hasher.result()
+    }
+
     pub fn hash_internal_state(&self, fork_id: Fork) -> Option<Hash> {
         let accumulator: Vec<Vec<(Pubkey, u64, Hash)>> = self.scan_account_storage(
             fork_id,
             |stored_account: &StoredAccount, accum: &mut Vec<(Pubkey, u64, Hash)>| {
                 accum.push((
-                    stored_account.pubkey,
+                    stored_account.meta.pubkey,
-                    stored_account.write_version,
+                    stored_account.meta.write_version,
-                    hash(&serialize(&stored_account.account).unwrap()),
+                    Self::hash_account(stored_account),
                 ));
             },
         );
@@ -313,7 +320,7 @@ impl AccountsDB {
         //TODO: thread this as a ref
         storage
             .get(info.id)
-            .map(|store| store.accounts.get_account(info.offset).account.clone())
+            .and_then(|store| Some(store.accounts.get_account(info.offset)?.0.clone_account()))
     }
 
     fn load_tx_accounts(
@@ -530,13 +537,12 @@ impl AccountsDB {
             {
                 let accounts = &self.storage.read().unwrap()[id];
                 let write_version = self.write_version.fetch_add(1, Ordering::Relaxed) as u64;
-                let stored_account = StoredAccount {
+                let meta = StorageMeta {
                     write_version,
                     pubkey: *pubkey,
-                    //TODO: fix all this copy
+                    data_len: account.data.len() as u64,
-                    account: account.clone(),
                 };
-                result = accounts.accounts.append_account(&stored_account);
+                result = accounts.accounts.append_account(meta, account);
                 accounts.add_account();
             }
             if let Some(val) = result {
@@ -691,21 +697,24 @@ impl Accounts {
     }
 
     pub fn load_by_program(&self, fork: Fork, program_id: &Pubkey) -> Vec<(Pubkey, Account)> {
-        let accumulator: Vec<Vec<StoredAccount>> = self.accounts_db.scan_account_storage(
+        let accumulator: Vec<Vec<(Pubkey, u64, Account)>> = self.accounts_db.scan_account_storage(
             fork,
-            |stored_account: &StoredAccount, accum: &mut Vec<StoredAccount>| {
+            |stored_account: &StoredAccount, accum: &mut Vec<(Pubkey, u64, Account)>| {
-                if stored_account.account.owner == *program_id {
+                if stored_account.balance.owner == *program_id {
-                    accum.push(stored_account.clone())
+                    let val = (
+                        stored_account.meta.pubkey,
+                        stored_account.meta.write_version,
+                        stored_account.clone_account(),
+                    );
+                    accum.push(val)
                 }
             },
         );
-        let mut versions: Vec<StoredAccount> = accumulator.into_iter().flat_map(|x| x).collect();
+        let mut versions: Vec<(Pubkey, u64, Account)> =
-        versions.sort_by_key(|s| (s.pubkey, (s.write_version as i64).neg()));
+            accumulator.into_iter().flat_map(|x| x).collect();
-        versions.dedup_by_key(|s| s.pubkey);
+        versions.sort_by_key(|s| (s.0, (s.1 as i64).neg()));
-        versions
+        versions.dedup_by_key(|s| s.0);
-            .into_iter()
+        versions.into_iter().map(|s| (s.0, s.2)).collect()
-            .map(|s| (s.pubkey, s.account))
-            .collect()
     }
 
     /// Slow because lock is held for 1 operation instead of many

runtime/src/append_vec.rs

@@ -16,16 +16,44 @@ macro_rules! align_up {
     };
 }
 
-//TODO: This structure should contain references
+/// StorageMeta contains enough context to recover the index from storage itself
-/// StoredAccount contains enough context to recover the index from storage itself
 #[derive(Clone, PartialEq, Debug)]
-pub struct StoredAccount {
+pub struct StorageMeta {
     /// global write version
     pub write_version: u64,
     /// key for the account
     pub pubkey: Pubkey,
+    pub data_len: u64,
+}
+
+#[derive(Serialize, Deserialize, Clone, Default, Eq, PartialEq)]
+pub struct AccountBalance {
+    /// lamports in the account
+    pub lamports: u64,
+    /// the program that owns this account. If executable, the program that loads this account.
+    pub owner: Pubkey,
+    /// this account's data contains a loaded program (and is now read-only)
+    pub executable: bool,
+}
+
+/// References to Memory Mapped memory
+/// The Account is stored separately from its data, so getting the actual account requires a clone
+pub struct StoredAccount<'a> {
+    pub meta: &'a StorageMeta,
     /// account data
-    pub account: Account,
+    pub balance: &'a AccountBalance,
+    pub data: &'a [u8],
+}
+
+impl<'a> StoredAccount<'a> {
+    pub fn clone_account(&self) -> Account {
+        Account {
+            lamports: self.balance.lamports,
+            owner: self.balance.owner,
+            executable: self.balance.executable,
+            data: self.data.to_vec(),
+        }
+    }
 }
 
 pub struct AppendVec {
@@ -81,17 +109,20 @@ impl AppendVec {
         self.file_size
     }
 
-    // The reason for the `mut` is to allow the account data pointer to be fixed up after
+    // The reason for the `mut` is to allow Vec::from_raw_parts
-    // the structure is loaded
+    fn get_slice(&self, offset: usize, size: usize) -> Option<(&[u8], usize)> {
-    #[allow(clippy::mut_from_ref)]
-    fn get_slice(&self, offset: usize, size: usize) -> &mut [u8] {
         let len = self.len();
-        assert!(len >= offset + size);
+        if len < offset + size {
-        let data = &self.map[offset..offset + size];
+            return None;
-        unsafe {
-            let dst = data.as_ptr() as *mut u8;
-            std::slice::from_raw_parts_mut(dst, size)
         }
+        let data = &self.map[offset..offset + size];
+        //Data is aligned at the next 64 byte offset. Without alignment loading the memory may
+        //crash on some architectures.
+        let next = align_up!(offset + size, mem::size_of::<u64>());
+        Some((
+            unsafe { std::slice::from_raw_parts(data.as_ptr() as *const u8, size) },
+            next,
+        ))
     }
 
     fn append_ptr(&self, offset: &mut usize, src: *const u8, len: usize) {
@@ -99,6 +130,7 @@ impl AppendVec {
         //crash on some architectures.
         let pos = align_up!(*offset as usize, mem::size_of::<u64>());
         let data = &self.map[pos..(pos + len)];
+        //this mut append is safe because only 1 thread can append at a time
         unsafe {
             let dst = data.as_ptr() as *mut u8;
             std::ptr::copy(src, dst, len);
@@ -132,71 +164,64 @@ impl AppendVec {
         Some(pos)
     }
 
-    //TODO: Make this safer
+    fn get_type<T>(&self, offset: usize) -> Option<(&T, usize)> {
-    //StoredAccount should be a struct of references with the same lifetime as &self
+        let (data, next) = self.get_slice(offset, mem::size_of::<T>())?;
-    //The structure should have a method to clone the account out
+        let ptr: *const T = data.as_ptr() as *const T;
-    #[allow(clippy::transmute_ptr_to_ptr)]
+        Some((unsafe { &*ptr }, next))
-    pub fn get_account(&self, offset: usize) -> &StoredAccount {
-        let account: *mut StoredAccount = {
-            let data = self.get_slice(offset, mem::size_of::<StoredAccount>());
-            unsafe { std::mem::transmute::<*const u8, *mut StoredAccount>(data.as_ptr()) }
-        };
-        //Data is aligned at the next 64 byte offset. Without alignment loading the memory may
-        //crash on some architectures.
-        let data_at = align_up!(
-            offset + mem::size_of::<StoredAccount>(),
-            mem::size_of::<u64>()
-        );
-        let account_ref: &mut StoredAccount = unsafe { &mut *account };
-        let data = self.get_slice(data_at, account_ref.account.data.len());
-        unsafe {
-            let mut new_data = Vec::from_raw_parts(data.as_mut_ptr(), data.len(), data.len());
-            std::mem::swap(&mut account_ref.account.data, &mut new_data);
-            std::mem::forget(new_data);
-        };
-        account_ref
     }
 
-    pub fn accounts(&self, mut start: usize) -> Vec<&StoredAccount> {
+    pub fn get_account<'a>(&'a self, offset: usize) -> Option<(StoredAccount<'a>, usize)> {
+        let (meta, next): (&'a StorageMeta, _) = self.get_type(offset)?;
+        let (balance, next): (&'a AccountBalance, _) = self.get_type(next)?;
+        let (data, next) = self.get_slice(next, meta.data_len as usize)?;
+        Some((
+            StoredAccount {
+                meta,
+                balance,
+                data,
+            },
+            next,
+        ))
+    }
+    pub fn get_account_test(&self, offset: usize) -> Option<(StorageMeta, Account)> {
+        let stored = self.get_account(offset)?;
+        let meta = stored.0.meta.clone();
+        Some((meta, stored.0.clone_account()))
+    }
+
+    pub fn accounts<'a>(&'a self, mut start: usize) -> Vec<StoredAccount<'a>> {
         let mut accounts = vec![];
-        loop {
+        while let Some((account, next)) = self.get_account(start) {
-            //Data is aligned at the next 64 byte offset. Without alignment loading the memory may
+            accounts.push(account);
-            //crash on some architectures.
-            let end = align_up!(
-                start + mem::size_of::<StoredAccount>(),
-                mem::size_of::<u64>()
-            );
-            if end > self.len() {
-                break;
-            }
-            let first = self.get_account(start);
-            accounts.push(first);
-            //Data is aligned at the next 64 byte offset. Without alignment loading the memory may
-            //crash on some architectures.
-            let data_at = align_up!(
-                start + mem::size_of::<StoredAccount>(),
-                mem::size_of::<u64>()
-            );
-            let next = align_up!(data_at + first.account.data.len(), mem::size_of::<u64>());
             start = next;
         }
         accounts
     }
 
-    pub fn append_account(&self, account: &StoredAccount) -> Option<usize> {
+    pub fn append_account(&self, storage_meta: StorageMeta, account: &Account) -> Option<usize> {
-        let acc_ptr = account as *const StoredAccount;
+        let meta_ptr = &storage_meta as *const StorageMeta;
-        let data_len = account.account.data.len();
+        let balance = AccountBalance {
-        let data_ptr = account.account.data.as_ptr();
+            lamports: account.lamports,
+            owner: account.owner,
+            executable: account.executable,
+        };
+        let balance_ptr = &balance as *const AccountBalance;
+        let data_len = account.data.len();
+        let data_ptr = account.data.as_ptr();
         let ptrs = [
-            (acc_ptr as *const u8, mem::size_of::<StoredAccount>()),
+            (meta_ptr as *const u8, mem::size_of::<StorageMeta>()),
+            (balance_ptr as *const u8, mem::size_of::<AccountBalance>()),
             (data_ptr, data_len),
         ];
         self.append_ptrs(&ptrs)
     }
+    pub fn append_account_test(&self, data: &(StorageMeta, Account)) -> Option<usize> {
+        self.append_account(data.0.clone(), &data.1)
+    }
 }
 
 pub mod test_utils {
-    use super::StoredAccount;
+    use super::StorageMeta;
     use rand::distributions::Alphanumeric;
     use rand::{thread_rng, Rng};
     use solana_sdk::account::Account;
@@ -226,15 +251,16 @@ pub mod test_utils {
         TempFile { path: buf }
     }
 
-    pub fn create_test_account(sample: usize) -> StoredAccount {
+    pub fn create_test_account(sample: usize) -> (StorageMeta, Account) {
         let data_len = sample % 256;
         let mut account = Account::new(sample as u64, 0, &Pubkey::default());
         account.data = (0..data_len).map(|_| data_len as u8).collect();
-        StoredAccount {
+        let storage_meta = StorageMeta {
             write_version: 0,
             pubkey: Pubkey::default(),
-            account,
+            data_len: data_len as u64,
-        }
+        };
+        (storage_meta, account)
     }
 }
 
@@ -248,12 +274,12 @@ pub mod tests {
     use std::time::Instant;
 
     #[test]
-    fn test_append_vec() {
+    fn test_append_vec_one() {
         let path = get_append_vec_path("test_append");
         let av = AppendVec::new(&path.path, true, 1024 * 1024);
         let account = create_test_account(0);
-        let index = av.append_account(&account).unwrap();
+        let index = av.append_account_test(&account).unwrap();
-        assert_eq!(*av.get_account(index), account);
+        assert_eq!(av.get_account_test(index).unwrap(), account);
     }
 
     #[test]
@@ -261,12 +287,12 @@ pub mod tests {
         let path = get_append_vec_path("test_append_data");
         let av = AppendVec::new(&path.path, true, 1024 * 1024);
         let account = create_test_account(5);
-        let index = av.append_account(&account).unwrap();
+        let index = av.append_account_test(&account).unwrap();
-        assert_eq!(*av.get_account(index), account);
+        assert_eq!(av.get_account_test(index).unwrap(), account);
         let account1 = create_test_account(6);
-        let index1 = av.append_account(&account1).unwrap();
+        let index1 = av.append_account_test(&account1).unwrap();
-        assert_eq!(*av.get_account(index), account);
+        assert_eq!(av.get_account_test(index).unwrap(), account);
-        assert_eq!(*av.get_account(index1), account1);
+        assert_eq!(av.get_account_test(index1).unwrap(), account1);
     }
 
     #[test]
@@ -278,8 +304,8 @@ pub mod tests {
         let now = Instant::now();
         for sample in 0..size {
             let account = create_test_account(sample);
-            let pos = av.append_account(&account).unwrap();
+            let pos = av.append_account_test(&account).unwrap();
-            assert_eq!(*av.get_account(pos), account);
+            assert_eq!(av.get_account_test(pos).unwrap(), account);
             indexes.push(pos)
         }
         trace!("append time: {} ms", duration_as_ms(&now.elapsed()),);
@@ -288,18 +314,19 @@ pub mod tests {
         for _ in 0..size {
             let sample = thread_rng().gen_range(0, indexes.len());
             let account = create_test_account(sample);
-            assert_eq!(*av.get_account(indexes[sample]), account);
+            assert_eq!(av.get_account_test(indexes[sample]).unwrap(), account);
         }
         trace!("random read time: {} ms", duration_as_ms(&now.elapsed()),);
 
         let now = Instant::now();
         assert_eq!(indexes.len(), size);
         assert_eq!(indexes[0], 0);
-        let accounts = av.accounts(indexes[0]);
+        let mut accounts = av.accounts(indexes[0]);
         assert_eq!(accounts.len(), size);
-        for (sample, v) in accounts.iter().enumerate() {
+        for (sample, v) in accounts.iter_mut().enumerate() {
             let account = create_test_account(sample);
-            assert_eq!(**v, account)
+            let recovered = v.clone_account();
+            assert_eq!(recovered, account.1)
         }
         trace!(
             "sequential read time: {} ms",

runtime/src/lib.rs

@@ -14,3 +14,6 @@ mod system_instruction_processor;
 
 #[macro_use]
 extern crate solana_metrics;
+
+#[macro_use]
+extern crate serde_derive;