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: account_loader.rs generic over SVMMessage #2303

Merged
merged 3 commits into from
Aug 1, 2024
Merged

SVM: account_loader.rs generic over SVMMessage #2303

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8df9a68
Generic construct_instructions_account - same impl as SanitizedMessag…
apfitzge Jul 26, 2024
1b509e6
generic load_transaction_accounts
apfitzge Jul 26, 2024
7d33ed7
generic load_accounts
apfitzge Jul 26, 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
52 changes: 37 additions & 15 deletions svm/src/account_loader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,21 @@ use {
account::{Account, AccountSharedData, ReadableAccount, WritableAccount},
feature_set::{self, FeatureSet},
fee::FeeDetails,
message::SanitizedMessage,
native_loader,
nonce::State as NonceState,
pubkey::Pubkey,
rent::RentDue,
rent_collector::{CollectedInfo, RentCollector, RENT_EXEMPT_RENT_EPOCH},
rent_debits::RentDebits,
saturating_add_assign,
sysvar::{self, instructions::construct_instructions_data},
transaction::{Result, SanitizedTransaction, TransactionError},
sysvar::{
self,
instructions::{construct_instructions_data, BorrowedAccountMeta, BorrowedInstruction},
},
transaction::{Result, TransactionError},
transaction_context::{IndexOfAccount, TransactionAccount},
},
solana_svm_transaction::svm_message::SVMMessage,
solana_system_program::{get_system_account_kind, SystemAccountKind},
std::num::NonZeroU32,
};
Expand Down Expand Up @@ -155,7 +158,7 @@ pub fn validate_fee_payer(
/// second element.
pub(crate) fn load_accounts<CB: TransactionProcessingCallback>(
callbacks: &CB,
txs: &[SanitizedTransaction],
txs: &[impl SVMMessage],
validation_results: Vec<TransactionValidationResult>,
error_metrics: &mut TransactionErrorMetrics,
account_overrides: Option<&AccountOverrides>,
Expand All @@ -167,12 +170,10 @@ pub(crate) fn load_accounts<CB: TransactionProcessingCallback>(
.zip(validation_results)
.map(|etx| match etx {
(tx, Ok(tx_details)) => {
let message = tx.message();

// load transactions
load_transaction_accounts(
callbacks,
message,
tx,
tx_details,
error_metrics,
account_overrides,
Expand All @@ -188,7 +189,7 @@ pub(crate) fn load_accounts<CB: TransactionProcessingCallback>(

fn load_transaction_accounts<CB: TransactionProcessingCallback>(
callbacks: &CB,
message: &SanitizedMessage,
message: &impl SVMMessage,
tx_details: ValidatedTransactionDetails,
error_metrics: &mut TransactionErrorMetrics,
account_overrides: Option<&AccountOverrides>,
Expand All @@ -203,9 +204,8 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
let mut accumulated_accounts_data_size: u32 = 0;

let instruction_accounts = message
.instructions()
.iter()
.flat_map(|instruction| &instruction.accounts)
.instructions_iter()
.flat_map(|instruction| instruction.accounts)
.unique()
.collect::<Vec<&u8>>();

Expand Down Expand Up @@ -291,8 +291,7 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(

let builtins_start_index = accounts.len();
let program_indices = message
.instructions()
.iter()
.instructions_iter()
.map(|instruction| {
let mut account_indices = Vec::with_capacity(2);
let program_index = instruction.program_id_index as usize;
Expand Down Expand Up @@ -393,9 +392,32 @@ fn accumulate_and_check_loaded_account_data_size(
}
}

fn construct_instructions_account(message: &SanitizedMessage) -> AccountSharedData {
fn construct_instructions_account(message: &impl SVMMessage) -> AccountSharedData {
let account_keys = message.account_keys();
let mut decompiled_instructions = Vec::with_capacity(message.num_instructions());
for (program_id, instruction) in message.program_instructions_iter() {
let accounts = instruction
.accounts
.iter()
.map(|account_index| {
let account_index = usize::from(*account_index);
BorrowedAccountMeta {
is_signer: message.is_signer(account_index),
is_writable: message.is_writable(account_index),
pubkey: account_keys.get(account_index).unwrap(),
}
})
.collect();

decompiled_instructions.push(BorrowedInstruction {
accounts,
data: instruction.data,
program_id,
});
}

AccountSharedData::from(Account {
data: construct_instructions_data(&message.decompile_instructions()),
data: construct_instructions_data(&decompiled_instructions),
Copy link

@LucasSte LucasSte Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The original decompile_instructions function is also used in benchmarks and some tests. Have you considered adding it to your trait?

I am thinking that these new lines may become repeated code at some point.

Copy link
Author

Choose a reason for hiding this comment

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

I originally had it as a function, but the only part in actual code it was called was here. Removed as part of the trait simplification.

But also wanted to remove the allocation that BorrowedInstruction requires:

pub struct BorrowedInstruction<'a> {
    pub program_id: &'a Pubkey,
    pub accounts: Vec<BorrowedAccountMeta<'a>>, //<-- allocation here
    pub data: &'a [u8],
}

In this first step I'm just doing a drop-in replacement where we still do this allocation.
As a follow-up I will write a version of construct_instructions_data (and its' input) that require no allocations).
This was the reasoning I had behind not having this as part of the trait.

owner: sysvar::id(),
..Account::default()
})
Expand Down