SIMD-406: Restrict the number of accounts in an instruction#406
Conversation
|
Hello @LucasSte! Welcome to the SIMD process. By opening this PR you are affirming that your SIMD has been thoroughly discussed and vetted in the SIMD discussion section. The SIMD PR section should only be used to submit a final technical specification for review. If your design / idea still needs discussion, please close this PR and create a new discussion here. This PR requires the following approvals before it can be merged:
Once all requirements are met, you can merge this PR by commenting |
05b895f to
d01e14b
Compare
d01e14b to
6fe01cb
Compare
|
Thanks, @Lichtso!
|
|
Thanks, @mjain-jump!
|
|
Can you clarify what happens if there are exactly 256 accounts?
vs.
|
| accounts, each being a `u8` index referencing a transaction account. | ||
|
|
||
| Allowing instructions to reference more accounts than those contained in the | ||
| transaction is pointless, because it entails some accounts will be aliased, |
There was a problem hiding this comment.
pointless
is it? instruction account references map to roles as defined by that instruction. multiple roles may reasonably be filled by the same account
restricting instruction account references to a unique set would force role mapping on chain, either statically in the program or dynamically via instruction data. the latter would cost addition transaction byte bloat, both would incur additional cu cost and risk of error
this seems like a naive motivation
There was a problem hiding this comment.
My wording there might have been imprecise. What I wanted to say is that allowing instructions to reference more accounts than the number of pubkeys the message format allows (256) is not reasonable.
The argument for that is: for what the protocol offers today, that is not necessary. On one side, there is a hard limit of 255 for CPI and programs. On the other, builtins and precompiles don't use that many.
We will continue to allow account aliasing and instructions referencing more accounts than the number of pubkeys in a transaction, provided that you are not overusing the message format.
| This SIMD imposes a hard restriction on the maximum number of accounts an | ||
| instruction may refer to. There is already a limit of 255 accounts during | ||
| serialization in ABI v0 and v1, but it does not apply to builtin invocations | ||
| or precompiles. |
There was a problem hiding this comment.
| This SIMD imposes a hard restriction on the maximum number of accounts an | |
| instruction may refer to. There is already a limit of 255 accounts during | |
| serialization in ABI v0 and v1, but it does not apply to builtin invocations | |
| or precompiles. | |
| This SIMD imposes a hard restriction on the maximum number of account | |
| references that an instruction may declare. There is already a limit of | |
| 255 account rederences imposed during serialization in ABI v0 and v1, | |
| but it does not current apply to builtin invocations or precompiles. |
Good point. We can keep erroring out at 255 for programs and CPI, or we can increase the serialization limit to 256. Any thoughts @Lichtso? |
|
Limit should be |
|
Thanks, @cmoyes-jump!
|
|
I addressed all the feedback in the previous three commits, including the reduction of the limit to 255. |
|
Thanks, @Lichtso!
|
|
Thanks, @topointon-jump!
|
| For every instruction that goes through program runtime (CPI, user deployed | ||
| program invocation, builtin invocation or precompiles), program runtime must | ||
| check if it references more than 255 accounts, and must throw | ||
| `InstructionError::MaxAccountsExceeded` when that is the case. |
There was a problem hiding this comment.
Where should this check occur exactly? When should it throw? Ideally as early as possible.
There was a problem hiding this comment.
Yeah agreed. I think that in addition to the program runtime restriction, we should also add a sanitization check for the number of accounts passed to top-level instructions.
There was a problem hiding this comment.
Where should this check occur exactly? When should it throw?
The exact position I was thinking is in prepare_next_instruction and prepare_next_top_level_instruction. I did not detail the function names since this is Agave specific. I'll write down "right before account deduplication" as an alternative.
we should also add a sanitization check for the number of accounts passed to top-level instructions.
Yes, I also had that idea, but thought it would difficult to pass down a boolean through the sanitization trait we have. Since everyone also believes in that being useful, I am going to update the SIMD with that requirement.
There was a problem hiding this comment.
we should also add a sanitization check for the number of accounts passed to top-level instructions.
On a second thought, transaction sanitization is used in far too many places throughout the mono repo. It is going to be time consuming to get everything right there. Do you think the additional check is still worth it?
There was a problem hiding this comment.
I mentioned where the check happens in 959ecc3.
I didn't yet write about sanitization, because I'm waiting for your opinion on that.
There was a problem hiding this comment.
Just so I can compile all feedback and amend the SIMD, are we going to place the check in both runtime and sanitization?
There was a problem hiding this comment.
If we throw in sanitization then the runtime check becomes unreachable. So, for the SIMD it is either or. Specifying both is inconsistent / self conflicting.
There was a problem hiding this comment.
Regarding the implementation in Agave, the check in the program-runtime is easier to do, since we already have the feature gating infrastructure in place. Implementing it in sanitization involves more code changes throughout the monorepo.
I see people weighing on both approaches, but I still did not understood if one is preferable over the other. Does anyone have any strong reason for the check not to be in runtime?
I'm happy to change if that is the case, but for simplicity my preference would be runtime.
There was a problem hiding this comment.
Looks like we have most people agreeing on adding a protocol error for transactions which use too many accounts in an instruction. If you need some additional convincing, v1 Transactions will have a sanitization limit of 255 accounts per instructions, so I think it makes sense to limit ix accounts in legacy and v0 transactions in the same way. I think the Agave implementation should be fine, we can discuss later if you want.
There was a problem hiding this comment.
Thanks for the feedback. I updated the SIMD to use only a check during sanitization. The latest version is in 5bc845a.
| For every instruction that goes through program runtime (CPI, user deployed | ||
| program invocation, builtin invocation or precompiles), program runtime must | ||
| check if it references more than 255 accounts, and must throw | ||
| `InstructionError::MaxAccountsExceeded` when that is the case. |
There was a problem hiding this comment.
did not detail the function names since this is Agave specific. I'll write down "right before account deduplication" as an alternative.
I think this is as close as we can get without a spec. 😅
Maybe FD can help us clear up the exact location/timing of this check - @mjain-jump @topointon-jump ? The most important piece is specifying which errors this should eclipse and which ones it should not.
Co-authored-by: Joe C <jcaulfield135@gmail.com>
|
Thanks, topointon-jump! |
|
Thanks, topointon-jump! |
| A new verification step must be included in message sanitization for message | ||
| formats legacy and v0. The verification step must examine every instruction in | ||
| a transaction, check if the former references more than 255 accounts, and | ||
| throw `SanitizationError::ValueOutOfBounds` otherwise. Transactions violating |
There was a problem hiding this comment.
We don't need to specify the error here, it's an implementation detail. It's enough to say that the transaction and any block including this transaction will be rejected.
There was a problem hiding this comment.
While the specific error code is not part of consensus it is still good to specify it for fuzzing between validator implementations. At least that is how we handled it in other SIMDs.
|
@LucasSte shall we merge this? |
|
@bw-solana mind taking a look? |
| A new verification step must be included in message sanitization for message | ||
| formats legacy and v0. The verification step must examine every instruction in |
There was a problem hiding this comment.
how about the new Transaction v1 format?
There was a problem hiding this comment.
The new transaction v1 format also specifies this limit, so this applies to all transaction formats. Definitely worth specifying 👍
There was a problem hiding this comment.
The limit for v1 transactions won't be behind the feature gate implied by this SIMD. Is it worth specifying if that is the case?
There was a problem hiding this comment.
Maybe just mention it for clarity?
There was a problem hiding this comment.
I mentioned v1 transactions and dead blocks (from the following thread) in f600fbc
| formats legacy and v0. The verification step must examine every instruction in | ||
| a transaction, check if the former references more than 255 accounts, and | ||
| throw `SanitizationError::ValueOutOfBounds` otherwise. Transactions violating | ||
| such a restriction must be considered invalid and not included in a block. |
There was a problem hiding this comment.
Should we mention that blocks should be declared dead if they include such transactions? Or is the block still good but the transaction does not execute?
Is this fine for bankless leader because we can perform static analysis and determine validity?
There was a problem hiding this comment.
Should we mention that blocks should be declared dead if they include such transactions?
Yes we should mention this.
Is this fine for bankless leader because we can perform static analysis and determine validity?
Yes this is correct.
|
Thanks, bw-solana!
|
|
✅ All approvals received! @LucasSte, you can now merge this by commenting ✅ Status: Ready to merge |
|
/merge |
|
✅ Merge successful! LucasSte's PR has been merged. |
No description provided.