SIMD-406: Limit number of accounts in an instruction#10223
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10223 +/- ##
========================================
Coverage 82.8% 82.8%
========================================
Files 848 848
Lines 321446 321688 +242
========================================
+ Hits 266394 266608 +214
- Misses 55052 55080 +28 🚀 New features to boost your workflow:
|
|
|
||
| if enable_instruction_account_limit { | ||
| for instr in tx.message.instructions() { | ||
| if instr.accounts.len() > solana_transaction_context::MAX_ACCOUNTS_PER_INSTRUCTION { | ||
| return Err(solana_transaction_error::TransactionError::SanitizeFailure); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
To me this looks like the right place for this check, but the SIMD does say it will throw SanitizeError::ValueOutOfBounds.
Maybe we just need to update the proposal?
There was a problem hiding this comment.
this is an internal type and error, it's not reported to users in any way; irrelevant for the SIMD since the error type is entirely different.
There was a problem hiding this comment.
Likely that error should be reported by the actual SDK changes, right?
There was a problem hiding this comment.
We wanted to avoid making a breaking change to the SDK sanitization API so that's why the sanitization check is here. When the feature is activated, the check can be moved into the SDK and return the ValueOutOfBounds error.
There was a problem hiding this comment.
Maybe we just need to update the proposal?
If this PR is merged, I can amend the SIMD by either removing the error type at all or updating it.
There was a problem hiding this comment.
If this PR is merged, I can amend the SIMD by either removing the error type at all or updating it.
Yeah, this is what I was thinking, maybe just take the variant name out of the proposal.
| #[test_case(false; "pre_simd406_limit_instruction_accounts")] | ||
| #[test_case(true; "simd406_limit_instruction_accounts")] | ||
| fn test_verify_transactions_accounts_limit(simd_406_enabled: bool) { |
There was a problem hiding this comment.
Wouldn't you rather test this in runtime-transaction/src/runtime_transaction/sdk_transactions.rs? You're asserting the same error and it's only bubbling its way up to your bank caller.
Instead, you can just get rid of a lot of this setup and test the functionality directly.
There was a problem hiding this comment.
Also, do you think you could add a test that ensures exactly 255 is allowed?
There was a problem hiding this comment.
I like this test because all replay verification goes through Bank::verify_transaction so it's a nice place for a sanity check. But I agree that unit tests in sdk_transactions.rs should be added as well.
| // Vote instructions are created in the validator code, and they are not | ||
| // referencing more than 255 accounts, so it is safe to set this to true. | ||
| true, |
There was a problem hiding this comment.
Even though true is the correct choice here, I think the comment is misleading.
Sure, Vote instructions are typically created by validator software, but there's nothing that says they have to be. The simple vote transaction filtering doesn't know where the transaction originated and it also doesn't enforce a specific number of accounts. For the latter reason, though, true is the right setting here.
There was a problem hiding this comment.
it also doesn't enforce a specific number of accounts.
I didn't follow. If it does not enforce a specific number of accounts, then, theoretically, it could receive instructions with more than 255 accounts, right?
There was a problem hiding this comment.
That's correct. If someone crafted a "simple vote" transaction on their own, outside validator software, they could include > 255 accounts and it would still be filtered as a simple vote transaction. This is why I said using true is correct.
Your comment says that the transactions are created in the validator code, but that's not guaranteed.
There was a problem hiding this comment.
What about changing the comment to "Vote instructions do not need more than 255 accounts, and the vote transaction filtering does not enforce that, so enforcing the constraint here is right" ?
| enable_static_instruction_limit: bool, | ||
| enable_instruction_accounts_limit: bool, |
There was a problem hiding this comment.
might consider adding a struct here: SanitizationFeatures or something with these bools on it. Rather than we continue to add more and more bools which are easily confused with order. wdyt?
apfitzge
left a comment
There was a problem hiding this comment.
Commenting as a quick note; TxV1 automatically supports this feature because the new format directly uses a u8 for the number of accounts in top-level instructions.
|
It would be awesome if we could get this into the 4.0 release 🙌 |
|
|
||
| if enable_instruction_account_limit { | ||
| for instr in tx.message.instructions() { | ||
| if instr.accounts.len() > solana_transaction_context::MAX_ACCOUNTS_PER_INSTRUCTION { | ||
| return Err(solana_transaction_error::TransactionError::SanitizeFailure); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
We wanted to avoid making a breaking change to the SDK sanitization API so that's why the sanitization check is here. When the feature is activated, the check can be moved into the SDK and return the ValueOutOfBounds error.
| #[test_case(false; "pre_simd406_limit_instruction_accounts")] | ||
| #[test_case(true; "simd406_limit_instruction_accounts")] | ||
| fn test_verify_transactions_accounts_limit(simd_406_enabled: bool) { |
There was a problem hiding this comment.
I like this test because all replay verification goes through Bank::verify_transaction so it's a nice place for a sanity check. But I agree that unit tests in sdk_transactions.rs should be added as well.
Problem
Currently, there is no restriction on how many accounts an instruction may reference. Even though transactions may be limited to 256 account keys, an instruction may reference up to
u16::MAXkeys with duplicates. In SIMD-406, we proposed restricting the number of accounts an instruction may reference to 255 to remove complexity from runtime.It was decided that such a check belongs in message sanitization, and the implementation fitted best in the validator code rather than the SDK (see anza-xyz/solana-sdk#520 (comment)).
The check only affects transactions v0 and legacy. Moreover, it will only impact top-level instructions that invoke native programs and pre-compiles, since the runtime serialization cannot serialize more than 255 accounts and will already throw an error for that case.
Summary of Changes
fn RuntimeTransaction<SanitizedTransaction>::try_createinsdk_transactions.rs.fn sanitize_instructionsinsanitize.rs.sanitize.rsand the other one inbank/tests.rs.