-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
svm: test account loader edge cases #3045
base: master
Are you sure you want to change the base?
Conversation
351748c
to
5680496
Compare
5680496
to
124ebfa
Compare
e545d22
to
6018221
Compare
(need to wait for dependabot to upstream a dependency version update for this to pass again) |
these tests are intended to ensure account loader v2 conforms to existing behavior
6018221
to
eb95bfd
Compare
Probably good to get a second opionion on these tests, but overall they look good to me (in that they are testing the current behavior) |
@@ -1391,6 +1393,148 @@ mod tests { | |||
assert_eq!(result.err(), Some(TransactionError::AccountNotFound)); | |||
} | |||
|
|||
#[test] | |||
fn test_load_transaction_accounts_program_account_executable_bypass() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, this is really crazy behavior. Do you mind adding some comments explaining why this happens?
From my current understanding:
- All builtins and any account owned by a loader are added to the program cache before tx account loading
- If a tx account loads an account which is not writable and not used as an instruction account input, we check if it's in the program cache
- If the tx account is in the program cache, then load it as if it's a valid program for execution
- Later if that account is invoked as a program, the bpf loaders will check the actual program cache entry to see if it's a valid program. If not, the tx is "executed" but will return an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats correct. the issue is the current code just checks if something exists in the cache, and then assumes its executable, so it attempts to execute and it fails. it does load the data anyway (so using the cache isnt really an optimization) but just to see if it exists, which i assume was added to fix a bug
} else if let Some(program) = (!is_instruction_account && !is_writable)
.then_some(())
.and_then(|_| loaded_programs.find(account_key))
{
callbacks
.get_account_shared_data(account_key)
.ok_or(TransactionError::AccountNotFound)?;
// Optimization to skip loading of accounts which are only used as
// programs in top-level instructions and not passed as instruction accounts.
LoadedTransactionAccount {
loaded_size: program.account_size,
account: account_shared_data_from_program(&program),
rent_collected: 0,
}
} else {
checking executable
on the account here requires a feature gate because (until fee-only txn are enabled) it would change whether the transaction pays fees and thus affects consensus. but i talked with pankaj the other week and he said that the status on the cache item is sufficient to determine if it is in fact executable or not (this is what i meant in the tombstone comment, ill expand it in next commit). so in the future when we fix this i believe we can just do this
} else if let Some(ref program) = (!is_instruction_account && !is_writable)
.then_some(())
.and_then(|_| program_cache.find(account_key))
.filter(|program| !program.is_tombstone())
{
loaded_accounts_map.insert_cached_program(*account_key, program);
} else if ... {
let transaction = | ||
SanitizedTransaction::from_transaction_for_tests(Transaction::new_signed_with_payer( | ||
&[Instruction::new_with_bytes( | ||
program_keypair.pubkey(), | ||
&[], | ||
vec![], | ||
)], | ||
Some(&account_keypair.pubkey()), | ||
&[&account_keypair], | ||
Hash::default(), | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add separate variants of this test where the program is 1) loaded as writeable and 2) passed as an instruction account to show the difference in behavior?
@@ -2227,4 +2371,261 @@ mod tests { | |||
|
|||
assert_eq!(actual_inspected_accounts, expected_inspected_accounts,); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a test case for loading a tx which invokes native loader directly?
program2_size + programdata2_size + upgradeable_loader_size + fee_payer_size, | ||
); | ||
|
||
// program as instruction account bypasses the cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also have a way to set invoked programs as writable when the upgradeable loader is present because that will also bypass the cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think i understand the point about the upgradeable loader being present but ive added test cases for both data size and executable flag check which use a message with a writable, non-instruction, invoked program
svm/src/account_loader.rs
Outdated
// also we will need to create a fake vm and use ProgramCacheEntryType::Loaded | ||
// since the old loader does not check tombstones, but the new one does |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like process_instruction_inner
is called for both the old and the upgradeable loader and will check that program cache entries are valid. Can you explain more about what you mean that the old loader doesn't check tombstones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i deleted this part of the comment since it is no longer relevant (i previously wrote a batched account loader that checked tombstones with an escape hatch to emulate old behavior, i now have a jit account loader which retains the old behavior unmodified), but sorry this was unclear, i meant the old and new account loaders, not the bpf loaders
spent the rest of the week writing a jit account loader (good news: its diff against master is about 1300 fewer lines than the batch account loader), will get to your suggestions on these tests on monday! |
Problem
the existing account loader has a few unique edge cases:
all of these affect consensus because they may determine whether the transaction is executed. as we are implementing a new loader without a feature gate, we must test that the new loader preserves these behaviors
Summary of Changes
add tests for them. we do this as a separate pr from the new loader so we are sure they pass for the old loader