Skip to content
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: improve integration test framework for SIMD83 #3181

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

2501babe
Copy link
Member

Problem

the first simd83 pr draft (#3146) has a nearly 2500 line diff. since the new tests im adding for transaction batches with reader-writer and writer-writer account overlap require some changes and additions to the svm integration test framework, im pulling as much as i can out into a separate pr to go in first

Summary of Changes

  • add a new example program, write-to-account, which can read, write, deallocate, and reallocate an account
  • add convenience helpers for this new program to the integration test file
  • add a test confirming that nonce fee-payers must be rent exempt, to complement upcoming tests for what happens when a regular rent-paying fee-payer is deallocated as a side effect of transaction execution
  • add compute budget and program size helper, for upcoming tests for what happens when an account is reallocated mid-batch
  • improve some ergonomics, eg not advancing expected nonce state for discarded transactions, and checking transaction execution status before account states to aid test-driven development

@2501babe 2501babe self-assigned this Oct 15, 2024
@2501babe 2501babe marked this pull request as ready for review October 15, 2024 18:53
@2501babe 2501babe requested a review from apfitzge October 15, 2024 18:53
let target_account_info = next_account_info(accounts_iter)?;
match data[0] {
// print account size
0 => {

Choose a reason for hiding this comment

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

why don't we use enum here with meaningful names for variants?

Copy link
Member Author

Choose a reason for hiding this comment

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

like the other example programs that already exist, i just wanted to do the simplest thing possible. theyre only used for one set of tests and arent expected to change, and are trivial 20-40 line single-file programs that arent part of the toplevel monorepo Cargo.toml workspace and arent imported by anything. i have helpers in svm/tests/integration_test.rs tho

}
// reallocate account
3 => {
let new_size = usize::from_le_bytes(data[1..9].try_into().unwrap());
Copy link

@KirillLykov KirillLykov Oct 16, 2024

Choose a reason for hiding this comment

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

Do you avoid using borsh to avoid associated deserialization cost?

Copy link
Member Author

Choose a reason for hiding this comment

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

cost isnt a concern, as above its just doing the simplest thing possible

match data[0] {
// print account size
0 => {
msg!(

Choose a reason for hiding this comment

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

Do you print here for debug purpose or to imitate reading the data without writing back? In case of the second, I believe msg cost is not negligible and instead I would call std::ptr::read_volatile which prevents compiler to optimize unused code (which doesn't happen with our compiler anyways).

Copy link
Member Author

Choose a reason for hiding this comment

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

i use this to check which specific transactions cause accounts to be deallocated or reallocated in a multi-transaction batch. svm integration tests can directly read transaction logs off the execution result

fn process_instruction(
_program_id: &Pubkey,
accounts: &[AccountInfo],
data: &[u8],

Choose a reason for hiding this comment

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

Does data contains some random number to prevent duplicate txs? This is not the only way but the simplest

Choose a reason for hiding this comment

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

no check on length - if need that functionality easy to just append random bytes at the end; program has no need to read them

Copy link
Member Author

Choose a reason for hiding this comment

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

no, duplicates arent a concern because all the svm integration tests are structured like "here is a specific list of transactions to execute, heres what the execution results, final account states, and log messages should be"

Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

Changes look fine to me; there seem to be a few unrelated changes here which made reviewing it a bit confusing tbh, but not going to block on it.

Commented a few questions

Choose a reason for hiding this comment

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

never a fan of adding binary objects to the repo, but it seems this is the common practice for svm tests

svm/tests/mock_bank.rs Show resolved Hide resolved
fn process_instruction(
_program_id: &Pubkey,
accounts: &[AccountInfo],
data: &[u8],

Choose a reason for hiding this comment

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

no check on length - if need that functionality easy to just append random bytes at the end; program has no need to read them

@@ -682,8 +706,12 @@ fn simple_nonce(enable_fee_only_transactions: bool, fee_paying_nonce: bool) -> V
let mut fee_payer_data = AccountSharedData::default();
fee_payer_data.set_lamports(LAMPORTS_PER_SOL);
test_entry.add_initial_account(fee_payer, &fee_payer_data);
} else if rent_paying_nonce {
assert!(fee_paying_nonce);

Choose a reason for hiding this comment

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

not sure I understand the connection between fee paying and rent paying for the nonce here.

Copy link
Member Author

Choose a reason for hiding this comment

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

a nonce account used as a transaction fee-payer that would be brought one lamport below rent exemption if it was allowed to pay for the transaction

Copy link
Member Author

@2501babe 2501babe Oct 16, 2024

Choose a reason for hiding this comment

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

oh you mean the assert, its just to make sure the annoyingly complex lambda wasnt misused. if you wanted a rent-paying non-fee-paying nonce you wouldnt add LAMPORTS_PER_SIGNATURE, or else youd still have a rent-exempt nonce, and might write a test that doesnt test what it means to test. but no tests need an account like that so i didnt add a branch for it

Comment on lines +894 to +895
// 5: rent-paying nonce fee-payers are also not charged for fee-only transactions
if enable_fee_only_transactions && fee_paying_nonce {

Choose a reason for hiding this comment

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

this seems exactly the same as above, or am I missing something?

Copy link
Member Author

@2501babe 2501babe Oct 16, 2024

Choose a reason for hiding this comment

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

maybe im overly paranoid but case 4 uses a real program id, so account loading can succeed, but the transaction is discarded because charging for it would make the valid nonce fee-payer become invalid. case 5 uses a fake program id, which aborts account loading, so we test that with fee-only transactions enabled there isnt any kind of short-circuit code path that would cause it to pay fees

i wanted to be very thorough about this because there are a number of edge cases to cover if a) a regular fee-payer has to pay rent, and b) nonce accounts are mutated in-batch, so we want to be absolutely sure the overlap of those two things is impossible

Choose a reason for hiding this comment

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

thanks. I just didn't notice the difference in 2nd arge of mk_nonce_transaction

@2501babe
Copy link
Member Author

sorry this is a bit confusing out of context, the goal is purely to make reviewing #3146 easier (since it is extremely large) by a) reducing it by a few hundred loc, and b) making its diff of integration_test.rs purely adding more tests, with no changes to the code that runs tests

@2501babe 2501babe force-pushed the 20241015_svminteg_imprv branch from dbd9cf1 to ae63530 Compare October 17, 2024 18:07
Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

lgtm

@2501babe 2501babe added the automerge automerge Merge this Pull Request automatically once CI passes label Oct 17, 2024
@mergify mergify bot merged commit b032f78 into anza-xyz:master Oct 17, 2024
52 checks passed
@2501babe 2501babe deleted the 20241015_svminteg_imprv branch October 17, 2024 20:54
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
* svm: improve integration test framework for SIMD83

* merge write program match expressions

* clarify nonce comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants