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

Replace hardcoded loaded accounts size limit with compute budget instruction #30506

Merged

Conversation

tao-stones
Copy link
Contributor

@tao-stones tao-stones commented Feb 24, 2023

Problem

#30366 added compute budget instruction to request transaction's loaded accounts size limit. Should replace hardcoded limit with this instruction.

Summary of Changes

  1. replace hardcoded loaded accounts data size limit with compute budget instruction;
  2. sanitize requested limit to > 0, otherwise throw transaction error

Fix: #30366

Copy link
Contributor

@hydrogenbond007 hydrogenbond007 left a comment

Choose a reason for hiding this comment

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

seems like its good to go not sure about the tests

runtime/src/accounts.rs Show resolved Hide resolved
runtime/src/accounts.rs Show resolved Hide resolved
@tao-stones tao-stones requested a review from brooksprumo March 8, 2023 15:54
@tao-stones tao-stones added feature-gate Pull Request adds or modifies a runtime feature gate and removed feature-gate Pull Request adds or modifies a runtime feature gate labels Mar 8, 2023
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Code looks good. Are there tests that combine the new instruction to set a load limit and then using/checking that limit via get_requested_loaded_accounts_data_size_limit?

@tao-stones
Copy link
Contributor Author

tao-stones commented Mar 8, 2023

Code looks good. Are there tests that combine the new instruction to set a load limit and then using/checking that limit via get_requested_loaded_accounts_data_size_limit?

Thanks @brooksprumo, add a test to check get_requested_loaded_accounts_data_size_limit results with combination of feature gates and transactions.

Comment on lines 4109 to 4125
macro_rules! test {
( $instructions: expr, $feature_set: expr, $expected_result: expr ) => {
let payer_keypair = Keypair::new();
let tx = SanitizedTransaction::from_transaction_for_tests(Transaction::new(
&[&payer_keypair],
Message::new($instructions, Some(&payer_keypair.pubkey())),
Hash::default(),
));
assert_eq!(
$expected_result,
Accounts::get_requested_loaded_accounts_data_size_limit(&tx, $feature_set)
);
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can this be a function instead of a macro? (It can still stay within this test too!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if macro is worse than function-in-function here, in term of styling. Assume prefer functions because macro is not type checked?

Copy link
Contributor

Choose a reason for hiding this comment

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

And macros take longer to compile :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but quicker to run 💨 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Really? Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the extra time it spent during compile, macros are expanded and inlined. So for the extra compile time and larger size, macros would shave few execution time. Worth it or not perhaps is another debate 😸

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it different than #[inline(always)]?

brooksprumo
brooksprumo previously approved these changes Mar 8, 2023
Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

lgtm!

@tao-stones tao-stones merged commit 3b9438f into solana-labs:master Mar 9, 2023
@tao-stones tao-stones deleted the loaded_account_size_limit_by_ix branch March 9, 2023 01:41
nickfrosty pushed a commit to nickfrosty/solana that referenced this pull request Mar 12, 2023
…ruction (solana-labs#30506)

1. replace hardcoded loaded accounts data size limit with compute budget instruction;
2. new transaction error for invalid account data size limit
3. test requested limit with combination of features and transactions
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.

Feature Gate: add compute budget instruction for setting account data size per transaction
3 participants