Skip to content

fix(levm)!: fix jumpdests for large initcodes #5254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Nov 11, 2025
Merged

fix(levm)!: fix jumpdests for large initcodes #5254

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
226f91a
fix jumpdests for large initcodes
iovoid Nov 10, 2025
9d129ce
use sizeof instead of constants
iovoid Nov 11, 2025
084c8e2
save jump_targets as rlp
iovoid Nov 11, 2025
da77868
add comment explaining why u32
iovoid Nov 11, 2025
8c7b633
clippy
iovoid Nov 11, 2025
4d3ef13
fix deserialization
iovoid Nov 11, 2025
4a7156a
Merge branch 'main' into jumpdest_large_initcode
jrchatruc Nov 11, 2025
6f974b4
Merge branch 'main' into jumpdest_large_initcode
jrchatruc Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions crates/common/types/account.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ use crate::{
pub struct Code {
pub hash: H256,
pub bytecode: Bytes,
// TODO: Consider using Arc<[u16]> (needs to enable serde rc feature)
pub jump_targets: Vec<u16>,
// TODO: Consider using Arc<[u32]> (needs to enable serde rc feature)
Copy link
Contributor

@JereSalo JereSalo Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment here or anywhere you consider convenient about the reason why we use 32 bits? So nobody changes it to u16 back again
If you want you can link EIP-3860 but it's not necessary

Copy link
Contributor

@Oppen Oppen Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it would be a good idea to link to the EIP. Also to the failing EEST test.

Copy link
Contributor

@Oppen Oppen Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When in doubt, overcommenting is better than undercommenting. Don't let school tell you otherwise.

Copy link
Contributor Author

@iovoid iovoid Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment in da77868

pub jump_targets: Vec<u32>,
}

impl Code {
Expand All @@ -40,16 +40,16 @@ impl Code {
}
}

fn compute_jump_targets(code: &[u8]) -> Vec<u16> {
debug_assert!(code.len() <= u16::MAX as usize);
fn compute_jump_targets(code: &[u8]) -> Vec<u32> {
debug_assert!(code.len() <= u32::MAX as usize);
let mut targets = Vec::new();
let mut i = 0;
while i < code.len() {
// TODO: we don't use the constants from the vm module to avoid a circular dependency
match code[i] {
// OP_JUMPDEST
0x5B => {
targets.push(i as u16);
targets.push(i as u32);
}
// OP_PUSH1..32
c @ 0x60..0x80 => {
Expand Down
10 changes: 5 additions & 5 deletions crates/storage/store_db/rocksdb.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,7 +884,7 @@ impl StoreEngine for Store {
}

for (code_hash, code) in update_batch.code_updates {
let mut buf = Vec::with_capacity(6 + code.bytecode.len() + 2 * code.jump_targets.len());
let mut buf = Vec::with_capacity(6 + code.bytecode.len() + 4 * code.jump_targets.len());
Copy link
Contributor

@pablodeymo pablodeymo Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to use size_of to avoid hard-coding this lenght

Copy link
Contributor Author

@iovoid iovoid Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved in 9d129ce

code.bytecode.encode(&mut buf);
code.jump_targets
.into_iter()
Expand Down Expand Up @@ -1258,7 +1258,7 @@ impl StoreEngine for Store {

async fn add_account_code(&self, code: Code) -> Result<(), StoreError> {
let hash_key = code.hash.0.to_vec();
let mut buf = Vec::with_capacity(6 + code.bytecode.len() + 2 * code.jump_targets.len());
let mut buf = Vec::with_capacity(6 + code.bytecode.len() + 4 * code.jump_targets.len());
code.bytecode.encode(&mut buf);
code.jump_targets
.into_iter()
Expand Down Expand Up @@ -1306,8 +1306,8 @@ impl StoreEngine for Store {
hash: code_hash,
bytecode: Bytes::copy_from_slice(bytecode),
jump_targets: targets
.chunks_exact(2)
.map(|c| u16::from_le_bytes([c[0], c[1]]))
.chunks_exact(4)
.map(|c| u32::from_le_bytes([c[0], c[1], c[2], c[3]]))
Copy link
Contributor

@pablodeymo pablodeymo Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talking with @Oppen, we think that it's better to encode the jump_target in RLP, which will be smaller than 32 bits for the common case.

Copy link
Contributor

@Oppen Oppen Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better is a strong word for any use of RLP. But in the tradeoff IO probably wins, and RLP is better for IO than always using all the bytes.

Copy link
Contributor Author

@iovoid iovoid Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 084c8e2

.collect(),
};
Ok(Some(code))
Expand Down Expand Up @@ -1868,7 +1868,7 @@ impl StoreEngine for Store {

for (code_hash, code) in account_codes {
let key = code_hash.as_bytes().to_vec();
let mut buf = Vec::with_capacity(6 + code.bytecode.len() + 2 * code.jump_targets.len());
let mut buf = Vec::with_capacity(6 + code.bytecode.len() + 4 * code.jump_targets.len());
code.bytecode.encode(&mut buf);
code.jump_targets
.into_iter()
Expand Down
10 changes: 6 additions & 4 deletions crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ impl<'a> VM<'a> {
/// - Checking that the byte at the requested target PC is a JUMPDEST (0x5B).
/// - Ensuring the byte is not blacklisted. In other words, the 0x5B value is not part of a
/// constant associated with a push instruction.
fn target_address_is_valid(call_frame: &mut CallFrame, jump_address: u16) -> bool {
fn target_address_is_valid(call_frame: &mut CallFrame, jump_address: u32) -> bool {
call_frame
.bytecode
.jump_targets
Expand All @@ -305,14 +305,16 @@ impl<'a> VM<'a> {
/// to be equal to the specified address. If the address is not a
/// valid JUMPDEST, it will return an error
pub fn jump(call_frame: &mut CallFrame, jump_address: U256) -> Result<(), VMError> {
let jump_address_u16 = jump_address
let jump_address_u32 = jump_address
.try_into()
.map_err(|_err| ExceptionalHalt::VeryLargeNumber)?;

#[expect(clippy::arithmetic_side_effects)]
if Self::target_address_is_valid(call_frame, jump_address_u16) {
if Self::target_address_is_valid(call_frame, jump_address_u32) {
call_frame.increase_consumed_gas(gas_cost::JUMPDEST)?;
call_frame.pc = usize::from(jump_address_u16) + 1;
call_frame.pc = usize::try_from(jump_address_u32)
.map_err(|_err| ExceptionalHalt::VeryLargeNumber)?
+ 1;
Ok(())
} else {
Err(ExceptionalHalt::InvalidJump.into())
Expand Down
Loading