Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

API to match account owner against a set of owners #30154

Merged
merged 6 commits into from
Feb 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 156 additions & 3 deletions runtime/src/accounts_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ use {
get_ancient_append_vec_capacity, is_ancient, AccountsToStore, StorageSelector,
},
append_vec::{
aligned_stored_size, AppendVec, StorableAccountsWithHashesAndWriteVersions,
StoredAccountMeta, StoredMetaWriteVersion, APPEND_VEC_MMAPPED_FILES_OPEN,
STORE_META_OVERHEAD,
aligned_stored_size, AppendVec, MatchAccountOwnerError,
StorableAccountsWithHashesAndWriteVersions, StoredAccountMeta, StoredMetaWriteVersion,
APPEND_VEC_MMAPPED_FILES_OPEN, STORE_META_OVERHEAD,
},
cache_hash_data::{CacheHashData, CacheHashDataFile},
contains::Contains,
Expand Down Expand Up @@ -815,6 +815,32 @@ impl<'a> LoadedAccountAccessor<'a> {
}
}
}

fn account_matches_owners(&self, owners: &[&Pubkey]) -> Result<(), MatchAccountOwnerError> {
match self {
LoadedAccountAccessor::Cached(cached_account) => cached_account
.as_ref()
.and_then(|cached_account| {
(!cached_account.account.is_zero_lamport()
&& owners.contains(&cached_account.account.owner()))
.then_some(())
})
.ok_or(MatchAccountOwnerError::NoMatch),
LoadedAccountAccessor::Stored(maybe_storage_entry) => {
// storage entry may not be present if slot was cleaned up in
// between reading the accounts index and calling this function to
// get account meta from the storage entry here
maybe_storage_entry
.as_ref()
.map(|(storage_entry, offset)| {
storage_entry
.accounts
.account_matches_owners(*offset, owners)
})
.unwrap_or(Err(MatchAccountOwnerError::UnableToLoad))
}
}
}
}

pub enum LoadedAccount<'a> {
Expand Down Expand Up @@ -4918,6 +4944,38 @@ impl AccountsDb {
self.do_load(ancestors, pubkey, None, load_hint, LoadZeroLamports::None)
}

pub fn account_matches_owners(
&self,
ancestors: &Ancestors,
account: &Pubkey,
owners: &[&Pubkey],
) -> Result<(), MatchAccountOwnerError> {
let (slot, storage_location, _maybe_account_accesor) = self
.read_index_for_accessor_or_load_slow(ancestors, account, None, false)
.ok_or(MatchAccountOwnerError::UnableToLoad)?;

if !storage_location.is_cached() {
let result = self.read_only_accounts_cache.load(*account, slot);
if let Some(account) = result {
return (!account.is_zero_lamport() && owners.contains(&account.owner()))
.then_some(())
.ok_or(MatchAccountOwnerError::NoMatch);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this section of code was repurposed from here:

let (slot, storage_location, _maybe_account_accesor) =
self.read_index_for_accessor_or_load_slow(ancestors, pubkey, max_root, false)?;
// Notice the subtle `?` at previous line, we bail out pretty early if missing.
let in_write_cache = storage_location.is_cached();
if !load_into_read_cache_only {
if !in_write_cache {
let result = self.read_only_accounts_cache.load(*pubkey, slot);
if let Some(account) = result {
if matches!(load_zero_lamports, LoadZeroLamports::None)
&& account.is_zero_lamport()
{
return None;
}
return Some((account, slot));
}
}
.

@jeffwashington is it important to also make the zero lamports check https://github.com/solana-labs/solana/blob/c7fb4b10401a10c21f27a2ccf47176ccb659b646/runtime/src/accounts_db.rs#L5350C5-L5351 in order to be consistent? I.e. right now when we look up accounts with zero lamports it returns None, what should be done here in those cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with a 0 lamport account, and the owner gets set to SystemProgram. I don't think the check is useful for our usecase (filtering executables/programs). If some other usecase tries to filter SystemProgram accounts, 0 lamport account will show up there. Not sure if that's a problem though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the answer will be wrong if the account is zero lamport and you did look for system program owner. The result should be 'false' for zero lamport account to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, missed this comment. I'll push another update.

let (account_accessor, _slot) = self
.retry_to_get_account_accessor(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't look like zero lamport is handled for the case where we load from the append vec. I think you need this same type of code here:

!account.is_zero_lamport() && owners.contains(&account.owner()))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The append_vec.rs makes the check and returns the error for 0 lamports.
The append_vec API is not returning the account meta, otherwise we could've consolidated this check in accounts_db.rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my apologies. I was looking at the CURRENT master code to see that we weren't doing anything special on zero lamport accounts...

slot,
storage_location,
ancestors,
account,
None,
LoadHint::Unspecified,
)
.ok_or(MatchAccountOwnerError::UnableToLoad)?;
account_accessor.account_matches_owners(owners)
}

pub fn load_account_into_read_cache(&self, ancestors: &Ancestors, pubkey: &Pubkey) {
self.do_load_with_populate_read_cache(
ancestors,
Expand Down Expand Up @@ -14068,6 +14126,101 @@ pub mod tests {
assert_eq!(db.read_only_accounts_cache.cache_len(), 1);
}

#[test]
fn test_account_matches_owners() {
let db = Arc::new(AccountsDb::new_with_config_for_tests(
Vec::new(),
&ClusterType::Development,
AccountSecondaryIndexes::default(),
AccountShrinkThreshold::default(),
));

let owners: Vec<Pubkey> = (0..2).map(|_| Pubkey::new_unique()).collect();
let owners_refs: Vec<&Pubkey> = owners.iter().collect();

let account1_key = Pubkey::new_unique();
let account1 = AccountSharedData::new(321, 10, &owners[0]);

let account2_key = Pubkey::new_unique();
let account2 = AccountSharedData::new(1, 1, &owners[1]);

let account3_key = Pubkey::new_unique();
let account3 = AccountSharedData::new(1, 1, &Pubkey::new_unique());

// Account with 0 lamports
let account4_key = Pubkey::new_unique();
let account4 = AccountSharedData::new(0, 1, &owners[1]);

db.store_cached((0, &[(&account1_key, &account1)][..]), None);
db.store_cached((1, &[(&account2_key, &account2)][..]), None);
db.store_cached((2, &[(&account3_key, &account3)][..]), None);
db.store_cached((3, &[(&account4_key, &account4)][..]), None);

db.add_root(0);
db.add_root(1);
db.add_root(2);
db.add_root(3);

// Flush the cache so that the account meta will be read from the storage
db.flush_accounts_cache(true, None);
db.clean_accounts_for_tests();

assert_eq!(
db.account_matches_owners(&Ancestors::default(), &account1_key, &owners_refs),
Ok(())
);
assert_eq!(
db.account_matches_owners(&Ancestors::default(), &account2_key, &owners_refs),
Ok(())
);
assert_eq!(
db.account_matches_owners(&Ancestors::default(), &account3_key, &owners_refs),
Err(MatchAccountOwnerError::NoMatch)
);
assert_eq!(
db.account_matches_owners(&Ancestors::default(), &account4_key, &owners_refs),
Err(MatchAccountOwnerError::NoMatch)
);
assert_eq!(
db.account_matches_owners(&Ancestors::default(), &Pubkey::new_unique(), &owners_refs),
Err(MatchAccountOwnerError::UnableToLoad)
);

// Flush the cache and load account1 (so that it's in the cache)
db.flush_accounts_cache(true, None);
db.clean_accounts_for_tests();
let _ = db
.do_load(
&Ancestors::default(),
&account1_key,
Some(0),
LoadHint::Unspecified,
LoadZeroLamports::SomeWithZeroLamportAccountForTests,
)
.unwrap();

assert_eq!(
db.account_matches_owners(&Ancestors::default(), &account1_key, &owners_refs),
Ok(())
);
assert_eq!(
db.account_matches_owners(&Ancestors::default(), &account2_key, &owners_refs),
Ok(())
);
assert_eq!(
db.account_matches_owners(&Ancestors::default(), &account3_key, &owners_refs),
Err(MatchAccountOwnerError::NoMatch)
);
assert_eq!(
db.account_matches_owners(&Ancestors::default(), &account4_key, &owners_refs),
Err(MatchAccountOwnerError::NoMatch)
);
assert_eq!(
db.account_matches_owners(&Ancestors::default(), &Pubkey::new_unique(), &owners_refs),
Err(MatchAccountOwnerError::UnableToLoad)
);
}

/// a test that will accept either answer
const LOAD_ZERO_LAMPORTS_ANY_TESTS: LoadZeroLamports = LoadZeroLamports::None;

Expand Down
77 changes: 76 additions & 1 deletion runtime/src/append_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ use {
Mutex,
},
},
thiserror::Error,
};

pub mod test_utils;
Expand Down Expand Up @@ -270,6 +271,14 @@ impl<'a> Iterator for AppendVecAccountsIter<'a> {
}
}

#[derive(Error, Debug, PartialEq, Eq)]
pub enum MatchAccountOwnerError {
#[error("The account owner does not match with the provided list")]
NoMatch,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this.
I was thinking though Result<bool, MatchAccountOwnerError>
Where bool was matched true/false
And error was only for the frustrating UnableToLoad case. Either way is ok with me and could be tweaked later. Making whatever you can pub(crate) would be great.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but to be clear, either way is ok. This is like an enum, which is the most clear, imho.

#[error("Unable to load the account")]
UnableToLoad,
}

/// A thread-safe, file-backed block of memory used to store `Account` instances. Append operations
/// are serialized such that only one thread updates the internal `append_lock` at a time. No
/// restrictions are placed on reading. That is, one may read items from one thread while another
Expand Down Expand Up @@ -572,7 +581,7 @@ impl AppendVec {
Some((unsafe { &*ptr }, next))
}

/// Return account metadata for the account at `offset` if its data doesn't overrun
/// Return stored account metadata for the account at `offset` if its data doesn't overrun
/// the internal buffer. Otherwise return None. Also return the offset of the first byte
/// after the requested data that falls on a 64-byte boundary.
pub fn get_account<'a>(&'a self, offset: usize) -> Option<(StoredAccountMeta<'a>, usize)> {
Expand All @@ -594,6 +603,31 @@ impl AppendVec {
))
}

fn get_account_meta<'a>(&self, offset: usize) -> Option<&'a AccountMeta> {
// Skip over StoredMeta data in the account
let offset = offset.checked_add(mem::size_of::<StoredMeta>())?;
// u64_align! does an unchecked add for alignment. Check that it won't cause an overflow.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a danger in the current callers of u64_align right now like

let next = u64_align!(next);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note it might be good to extract all of these "align and offset" calculation functions into a common utility that can be reused in places like this:

let (next, overflow) = offset.overflowing_add(size);
if overflow || next > self.len() {
return None;
}
let data = &self.map[offset..next];
let next = u64_align!(next);
into a common utility so that we can reuse them across these functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a danger in the current callers of u64_align right now like

let next = u64_align!(next);

Yes. In my test, if next is usize::MAX, it'll panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note it might be good to extract all of these "align and offset" calculation functions into a common utility that can be reused in places like this:

let (next, overflow) = offset.overflowing_add(size);
if overflow || next > self.len() {
return None;
}
let data = &self.map[offset..next];
let next = u64_align!(next);

into a common utility so that we can reuse them across these functions

Yes, that will be ideal. Separate PR though?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote for separate pr. @alessandrod is good at getting this right. And @brooksprumo

offset.checked_add(ALIGN_BOUNDARY_OFFSET - 1)?;
let (account_meta, _): (&AccountMeta, _) = self.get_type(u64_align!(offset))?;
Some(account_meta)
}

/// Return Some(true) if the account owner at `offset` is one of the pubkeys in `owners`.
/// Return Some(false) if the account owner is not one of the pubkeys in `owners`.
/// It returns None if the `offset` value causes a data overrun.
pub fn account_matches_owners(
&self,
offset: usize,
owners: &[&Pubkey],
) -> Result<(), MatchAccountOwnerError> {
let account_meta = self
.get_account_meta(offset)
.ok_or(MatchAccountOwnerError::UnableToLoad)?;
(account_meta.lamports != 0 && owners.contains(&&account_meta.owner))
.then_some(())
.ok_or(MatchAccountOwnerError::NoMatch)
}

#[cfg(test)]
pub fn get_account_test(&self, offset: usize) -> Option<(StoredMeta, AccountSharedData)> {
let (stored_account, _) = self.get_account(offset)?;
Expand Down Expand Up @@ -1047,6 +1081,47 @@ pub mod tests {
assert_eq!(av.get_account_test(index1).unwrap(), account1);
}

#[test]
fn test_account_matches_owners() {
let path = get_append_vec_path("test_append_data");
let av = AppendVec::new(&path.path, true, 1024 * 1024);
let owners: Vec<Pubkey> = (0..2).map(|_| Pubkey::new_unique()).collect();
let owners_refs: Vec<&Pubkey> = owners.iter().collect();

let mut account = create_test_account(5);
account.1.set_owner(owners[0]);
let index = av.append_account_test(&account).unwrap();
assert_eq!(av.account_matches_owners(index, &owners_refs), Ok(()));

let mut account1 = create_test_account(6);
account1.1.set_owner(owners[1]);
let index1 = av.append_account_test(&account1).unwrap();
assert_eq!(av.account_matches_owners(index1, &owners_refs), Ok(()));
assert_eq!(av.account_matches_owners(index, &owners_refs), Ok(()));

let mut account2 = create_test_account(6);
account2.1.set_owner(Pubkey::new_unique());
let index2 = av.append_account_test(&account2).unwrap();
assert_eq!(
av.account_matches_owners(index2, &owners_refs),
Err(MatchAccountOwnerError::NoMatch)
);

// tests for overflow
assert_eq!(
av.account_matches_owners(usize::MAX - mem::size_of::<StoredMeta>(), &owners_refs),
Err(MatchAccountOwnerError::UnableToLoad)
);

assert_eq!(
av.account_matches_owners(
usize::MAX - mem::size_of::<StoredMeta>() - mem::size_of::<AccountMeta>() + 1,
&owners_refs
),
Err(MatchAccountOwnerError::UnableToLoad)
);
}

#[test]
fn test_append_vec_append_many() {
let path = get_append_vec_path("test_append_many");
Expand Down