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.

Sign up for GitHub

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: allow conflicting transactions in entries #3453

Merged
merged 6 commits into from
Nov 20, 2024
Merged

svm: allow conflicting transactions in entries #3453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3916671
svm: allow conflicting transactions in entries
2501babe Nov 4, 2024
cb9ca92
add integration tests back
2501babe Nov 5, 2024
1b8a79c
actually these tests are meaningless now
2501babe Nov 5, 2024
9e8eb4b
fix unit tests
2501babe Nov 9, 2024
96ef843
fix concurrent test
2501babe Nov 14, 2024
755375d
improve writable lookup in integration tests
2501babe Nov 15, 2024
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions programs/sbf/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions svm/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ license = { workspace = true }
edition = { workspace = true }

[dependencies]
ahash = { workspace = true }
itertools = { workspace = true }
log = { workspace = true }
percentage = { workspace = true }
Expand Down
146 changes: 122 additions & 24 deletions svm/src/account_loader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ use {
nonce_info::NonceInfo,
rollback_accounts::RollbackAccounts,
transaction_error_metrics::TransactionErrorMetrics,
transaction_execution_result::ExecutedTransaction,
transaction_processing_callback::{AccountState, TransactionProcessingCallback},
},
ahash::AHashMap,
solana_compute_budget::compute_budget_limits::ComputeBudgetLimits,
solana_feature_set::{self as feature_set, FeatureSet},
solana_program_runtime::loaded_programs::ProgramCacheForTxBatch,
Expand All @@ -22,6 +24,7 @@ use {
sysvar::{
self,
instructions::{construct_instructions_data, BorrowedAccountMeta, BorrowedInstruction},
slot_history,
},
transaction::{Result, TransactionError},
transaction_context::{IndexOfAccount, TransactionAccount},
Expand Down Expand Up @@ -97,23 +100,34 @@ pub struct FeesOnlyTransaction {

#[cfg_attr(feature = "dev-context-only-utils", derive(Clone))]
pub(crate) struct AccountLoader<'a, CB: TransactionProcessingCallback> {
account_overrides: Option<&'a AccountOverrides>,
pub(crate) program_cache: ProgramCacheForTxBatch,
program_accounts: HashMap<Pubkey, (&'a Pubkey, u64)>,
account_cache: AHashMap<Pubkey, AccountSharedData>,
callbacks: &'a CB,
pub(crate) feature_set: Arc<FeatureSet>,
}
impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> {
pub fn new(
pub fn new_with_account_cache_capacity(
account_overrides: Option<&'a AccountOverrides>,
program_cache: ProgramCacheForTxBatch,
program_accounts: HashMap<Pubkey, (&'a Pubkey, u64)>,
callbacks: &'a CB,
feature_set: Arc<FeatureSet>,
capacity: usize,
) -> AccountLoader<'a, CB> {
let mut account_cache = AHashMap::with_capacity(capacity);

// SlotHistory may be overridden for simulation.
// No other uses of AccountOverrides are expected.
if let Some(slot_history) =
apfitzge marked this conversation as resolved.
Show resolved Hide resolved
account_overrides.and_then(|overrides| overrides.get(&slot_history::id()))
{
account_cache.insert(slot_history::id(), slot_history.clone());
Copy link

Choose a reason for hiding this comment

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

Should we add a debug assert that the size of account_overrides is equal to 1 if sysvar is present and 0 otherwise? Or is it basically impossible now that we decreased the vis of the account overrides set_account function?

Copy link
Member Author

Choose a reason for hiding this comment

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

it isnt possible, the only exposed setter is set_slot_history now (which accepts AccountSharedData and hardcodes the Pubkey as sysvar::slot_history::id(). we can remove AccountOverrides entirely now as a cleanup. im thinking ill look into it when i start on simd186 since id like to change something about simulation results, and this is related to simulation only

}

Self {
program_cache,
account_overrides,
account_cache,
callbacks,
program_accounts,
feature_set,
Expand All @@ -131,43 +145,123 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> {
.feature_set
.is_active(&feature_set::disable_account_loader_special_case::id());

if let Some(account_override) = self
.account_overrides
.and_then(|overrides| overrides.get(account_key))
{
Some(LoadedTransactionAccount {
loaded_size: account_override.data().len(),
account: account_override.clone(),
rent_collected: 0,
})
} else if let Some(program) = (use_program_cache && is_invisible_read)
if let Some(program) = (use_program_cache && is_invisible_read)
.then_some(())
.and_then(|_| self.program_cache.find(account_key))
{
// Optimization to skip loading of accounts which are only used as
// programs in top-level instructions and not passed as instruction accounts.
Some(LoadedTransactionAccount {
return Some(LoadedTransactionAccount {
loaded_size: program.account_size,
account: account_shared_data_from_program(account_key, &self.program_accounts)
.ok()?,
rent_collected: 0,
})
});
}
Comment on lines +148 to +160
Copy link
Member Author

@2501babe 2501babe Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i made this a separate block because once disable_account_loader_special_case activates it is simply deleted. we will also be able to remove program_cache and program_accounts from AccountLoader and they become free-floating variables in transaction_processor.rs again

Copy link
Member Author

Choose a reason for hiding this comment

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

also an idea i have been toying with after we do that is to simply implement TransactionProcessingCallback for AccountLoader itself so we can pass it to program cache setup, which will happen after account loading. this depends on how feasible it is to restrict callbacks to a concrete type (unless we just make two traits)


let account = if let Some(account) = self.account_cache.get(account_key) {
// Inspect the account prior to collecting rent, since
// rent collection can modify the account.
self.callbacks
.inspect_account(account_key, AccountState::Alive(account), is_writable);
Comment on lines +165 to +166
Copy link

Choose a reason for hiding this comment

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

Shouldn't we check if lamports is 0 before saying the account is alive? Otherwise you could read a dead account into the cache and later write to it and even tho it's still dead, we say it's alive.

Copy link
Member Author

@2501babe 2501babe Nov 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

youre right, ill open a pr for this on monday. thankfully it doesnt affect bank because to get here we would have already had to inspect as writable (which is why this was missed; before removing AccountCacheItem, inspecting a previously-alive now-dead account never happened at all)

edit: actually there is a slight edge case of taking a dead account read-only, leaving it dead, and making it alive, but this is only possible post-simd83 and the fix is the same

edit2: nevermind there actually is no way to get a bad first inspect because to bring the account to zero lamports in the first place it must necessarily be inspected as writable

Copy link

Choose a reason for hiding this comment

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

Hmm isn't there a bug post simd-83 where you read a dead account in one tx and then write to it in another? The inspect for the write tx would say it's alive when it's actually dead

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, i made my first edit thinking of that, but made the second edit thinking of the reason why we didnt need to check for equality with AccountSharedData::default(). but in this case the account doesnt need to exist

(i have a pr for this now but have been blocked by ci build issues, will add you once its green)


// If lamports is 0, a previous transaction deallocated this account.
// Return None without inspecting, so it can be recreated.
if account.lamports() == 0 {
None
} else {
Some(account.clone())
}
} else if let Some(account) = self.callbacks.get_account_shared_data(account_key) {
// Inspect the account prior to collecting rent, since
// rent collection can modify the account.
self.callbacks
.inspect_account(account_key, AccountState::Alive(&account), is_writable);

Some(LoadedTransactionAccount {
loaded_size: account.data().len(),
account,
rent_collected: 0,
})
self.account_cache.insert(*account_key, account.clone());

Some(account)
} else {
self.callbacks
.inspect_account(account_key, AccountState::Dead, is_writable);

None
};

account.map(|account| LoadedTransactionAccount {
loaded_size: account.data().len(),
account,
rent_collected: 0,
})
}

pub fn update_accounts_for_executed_tx(
&mut self,
message: &impl SVMMessage,
executed_transaction: &ExecutedTransaction,
) {
if executed_transaction.was_successful() {
self.program_cache
.merge(&executed_transaction.programs_modified_by_tx);

self.update_accounts_for_successful_tx(
message,
&executed_transaction.loaded_transaction.accounts,
);
} else {
self.update_accounts_for_failed_tx(
message,
&executed_transaction.loaded_transaction.rollback_accounts,
);
}
}

pub fn update_accounts_for_failed_tx(
&mut self,
message: &impl SVMMessage,
rollback_accounts: &RollbackAccounts,
) {
let fee_payer_address = message.fee_payer();
match rollback_accounts {
RollbackAccounts::FeePayerOnly { fee_payer_account } => {
self.account_cache
.insert(*fee_payer_address, fee_payer_account.clone());
}
RollbackAccounts::SameNonceAndFeePayer { nonce } => {
self.account_cache
.insert(*nonce.address(), nonce.account().clone());
}
RollbackAccounts::SeparateNonceAndFeePayer {
nonce,
fee_payer_account,
} => {
self.account_cache
.insert(*nonce.address(), nonce.account().clone());
self.account_cache
.insert(*fee_payer_address, fee_payer_account.clone());
}
}
}

fn update_accounts_for_successful_tx(
&mut self,
message: &impl SVMMessage,
transaction_accounts: &[TransactionAccount],
) {
for (i, (address, account)) in (0..message.account_keys().len()).zip(transaction_accounts) {
if !message.is_writable(i) {
continue;
}

// Accounts that are invoked and also not passed as an instruction
// account to a program don't need to be stored because it's assumed
// to be impossible for a committable transaction to modify an
// invoked account if said account isn't passed to some program.
if message.is_invoked(i) && !message.is_instruction_account(i) {
continue;
}

self.account_cache.insert(*address, account.clone());
}
}
}
Expand Down Expand Up @@ -660,12 +754,13 @@ mod tests {

impl<'a> From<&'a TestCallbacks> for AccountLoader<'a, TestCallbacks> {
fn from(callbacks: &'a TestCallbacks) -> AccountLoader<'a, TestCallbacks> {
AccountLoader::new(
AccountLoader::new_with_account_cache_capacity(
None,
ProgramCacheForTxBatch::default(),
HashMap::default(),
callbacks,
Arc::<FeatureSet>::default(),
0,
)
}
}
Expand Down Expand Up @@ -1004,12 +1099,13 @@ mod tests {
accounts_map,
..Default::default()
};
let mut account_loader = AccountLoader::new(
let mut account_loader = AccountLoader::new_with_account_cache_capacity(
account_overrides,
ProgramCacheForTxBatch::default(),
HashMap::default(),
&callbacks,
Arc::new(FeatureSet::all_enabled()),
0,
);
load_transaction(
&mut account_loader,
Expand Down Expand Up @@ -1451,12 +1547,13 @@ mod tests {
)),
);

let mut account_loader = AccountLoader::new(
let mut account_loader = AccountLoader::new_with_account_cache_capacity(
None,
loaded_programs,
program_accounts,
&mock_bank,
Arc::<FeatureSet>::default(),
0,
);

let mut error_metrics = TransactionErrorMetrics::default();
Expand Down Expand Up @@ -2450,12 +2547,13 @@ mod tests {
program_cache.replenish(program2, Arc::new(program2_entry));

let test_transaction_data_size = |transaction, expected_size| {
let mut account_loader = AccountLoader::new(
let mut account_loader = AccountLoader::new_with_account_cache_capacity(
None,
program_cache.clone(),
program_accounts.clone(),
&mock_bank,
Arc::<FeatureSet>::default(),
0,
);

let loaded_transaction_accounts = load_transaction_accounts(
Expand Down
2 changes: 1 addition & 1 deletion svm/src/account_overrides.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub struct AccountOverrides {

impl AccountOverrides {
/// Insert or remove an account with a given pubkey to/from the list of overrides.
pub fn set_account(&mut self, pubkey: &Pubkey, account: Option<AccountSharedData>) {
fn set_account(&mut self, pubkey: &Pubkey, account: Option<AccountSharedData>) {
match account {
Some(account) => self.accounts.insert(*pubkey, account),
None => self.accounts.remove(pubkey),
Expand Down
Loading
Loading