Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
7 changes: 7 additions & 0 deletions actors/evm/src/interpreter/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ pub mod opcodes {
0x1b: SHL,
0x1c: SHR,
0x1d: SAR,
0x1e: CLZ,
0x20: KECCAK256,
0x30: ADDRESS,
0x31: BALANCE,
Expand Down Expand Up @@ -292,6 +293,11 @@ pub fn execute(
mod tests {
use crate::evm_unit_test;

#[test]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think it's a valuable test. If we want to check the def_opcodes macro works, lets write a test to for it entirely.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I second this. Introduce this only if other opcodes do it as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the standalone test_clz_opcode_value check. The execution underflow coverage still exercises CLZ alongside the other opcodes.

fn test_clz_opcode_value() {
assert_eq!(super::opcodes::CLZ, 0x1e);
}

macro_rules! check_underflow_err {
($($ins:ident,)*) => {
$(do_check_underflow_err!($ins);)*
Expand Down Expand Up @@ -415,6 +421,7 @@ mod tests {
CODECOPY,
CREATE,
CREATE2,
CLZ,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the rest of these opcodes seem to be mostly in sequential order, so this belongs after SAR instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adjusted the execution underflow list so CLZ now sits after SAR with the other bitwise opcodes.

RETURN,
REVERT,
SELFDESTRUCT,
Expand Down
72 changes: 72 additions & 0 deletions actors/evm/src/interpreter/instructions/bitwise.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,81 @@ pub fn sar(shift: U256, mut value: U256) -> U256 {
}
}

#[inline]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
#[inline]
#[inline]
/// Implements EIP-7939 `CLZ` opcode, returns number of leading zero bits

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied this in spirit. I kept the doc comment a bit more explicit and also called out CLZ(0) = 256.

pub fn clz(value: U256) -> U256 {
U256::from(value.leading_zeros())
}
Comment on lines +48 to +51
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: it might be obvious to some, but my brain doesn't immediately click that clz is count leading zeroes. One line of docs would reduce mental overhead.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

+1 on this. Try /// docstring. This shows up in editors cleanly.

I see that this is also okay in a way that EIP calling it "CLZ", so should be fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a /// doc comment on clz clarifying that it implements EIP-7939 Count Leading Zeroes over a 256-bit word, with CLZ(0) = 256.


#[cfg(test)]
mod tests {
use super::*;
use crate::interpreter::opcodes;
use crate::interpreter::{execution::Machine, system::System, Output};
use crate::{Bytecode, EthAddress, ExecutionState};
use fil_actors_runtime::test_utils::MockRuntime;
use fvm_shared::econ::TokenAmount;

#[test]
fn test_clz_eip7939_vectors_unit() {
// Directly matches the EIP-7939 test cases.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Given this, kindly add a link for future reference on where the test cases were borrowed from.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a shared test_vectors.rs helper and linked it to the EIP so the provenance is explicit.

assert_eq!(clz(U256::ZERO), U256::from(256));
assert_eq!(clz(U256::ONE << 255), U256::ZERO);
assert_eq!(clz(U256::MAX), U256::ZERO);
assert_eq!(clz(U256::ONE << 254), U256::ONE);
assert_eq!(clz((U256::ONE << 255) - U256::ONE), U256::ONE);
assert_eq!(clz(U256::ONE), U256::from(255));
}

#[test]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not entirely sure of this test. Either we introduce a wide range of tests, or a fuzzy test here, or none at all. This seems quite arbitrary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the extra test_clz_misc_unit case. The remaining unit, machine-step, and end-to-end coverage now all use the canonical EIP-7939 vectors.

fn test_clz_misc_unit() {
// 2 is 10 binary, so 256 - 2 = 254
assert_eq!(clz(U256::from(2)), U256::from(254));
}

fn clz_via_evm(value: U256) -> U256 {
let rt = MockRuntime::default();
rt.in_call.replace(true);

let mut state = ExecutionState::new(
EthAddress::from_id(1000),
EthAddress::from_id(1000),
TokenAmount::from_atto(0),
Vec::new(),
);

let mut imm = [0u8; 32];
value.write_as_big_endian(&mut imm);

let mut code = Vec::with_capacity(1 + 32 + 1);
code.push(opcodes::PUSH32);
code.extend_from_slice(&imm);
code.push(opcodes::CLZ);

let mut system = System::new(&rt, false);
let bytecode = Bytecode::new(code);
let mut machine = Machine {
system: &mut system,
state: &mut state,
bytecode: &bytecode,
pc: 0,
output: Output::default(),
};

machine.step().expect("PUSH32 step failed");
machine.step().expect("CLZ step failed");
machine.state.stack.pop_many::<1>().expect("missing CLZ result")[0]
}

#[test]
fn test_clz_eip7939_vectors() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Kindly remove. This seems like a copy of what you have already done earlier in the file.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the main difference is clz_via_evm, so instead I suggest renaming the test

Suggested change
fn test_clz_eip7939_vectors() {
fn test_clz_eip7939_vectors_via_evm() {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Kept the machine-level coverage, renamed it to test_clz_eip7939_vectors_via_evm, and switched it to the shared vectors so it's no longer a copy/paste duplicate.

// From EIP-7939 test cases.
assert_eq!(clz_via_evm(U256::ZERO), U256::from(256));
assert_eq!(clz_via_evm(U256::ONE << 255), U256::ZERO);
assert_eq!(clz_via_evm(U256::MAX), U256::ZERO);
assert_eq!(clz_via_evm(U256::ONE << 254), U256::ONE);
assert_eq!(clz_via_evm((U256::ONE << 255) - U256::ONE), U256::ONE);
assert_eq!(clz_via_evm(U256::ONE), U256::from(255));
}

#[test]
fn test_shl() {
Expand Down
1 change: 1 addition & 0 deletions actors/evm/src/interpreter/instructions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ def_primop! { BYTE(a, b) => bitwise::byte }
def_primop! { SHL(a, b) => bitwise::shl }
def_primop! { SHR(a, b) => bitwise::shr }
def_primop! { SAR(a, b) => bitwise::sar }
def_primop! { CLZ(a) => bitwise::clz }
// dup
def_stackop! { DUP1 => stack::dup::<1> }
def_stackop! { DUP2 => stack::dup::<2> }
Expand Down
89 changes: 89 additions & 0 deletions actors/evm/tests/eip7939_clz.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
use fil_actor_evm::interpreter::opcodes;
use fil_actors_evm_shared::uints::U256;

mod util;

fn initcode_for_runtime(runtime: &[u8]) -> Vec<u8> {
let len: u16 = runtime.len().try_into().expect("runtime too large");

// initcode:
// PUSH2 len
// PUSH2 offset
// PUSH1 0x00
// CODECOPY
// PUSH2 len
// PUSH1 0x00
// RETURN
// <runtime bytes>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There's a shorter initcode prefix that doesn't parameterize len, sometimes called the universal runtime constructor.

SUB(CODESIZE,11)
CODECOPY(RETURNDATASIZE,11,DUP1)
RETURNDATASIZE
RETURN

600b380380600b3d393df3

(It uses RETURNDATASIZE for backwards compatibility since not all chains have PUSH0)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Switched the end-to-end initcode to the universal runtime constructor from your link.

let init_len: u16 = 15;
let offset = init_len;

let [len_hi, len_lo] = len.to_be_bytes();
let [off_hi, off_lo] = offset.to_be_bytes();

let mut code = vec![
opcodes::PUSH2,
len_hi,
len_lo,
opcodes::PUSH2,
off_hi,
off_lo,
opcodes::PUSH1,
0x00,
opcodes::CODECOPY,
opcodes::PUSH2,
len_hi,
len_lo,
opcodes::PUSH1,
0x00,
opcodes::RETURN,
];
code.extend_from_slice(runtime);
code
}

fn clz_runtime() -> Vec<u8> {
// Reads a 32-byte word from calldata[0..32], computes CLZ, and returns the 32-byte result.
vec![
opcodes::PUSH1,
0x00,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think you should prefer PUSH0 over PUSH1 0x00.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Replaced the zero literals in the CLZ runtime with PUSH0.

opcodes::CALLDATALOAD,
opcodes::CLZ,
opcodes::PUSH1,
0x00,
opcodes::MSTORE,
opcodes::PUSH1,
0x20,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MSIZE

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Used MSIZE for the return length after MSTORE.

opcodes::PUSH1,
0x00,
opcodes::RETURN,
]
}

fn u256_be_bytes(value: U256) -> [u8; 32] {
let mut out = [0u8; 32];
value.write_as_big_endian(&mut out);
out
}

#[test]
fn eip7939_clz_vectors_end_to_end() {
let initcode = initcode_for_runtime(&clz_runtime());
let rt = util::construct_and_verify(initcode);

let vectors = [
(U256::ZERO, U256::from(256)),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These combinations are used often, once in unit test and here as well. Kindly don't repeat yourself, hoist it in some "test_vectors.rs" or something. Otherwise, it will go out of sync later

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hoisted the vectors into shared tests/test_vectors.rs, so the unit and end-to-end tests stay in sync.

(U256::ONE << 255, U256::ZERO),
(U256::MAX, U256::ZERO),
(U256::ONE << 254, U256::ONE),
((U256::ONE << 255) - U256::ONE, U256::ONE),
(U256::ONE, U256::from(255)),
];

for (input, expected) in vectors {
let ret = util::invoke_contract(&rt, &u256_be_bytes(input));
assert_eq!(ret.len(), 32);
assert_eq!(ret.as_slice(), &u256_be_bytes(expected));
}
}

Loading