SIMD-0339: Increase CPI Account Infos Limit#8513
Conversation
| solana_pubkey::declare_id!("8MhfKhoZEoiySpVe248bDkisyEcBA7JQLyUS94xoTSqN"); | ||
| } | ||
|
|
||
| pub mod increase_cpi_info_account_limit { |
There was a problem hiding this comment.
You probably meant AccountInfo not "info account".
There was a problem hiding this comment.
do you mean change it to 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.
Just insert std::mem::sizeof::<AccountInfo>() then
There was a problem hiding this comment.
std::mem::size_of::<AccountInfo>() returns 48, because it contains references to the 2 Pubkey of owner and key, so that's why I didn't use it. The comment is a bit misleading I will update it
There was a problem hiding this comment.
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.
Since 80 bytes is not really correlated with the size of AccountInfo, this should just be a constant.
| 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.
Would rather have this combined with the consume_compute_meter() call below.
There was a problem hiding this comment.
Do you mean do the compute unit consumption inside translate_accounts_common()? Or did you mean to combine the 2 in translate_instruction_rust()?
| if invoke_context.get_feature_set().increase_cpi_info_account_limit { | ||
| INVOKE_UNITS_COST_SIMD_0339 | ||
| } else { | ||
| invoke_context.get_execution_cost().invoke_units |
There was a problem hiding this comment.
This should be feature gated in ComputeBudget not here. See
agave/compute-budget/src/compute_budget.rs
Line 131 in 8e696b9
There was a problem hiding this comment.
Makes sense, this should be in the right place now
| 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.
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 TEST_MAX_ACCOUNT_INFOS_EXCEEDED and the other one would be TEST_MAX_ACCOUNT_INFOS_EXCEEDED_BEFORE_INCREASE or something like that.
There was a problem hiding this comment.
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
…v/agave into increase_cpi_info_account_limit
| if invoke_context.get_feature_set().increase_cpi_info_account_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(34); |
There was a problem hiding this comment.
I believe you can use size_of::<AccountMeta>() here.
|
Run |
| 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 { |
There was a problem hiding this comment.
These two feature gates conflict with each other. Is SIMD-0339 supposed to supersede increase_tx_account_lock_limit ?
There was a problem hiding this comment.
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.
Yes, SIMD-0339 will override the increase_tx_account_lock_limit feature here
| 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.
ah yes, good spot, I missed that one
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8513 +/- ##
=======================================
Coverage 83.3% 83.3%
=======================================
Files 855 855
Lines 372188 372286 +98
=======================================
+ Hits 310272 310364 +92
- Misses 61916 61922 +6 🚀 New features to boost your workflow:
|
| //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.
I don't think it makes sense to use std::mem::size_of::<AccountInfo>() here. The 80 bytes from the SIMD is just a sum of the sizes of pubkey, owner, data length, and lamports because these are the things that are translated from vm to host memory. I think it's better to define a constant with a comment explaining the calculation.
There was a problem hiding this comment.
makes sense, I do agree, it looks very messy otherwise. It works nicely for size_of::<AccountMeta>() but not here
| check_aligned, | ||
| )?; | ||
|
|
||
| if invoke_context |
There was a problem hiding this comment.
Any particular reason you chose this location rather than putting the metering inside translate_account_infos after the check_account_infos call? I think that would be a better place because then we meter CU's before doing any allocation.
There was a problem hiding this comment.
that is a very good point, it makes sense to move it there, it also somewhat mimics the behaviour in translate_instruction_rust/c()
| ), | ||
| ( | ||
| increase_cpi_account_info_limit::id(), | ||
| "SIMD-0339: increase CPI info account limit", |
| } | ||
|
|
||
| pub mod increase_cpi_account_info_limit { | ||
| solana_pubkey::declare_id!("7wM2pdjwmSXviEdsorpcBY3T4YWUPQXDMepZudub7nGQ"); // Placeholder ID HAVE TO CHANGE BEFORE USE |
| pub mod discard_unexpected_data_complete_shreds { | ||
| solana_pubkey::declare_id!("8MhfKhoZEoiySpVe248bDkisyEcBA7JQLyUS94xoTSqN"); | ||
| } | ||
|
|
There was a problem hiding this comment.
this is very nit picky but this doesn't look intentional, can you undo this?
| consume_compute_meter( | ||
| invoke_context, | ||
| (data.len() as u64) | ||
| (total_cu_translation_cost) |
There was a problem hiding this comment.
Summing the total data size (ix data + account meta's) before dividing by "cpi bytes per unit" could reduce the impact of rounding error.. and that behavior doesn't match the SIMD. Combining the total cost is fine, just make sure you do the division first before summing the ix_data_cost with the ix_account_meta_cost
Problem
Adding SIMD-0339 functionality
Summary of Changes
increase_cpi_info_account_limitaccount_infoandix_account_metadata.Questions/still to do: