feat: enable p256Verify in EuclidV2 #1111
Conversation
WalkthroughThis pull request adds support for the new EuclidV2 release by introducing a new mapping of precompiled contracts. It creates new variables and updates functions in the virtual machine to account for the EuclidV2 chain rule. Additionally, configuration methods and comments are modified to include fork time checks for EuclidV2. These updates span contract mappings, VM control flow modifications, and chain configuration adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EVM
participant ChainConfig
participant PrecompiledMapping
Client->>EVM: Call precompile method
EVM->>ChainConfig: Check fork rule (IsEuclidV2)
alt EuclidV2 active
ChainConfig-->>EVM: Return true (EuclidV2)
EVM->>PrecompiledMapping: Retrieve PrecompiledAddressesEuclidV2
else Default rules
ChainConfig-->>EVM: Return false
EVM->>PrecompiledMapping: Retrieve default precompiled addresses
end
PrecompiledMapping-->>EVM: Return addresses
EVM-->>Client: Process precompiled addresses
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
colinlyguo
force-pushed
the
feat-schedule-RIP-7212
branch
from
9a1fa6f to
742f35b
Compare
Co-authored-by: Péter Garamvölgyi <[email protected]>
![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
🔭 Outside diff range comments (1)
params/config.go (1)
771-814: Update String method to include Euclid and EuclidV2.The
Stringmethod ofChainConfigshould be updated to include the new time-based forks for completeness.Apply this diff to add the missing forks:
func (c *ChainConfig) String() string { var engine interface{} switch { case c.Ethash != nil: engine = c.Ethash case c.Clique != nil: engine = c.Clique default: engine = "unknown" } darwinTime := "<nil>" if c.DarwinTime != nil { darwinTime = fmt.Sprintf("@%v", *c.DarwinTime) } darwinV2Time := "<nil>" if c.DarwinV2Time != nil { darwinV2Time = fmt.Sprintf("@%v", *c.DarwinV2Time) } + euclidTime := "<nil>" + if c.EuclidTime != nil { + euclidTime = fmt.Sprintf("@%v", *c.EuclidTime) + } + euclidV2Time := "<nil>" + if c.EuclidV2Time != nil { + euclidV2Time = fmt.Sprintf("@%v", *c.EuclidV2Time) + } - return fmt.Sprintf("{ChainID: %v Homestead: %v DAO: %v DAOSupport: %v EIP150: %v EIP155: %v EIP158: %v Byzantium: %v Constantinople: %v Petersburg: %v Istanbul: %v, Muir Glacier: %v, Berlin: %v, London: %v, Arrow Glacier: %v, Archimedes: %v, Shanghai: %v, Bernoulli: %v, Curie: %v, Darwin: %v, DarwinV2: %v, Engine: %v, Scroll config: %v}", + return fmt.Sprintf("{ChainID: %v Homestead: %v DAO: %v DAOSupport: %v EIP150: %v EIP155: %v EIP158: %v Byzantium: %v Constantinople: %v Petersburg: %v Istanbul: %v, Muir Glacier: %v, Berlin: %v, London: %v, Arrow Glacier: %v, Archimedes: %v, Shanghai: %v, Bernoulli: %v, Curie: %v, Darwin: %v, DarwinV2: %v, Euclid: %v, EuclidV2: %v, Engine: %v, Scroll config: %v}",
🧹 Nitpick comments (3)
params/config.go (3)
903-921: Consider updating the comment for IsEuclid method.The comment for
IsEuclidmethod incorrectly references "Darwin fork time" instead of "Euclid fork time".Apply this diff to fix the comment:
-// IsEuclid returns whether time is either equal to the Darwin fork time or greater. +// IsEuclid returns whether time is either equal to the Euclid fork time or greater.
931-996: Consider updating CheckConfigForkOrder to include time-based forks.The
CheckConfigForkOrdermethod currently only checks block-based forks. Consider extending it to validate the ordering of time-based forks (Darwin, DarwinV2, Euclid, EuclidV2) to ensure they are properly ordered.
998-1061: Consider updating checkCompatible for time-based forks.The
checkCompatiblemethod should be extended to validate compatibility of time-based forks (Darwin, DarwinV2, Euclid, EuclidV2) when upgrading chain configurations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
params/config.go(4 hunks)
🔇 Additional comments (4)
params/config.go (4)
641-642: LGTM! Field addition follows the established pattern.The
EuclidV2Timefield is correctly defined with proper type and JSON tag, maintaining consistency with other time-based fork configurations.
918-921: LGTM! Method implementation is consistent and well-documented.The
IsEuclidV2method follows the established pattern for time-based fork checks, using the sharedisForkedTimehelper function.
1135-1135: LGTM! Rules struct update maintains consistency.The
IsEuclidV2flag is correctly added to the Rules struct, maintaining alphabetical order and grouped with related flags.
1162-1162: LGTM! Rules method update is properly implemented.The
IsEuclidV2flag is correctly set based on the time check, consistent with other time-based fork flags.
1. Purpose or design rationale of this PR
This PR adds the fork activation of
RIP-7212(p256Verify) precompile inEuclidV2. together with other small tweaks and typos fixes, including:Revisit of the implementation:
p256r1test vectors in this repo, which is first prepared in this comment. The test vectors in our repo are here.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