From 909f73f55704b92855084460bcc1e2258d4ad2b6 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang <93241502+yhchiang-sol@users.noreply.github.com> Date: Fri, 9 Feb 2024 16:35:40 -0800 Subject: [PATCH] [TieredStorage] Put commonly used test functions into test_utils.rs (#35065) There're some test functions that have been used in different mod in TieredStorage. It's better to have one same place for all tiere-storage related test functions. Created test_utils.rs under /tiered_storage and move test-related functions into it. Existing tests. --- accounts-db/src/tiered_storage.rs | 76 +++---------- accounts-db/src/tiered_storage/byte_block.rs | 2 +- accounts-db/src/tiered_storage/footer.rs | 19 ++-- accounts-db/src/tiered_storage/hot.rs | 106 +++++-------------- accounts-db/src/tiered_storage/index.rs | 11 +- accounts-db/src/tiered_storage/mmap_utils.rs | 8 +- accounts-db/src/tiered_storage/test_utils.rs | 76 +++++++++++++ 7 files changed, 138 insertions(+), 160 deletions(-) create mode 100644 accounts-db/src/tiered_storage/test_utils.rs diff --git a/accounts-db/src/tiered_storage.rs b/accounts-db/src/tiered_storage.rs index bfa4796485523a..335e93c72e9750 100644 --- a/accounts-db/src/tiered_storage.rs +++ b/accounts-db/src/tiered_storage.rs @@ -10,6 +10,7 @@ pub mod meta; pub mod mmap_utils; pub mod owners; pub mod readable; +mod test_utils; pub mod writer; use { @@ -27,7 +28,7 @@ use { solana_sdk::account::ReadableAccount, std::{ borrow::Borrow, - fs::OpenOptions, + fs::{self, OpenOptions}, path::{Path, PathBuf}, sync::OnceLock, }, @@ -54,8 +55,11 @@ pub struct TieredStorage { impl Drop for TieredStorage { fn drop(&mut self) { - if let Err(err) = fs_err::remove_file(&self.path) { - panic!("TieredStorage failed to remove backing storage file: {err}"); + if let Err(err) = fs::remove_file(&self.path) { + panic!( + "TieredStorage failed to remove backing storage file '{}': {err}", + self.path.display(), + ); } } } @@ -157,17 +161,12 @@ impl TieredStorage { mod tests { use { super::*, - crate::account_storage::meta::{StoredAccountMeta, StoredMeta, StoredMetaWriteVersion}, + crate::account_storage::meta::StoredMetaWriteVersion, footer::{TieredStorageFooter, TieredStorageMagicNumber}, hot::HOT_FORMAT, index::IndexOffset, - owners::OWNER_NO_OWNER, - solana_accounts_db::rent_collector::RENT_EXEMPT_RENT_EPOCH, solana_sdk::{ - account::{Account, AccountSharedData}, - clock::Slot, - hash::Hash, - pubkey::Pubkey, + account::AccountSharedData, clock::Slot, hash::Hash, pubkey::Pubkey, system_instruction::MAX_PERMITTED_DATA_LENGTH, }, std::{ @@ -175,6 +174,7 @@ mod tests { mem::ManuallyDrop, }, tempfile::tempdir, + test_utils::{create_test_account, verify_test_account}, }; impl TieredStorage { @@ -307,58 +307,6 @@ mod tests { assert!(!tiered_storage_path.try_exists().unwrap()); } - /// Create a test account based on the specified seed. - fn create_account(seed: u64) -> (StoredMeta, AccountSharedData) { - let data_byte = seed as u8; - let account = Account { - lamports: seed, - data: std::iter::repeat(data_byte).take(seed as usize).collect(), - owner: Pubkey::new_unique(), - executable: seed % 2 > 0, - rent_epoch: if seed % 3 > 0 { - seed - } else { - RENT_EXEMPT_RENT_EPOCH - }, - }; - - let stored_meta = StoredMeta { - write_version_obsolete: StoredMetaWriteVersion::default(), - pubkey: Pubkey::new_unique(), - data_len: seed, - }; - (stored_meta, AccountSharedData::from(account)) - } - - fn verify_account( - stored_meta: &StoredAccountMeta<'_>, - account: Option<&impl ReadableAccount>, - account_hash: &AccountHash, - ) { - let (lamports, owner, data, executable, account_hash) = account - .map(|acc| { - ( - acc.lamports(), - acc.owner(), - acc.data(), - acc.executable(), - // only persist rent_epoch for those rent-paying accounts - Some(*account_hash), - ) - }) - .unwrap_or((0, &OWNER_NO_OWNER, &[], false, None)); - - assert_eq!(stored_meta.lamports(), lamports); - assert_eq!(stored_meta.data().len(), data.len()); - assert_eq!(stored_meta.data(), data); - assert_eq!(stored_meta.executable(), executable); - assert_eq!(stored_meta.owner(), owner); - assert_eq!( - *stored_meta.hash(), - account_hash.unwrap_or(AccountHash(Hash::default())) - ); - } - /// The helper function for all write_accounts tests. /// Currently only supports hot accounts. fn do_test_write_accounts( @@ -368,7 +316,7 @@ mod tests { ) { let accounts: Vec<_> = account_data_sizes .iter() - .map(|size| create_account(*size)) + .map(|size| create_test_account(*size)) .collect(); let account_refs: Vec<_> = accounts @@ -412,7 +360,7 @@ mod tests { let mut verified_accounts = HashSet::new(); while let Some((stored_meta, next)) = reader.get_account(index_offset).unwrap() { if let Some((account, account_hash)) = expected_accounts_map.get(stored_meta.pubkey()) { - verify_account(&stored_meta, *account, account_hash); + verify_test_account(&stored_meta, *account, stored_meta.pubkey(), account_hash); verified_accounts.insert(stored_meta.pubkey()); } index_offset = next; diff --git a/accounts-db/src/tiered_storage/byte_block.rs b/accounts-db/src/tiered_storage/byte_block.rs index d4672ac8c58d18..1cd80add0c2307 100644 --- a/accounts-db/src/tiered_storage/byte_block.rs +++ b/accounts-db/src/tiered_storage/byte_block.rs @@ -85,7 +85,7 @@ impl ByteBlockWriter { /// Write all the Some fields of the specified AccountMetaOptionalFields. /// - /// Note that the existance of each optional field is stored separately in + /// Note that the existence of each optional field is stored separately in /// AccountMetaFlags. pub fn write_optional_fields( &mut self, diff --git a/accounts-db/src/tiered_storage/footer.rs b/accounts-db/src/tiered_storage/footer.rs index f7526dc5d8008b..1eb4fbdb3ff2ec 100644 --- a/accounts-db/src/tiered_storage/footer.rs +++ b/accounts-db/src/tiered_storage/footer.rs @@ -33,6 +33,9 @@ pub const FOOTER_MAGIC_NUMBER: u64 = 0x502A2AB5; // SOLALABS -> SOLANA LABS #[repr(C)] pub struct TieredStorageMagicNumber(pub u64); +// Ensure there are no implicit padding bytes +const _: () = assert!(std::mem::size_of::() == 8); + impl Default for TieredStorageMagicNumber { fn default() -> Self { Self(FOOTER_MAGIC_NUMBER) @@ -123,12 +126,12 @@ pub struct TieredStorageFooter { /// A hash that represents a tiered accounts file for consistency check. pub hash: Hash, + /// The format version of the tiered accounts file. + pub format_version: u64, // The below fields belong to footer tail. // The sum of their sizes should match FOOTER_TAIL_SIZE. /// The size of the footer including the magic number. pub footer_size: u64, - /// The format version of the tiered accounts file. - pub format_version: u64, // This field is persisted in the storage but not in this struct. // The number should match FOOTER_MAGIC_NUMBER. // pub magic_number: u64, @@ -153,8 +156,8 @@ const _: () = assert!( + std::mem::size_of::() // min_account_address + std::mem::size_of::() // max_account_address + std::mem::size_of::() // hash - + std::mem::size_of::() // footer_size - + std::mem::size_of::(), // format_version + + std::mem::size_of::() // format_version + + std::mem::size_of::(), // footer_size "TieredStorageFooter cannot have any padding" ); @@ -175,8 +178,8 @@ impl Default for TieredStorageFooter { hash: Hash::new_unique(), min_account_address: Pubkey::default(), max_account_address: Pubkey::default(), - footer_size: FOOTER_SIZE as u64, format_version: FOOTER_FORMAT_VERSION, + footer_size: FOOTER_SIZE as u64, } } } @@ -347,8 +350,8 @@ mod tests { hash: Hash::new_unique(), min_account_address: Pubkey::default(), max_account_address: Pubkey::new_unique(), - footer_size: FOOTER_SIZE as u64, format_version: FOOTER_FORMAT_VERSION, + footer_size: FOOTER_SIZE as u64, }; // Persist the expected footer. @@ -384,8 +387,8 @@ mod tests { assert_eq!(offset_of!(TieredStorageFooter, min_account_address), 0x30); assert_eq!(offset_of!(TieredStorageFooter, max_account_address), 0x50); assert_eq!(offset_of!(TieredStorageFooter, hash), 0x70); - assert_eq!(offset_of!(TieredStorageFooter, footer_size), 0x90); - assert_eq!(offset_of!(TieredStorageFooter, format_version), 0x98); + assert_eq!(offset_of!(TieredStorageFooter, format_version), 0x90); + assert_eq!(offset_of!(TieredStorageFooter, footer_size), 0x98); } #[test] diff --git a/accounts-db/src/tiered_storage/hot.rs b/accounts-db/src/tiered_storage/hot.rs index 0f20ba865ae59b..c88690cd2853dc 100644 --- a/accounts-db/src/tiered_storage/hot.rs +++ b/accounts-db/src/tiered_storage/hot.rs @@ -1,4 +1,3 @@ -#![allow(dead_code)] //! The account meta and related structs for hot accounts. use { @@ -23,7 +22,9 @@ use { bytemuck::{Pod, Zeroable}, memmap2::{Mmap, MmapOptions}, modular_bitfield::prelude::*, - solana_sdk::{account::ReadableAccount, pubkey::Pubkey, stake_history::Epoch}, + solana_sdk::{ + account::ReadableAccount, pubkey::Pubkey, stake_history::Epoch, + }, std::{borrow::Borrow, fs::OpenOptions, option::Option, path::Path}, }; @@ -96,11 +97,17 @@ struct HotMetaPackedFields { owner_offset: B29, } +// Ensure there are no implicit padding bytes +const _: () = assert!(std::mem::size_of::() == 4); + /// The offset to access a hot account. #[repr(C)] #[derive(Debug, Default, Copy, Clone, Eq, PartialEq, Pod, Zeroable)] pub struct HotAccountOffset(u32); +// Ensure there are no implicit padding bytes +const _: () = assert!(std::mem::size_of::() == 4); + impl AccountOffset for HotAccountOffset {} impl HotAccountOffset { @@ -143,6 +150,9 @@ pub struct HotAccountMeta { flags: AccountMetaFlags, } +// Ensure there are no implicit padding bytes +const _: () = assert!(std::mem::size_of::() == 8 + 4 + 4); + impl TieredAccountMeta for HotAccountMeta { /// Construct a HotAccountMeta instance. fn new() -> Self { @@ -647,26 +657,21 @@ impl HotStorageWriter { pub mod tests { use { super::*, - crate::{ - account_storage::meta::StoredMeta, - tiered_storage::{ - byte_block::ByteBlockWriter, - file::TieredStorageFile, - footer::{AccountBlockFormat, AccountMetaFormat, TieredStorageFooter, FOOTER_SIZE}, - hot::{HotAccountMeta, HotStorageReader}, - index::{AccountIndexWriterEntry, IndexBlockFormat, IndexOffset}, - meta::{AccountMetaFlags, AccountMetaOptionalFields, TieredAccountMeta}, - owners::{OwnersBlockFormat, OwnersTable}, - }, + crate::tiered_storage::{ + byte_block::ByteBlockWriter, + file::TieredStorageFile, + footer::{AccountBlockFormat, AccountMetaFormat, TieredStorageFooter, FOOTER_SIZE}, + hot::{HotAccountMeta, HotStorageReader}, + index::{AccountIndexWriterEntry, IndexBlockFormat, IndexOffset}, + meta::{AccountMetaFlags, AccountMetaOptionalFields, TieredAccountMeta}, + owners::{OwnersBlockFormat, OwnersTable}, + test_utils::{create_test_account, verify_test_account}, }, assert_matches::assert_matches, memoffset::offset_of, rand::{seq::SliceRandom, Rng}, solana_sdk::{ - account::{Account, AccountSharedData, ReadableAccount}, - hash::Hash, - pubkey::Pubkey, - slot_history::Slot, + account::ReadableAccount, hash::Hash, pubkey::Pubkey, slot_history::Slot, stake_history::Epoch, }, tempfile::TempDir, @@ -1278,67 +1283,6 @@ pub mod tests { assert_matches!(HotStorageWriter::new(&path), Err(_)); } - /// Create a test account based on the specified seed. - /// The created test account might have default rent_epoch - /// and write_version. - /// - /// When the seed is zero, then a zero-lamport test account will be - /// created. - fn create_test_account(seed: u64) -> (StoredMeta, AccountSharedData) { - let data_byte = seed as u8; - let owner_byte = u8::MAX - data_byte; - let account = Account { - lamports: seed, - data: std::iter::repeat(data_byte).take(seed as usize).collect(), - // this will allow some test account sharing the same owner. - owner: [owner_byte; 32].into(), - executable: seed % 2 > 0, - rent_epoch: if seed % 3 > 0 { - seed - } else { - RENT_EXEMPT_RENT_EPOCH - }, - }; - - let stored_meta = StoredMeta { - write_version_obsolete: u64::MAX, - pubkey: Pubkey::new_unique(), - data_len: seed, - }; - (stored_meta, AccountSharedData::from(account)) - } - - fn verify_account( - stored_meta: &StoredAccountMeta<'_>, - account: Option<&impl ReadableAccount>, - address: &Pubkey, - account_hash: &AccountHash, - ) { - let (lamports, owner, data, executable, account_hash) = account - .map(|acc| { - ( - acc.lamports(), - acc.owner(), - acc.data(), - acc.executable(), - // only persist rent_epoch for those rent-paying accounts - Some(*account_hash), - ) - }) - .unwrap_or((0, &OWNER_NO_OWNER, &[], false, None)); - - assert_eq!(stored_meta.lamports(), lamports); - assert_eq!(stored_meta.data().len(), data.len()); - assert_eq!(stored_meta.data(), data); - assert_eq!(stored_meta.executable(), executable); - assert_eq!(stored_meta.owner(), owner); - assert_eq!(stored_meta.pubkey(), address); - assert_eq!( - *stored_meta.hash(), - account_hash.unwrap_or(AccountHash(Hash::default())) - ); - } - #[test] fn test_write_account_and_index_blocks() { let account_data_sizes = &[ @@ -1391,7 +1335,7 @@ pub mod tests { .unwrap(); let (account, address, account_hash, _write_version) = storable_accounts.get(i); - verify_account(&stored_meta, account, address, account_hash); + verify_test_account(&stored_meta, account, address, account_hash); assert_eq!(i + 1, next.0 as usize); } @@ -1410,7 +1354,7 @@ pub mod tests { let (account, address, account_hash, _write_version) = storable_accounts.get(stored_info.offset); - verify_account(&stored_meta, account, address, account_hash); + verify_test_account(&stored_meta, account, address, account_hash); } // verify get_accounts @@ -1419,7 +1363,7 @@ pub mod tests { // first, we verify everything for (i, stored_meta) in accounts.iter().enumerate() { let (account, address, account_hash, _write_version) = storable_accounts.get(i); - verify_account(stored_meta, account, address, account_hash); + verify_test_account(stored_meta, account, address, account_hash); } // second, we verify various initial position diff --git a/accounts-db/src/tiered_storage/index.rs b/accounts-db/src/tiered_storage/index.rs index 907fabf08ea74f..c82e65ce6d275a 100644 --- a/accounts-db/src/tiered_storage/index.rs +++ b/accounts-db/src/tiered_storage/index.rs @@ -23,9 +23,13 @@ pub trait AccountOffset: Clone + Copy + Pod + Zeroable {} /// The offset to an account/address entry in the accounts index block. /// This can be used to obtain the AccountOffset and address by looking through /// the accounts index block. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +#[repr(C)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, Pod, Zeroable)] pub struct IndexOffset(pub u32); +// Ensure there are no implicit padding bytes +const _: () = assert!(std::mem::size_of::() == 4); + /// The index format of a tiered accounts file. #[repr(u16)] #[derive( @@ -47,6 +51,9 @@ pub enum IndexBlockFormat { AddressesThenOffsets = 0, } +// Ensure there are no implicit padding bytes +const _: () = assert!(std::mem::size_of::() == 2); + impl IndexBlockFormat { /// Persists the specified index_entries to the specified file and returns /// the total number of bytes written. @@ -97,7 +104,7 @@ impl IndexBlockFormat { } /// Returns the offset to the account given the specified index. - pub fn get_account_offset( + pub fn get_account_offset( &self, mmap: &Mmap, footer: &TieredStorageFooter, diff --git a/accounts-db/src/tiered_storage/mmap_utils.rs b/accounts-db/src/tiered_storage/mmap_utils.rs index 444e0fe9ea0d4d..610384efd271c4 100644 --- a/accounts-db/src/tiered_storage/mmap_utils.rs +++ b/accounts-db/src/tiered_storage/mmap_utils.rs @@ -36,21 +36,21 @@ pub unsafe fn get_type(mmap: &Mmap, offset: usize) -> IoResult<(&T, usize)> { /// doesn't overrun the internal buffer. Otherwise return an Error. /// Also return the offset of the first byte after the requested data that /// falls on a 64-byte boundary. -pub fn get_slice(map: &Mmap, offset: usize, size: usize) -> IoResult<(&[u8], usize)> { +pub fn get_slice(mmap: &Mmap, offset: usize, size: usize) -> IoResult<(&[u8], usize)> { let (next, overflow) = offset.overflowing_add(size); - if overflow || next > map.len() { + if overflow || next > mmap.len() { error!( "Requested offset {} and size {} while mmap only has length {}", offset, size, - map.len() + mmap.len() ); return Err(std::io::Error::new( std::io::ErrorKind::AddrNotAvailable, "Requested offset and data length exceeds the mmap slice", )); } - let data = &map[offset..next]; + let data = &mmap[offset..next]; let next = u64_align!(next); let ptr = data.as_ptr(); diff --git a/accounts-db/src/tiered_storage/test_utils.rs b/accounts-db/src/tiered_storage/test_utils.rs new file mode 100644 index 00000000000000..8b6860237ba085 --- /dev/null +++ b/accounts-db/src/tiered_storage/test_utils.rs @@ -0,0 +1,76 @@ +#![cfg(test)] +//! Helper functions for TieredStorage tests +use { + crate::{ + account_storage::meta::{StoredAccountMeta, StoredMeta}, + accounts_hash::AccountHash, + rent_collector::RENT_EXEMPT_RENT_EPOCH, + tiered_storage::owners::OWNER_NO_OWNER, + }, + solana_sdk::{ + account::{Account, AccountSharedData, ReadableAccount}, + hash::Hash, + pubkey::Pubkey, + }, +}; + +/// Create a test account based on the specified seed. +/// The created test account might have default rent_epoch +/// and write_version. +/// +/// When the seed is zero, then a zero-lamport test account will be +/// created. +pub(super) fn create_test_account(seed: u64) -> (StoredMeta, AccountSharedData) { + let data_byte = seed as u8; + let owner_byte = u8::MAX - data_byte; + let account = Account { + lamports: seed, + data: std::iter::repeat(data_byte).take(seed as usize).collect(), + // this will allow some test account sharing the same owner. + owner: [owner_byte; 32].into(), + executable: seed % 2 > 0, + rent_epoch: if seed % 3 > 0 { + seed + } else { + RENT_EXEMPT_RENT_EPOCH + }, + }; + + let stored_meta = StoredMeta { + write_version_obsolete: u64::MAX, + pubkey: Pubkey::new_unique(), + data_len: seed, + }; + (stored_meta, AccountSharedData::from(account)) +} + +pub(super) fn verify_test_account( + stored_meta: &StoredAccountMeta<'_>, + account: Option<&impl ReadableAccount>, + address: &Pubkey, + account_hash: &AccountHash, +) { + let (lamports, owner, data, executable, account_hash) = account + .map(|acc| { + ( + acc.lamports(), + acc.owner(), + acc.data(), + acc.executable(), + // only persist rent_epoch for those rent-paying accounts + Some(*account_hash), + ) + }) + .unwrap_or((0, &OWNER_NO_OWNER, &[], false, None)); + + assert_eq!(stored_meta.lamports(), lamports); + assert_eq!(stored_meta.data().len(), data.len()); + assert_eq!(stored_meta.data(), data); + assert_eq!(stored_meta.executable(), executable); + assert_eq!(stored_meta.owner(), owner); + assert_eq!(stored_meta.pubkey(), address); + assert_eq!( + *stored_meta.hash(), + account_hash.unwrap_or(AccountHash(Hash::default())) + ); +}