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

pgarg66 · 2023-02-06T21:48:20Z

Problem

The runtime V2 needs an API to filter executable/program accounts in a transaction, without reading the whole account in the memory. We don't currently have such an API.

An API that can match account's owner with a set of pubkeys (loader pubkeys) will be ideal.

Summary of Changes

Add API to account_matches_owners in append_vec.rs and accounts_db.rs. Also, add unit tests.

Fixes #

jeffwashington · 2023-02-06T23:23:49Z

looks reasonable to me. Not ideal to be copying these fields out (like owner). Depends on usage I guess. I'm not sure what happens in the case where the account is in the read cache.

pgarg66 · 2023-02-06T23:29:45Z

looks reasonable to me. Not ideal to be copying these fields out (like owner). Depends on usage I guess. I'm not sure what happens in the case where the account is in the read cache.

Actually owner is the specific field we need for runtime V2. If the owner is rBPF/SBF loader, we'll treat it as an executable. (The executable flag by itself will be deprecated in the future).

jeffwashington · 2023-02-06T23:33:50Z

Actually owner is the specific field we need for runtime

Depends then on how many times this is called then. Is it just on suspected exes or is it for every account in every tx?
You may want an api that asks, "Does the owner match &Pubkey or any of &[&Pubkey]. That would allow the internals to check the owner as a ref without copying it out so you can check it and usually decide owner is not what you need.

pgarg66 · 2023-02-06T23:45:55Z

Actually owner is the specific field we need for runtime

Depends then on how many times this is called then. Is it just on suspected exes or is it for every account in every tx? You may want an api that asks, "Does the owner match &Pubkey or any of &[&Pubkey]. That would allow the internals to check the owner as a ref without copying it out so you can check it and usually decide owner is not what you need.

Yes that's a good point. I'll update the PR.
At a high level, it looked nice to have an accessor for meta. But it can easily become an overhead.

Thanks for reviewing it!

pgarg66 · 2023-02-07T00:47:38Z

Actually owner is the specific field we need for runtime

Depends then on how many times this is called then. Is it just on suspected exes or is it for every account in every tx? You may want an api that asks, "Does the owner match &Pubkey or any of &[&Pubkey]. That would allow the internals to check the owner as a ref without copying it out so you can check it and usually decide owner is not what you need.

Updated the PR as per your suggestion.

carllin · 2023-02-07T00:43:27Z

runtime/src/append_vec.rs

+    pub fn get_account_meta(&self, offset: usize) -> Option<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.


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

solana/runtime/src/append_vec.rs

Line 516 in 9ed4eac

let next = u64_align!(next);

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:

solana/runtime/src/append_vec.rs

Lines 511 to 516 in 9ed4eac

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

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

solana/runtime/src/append_vec.rs

Line 516 in 9ed4eac

let next = u64_align!(next);

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

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:

solana/runtime/src/append_vec.rs

Lines 511 to 516 in 9ed4eac

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?

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

carllin · 2023-02-07T01:13:16Z

runtime/src/accounts_db.rs

+                return Some(owners.contains(&account.owner()));
+            }
+        }
+


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

solana/runtime/src/accounts_db.rs

Lines 5341 to 5357 in c7fb4b1

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?

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.

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.

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

jeffwashington · 2023-02-07T19:32:52Z

runtime/src/accounts_db.rs

@@ -809,6 +809,26 @@ impl<'a> LoadedAccountAccessor<'a> {
            }
        }
    }
+
+    fn account_matches_owners(&self, owners: &[&Pubkey]) -> Option<bool> {


I think the Option return value is more confusing than it could be. I suggest an enum:
matches, does not match, unable to determine (for overflow case). Or, maybe even a result since unable to determine is an error (which should ideally never happen!?!?)

Would you like a similar change for the append_vec.rs API?
I had modeled this mostly on load() API. And, a lot of internal APIs currently return an Option. I do agree that a Result with specific errors will be cleaner approach.

Changed all the new APIs to return Result

jeffwashington · 2023-02-07T22:54:13Z

runtime/src/append_vec.rs

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


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.

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

jeffwashington

~~lgtm~~
Actually,
#30154 (comment)
is outstanding.

pgarg66 · 2023-02-07T23:27:42Z

lgtm Actually, #30154 (comment) is outstanding.

The latest commit should handle account with 0 lamports.

jeffwashington · 2023-02-08T00:01:02Z

runtime/src/accounts_db.rs

+        }
+
+        let (account_accessor, _slot) = self
+            .retry_to_get_account_accessor(


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()))

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.

https://github.com/solana-labs/solana/pull/30154/files#diff-f369d6a7bff2bb6f2bbbd60cdb9f8255b51a60a7af47b135a9fa592178266294R626

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

jeffwashington

lgtm

pgarg66 force-pushed the account-meta branch from 2f63828 to 9b27d8c Compare February 6, 2023 21:48

pgarg66 requested review from carllin, jeffwashington and Lichtso February 6, 2023 21:50

pgarg66 force-pushed the account-meta branch from 6fbfd76 to 9ed4eac Compare February 6, 2023 22:11

jeffwashington requested a review from yhchiang-sol February 6, 2023 23:22

pgarg66 changed the title ~~API to read account meta data from storage~~ API to match account owner against a set of owners Feb 7, 2023

pgarg66 force-pushed the account-meta branch from deb6861 to c7fb4b1 Compare February 7, 2023 00:48

carllin reviewed Feb 7, 2023

View reviewed changes

Lichtso mentioned this pull request Feb 7, 2023

LoadedPrograms #29803

Closed

pgarg66 force-pushed the account-meta branch from c7fb4b1 to 338e4be Compare February 7, 2023 18:54

pgarg66 marked this pull request as ready for review February 7, 2023 18:54

jeffwashington reviewed Feb 7, 2023

View reviewed changes

pgarg66 added 5 commits February 7, 2023 14:48

API to read account meta data from storage

4807574

cleanup

11cbb2c

fix clippy warnings

cdbd290

Change API to check owners, instead of returning account meta

86926ea

return Result instead of Option

4f178a3

pgarg66 force-pushed the account-meta branch from 338e4be to 4f178a3 Compare February 7, 2023 22:48

pgarg66 requested a review from jeffwashington February 7, 2023 22:49

jeffwashington reviewed Feb 7, 2023

View reviewed changes

jeffwashington self-requested a review February 7, 2023 22:54

jeffwashington approved these changes Feb 7, 2023

View reviewed changes

handle account with 0 lamports

ca0e3b1

pgarg66 force-pushed the account-meta branch from 2b6d48c to ca0e3b1 Compare February 7, 2023 23:27

pgarg66 requested a review from jeffwashington February 7, 2023 23:53

jeffwashington reviewed Feb 8, 2023

View reviewed changes

jeffwashington self-requested a review February 8, 2023 00:08

jeffwashington approved these changes Feb 8, 2023

View reviewed changes

pgarg66 merged commit d0623a3 into solana-labs:master Feb 8, 2023

pgarg66 deleted the account-meta branch February 8, 2023 01:48

Lichtso mentioned this pull request Feb 8, 2023

Program-Runtime v2 - Road Map #28755

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pgarg66 commented Feb 6, 2023 •

edited

Loading

jeffwashington commented Feb 6, 2023

pgarg66 commented Feb 6, 2023

jeffwashington commented Feb 6, 2023

pgarg66 commented Feb 6, 2023

pgarg66 commented Feb 7, 2023

carllin Feb 7, 2023

carllin Feb 7, 2023

pgarg66 Feb 7, 2023

pgarg66 Feb 7, 2023

jeffwashington Feb 7, 2023

carllin Feb 7, 2023

pgarg66 Feb 7, 2023

jeffwashington Feb 7, 2023

pgarg66 Feb 7, 2023

jeffwashington Feb 7, 2023

pgarg66 Feb 7, 2023

pgarg66 Feb 7, 2023

jeffwashington Feb 7, 2023

jeffwashington Feb 7, 2023

jeffwashington left a comment •

edited

Loading

pgarg66 commented Feb 7, 2023

jeffwashington Feb 8, 2023

pgarg66 Feb 8, 2023

pgarg66 Feb 8, 2023

jeffwashington Feb 8, 2023

jeffwashington left a comment

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

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

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

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

Conversation

pgarg66 commented Feb 6, 2023 • edited Loading

Problem

Summary of Changes

jeffwashington commented Feb 6, 2023

pgarg66 commented Feb 6, 2023

jeffwashington commented Feb 6, 2023

pgarg66 commented Feb 6, 2023

pgarg66 commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffwashington left a comment • edited Loading

Choose a reason for hiding this comment

pgarg66 commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffwashington left a comment

Choose a reason for hiding this comment

pgarg66 commented Feb 6, 2023 •

edited

Loading

jeffwashington left a comment •

edited

Loading