Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements EIP-7939 (Count Leading Zeros) for the FEVM EVM interpreter by adding the CLZ opcode at byte position 0x1e. The opcode counts the number of leading zero bits in a 256-bit word, returning 256 when the input is zero.
Changes:
- Added CLZ opcode (0x1e) to the opcode dispatch table
- Implemented CLZ semantics using U256's
leading_zeros()method - Added comprehensive test coverage including unit tests, bytecode-level tests, and end-to-end actor tests
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| actors/evm/src/interpreter/execution.rs | Adds CLZ opcode at 0x1e to the opcode table and includes it in underflow testing |
| actors/evm/src/interpreter/instructions/mod.rs | Defines CLZ instruction using the def_primop! macro with single operand |
| actors/evm/src/interpreter/instructions/bitwise.rs | Implements clz function and includes unit tests and bytecode-level integration tests |
| actors/evm/tests/eip7939_clz.rs | Adds end-to-end test with EIP-7939 test vectors using a deployed EVM actor |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@LesnyRumcajs : are you or someone on Forest able to take a look at this one? |
There was a problem hiding this comment.
nit: this PR doesn't need to change the lockfile (fine to do it in a separate PR); especially the syn major version downgrade is suspicious.
There was a problem hiding this comment.
Removed Cargo.lock from this PR. The ruint/syn drift was unrelated to CLZ and shouldn't ride along here.
| #[inline] | ||
| pub fn clz(value: U256) -> U256 { | ||
| U256::from(value.leading_zeros()) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
+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.
There was a problem hiding this comment.
Added a /// doc comment on clz clarifying that it implements EIP-7939 Count Leading Zeroes over a 256-bit word, with CLZ(0) = 256.
| mod tests { | ||
| use crate::evm_unit_test; | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I second this. Introduce this only if other opcodes do it as well.
There was a problem hiding this comment.
Removed the standalone test_clz_opcode_value check. The execution underflow coverage still exercises CLZ alongside the other opcodes.
redpanda-f
left a comment
There was a problem hiding this comment.
Thank you so much for this work!
I second what @LesnyRumcajs has already pointed out but would really like these my suggestions to be incorporated for maintainability.
| #[inline] | ||
| pub fn clz(value: U256) -> U256 { | ||
| U256::from(value.leading_zeros()) | ||
| } |
There was a problem hiding this comment.
+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.
| } | ||
| } | ||
|
|
||
| #[inline] |
There was a problem hiding this comment.
| #[inline] | |
| #[inline] | |
| /// Implements EIP-7939 `CLZ` opcode, returns number of leading zero bits |
There was a problem hiding this comment.
Applied this in spirit. I kept the doc comment a bit more explicit and also called out CLZ(0) = 256.
|
|
||
| #[test] | ||
| fn test_clz_eip7939_vectors_unit() { | ||
| // Directly matches the EIP-7939 test cases. |
There was a problem hiding this comment.
Given this, kindly add a link for future reference on where the test cases were borrowed from.
There was a problem hiding this comment.
Added a shared test_vectors.rs helper and linked it to the EIP so the provenance is explicit.
| } | ||
|
|
||
| #[test] | ||
| fn test_clz_eip7939_vectors() { |
There was a problem hiding this comment.
Kindly remove. This seems like a copy of what you have already done earlier in the file.
There was a problem hiding this comment.
the main difference is clz_via_evm, so instead I suggest renaming the test
| fn test_clz_eip7939_vectors() { | |
| fn test_clz_eip7939_vectors_via_evm() { |
There was a problem hiding this comment.
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.
| assert_eq!(clz(U256::ONE), U256::from(255)); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| mod tests { | ||
| use crate::evm_unit_test; | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
I second this. Introduce this only if other opcodes do it as well.
actors/evm/tests/eip7939_clz.rs
Outdated
| let rt = util::construct_and_verify(initcode); | ||
|
|
||
| let vectors = [ | ||
| (U256::ZERO, U256::from(256)), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Hoisted the vectors into shared tests/test_vectors.rs, so the unit and end-to-end tests stay in sync.
| CODECOPY, | ||
| CREATE, | ||
| CREATE2, | ||
| CLZ, |
There was a problem hiding this comment.
the rest of these opcodes seem to be mostly in sequential order, so this belongs after SAR instead
There was a problem hiding this comment.
Adjusted the execution underflow list so CLZ now sits after SAR with the other bitwise opcodes.
actors/evm/tests/eip7939_clz.rs
Outdated
| // PUSH2 len | ||
| // PUSH2 offset | ||
| // PUSH1 0x00 | ||
| // CODECOPY | ||
| // PUSH2 len | ||
| // PUSH1 0x00 | ||
| // RETURN | ||
| // <runtime bytes> |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Switched the end-to-end initcode to the universal runtime constructor from your link.
actors/evm/tests/eip7939_clz.rs
Outdated
| opcodes::PUSH1, | ||
| 0x00, |
There was a problem hiding this comment.
I think you should prefer PUSH0 over PUSH1 0x00.
There was a problem hiding this comment.
Replaced the zero literals in the CLZ runtime with PUSH0.
actors/evm/tests/eip7939_clz.rs
Outdated
| opcodes::PUSH1, | ||
| 0x20, |
There was a problem hiding this comment.
Used MSIZE for the return length after MSTORE.
|
How does activation work? How do we enable the opcode only after a certain network version? |
The code in here is all latest, unlike go-state-types which has a directory for each network version, or lotus which maintains the historical versions of actors all the way back through to genesis, this code on So there's (almost) no epoch or network version aware code in builtin-actors, it just cares about "state in, state out" for any particular message it needs to process. Implementing CLZ here just means that any message in using this code gets to use it, but any builtin-actors run before now won't. We hide (almost) all of network version switching logic in Lotus/Forest/Venus, and there's a lot more than just builtin-actors versions obviously, it's a huge amount of technical debt that keeps building up. But as long as we persist with "Lotus needs to be able to run the network from genesis" then it's not going away. |
|
Addressed the review feedback in
Replied inline on the individual review threads as well. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1709 +/- ##
==========================================
- Coverage 91.23% 90.51% -0.72%
==========================================
Files 151 139 -12
Lines 32006 27638 -4368
==========================================
- Hits 29200 25017 -4183
+ Misses 2806 2621 -185
🚀 New features to boost your workflow:
|
Summary
Implements Ethereum EIP-7939 (Count Leading Zeros) for the FEVM EVM interpreter by adding the
CLZopcode at byte0x1e.Changes
CLZ(0x1e) to the opcode table and instruction dispatch.CLZsemantics over 256-bit words (CLZ(0) = 256).CLZin a deployed EVM actorNotes
cargo testmay require a wasm32-capable clang; on macOS this can be run withCC=/opt/homebrew/opt/llvm/bin/clang cargo test.Related