Poc/simd 0160 static instruction limit feature gated#8162
Conversation
…igverify. This stage filters banking stages only, does not required feature gate.
| } | ||
| let message_hash = if verification_mode == TransactionVerificationMode::FullVerification | ||
| { | ||
| // SIMD-0160, check instruction limit before signature verificaton |
There was a problem hiding this comment.
originally did this in solana-sdk, but not love the additional dependencies. Maybe having bank to enforce the limit for Replay is better?
There was a problem hiding this comment.
I think here is fine for replay verification.
Either after activation, or just now(?), we should make sdk throw an unconditional error for creating >64 ixs. It will fail no matter what so there's no use-case for it even before activation of the feature.
^creating, but not deserializing. So the "builder" fns for adding ixs.
There was a problem hiding this comment.
a separate change to SDK to not to build TX with more than 64 ix sounds good to me.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8162 +/- ##
========================================
Coverage 83.1% 83.1%
========================================
Files 810 810
Lines 357480 357644 +164
========================================
+ Hits 297163 297355 +192
+ Misses 60317 60289 -28 🚀 New features to boost your workflow:
|
| c.iter(|| { | ||
| for bytes in serialized_transactions.iter() { | ||
| let _ = TransactionView::try_new_sanitized(black_box(bytes.as_ref())).unwrap(); | ||
| let _ = |
There was a problem hiding this comment.
can't comment, but I think there's a benchmark case in here that has >64 txs. We should probably change it, otherwise they will fail.
| let Some(priority) = SanitizedTransactionView::try_new_sanitized(packet_data) | ||
| let Some(priority) = SanitizedTransactionView::try_new_sanitized( | ||
| packet_data, | ||
| bank.feature_set |
There was a problem hiding this comment.
let's move this out of the loop, otherwise it looks it up everytime
| .ok_or(PacketError::InvalidLen)?; | ||
|
|
||
| // SIMD-0160: skip if has more than 64 top instructions | ||
| if instruction_len > 64 { |
There was a problem hiding this comment.
this is unconditional. i'm like 99% sure this fn is only run for BP, where that would be fine, but we should verify that's the case.
There was a problem hiding this comment.
BP == "block producer/packer"? sigverify only feed to leaders, it does not impact consensus, if that is what you mean. I'm thinking changes to sigverify can be o nits own PR without feature gate.
| } | ||
| let message_hash = if verification_mode == TransactionVerificationMode::FullVerification | ||
| { | ||
| // SIMD-0160, check instruction limit before signature verificaton |
There was a problem hiding this comment.
I think here is fine for replay verification.
Either after activation, or just now(?), we should make sdk throw an unconditional error for creating >64 ixs. It will fail no matter what so there's no use-case for it even before activation of the feature.
^creating, but not deserializing. So the "builder" fns for adding ixs.
3 participants
Problem
draft for simd-160 for discussion
Summary of Changes
Fixes #