-
Notifications
You must be signed in to change notification settings - Fork 1.1k
SIMD-0339: Increase CPI Account Infos Limit #8513
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
Changes from 7 commits
23f8d97
1c9619b
2f3446d
12b7a09
8a65c68
0d31630
620ff0e
917abad
b0407e6
01466de
df4d61d
8e3e3d1
3c81a81
4629daa
abc7e34
c51ec8c
9071ea7
004ac03
c4b8563
9076cff
4db843a
ac84ebd
3f722bd
bcf5f2b
634f923
f44b4c8
c9e055d
171f59b
3e71a15
4f474f5
7fa3a0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -159,6 +159,8 @@ impl FeatureSet { | |
| raise_cpi_nesting_limit_to_8: self.is_active(&raise_cpi_nesting_limit_to_8::id()), | ||
| provide_instruction_data_offset_in_vm_r2: self | ||
| .is_active(&provide_instruction_data_offset_in_vm_r2::id()), | ||
| increase_cpi_account_info_limit: self.is_active(&increase_cpi_account_info_limit::id()), | ||
|
|
||
| } | ||
| } | ||
| } | ||
|
|
@@ -1145,6 +1147,10 @@ pub mod discard_unexpected_data_complete_shreds { | |
| solana_pubkey::declare_id!("8MhfKhoZEoiySpVe248bDkisyEcBA7JQLyUS94xoTSqN"); | ||
| } | ||
|
|
||
| pub mod increase_cpi_account_info_limit { | ||
| solana_pubkey::declare_id!("7wM2pdjwmSXviEdsorpcBY3T4YWUPQXDMepZudub7nGQ"); // Placeholder ID HAVE TO CHANGE BEFORE USE | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you update this with a real key? |
||
| } | ||
|
|
||
| pub static FEATURE_NAMES: LazyLock<AHashMap<Pubkey, &'static str>> = LazyLock::new(|| { | ||
| [ | ||
| (secp256k1_program_enabled::id(), "secp256k1 program"), | ||
|
|
@@ -2070,7 +2076,12 @@ pub static FEATURE_NAMES: LazyLock<AHashMap<Pubkey, &'static str>> = LazyLock::n | |
| discard_unexpected_data_complete_shreds::id(), | ||
| "SIMD-0337: Markers for Alpenglow Fast Leader Handover, DATA_COMPLETE_SHRED placement \ | ||
| rules", | ||
| ), /*************** ADD NEW FEATURES HERE ***************/ | ||
| ), | ||
| ( | ||
| increase_cpi_account_info_limit::id(), | ||
| "SIMD-0339: increase CPI info account limit", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This title doesn't match the SIMD |
||
| ), | ||
| /*************** ADD NEW FEATURES HERE ***************/ | ||
| ] | ||
| .iter() | ||
| .cloned() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,6 +114,9 @@ struct SolSignerSeedsC { | |
|
|
||
| /// Maximum number of account info structs that can be used in a single CPI invocation | ||
| const MAX_CPI_ACCOUNT_INFOS: usize = 128; | ||
| /// Maximum number of account info structs that can be used in a single CPI invocation with SIMD-0339 active | ||
| const MAX_CPI_ACCOUNT_INFOS_SIMD_0339: usize = 255; | ||
|
|
||
|
|
||
| /// Check that an account info pointer field points to the expected address | ||
| fn check_account_info_pointer( | ||
|
|
@@ -159,7 +162,12 @@ fn check_account_infos( | |
| ) -> Result<(), Error> { | ||
| let max_cpi_account_infos = if invoke_context | ||
| .get_feature_set() | ||
| .increase_tx_account_lock_limit | ||
| .increase_cpi_account_info_limit | ||
| { | ||
| MAX_CPI_ACCOUNT_INFOS_SIMD_0339 | ||
| } else if invoke_context | ||
| .get_feature_set() | ||
| .increase_tx_account_lock_limit | ||
| { | ||
| MAX_CPI_ACCOUNT_INFOS | ||
| } else { | ||
|
Comment on lines
168
to
178
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two feature gates conflict with each other. Is SIMD-0339 supposed to supersede
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's my understanding of how this SIMD-0339 should behave, see here solana-foundation/solana-improvement-documents#339 (comment). But would leave it to @jstarry to confirm There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, SIMD-0339 will override the |
||
|
|
@@ -536,6 +544,20 @@ pub fn translate_instruction_rust( | |
|
|
||
| check_instruction_size(account_metas.len(), data.len())?; | ||
|
|
||
| if invoke_context.get_feature_set().increase_cpi_account_info_limit{ | ||
| // Each account meta is 34 bytes (32 for pubkey, 1 for is_signer, 1 for is_writable) | ||
| let account_meta_bytes = account_metas | ||
| .len() | ||
| .saturating_mul(size_of::<AccountMeta>()); | ||
|
|
||
| consume_compute_meter( | ||
| invoke_context, | ||
| (account_meta_bytes as u64) | ||
| .checked_div(invoke_context.get_execution_cost().cpi_bytes_per_unit) | ||
| .unwrap_or(u64::MAX), | ||
| )?; | ||
| } | ||
|
|
||
| consume_compute_meter( | ||
| invoke_context, | ||
| (data.len() as u64) | ||
|
|
@@ -580,6 +602,22 @@ pub fn translate_accounts_rust<'a>( | |
| check_aligned, | ||
| )?; | ||
|
|
||
| if invoke_context.get_feature_set().increase_cpi_account_info_limit{ | ||
| //std::mem::size_of::<AccountInfo>() returns 48 bytes, which contains references to the 2 Pubkeys of owner and key, | ||
| //but we need the full size here so, need to add (32 + 32) bytes for Pubkey types and account for 8 + 8 bytes already existing for refence types. | ||
| //Hence adding 32 here due to 5 bytes being the padding and 11 bytes being the other data, see SIMD-0339 for calculations. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it makes sense to use
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense, I do agree, it looks very messy otherwise. It works nicely for |
||
| let account_infos_bytes = account_infos | ||
| .len() | ||
| .saturating_mul(std::mem::size_of::<AccountInfo>().saturating_add(32)); | ||
|
|
||
| consume_compute_meter( | ||
| invoke_context, | ||
| (account_infos_bytes as u64) | ||
| .checked_div(invoke_context.get_execution_cost().cpi_bytes_per_unit) | ||
| .unwrap_or(u64::MAX), | ||
| )?; | ||
| } | ||
|
|
||
| translate_accounts_common( | ||
| &account_info_keys, | ||
| account_infos, | ||
|
|
@@ -655,6 +693,19 @@ pub fn translate_instruction_c( | |
|
|
||
| check_instruction_size(ix_c.accounts_len as usize, data.len())?; | ||
|
|
||
| if invoke_context.get_feature_set().increase_cpi_account_info_limit{ | ||
| // Each account meta is 34 bytes (32 for pubkey, 1 for is_signer, 1 for is_writable) | ||
| let account_meta_bytes = ix_c.accounts_len | ||
| .saturating_mul(34); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah yes, good spot, I missed that one |
||
|
|
||
| consume_compute_meter( | ||
| invoke_context, | ||
| (account_meta_bytes as u64) | ||
| .checked_div(invoke_context.get_execution_cost().cpi_bytes_per_unit) | ||
| .unwrap_or(u64::MAX), | ||
| )?; | ||
| } | ||
|
|
||
| consume_compute_meter( | ||
| invoke_context, | ||
| (data.len() as u64) | ||
|
|
@@ -705,6 +756,20 @@ pub fn translate_accounts_c<'a>( | |
| check_aligned, | ||
| )?; | ||
|
|
||
| if invoke_context.get_feature_set().increase_cpi_account_info_limit{ | ||
| //sizeof(AccountInfo) is 80 bytes | ||
| let account_infos_bytes = account_infos | ||
| .len() | ||
| .saturating_mul(80); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just insert
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ended up using base type to allign with the other calculation and adjusting it to 80 bytes, its a bit messy though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since 80 bytes is not really correlated with the size of |
||
|
|
||
| consume_compute_meter( | ||
| invoke_context, | ||
| (account_infos_bytes as u64) | ||
| .checked_div(invoke_context.get_execution_cost().cpi_bytes_per_unit) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would rather have this combined with the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean do the compute unit consumption inside |
||
| .unwrap_or(u64::MAX), | ||
| )?; | ||
| } | ||
|
|
||
| translate_accounts_common( | ||
| &account_info_keys, | ||
| account_infos, | ||
|
|
@@ -776,8 +841,8 @@ pub fn cpi_common<S: SyscallInvokeSigned>( | |
| // changes so the callee can see them. | ||
| consume_compute_meter( | ||
| invoke_context, | ||
| invoke_context.get_execution_cost().invoke_units, | ||
| )?; | ||
| invoke_context.get_execution_cost().invoke_units | ||
| )?; | ||
| if let Some(execute_time) = invoke_context.execute_time.as_mut() { | ||
| execute_time.stop(); | ||
| invoke_context.timings.execute_us += execute_time.as_us(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,7 @@ static const uint8_t ADD_LAMPORTS = 18; | |
| static const uint8_t TEST_RETURN_DATA_TOO_LARGE = 19; | ||
| static const uint8_t TEST_DUPLICATE_PRIVILEGE_ESCALATION_SIGNER = 20; | ||
| static const uint8_t TEST_DUPLICATE_PRIVILEGE_ESCALATION_WRITABLE = 21; | ||
| static const uint8_t TEST_MAX_ACCOUNT_INFOS_EXCEEDED = 22; | ||
| static const uint8_t TEST_MAX_ACCOUNT_INFOS_EXCEEDED_INCREASE_CPI_INFO = 22; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally we try to prepare the code base for an easy cleanup of the feature gates, so everything is named from the perspective of the "new normal". Thus, this should stay
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ohhhh I see what you mean, so now with feature gate enabled is the new norm, hence the previous tests become before the norm right? So we would have the following for example: TEST_MAX_ACCOUNT_INFOS_SIMD_0339_OK -> new normal ok so TEST_MAX_ACCOUNT_INFOS_OK TEST_MAX_ACCOUNT_INFOS_OK -> the original ok test so, TEST_MAX_ACCOUNT_INFOS_OK_BEFORE_CPI_INCREASE_BEFORE_SIMD_0339
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be updated now |
||
| // TEST_CPI_INVALID_* must match the definitions in | ||
| // https://github.com/solana-labs/solana/blob/master/programs/sbf/rust/invoke/src/instructions.rs | ||
| static const uint8_t TEST_CPI_INVALID_KEY_POINTER = 35; | ||
|
|
@@ -42,6 +42,11 @@ static const uint8_t TEST_WRITE_ACCOUNT = 40; | |
| static const uint8_t TEST_ACCOUNT_INFO_IN_ACCOUNT = 43; | ||
| static const uint8_t TEST_NESTED_INVOKE_SIMD_0268_OK = 46; | ||
| static const uint8_t TEST_NESTED_INVOKE_SIMD_0268_TOO_DEEP = 47; | ||
| static const uint8_t TEST_MAX_ACCOUNT_INFOS_SIMD_0339_OK = 48; | ||
| static const uint8_t TEST_MAX_ACCOUNT_INFOS_SIMD_0339_EXCEEDED = 49; | ||
| static const uint8_t TEST_MAX_ACCOUNT_INFOS_OK = 50; | ||
| static const uint8_t TEST_MAX_ACCOUNT_INFOS_OK_INCREASE_CPI_INFO = 51; | ||
| static const uint8_t TEST_MAX_ACCOUNT_INFOS_EXCEEDED = 52; | ||
|
|
||
| static const int MINT_INDEX = 0; | ||
| static const int ARGUMENT_INDEX = 1; | ||
|
|
@@ -548,9 +553,66 @@ extern uint64_t entrypoint(const uint8_t *input) { | |
|
|
||
| break; | ||
| } | ||
| case TEST_MAX_ACCOUNT_INFOS_OK: { | ||
| sol_log("Test max allowed account infos"); | ||
| SolAccountMeta arguments[] = {}; | ||
| uint64_t account_infos_len = 64; | ||
| SolAccountInfo *account_infos = sol_calloc(account_infos_len, sizeof(SolAccountInfo)); | ||
| sol_assert(0 != account_infos); | ||
| for (uint64_t i = 0; i < account_infos_len; i++) { | ||
| account_infos[i] = accounts[0]; | ||
| } | ||
| uint8_t data[] = {}; | ||
| const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key, | ||
| arguments, SOL_ARRAY_SIZE(arguments), | ||
| data, SOL_ARRAY_SIZE(data)}; | ||
| const SolSignerSeeds signers_seeds[] = {}; | ||
| sol_assert(SUCCESS == sol_invoke_signed( | ||
| &instruction, account_infos, account_infos_len, | ||
| signers_seeds, SOL_ARRAY_SIZE(signers_seeds))); | ||
|
|
||
| break; | ||
| } | ||
| case TEST_MAX_ACCOUNT_INFOS_EXCEEDED: { | ||
| sol_log("Test max account infos exceeded"); | ||
| SolAccountMeta arguments[] = {}; | ||
| uint64_t account_infos_len = 65; | ||
| SolAccountInfo *account_infos = sol_calloc(account_infos_len, sizeof(SolAccountInfo)); | ||
| sol_assert(0 != account_infos); | ||
| uint8_t data[] = {}; | ||
| const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key, | ||
| arguments, SOL_ARRAY_SIZE(arguments), | ||
| data, SOL_ARRAY_SIZE(data)}; | ||
| const SolSignerSeeds signers_seeds[] = {}; | ||
| sol_assert(SUCCESS == sol_invoke_signed( | ||
| &instruction, account_infos, account_infos_len, | ||
| signers_seeds, SOL_ARRAY_SIZE(signers_seeds))); | ||
|
|
||
| break; | ||
| } | ||
| case TEST_MAX_ACCOUNT_INFOS_OK_INCREASE_CPI_INFO: { | ||
| sol_log("Test max account infos allowed with increase cpi info"); | ||
| SolAccountMeta arguments[] = {}; | ||
| uint64_t account_infos_len = MAX_CPI_ACCOUNT_INFOS; | ||
| SolAccountInfo *account_infos = sol_calloc(account_infos_len, sizeof(SolAccountInfo)); | ||
| sol_assert(0 != account_infos); | ||
| for (uint64_t i = 0; i < account_infos_len; i++) { | ||
| account_infos[i] = accounts[0]; | ||
| } | ||
| uint8_t data[] = {}; | ||
| const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key, | ||
| arguments, SOL_ARRAY_SIZE(arguments), | ||
| data, SOL_ARRAY_SIZE(data)}; | ||
| const SolSignerSeeds signers_seeds[] = {}; | ||
| sol_assert(SUCCESS == sol_invoke_signed( | ||
| &instruction, account_infos, account_infos_len, | ||
| signers_seeds, SOL_ARRAY_SIZE(signers_seeds))); | ||
|
|
||
| break; | ||
| } | ||
| case TEST_MAX_ACCOUNT_INFOS_EXCEEDED_INCREASE_CPI_INFO: { | ||
| sol_log("Test max account infos exceeded with increase cpi info"); | ||
| SolAccountMeta arguments[] = {}; | ||
| uint64_t account_infos_len = MAX_CPI_ACCOUNT_INFOS + 1; | ||
| SolAccountInfo *account_infos = sol_calloc(account_infos_len, sizeof(SolAccountInfo)); | ||
| sol_assert(0 != account_infos); | ||
|
|
@@ -565,6 +627,44 @@ extern uint64_t entrypoint(const uint8_t *input) { | |
|
|
||
| break; | ||
| } | ||
| case TEST_MAX_ACCOUNT_INFOS_SIMD_0339_OK: { | ||
| sol_log("Test max account infos allowed with SIMD-0339"); | ||
| SolAccountMeta arguments[] = {}; | ||
| uint64_t account_infos_len = 255; | ||
| SolAccountInfo *account_infos = sol_calloc(account_infos_len, sizeof(SolAccountInfo)); | ||
| sol_assert(0 != account_infos); | ||
|
|
||
| for (uint64_t i = 0; i < account_infos_len; i++) { | ||
| account_infos[i] = accounts[0]; | ||
| } | ||
| uint8_t data[] = {}; | ||
| const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key, | ||
| arguments, SOL_ARRAY_SIZE(arguments), | ||
| data, SOL_ARRAY_SIZE(data)}; | ||
| const SolSignerSeeds signers_seeds[] = {}; | ||
| sol_assert(SUCCESS == sol_invoke_signed( | ||
| &instruction, account_infos, account_infos_len, | ||
| signers_seeds, SOL_ARRAY_SIZE(signers_seeds))); | ||
|
|
||
| break; | ||
| } | ||
| case TEST_MAX_ACCOUNT_INFOS_SIMD_0339_EXCEEDED: { | ||
| sol_log("Test max account infos exceeded with SIMD-0339"); | ||
| SolAccountMeta arguments[] = {}; | ||
| uint64_t account_infos_len = 256; | ||
| SolAccountInfo *account_infos = sol_calloc(account_infos_len, sizeof(SolAccountInfo)); | ||
| sol_assert(0 != account_infos); | ||
| uint8_t data[] = {}; | ||
| const SolInstruction instruction = {accounts[INVOKED_PROGRAM_INDEX].key, | ||
| arguments, SOL_ARRAY_SIZE(arguments), | ||
| data, SOL_ARRAY_SIZE(data)}; | ||
| const SolSignerSeeds signers_seeds[] = {}; | ||
| sol_assert(SUCCESS == sol_invoke_signed( | ||
| &instruction, account_infos, account_infos_len, | ||
| signers_seeds, SOL_ARRAY_SIZE(signers_seeds))); | ||
|
|
||
| break; | ||
| } | ||
| case TEST_RETURN_ERROR: { | ||
| sol_log("Test return error"); | ||
| SolAccountMeta arguments[] = {{accounts[ARGUMENT_INDEX].key, false, true}}; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is very nit picky but this doesn't look intentional, can you undo this?