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: interleave transaction validation and processing #3404

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

2501babe
Copy link
Member

@2501babe 2501babe commented Oct 31, 2024

Problem

simd83 intends to relax entry-level constraints, namely that a transaction which takes a write lock on an account cannot be batched with any other transaction which takes a read or write lock on it

before the locking code can be changed, however, svm must be able to support such batches. presently, if one transaction were to write to an account, and a subsequent transaction read from or wrote to that account, svm would not pass the updated account state to the second transaction; it gets them from accounts-db, which is not updated until after all transaction execution finishes

previously, in #3146, we attempted to implement a jit account loader with intermediate account cache to pass updated accounts forward to future transaction in the batch. however, due to existing patterns in how the program cache is used, this has proven quite difficult to get right, and the volume of code changes required is quite large

Summary of Changes

this pr extracts all code from #3146 which does not modify behavior. we would like to commit it standalone for ease of review, and focus specifically on the cache behaviors in a following pr

the existing transaction processing loop in master validates all transaction fee-payers together, loads all transaction accounts together, and then executes each transaction in serial. the new transaction processing loop validates one fee-payer, loads accounts for one transaction, and then executes that transaction before proceeding to the next. this prepares us for an svm where accounts can change in between transactions

we also validate nonce accounts before each transaction, because these accounts can also change in the future svm. a future pr may choose to eliminate most nonce validation code from the check_transactions, but that is out of scope here

futhermore, we create a new type, AccountLoader, which encapsulates the batch-local program cache, account overrides, and the accounts-db callback. it provides the method load_account, which is opaque to its caller, returning a LoadedTransactionAccount according to the exact same rules as the current load_transaction_account

this pr changes nothing about account loading, but introducing AccountLoader prepares us to add the internal account cache in support of simd83

@2501babe 2501babe self-assigned this Oct 31, 2024
@2501babe 2501babe force-pushed the 20241030_svmconflicts_attempt5 branch from b5fc0c1 to b351ffb Compare October 31, 2024 06:45
@2501babe
Copy link
Member Author

2501babe commented Nov 1, 2024

the first commit is the two files copied exactly from the previous pr, and the second commit deletes the intermediate account cache. this way you can look at the full changeset, or the difference between what you already reviewed, easily

@2501babe 2501babe marked this pull request as ready for review November 1, 2024 19:22
{
loaded_account.rent_collected = if usage_pattern == AccountUsagePattern::Writable {
Copy link

Choose a reason for hiding this comment

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

This change has the side effect of collecting rent from writable accounts that come from account overrides. Probably fine? I'm not too sure yet

// in the same batch may modify the same accounts. Transaction order is
// preserved within entries written to the ledger.
for (tx, check_result) in sanitized_txs.iter().zip(check_results) {
let (validate_result, single_validate_fees_us) = measure_us!(check_result
Copy link

Choose a reason for hiding this comment

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

nit: can you introduce a new function called validate_transaction which wraps validate_transaction_nonce and validate_transaction_fee_payer? There's a lot of nesting here that makes the code hard to read IMO

fee_payer_account
} else if let Some(fee_payer_account) = callbacks.get_account_shared_data(fee_payer_address)
{
callbacks.inspect_account(
Copy link

Choose a reason for hiding this comment

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

I love that all the inspect_account call sites are inside AccountLoader now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants