EIP7939: Count leading zeros (CLZ) opcode #1246
Conversation
WalkthroughAdds a new CLZ opcode behind the Galileo fork (EIP-7939), wires it into the jump table and interpreter selection, extends chain config to recognize Galileo by time, introduces corresponding tests, and bumps a Go module dependency in a toolkit submodule. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Caller
participant EVM as EVM
participant Int as Interpreter
participant JT as JumpTable
participant Stk as Stack
Note over EVM,Int: Execution during Galileo fork
App->>EVM: Execute bytecode containing CLZ
EVM->>Int: Step opcode
Int->>JT: Resolve handler for CLZ
JT-->>Int: opCLZ handler (gas/model configured)
rect rgba(200,230,255,0.18)
Int->>Stk: Peek top value
Int->>Stk: Replace top with leading-zero count
end
Int-->>EVM: Step complete
EVM-->>App: Continue/return
sequenceDiagram
autonumber
participant Cfg as ChainConfig
participant Rules as Rules()
participant Int as NewEVMInterpreter
participant JT as JumpTable
participant IS as galileoInstructionSet
Note over Cfg,Rules: Initialization at block time T
Cfg->>Rules: Rules(num, T)
Rules-->>Int: { IsGalileo: true/false, … }
alt IsGalileo == true
Int->>IS: Build instruction set (enable EIP-7939)
IS->>JT: Provide jump table with CLZ handler
else
Int->>JT: Select other instruction set
end
Int-->>JT: Use selected jump table
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/vm/jump_table.go (1)
72-74: Minor: Pre-existing typo in comment.The comment contains "contantinople" which should be "constantinople". This typo also appears in other function comments (e.g., lines 81-83) and is pre-existing, not introduced by this PR.
Apply this diff to fix the typo (optional cleanup):
// newGalileoInstructionSet returns the frontier, homestead, byzantium, -// contantinople, istanbul, petersburg, berlin, london, shanghai, curie, darwin, euclidV2, +// constantinople, istanbul, petersburg, berlin, london, shanghai, curie, darwin, euclidV2, // feynman and galileo instructions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rollup/missing_header_fields/export-headers-toolkit/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
core/vm/eips.go(2 hunks)core/vm/instructions_test.go(1 hunks)core/vm/interpreter.go(1 hunks)core/vm/jump_table.go(1 hunks)core/vm/opcodes.go(3 hunks)params/config.go(4 hunks)rollup/missing_header_fields/export-headers-toolkit/go.mod(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-09T19:18:56.728Z
Learnt from: Thegaram
PR: scroll-tech/go-ethereum#1245
File: crypto/kzg4844/kzg4844_gokzg.go:118-150
Timestamp: 2025-10-09T19:18:56.728Z
Learning: The scroll-tech/go-ethereum repository uses Go 1.22+ and supports range-over-integer syntax such as `for range n` and `for i := range len(slice)`, which are valid Go 1.22 language features.
Applied to files:
rollup/missing_header_fields/export-headers-toolkit/go.mod
🧬 Code graph analysis (2)
core/vm/eips.go (5)
core/vm/interpreter.go (2)
EVMInterpreter(58-67)ScopeContext(43-47)core/vm/stack.go (1)
Stack(35-37)core/vm/jump_table.go (1)
JumpTable(70-70)core/vm/opcodes.go (1)
CLZ(72-72)core/vm/gas.go (1)
GasFastestStep(26-26)
core/vm/instructions_test.go (4)
core/vm/evm.go (3)
NewEVM(141-152)BlockContext(73-89)TxContext(93-100)params/config.go (1)
TestChainConfig(505-545)core/vm/interpreter.go (2)
Config(29-39)ScopeContext(43-47)core/vm/stack.go (1)
Stack(35-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
rollup/missing_header_fields/export-headers-toolkit/go.mod (1)
8-8: LGTM: Routine dependency update.The da-codec dependency bump is a standard patch-level update with no behavioral impact on the core CLZ opcode implementation.
core/vm/opcodes.go (2)
72-72: LGTM: CLZ opcode constant added correctly.The opcode value 0x1e is appropriately placed in the 0x10-0x1f range designated for comparison and bit operations, immediately following SAR (0x1d).
259-259: LGTM: Bidirectional opcode mappings are consistent.Both the OpCode→string and string→OpCode mappings correctly include CLZ, following the established pattern.
Also applies to: 435-435
core/vm/jump_table.go (1)
66-66: LGTM: Galileo instruction set follows established pattern.The
galileoInstructionSetcorrectly extendsfeynmanInstructionSetand enables EIP-7939 (CLZ opcode), following the same pattern used for previous fork instruction sets.Also applies to: 72-79
core/vm/interpreter.go (1)
77-78: LGTM: Galileo rule integration is correct.The
IsGalileocheck is properly placed beforeIsFeynmanin the jump table selection logic, ensuring that Galileo-enabled chains select thegalileoInstructionSetwhich includes the CLZ opcode.core/vm/eips.go (2)
285-290: LGTM: opCLZ implementation is correct.The CLZ (Count Leading Zeros) operation correctly computes
256 - BitLen():
- For
0x0: BitLen() = 0 → result = 256 (all bits are leading zeros)- For
0x1: BitLen() = 1 → result = 255- For
0x8000...0: BitLen() = 256 → result = 0 (no leading zeros)The use of
peek()instead ofpop()followed by implicit stack modification is correct for a unary operation.
42-42: LGTM: EIP-7939 activation follows established pattern.The
enable7939function correctly registers the CLZ opcode with appropriate gas cost (GasFastestStep = 3) and stack constraints (minStack(1,1), maxStack(1,1)), consistent with other bit operation opcodes like SHL, SHR, and SAR.Also applies to: 292-299
core/vm/instructions_test.go (1)
852-890: LGTM: Comprehensive CLZ opcode test coverage.The test cases thoroughly validate the CLZ implementation:
- Zero value (all leading zeros)
- Single bit set (255 leading zeros)
- Multi-bit values (0x6ff, 0xffffffffff)
- Edge cases at bit boundaries (0x4000...0, 0x7fff...f)
- Maximum values (0x8000...0, 0xffff...f with zero leading zeros)
The test correctly:
- Uses a fresh stack and PC for each test case
- Calls
opCLZdirectly to test the operation in isolation- Validates stack depth remains 1 after operation
- Verifies computed results match expected leading zero counts
params/config.go (3)
673-673: LGTM: Galileo fork configuration follows established pattern.The
GalileoTimefield andIsGalileomethod correctly follow the pattern used for other time-based forks (Feynman, EuclidV2, etc.), using theisForkedTimehelper for activation checks.Also applies to: 1018-1020
1255-1255: LGTM: Galileo rule integrated correctly.The
IsGalileofield is properly added to theRulesstruct and populated in theRules()method, ensuring the Galileo fork state is available to the EVM interpreter for instruction set selection.Also applies to: 1284-1284
311-362: Note: Galileo activation time not yet scheduled.The
ScrollSepoliaChainConfigandScrollMainnetChainConfigdo not include aGalileoTimevalue, meaning the fork is not yet scheduled for activation. This appears intentional for this infrastructure PR.Confirm that the Galileo fork activation will be scheduled in a separate PR or configuration update.
Also applies to: 364-415
3 participants
1. Purpose or design rationale of this PR
Cherry pick EIP7939: Count leading zeros (CLZ) opcode
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit