EIP:7823 Set upper bounds for MODEXP && EIP-7883: ModExp Gas Cost && EIP-7951: Precompile for secp256r1 Curve Support #1248
Conversation
WalkthroughAdds Galileo fork support for precompiled contracts: introduces Galileo precompile addresses and maps, extends bigModExp with EIP‑2565/7823/7883 flags and gas paths (including input caps and uint256-safe lengths), adds EIP‑7951-aware p256Verify and Galileo gas constant, plus tests and EIP‑7883 modexp vectors. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PrecompileRouter
participant bigModExp
participant p256Verify
Note over PrecompileRouter: Fork-aware precompile selection
Caller->>PrecompileRouter: Request precompile (rules)
PrecompileRouter-->>Caller: Return fork-specific precompile set (Galileo or legacy)
rect rgb(255,250,240)
Caller->>bigModExp: invoke(modexp input)
bigModExp->>bigModExp: parse lengths from 32-byte uint256 (overflow-safe)
alt eip7823 enabled (Galileo)
bigModExp->>bigModExp: enforce 1024-byte cap, use galileoMultComplexity
else eip2565 enabled (Berlin)
bigModExp->>bigModExp: use berlinMultComplexity (EIP‑2565)
else
bigModExp->>bigModExp: use byzantiumMultComplexity
end
bigModExp->>bigModExp: compute base^exp mod mod (with edge-case handling)
bigModExp-->>Caller: left-padded result (or zero-length)
end
rect rgb(240,255,240)
Caller->>p256Verify: invoke(p256 input)
p256Verify->>p256Verify: check eip7951 flag
alt eip7951 enabled
p256Verify-->>Caller: cost = P256VerifyGasGalileo
else
p256Verify-->>Caller: cost = P256VerifyGas
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 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 (2)
core/vm/contracts.go(12 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
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/vm/contracts.go (1)
526-536: Approve optimization, but consider clarifying the comment.The switch statement refactoring and fast path optimization for
basewithBitLen() == 1are correct. When abig.Intderived fromSetByteshasBitLen() == 1, it can only be the value 1, making the optimization1^exp % mod = 1 % modvalid.Consider clarifying the comment at line 530-531 to explain why
BitLen() == 1means the base is 1:- case base.BitLen() == 1: // a bit length of 1 means it's 1 (or -1). - //If base == 1, then we can just return base % mod (if mod >= 1, which it is) + case base.BitLen() == 1: // BitLen() == 1 means base == 1 (since base is non-negative from SetBytes) + // Fast path: 1^exp % mod = 1 % mod v = base.Mod(base, mod).Bytes()
📜 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 (2)
core/vm/contracts.go(11 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 (1)
core/vm/contracts.go (3)
common/types.go (1)
BytesToAddress(206-210)params/config.go (1)
Rules(1244-1251)common/bytes.go (1)
LeftPadBytes(120-129)
🪛 GitHub Actions: CI
core/vm/contracts.go
[error] 233-233: core/vm/contracts.go:233:13: rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)
🪛 GitHub Check: build-mock-ccc-geth
core/vm/contracts.go
[failure] 233-233:
rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)
🪛 GitHub Check: check
core/vm/contracts.go
[failure] 233-233:
rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)) (typecheck)
[failure] 233-233:
rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)) (typecheck)
[failure] 233-233:
rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)) (typecheck)
🪛 GitHub Check: test
core/vm/contracts.go
[failure] 233-233:
rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
core/vm/contracts.go (5)
67-67: LGTM: Backward-compatible feature flag addition.The explicit
eip7823: falseflag added to existing precompiled contract maps ensures backward compatibility while preparing for the new Galileo release. This is a safe and well-structured change.Also applies to: 80-80, 94-94, 108-108, 122-122, 136-136, 151-151
159-172: LGTM: Galileo precompile map correctly enables EIP-7823.The new
PrecompiledContractsGalileomap follows the established pattern and correctly enables EIP-7823 for thebigModExpprecompile (line 166) while maintaining all other precompiles from the Feynman release.
189-189: LGTM: Address list initialization follows established pattern.The declaration and initialization of
PrecompiledAddressesGalileocorrectly follows the same pattern used for other releases.Also applies to: 225-227
373-373: LGTM: Feature flag field added to bigModExp struct.The addition of the
eip7823field to thebigModExpstruct is correct and enables EIP-7823 specific behavior in the precompile implementation.
494-513: 1024-byte limit matches EIP-7823. Verified each MODEXP length field is capped at 8192 bits (1024 bytes) per spec; implementation is correct.
georgehao
changed the title
georgehao
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/vm/contracts_test.go(3 hunks)crypto/secp256r1/verifier.go(1 hunks)params/protocol_params.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crypto/secp256r1/verifier.go (1)
crypto/ecies/ecies.go (1)
PublicKey(54-59)
core/vm/contracts_test.go (1)
common/types.go (1)
BytesToAddress(206-210)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
crypto/secp256r1/verifier.go (1)
9-15: LGTM! Cleaner validation and key construction.The refactor simplifies the verification logic by:
- Directly validating x and y are non-nil and on the P-256 curve
- Constructing the
ecdsa.PublicKeyinline instead of using a helper functionThe validation is equivalent to the previous approach and maintains the same security guarantees.
core/vm/contracts_test.go (2)
53-55: LGTM! Correct EIP-7883 precompile configuration.The addition of the
eip7883field tobigModExpinstances properly differentiates the three modexp variants:
- Address 5 (0x05): Original modexp (pre-EIP-2565)
- Address 0xf5: EIP-2565 only
- Address 0xf6: EIP-2565 + EIP-7883 (new)
This structure allows testing all modexp variations independently.
248-250: LGTM! EIP-7883 test coverage added.The new test and benchmark functions correctly reference:
- Test data file:
modexp_eip7883.json- Precompile address:
f6(the newly added EIP-7883-enabled modexp)This provides proper test coverage for the new EIP-7883 functionality.
params/protocol_params.go (1)
165-165: Clarify rationale for doubling P256VerifyGas.
The PR increases secp256r1 precompile gas from 3450 to 6900, but no existing EIP mandates this. Please provide:
- Spec or proposal reference (e.g. draft EIP-7951) or internal benchmarks justifying 6900
- Performance data supporting this increase
- Confirmation if this is a breaking change requiring a hard fork
|
The CI failure is becuase it need |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/vm/contracts.go (1)
379-379: Verify ifbig0is actually used.The variable
big0is declared but doesn't appear to be referenced anywhere in the provided code. If it's genuinely unused, consider removing it to keep the code clean.Run the following script to check for references to
big0:#!/bin/bash # Search for references to big0 variable rg -n '\bbig0\b' --type=go
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/vm/contracts.go(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/vm/contracts.go (3)
params/config.go (1)
Rules(1244-1251)common/math/integer.go (1)
MaxUint64(38-38)common/bytes.go (1)
LeftPadBytes(120-129)
🪛 GitHub Actions: CI
core/vm/contracts.go
[error] 235-235: core/vm/contracts.go:235:13: rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)
🪛 GitHub Check: build-mock-ccc-geth
core/vm/contracts.go
[failure] 235-235:
rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)
🪛 GitHub Check: check
core/vm/contracts.go
[failure] 235-235:
rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)) (typecheck)
[failure] 235-235:
rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)) (typecheck)
[failure] 235-235:
rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)) (typecheck)
🪛 GitHub Check: test
core/vm/contracts.go
[failure] 235-235:
rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
core/vm/contracts.go (10)
381-461: LGTM! Excellent overflow handling in complexity functions.The three complexity functions (
byzantiumMultComplexity,berlinMultComplexity,galileoMultComplexity) properly implement their respective EIP formulas with consistent overflow detection usingbits.Mul64andbits.Add64. Returningmath.MaxUint64on overflow is the correct approach.
463-486: LGTM! Correct iteration count calculation.The
modexpIterationCountfunction properly handles overflow cases and correctly implements the adjusted exponent length calculation. The use ofuint256.Int.BitLen()and the minimum return value of 1 are appropriate.
488-555: LGTM! Gas calculation functions correctly implement their respective EIPs.The three gas calculation functions properly implement Byzantium (EIP-198), Berlin (EIP-2565), and Galileo (EIP-7883) gas cost formulas with appropriate multipliers, divisors, and minimum gas values. Overflow handling is consistent across all variants.
615-623: Good backward compatibility preservation for EIP-2565.The conditional check at lines 617-623 correctly maintains the 32-byte input limit for pre-Galileo forks while allowing larger inputs for EIP-7823/7883. The comment explains this is retained for backward compatibility, which is the right approach to avoid state root mismatches on historical blocks.
632-634: Correct EIP-7823 input length cap enforcement.The 1024-byte cap is properly enforced by checking both
inputLenOverflow(for lengths > 64 bits) and the actual max value. This prevents excessive memory usage while maintaining compatibility with the EIP-7823 specification.
647-656: Good optimization for modexp edge cases.The special case handling for
mod.BitLen() == 0(undefined modulo) andbase.BitLen() == 1(base equals 1 or -1) provides correct results while avoiding unnecessary expensive exponentiation operations.
161-174: LGTM! Galileo precompile configuration is correct.The
PrecompiledContractsGalileomap properly configures all precompiles with thebigModExpat address 5 having all three EIP flags (eip2565,eip7823,eip7883) set totrue. This enables the new ModExp behavior for the Galileo fork.
69-168: LGTM! Correct EIP flag progression across forks.The
bigModExpinstantiations properly reflect the EIP activation timeline:
- Byzantium/Istanbul: No EIP-2565 (original Byzantium rules)
- Berlin through Feynman: EIP-2565 only
- Galileo: All three EIPs (2565, 7823, 7883) enabled
This maintains backward compatibility and correct gas calculations for each fork.
559-575: LGTM! Proper use of uint256 for overflow-safe length parsing.The conversion from
uint256.Inttouint64with overflow capping atmath.MaxUint64correctly handles edge cases where input length values exceed 64 bits. TheIsUint64()check is the right approach for safe conversion.
589-589: Remove the TODO comment at line 589.The TODO's concern about
getDatareturning an empty slice whenexpLen < baseLenis unfounded. ThegetDatafunction incore/vm/common.go(lines 55-65) correctly handles this case: it clamps bounds and returns a slice right-padded to the requested size. The edge case is already safe and well-defined, so the TODO can be removed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/vm/contracts.go (1)
379-379: Remove unused variable.The
big0variable is declared but never referenced in the code.Apply this diff:
-var big0 = big.NewInt(0) -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/vm/contracts.go(12 hunks)params/protocol_params.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/vm/contracts.go (5)
params/config.go (1)
Rules(1244-1251)common/math/integer.go (1)
MaxUint64(38-38)common/bytes.go (1)
LeftPadBytes(120-129)common/math/big.go (1)
Exp(246-259)params/protocol_params.go (1)
P256VerifyGasGalileo(166-166)
🪛 GitHub Actions: CI
core/vm/contracts.go
[error] 235-235: rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)
🪛 GitHub Check: build-mock-ccc-geth
core/vm/contracts.go
[failure] 235-235:
rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)
🪛 GitHub Check: test
core/vm/contracts.go
[failure] 235-235:
rules.IsGalileo undefined (type params.Rules has no field or method IsGalileo)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
params/protocol_params.go (1)
165-166: LGTM! Galileo gas constant properly introduced.The two-constant approach correctly preserves the existing
P256VerifyGasfor pre-Galileo blocks while introducingP256VerifyGasGalileofor the Galileo fork, preventing retroactive state root mismatches.Based on past review comments.
core/vm/contracts.go (10)
24-26: LGTM! Required imports for overflow-safe arithmetic.The
math/bitspackage enables proper overflow detection in the new gas calculation functions, anduint256handles large input lengths safely.
373-377: LGTM! Clean struct extension for EIP flags.The additional boolean flags follow the established pattern and enable conditional behavior for EIP-7823 (input size caps) and EIP-7883 (new gas formula).
381-461: LGTM! Complexity calculations with proper overflow handling.All three complexity functions correctly implement their respective EIP formulas:
byzantiumMultComplexity: EIP-198 formula with three-tier complexityberlinMultComplexity: ceiling(x/8)² formula from EIP-2565galileoMultComplexity: EIP-7883 formula with 16 minimum for x ≤ 32The use of
bits.Mul64andbits.Add64with overflow detection ensures safe arithmetic.
463-486: LGTM! Iteration count calculation with overflow safety.The function correctly computes the adjusted exponent length using the multiplier and expHead's bit length, with proper overflow detection throughout.
488-555: LGTM! Gas calculation functions correctly implement EIP formulas.All three gas functions properly implement their respective specifications:
- Byzantium: (complexity × iterations × 8) ÷ 20
- Berlin: max(200, (complexity × iterations × 8) ÷ 3)
- Galileo: max(500, complexity × iterations × 16)
Overflow handling is consistent throughout using
bits.Mul64.
558-602: LGTM with a TODO to address.The refactored
RequiredGascorrectly usesuint256for safe length parsing and delegates to the appropriate gas calculation based on EIP flags.However, please verify or remove the TODO comment on line 589. The current logic appears correct: when
expLen < baseLen,getData(input, baseLen, expLen)will return a slice of lengthexpLen(not empty), which is the intended behavior for extracting the exponent head.
604-658: LGTM! Well-structured Run implementation with proper edge case handling.Key strengths:
- Lines 617-623: Preserves EIP-2565 backward compatibility with 32-byte limit check
- Lines 632-634: Enforces EIP-7823's 1024-byte cap using overflow-safe check
- Lines 647-656: Handles edge cases (mod 0, base 1) efficiently before the general exponentiation
69-174: LGTM! Precompile configurations correctly reflect fork-specific EIP activations.The maps show a clean progression:
- Pre-Berlin forks: Byzantium modexp behavior
- Berlin through Feynman: EIP-2565 enabled
- Galileo: Adds EIP-7823 (size caps) and EIP-7883 (new gas formula) for modexp, plus EIP-7951 for p256Verify
All configurations maintain backward compatibility.
191-236: Galileo initialization follows established patterns.The initialization and
ActivePrecompilesrouting for Galileo correctly follows the same pattern as other forks. TheIsGalileofield missing fromparams.Rulesis a known dependency on PR #1246, as documented in the PR objectives and past review comments.Note: The compilation errors are expected and will be resolved once PR #1246 is merged.
1334-1344: LGTM! p256Verify correctly implements fork-dependent gas costs.The
eip7951flag controls whether to use the new Galileo gas cost (6900) or the existing EuclidV2 gas cost (3450), ensuring backward compatibility while supporting EIP-7951.
…ethereum into feat/cherry_pick_eip7823
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/vm/contracts.go (1)
380-380: Remove unused variable.The
big0variable is declared but never used in the code. Please remove it to keep the codebase clean.-var big0 = big.NewInt(0) - // byzantiumMultComplexity implements the bigModexp multComplexity formula, as defined in EIP-198.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/vm/contracts.go(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/vm/contracts.go (5)
params/config.go (1)
Rules(1249-1256)common/math/integer.go (1)
MaxUint64(38-38)common/bytes.go (1)
LeftPadBytes(120-129)common/math/big.go (1)
Exp(246-259)params/protocol_params.go (1)
P256VerifyGasGalileo(166-166)
⏰ 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 (18)
core/vm/contracts.go (18)
24-26: LGTM!The new imports (
math/bitsanduint256) are necessary for overflow-safe arithmetic in the modexp gas calculations and for handling large length values.
70-175: LGTM!The precompiled contract maps are correctly updated across all forks with appropriate EIP flags. The Galileo fork properly enables EIP-7823 and EIP-7883 for
bigModExp, and EIP-7951 forp256Verify.
192-192: LGTM!The
PrecompiledAddressesGalileovariable declaration follows the established pattern for fork-specific address lists.
228-230: LGTM!The initialization loop correctly populates
PrecompiledAddressesGalileofrom the Galileo contract map.
236-237: LGTM!The Galileo case is correctly added with proper prioritization in the fork switch statement. The dependency on
rules.IsGalileofrom PR #1246 is already acknowledged.
374-378: LGTM!The
bigModExpstruct is correctly extended witheip7823andeip7883flags to support the new EIPs.
382-422: LGTM!The
byzantiumMultComplexityfunction correctly implements the EIP-198 formula with proper overflow handling usingbits.Mul64andbits.Add64. The arithmetic is sound and edge cases are handled correctly.
424-445: LGTM!The
berlinMultComplexityfunction correctly implements the ceiling(x/8)^2 formula with proper overflow detection.
447-462: LGTM!The
galileoMultComplexityfunction correctly implements the Galileo-specific formula (returns 16 for x ≤ 32, otherwise 2 × ceiling(x/8)^2) with proper overflow handling.
464-487: LGTM!The
modexpIterationCountfunction correctly calculates the adjusted exponent length used in gas calculations, with comprehensive overflow checks.
489-510: LGTM!The
byzantiumModexpGasfunction correctly implements the Byzantium gas calculation with proper overflow detection. The carry check after division is correct because any non-zero carry from the multiplication indicates the final result would overflow uint64 regardless of the division.
512-534: LGTM!The
berlinModexpGasfunction correctly implements the Berlin/EIP-2565 gas calculation with a minimum gas floor of 200 and proper overflow handling.
536-556: LGTM!The
galileoModexpGasfunction correctly implements the Galileo/EIP-7883 gas calculation with a minimum gas floor of 500 and proper overflow handling.
559-603: LGTM with minor note!The
RequiredGasimplementation correctly handles large length values usinguint256.Intand properly routes to the appropriate gas calculation based on EIP flags. The overflow handling is comprehensive.Note: The TODO comment at line 590 should be addressed or removed in a follow-up. However, the
getDatahelper should already handle the edge case correctly.
616-624: LGTM!The backward compatibility check for
eip2565is correctly implemented and properly documented. This ensures pre-Galileo blocks don't fail validation, as discussed in previous review comments.
632-635: LGTM!The EIP-7823 input length cap is correctly implemented, enforcing a maximum of 1024 bytes for base, exponent, and modulus lengths.
648-658: LGTM!The modular exponentiation logic correctly handles edge cases:
- Returns zero-length result when modulus is zero
- Optimizes the case when base bit length is 1 (base is ±1)
- Uses standard
Expfor the general case- Properly left-pads the result to modulus length
1335-1345: LGTM!The
p256Verifyprecompile is correctly updated to support EIP-7951 with a Galileo-specific gas cost of 6900. The implementation follows the established pattern used for other EIP-gated features.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/vm/contracts_test.go (1)
416-425: Address format inconsistency: remove "0x" prefix for consistency with file conventions.All other precompile address references in this file use the format without "0x" prefix (e.g., "01", "05", "f5", "0a", "0c"), but the P256Verify tests use "0x0100" and "0x0101". The test data files exist and are valid. Apply this diff for consistency:
- benchmarkPrecompiled("0x0100", t, bench) + benchmarkPrecompiled("0100", t, bench) } func TestPrecompiledP256Verify(t *testing.T) { - testJson("p256Verify", "0x0100", t) + testJson("p256Verify", "0100", t) } func TestPrecompiledP256VerifyGalileo(t *testing.T) { - testJson("p256VerifyGalileo", "0x0101", t) + testJson("p256VerifyGalileo", "0101", t) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/vm/contracts_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/vm/contracts_test.go (1)
common/types.go (1)
BytesToAddress(206-210)
⏰ 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 (3)
core/vm/contracts_test.go (3)
53-55: LGTM: Correct setup for ModExp EIP variants.The three bigModExp instances correctly represent different fork stages:
- Address 0x05: Byzantium (no optimizations)
- Address 0xf5: Berlin (EIP-2565)
- Address 0xf6: Galileo (EIP-2565 + EIP-7883)
This allows comprehensive testing of the gas calculation evolution across forks.
71-72: LGTM: Correct setup for P256Verify EIP-7951 variants.The two p256Verify instances correctly map:
- Address 0x0100: Pre-Galileo behavior (eip7951: false)
- Address 0x0101: Galileo behavior (eip7951: true)
This properly addresses the critical issue from the previous review where address "0b" was incorrectly used.
249-250: Test data file confirmed to exist.The EIP-7883 ModExp tests reference the required test data file, which is present and properly formatted with valid test cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/vm/contracts.go (1)
380-380: Remove unused variable.The
big0variable is declared but never used in the implementation. Consider removing it to improve code cleanliness.-var big0 = big.NewInt(0) - -// byzantiumMultComplexity implements the bigModexp multComplexity formula, as defined in EIP-198. +// byzantiumMultComplexity implements the bigModexp multComplexity formula, as defined in EIP-198.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/vm/contracts.go(12 hunks)core/vm/testdata/precompiles/modexp_eip7883.json(1 hunks)crypto/secp256r1/verifier.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crypto/secp256r1/verifier.go
🧰 Additional context used
🧬 Code graph analysis (1)
core/vm/contracts.go (4)
params/config.go (1)
Rules(1249-1256)common/math/integer.go (1)
MaxUint64(38-38)common/bytes.go (1)
LeftPadBytes(120-129)params/protocol_params.go (1)
P256VerifyGasGalileo(166-166)
⏰ 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 (9)
core/vm/testdata/precompiles/modexp_eip7883.json (1)
1-317: LGTM: Test data structure is well-formed.The test data structure follows the standard Ethereum precompile test format with comprehensive coverage of edge cases (nagydani-1 through nagydani-5, plus additional scenarios). The JSON is properly formatted with all required fields (Input, Expected, Name, Gas, NoBenchmark) present for each test case.
core/vm/contracts.go (8)
24-26: LGTM: Necessary imports for overflow-safe arithmetic.The
math/bitsimport enables overflow-safe multiplication and addition (bits.Mul64, bits.Add64), whileholiman/uint256provides 256-bit integer support for handling large exponent heads in gas calculation. Both are correctly utilized throughout the implementation.
70-175: LGTM: Precompile configurations correctly reflect fork progression.The EIP flag progression is logically sound:
- Pre-Berlin forks (Byzantium, Istanbul): All modexp EIPs disabled
- Berlin through Feynman: EIP-2565 enabled (Berlin gas cost reduction)
- Galileo: All three modexp EIPs enabled (EIP-2565, EIP-7823, EIP-7883)
The p256Verify precompile correctly receives the eip7951 flag only in the Galileo fork, enabling the updated gas cost at line 174.
192-230: LGTM: Galileo precompile initialization follows established pattern.The initialization of
PrecompiledAddressesGalileo(lines 228-230) and its wiring inActivePrecompiles(lines 236-237) follows the same pattern as other forks. The implementation is correct, assuming PR #1246 has been merged to provide therules.IsGalileofield as noted in the PR description.Also applies to: 236-237
382-487: LGTM: Complexity and iteration count functions are correctly implemented.The refactored complexity functions properly implement their respective EIP specifications with robust overflow protection:
- byzantiumMultComplexity (lines 391-422): Correctly implements the EIP-198 formula with three cases, using
bits.Mul64andbits.Add64for overflow-safe arithmetic in the large-value case- berlinMultComplexity (lines 424-445): Properly calculates
ceiling(x/8)^2with overflow checking- galileoMultComplexity (lines 447-462): Returns the constant 16 for x ≤ 32, and
2 * berlinMultComplexity(x)otherwise, with overflow handling- modexpIterationCount (lines 464-487): Correctly computes the adjusted exponent length used in gas calculations, with proper handling of both the length-based component and the bit-length of expHead
All functions consistently return
math.MaxUint64on overflow, enabling safe gas calculation upstream.
489-556: LGTM: Gas calculation functions correctly implement EIP specifications.The three gas calculation functions properly implement their respective EIPs:
- byzantiumModexpGas (lines 489-510): Implements EIP-198 with
multiplier=8,divisor=20, and no minimum gas- berlinModexpGas (lines 512-534): Implements EIP-2565 with
multiplier=8,divisor=3, andminGas=200- galileoModexpGas (lines 536-556): Implements EIP-7883 with
multiplier=16, no divisor, andminGas=500All functions consistently use overflow-safe arithmetic via
bits.Mul64and returnmath.MaxUint64on overflow, enabling safe handling upstream inRequiredGas.
559-603: LGTM: RequiredGas correctly handles large inputs and routes to appropriate gas calculation.The refactored
RequiredGasmethod properly handles oversized length values usinguint256.Intfor parsing and safely converts touint64(lines 560-576). The expHead extraction logic (lines 586-593) correctly handles both cases whereexpLen > 32andexpLen <= 32.Note: The TODO comment at line 590 can be removed—
getDataalready handles the case whenexpLen < baseLenby returning an empty slice when the requested range is out of bounds, which is the correct behavior for a zero expHead.
605-658: LGTM: Run method correctly implements length validation and modexp execution.The refactored
Runmethod properly implements the required checks and edge-case handling:
- Overflow detection (lines 607-614): The
inputLenOverflowcheck ensures that any length field exceeding 64 bits is detected- Pre-Galileo restriction (lines 617-623): The condition
c.eip2565 && !(c.eip7823 || c.eip7883)correctly enforces the 32-byte limit for Berlin through Feynman forks, as confirmed in past review comments- EIP-7823 cap (lines 632-634): Enforces the 1024-byte limit for base, exponent, and modulus lengths in the Galileo fork
- Edge cases (lines 647-656):
mod.BitLen() == 0: Returns zero (correct per spec)base.BitLen() == 1: Optimizes1^exp mod modas1 mod mod(valid optimization)- Default: Uses
base.Exp(base, exp, mod)(correct)The result is correctly left-padded to
modLenbytes at line 657.
1334-1344: LGTM: p256Verify correctly implements Galileo gas cost.The addition of the
eip7951flag (line 1335) and the conditional gas cost logic (lines 1340-1343) properly implement EIP-7951's updated gas cost for the p256Verify precompile in the Galileo fork. The implementation follows the established pattern used by bigModExp and correctly leaves theRunmethod unchanged since only the gas cost differs between forks.
3 participants
1. Purpose or design rationale of this PR
cherry pick eip7823
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
New Features
Tests