SIMD-0339: Increase CPI Account Info Limit#339
Conversation
1b2911f to
8c17df6
Compare
8c17df6 to
1a5da24
Compare
|
|
||
| Since the list of account info's passed to a CPI can now be ~4 times longer, | ||
| there will be more overhead in the SVM to map each instruction account address | ||
| to one of the account info. |
There was a problem hiding this comment.
We should also charge a few CUs for duplicate instruction accounts. Currently we only charge after de-duplication I think.
There was a problem hiding this comment.
We should also charge a few CUs for duplicate instruction accounts
Do you mean charge CUs for duplicate account info's or duplicate instruction accounts or both?
Currently we only charge after de-duplication I think.
Yeah I think we are just charging for the account data length after de-duplication.
How about we just charge a fee based on the account info length? That way we don't necessarily need to check which account info's are duplicates or even used by the CPI instruction. Could charge total_serialized_account_info_len / cpi_bytes_per_unit CU's where
sol_invoke_signed_c:
total_serialized_account_info_len = 56 * account_infos_len
pre-simd-max-cus = 56 * 64 / 250 = 14
post-simd-max-cus = 48 * 255 / 250 = 57
sol_invoke_signed_rust:
total_serialized_account_info_len = 48 * account_infos_len
pre-simd-max-cus = 48 * 64 / 250 = 12
post-simd-max-cus = 48 * 255 / 250 = 48There was a problem hiding this comment.
Do you mean charge CUs for duplicate account info's or duplicate instruction accounts or both?
Isn't that the same? Every AccountInfo corresponds to one InstructionAccount. The only translation is from Pubkey to index.
How about we just charge a fee based on the account info length?
Yes, that is what I had in mind.
There was a problem hiding this comment.
Isn't that the same?
Not the same. The CPI syscall receives both account_infos as well as the account_metas from the instruction accounts slice. Both of which are keyed with an account address for some reason and therefore they can both contain duplicates.
Every
AccountInfocorresponds to oneInstructionAccount.
Nope. Every account meta corresponds to instruction account. There's an additional translation step from account metas to account info's before the translation from Pubkey to index.
So, sounds like we should charge for both the account metas length as well as the account infos length?
There was a problem hiding this comment.
Ah yes, sorry confused AccountInfo and AccountMeta again...
So, sounds like we should charge for both the account metas length as well as the account infos length?
Yup
|
|
||
| ## Motivation | ||
|
|
||
| CPI's are restricted to a limit of 64 account info's passed to the syscall. |
There was a problem hiding this comment.
Didn't you already raise the limit in solana-labs/solana#27242?
The feature gate is still there: https://github.com/anza-xyz/agave/blob/d54feac498c6ce29fce9b4b262ea3ba1d3bb5a5e/syscalls/src/cpi.rs#L932
There was a problem hiding this comment.
Yes, thanks for pointing it out. The increase_tx_account_lock_limit feature gate is currently only activated on devnet and it does two things:
- Increase tx account lock limit from 64 to 128
- Increase CPI account info limit from 64 to 128
SIMD-0339 should change the CPI account info limit to 255 no matter whether the increase_tx_account_lock_limit feature gate is active or not. This SIMD has nothing to do with the tx account lock limit. It's more of an acknowledgement that account info length can contain duplicates and should be allowed to be as long as the ix account length and not necessarily the same as the tx account lock limit.
|
We would have to either only charge CUs for things going over the current limit or offset CUs in the CPI base cost, not to break existing dApps. Also, this SIMD should probably depend on #219 for direct mapping as this will reduce some overhead of instruction accounts. |
| - 8 bytes for data length | ||
|
|
||
| The total cost of account info's can be computed with | ||
| `(account_info_size * account_infos_len) / cpi_bytes_per_unit` rounded down to |
There was a problem hiding this comment.
Just to confirm whether the cost is for each unique account info or not. This is not very clear from the equation here. For example, if I use a slice of 255 account infos, but all of them are copies from the same one, then the runtime has only to serialize a single account payload.
There was a problem hiding this comment.
My intention was to charge a cost for each account info in the account_infos slice passed to the CPI syscall including duplicates. The runtime needs to search for the first instance of each instruction account address in the account_infos slice. So when account_infos is longer, the runtime typically needs to do more work to do the account translation step even if some of those account info's are duplicates and skipped.
| @@ -0,0 +1,108 @@ | |||
| --- | |||
| simd: '0339' | |||
| title: Increase CPI Account Info Limit | |||
There was a problem hiding this comment.
cosmetic: I find it a bit confusing the wording "Account Info Limit" referring to the maximum number of account infos in a CPI. My first impression is that it is a limit on the length of an account info, not on the number of account infos.
There was a problem hiding this comment.
I updated to Increase CPI Account Infos Limit. Is that better or do you prefer something different?
| The max account info length increase for CPI's and new costs will be feature | ||
| gated. Since the limit is being increased, existing behavior will not be | ||
| restricted. However, new imposed costs for CPI instruction accounts and account | ||
| info's may cause onchain programs to consume additional CU's. |
There was a problem hiding this comment.
Is it possible to include a numeric example of an existing bahaviour (< 64 account) to illustrate the difference, like a before and after calculation? The cost of CPI is already perceived as high, and given that this change potentially will increase the cost, it would be nice to see by how much.
There was a problem hiding this comment.
After discussion with @Lichtso we decided it was better to offset the base cost of CPI's by the maximum account cost of existing allowed behavior. So consumed CU's for existing behavior will only be decreased, if at all.
Yeah, agreed. Just updated the SIMD to reflect an offset to the CPI base cost.
Do you mean the overhead of instruction account info's? Can you explain what overhead you mean? I don't think the overhead actually changes in this SIMD because the number of translated account's is still capped at 64 because we only translate unique instances of each account, we don't translate duplicates. |
|
Sorry, I meant the overhead of instruction accounts. How is that still limited after this SIMD? If you can pass in 255 |
Transactions can only have 64 unique accounts right now and it's not possible to reference other accounts besides those 64 in a CPI. Also, you can already pass 255 instruction account metas. Allowing 255 account info's in this SIMD just makes it easier for an onchain program invoked with 255 instruction account metas to pass many or all of those accounts to another onchain program CPI |
| compute units to 946 compute units to offset the new costs. | ||
|
|
||
| Before this SIMD, up to 255 instruction account metadata's could be passed to a | ||
| CPI syscall. The base cost offset is therefore `255 * 34 / 250 = 34` CU's. |
There was a problem hiding this comment.
It currently does not make sense to pass in that many, so it probably does not happen. Realistically this should maybe assume 64 as well?
There was a problem hiding this comment.
It is already over 64 in the wild (see here: https://explorer.solana.com/tx/69BYATGo3SrSPBKbF5PBs36DvJUssxrFBJtXhfRkmDBrLvLapGevr3NzpKPcBTPr52tYHCkdRNbCyXpuWjMrb4n). I think it's safer to assume 255 since it's only 34 CU's
Suggested by @Arrowana