perf(l1): replace index based jump targets with bit vector#6707
perf(l1): replace index based jump targets with bit vector#6707benbencik wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates how EVM jump destinations are represented and validated by switching from a Vec<u32> list of valid JUMPDEST offsets to a compact bitset (Vec<u8>), then uses that bitset for jump validation and storage encoding/decoding.
Changes:
- Replaced
jump_targets: Vec<u32>with a bit-vectorVec<u8>inCode, including a newis_valid_jump_target()helper. - Updated VM jump validation to use
is_valid_jump_target()instead of scanning bytecode + binary searching targets. - Updated storage encoding/decoding to persist the new
Vec<u8>jump target representation and adjusted buffer sizing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs | Uses the new Code::is_valid_jump_target() for jump validation. |
| crates/storage/store.rs | Updates on-disk (de)serialization and encoding buffer sizing for the new jump target representation. |
| crates/common/types/account.rs | Changes jump target representation to a bitset, adds is_valid_jump_target(), updates size estimation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // OP_JUMPDEST | ||
| 0x5B => { | ||
| targets.push(i as u32); | ||
| targets[i / 8] |= 1 << (i % 8); |
| pub fn is_valid_jump_target(&self, target: usize) -> bool { | ||
| self.jump_targets | ||
| .get(target / 8) | ||
| .is_some_and(|byte| byte & (1 << (target % 8)) != 0) |
| hash: code_hash, | ||
| bytecode, | ||
| jump_targets: <Vec<_>>::decode(targets)?, | ||
| jump_targets: <Vec<u8>>::decode(targets)?, |
Greptile SummaryReplaces the
Confidence Score: 3/5The core bit-vector logic is correct, but a stale #[expect(clippy::as_conversions)] in the jump handler will emit an unfulfilled_lint_expectations diagnostic that could break CI, and the breaking storage format has no migration guard. The jump validity check now relies entirely on the bit vector; the removed as casts mean the #[expect] attribute has no lint to suppress, which will fail the build under -D warnings. The storage format change also silently deserializes old Vec bytes as Vec with no version guard. crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs (stale #[expect] attribute) and crates/storage/store.rs (breaking storage format with no migration path)
|
| Filename | Overview |
|---|---|
| crates/common/types/account.rs | Switches jump_targets from Vec indices to a compact bit vector (Vec); adds is_valid_jump_target helper. One inaccurate comment about bit vector size. |
| crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs | Simplifies the jump validity check to use is_valid_jump_target, but leaves a now-stale #[expect(clippy::as_conversions)] that will trigger an unfulfilled_lint_expectations diagnostic. |
| crates/storage/store.rs | Minimal updates to decode type annotation (Vec) and buffer capacity calculation; straightforward and correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[JUMP or JUMPI opcode] --> B[Resolve target address from stack]
B --> C[call is_valid_jump_target with target]
C --> D{jump_targets.get at target/8}
D -- None --> E[InvalidJump halt]
D -- Some byte --> F{byte AND 1 shift target mod 8 != 0}
F -- false --> E
F -- true --> G[Set PC = target + 1]
G --> H[Charge JUMPDEST gas]
H --> I[Continue execution]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/common/types/account.rs:29-31
The comment "The size is exactly the bytecode length" is incorrect. The bit vector holds `ceil(bytecode.len() / 8)` bytes — roughly 8× smaller than the bytecode, not equal to it.
```suggestion
// Jump targets stored as a bit vector: bit i is set iff position i is a valid JUMPDEST
// The size is ceil(bytecode.len() / 8) bytes
pub jump_targets: Vec<u8>,
```
### Issue 2 of 2
crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs:436-437
The `#[expect(clippy::as_conversions)]` attribute was there to suppress the `target as u32` and `Opcode::JUMPDEST as u8` casts, both of which were removed in this PR. With `#[expect]` semantics, if the expected lint does **not** fire, the compiler emits an `unfulfilled_lint_expectations` diagnostic — which will break CI if warnings are treated as errors.
```suggestion
if vm.current_call_frame.bytecode.is_valid_jump_target(target) {
```
Reviews (1): Last reviewed commit: "perf(l1): replace index based jump targe..." | Re-trigger Greptile
| #[expect(clippy::as_conversions, reason = "safe")] | ||
| if vm | ||
| .current_call_frame | ||
| .bytecode | ||
| .bytecode | ||
| .get(target) | ||
| .is_some_and(|&value| { | ||
| value == Opcode::JUMPDEST as u8 | ||
| && vm | ||
| .current_call_frame | ||
| .bytecode | ||
| .jump_targets | ||
| .binary_search(&(target as u32)) | ||
| .is_ok() | ||
| }) | ||
| { | ||
| if vm.current_call_frame.bytecode.is_valid_jump_target(target) { |
There was a problem hiding this comment.
The
#[expect(clippy::as_conversions)] attribute was there to suppress the target as u32 and Opcode::JUMPDEST as u8 casts, both of which were removed in this PR. With #[expect] semantics, if the expected lint does not fire, the compiler emits an unfulfilled_lint_expectations diagnostic — which will break CI if warnings are treated as errors.
| #[expect(clippy::as_conversions, reason = "safe")] | |
| if vm | |
| .current_call_frame | |
| .bytecode | |
| .bytecode | |
| .get(target) | |
| .is_some_and(|&value| { | |
| value == Opcode::JUMPDEST as u8 | |
| && vm | |
| .current_call_frame | |
| .bytecode | |
| .jump_targets | |
| .binary_search(&(target as u32)) | |
| .is_ok() | |
| }) | |
| { | |
| if vm.current_call_frame.bytecode.is_valid_jump_target(target) { | |
| if vm.current_call_frame.bytecode.is_valid_jump_target(target) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs
Line: 436-437
Comment:
The `#[expect(clippy::as_conversions)]` attribute was there to suppress the `target as u32` and `Opcode::JUMPDEST as u8` casts, both of which were removed in this PR. With `#[expect]` semantics, if the expected lint does **not** fire, the compiler emits an `unfulfilled_lint_expectations` diagnostic — which will break CI if warnings are treated as errors.
```suggestion
if vm.current_call_frame.bytecode.is_valid_jump_target(target) {
```
How can I resolve this? If you propose a fix, please make it concise.Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Motivation
The motivation for this PR are EEST tests, that crash because of exceeding memory limits on zkVM.
Currently the jump targets are stored as a vector of
u32indices, this change switches to storing it as a bit vector.Description
This change fixes the crashing tests. However the test is constructed in an unrealistic adversarial setting where most of the operations are just jumps. The question is whether this is actually practical for normal execution.
I check the storage required for
jump_targeton 500 blocks 20260424 - 24950286, by counting the jump instructions compared to length of bytecode. The total storage for index-basedjump_targetacross all contracts is 21.37MB which is ~10% higher than 19.29MB for the bit vector approach. There are contracts for which bit vector consumes more storage but on average it seems to be more efficient.In addition, the bit vector is faster because lookups are constant time, compared to binary search. I benchmarked the costs on Zisk, and there is a slight ~2% improvement. But primarily, the point of this change is to resolve the possible attack vector.
It is worth mentioning that other clients also seem to use the bit vector approach:
The new format defined by
encode_codeis breaking! It might be reasonable to recompute and storeVec<u32>for backwards compatibility. I leave this up to discussion.