feat: remove the restriction on pre-compile #177
Conversation
ryanmorphl2
requested review from
SecurityLife,
FletcherMan and
a team
and removed request for
a team
WalkthroughThis pull request implements changes for the Morph203 upgrade, introducing a new set of precompiled Ethereum contracts, including contracts for various hashing and mathematical operations. It updates the EVM interpreter by removing support for the Darwin instruction set and adjusting the chain configuration to manage the Morph203 timing. Additionally, version constants have been reset to reflect a new versioning strategy, and various references to the Darwin upgrade have been replaced with Morph203 equivalents. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EVM
participant PrecompileLedger
participant Contract
User->>EVM: Submit transaction with contract address
EVM->>PrecompileLedger: Lookup address in PrecompiledContractsMorph203
PrecompileLedger-->>EVM: Return corresponding contract implementation
EVM->>Contract: Execute Run(input)
Contract-->>EVM: Return result or error
EVM-->>User: Return execution outcome
sequenceDiagram
participant Caller
participant EVMInterpreter
participant ChainRules
participant JumpTable
Caller->>EVMInterpreter: Begin execution
EVMInterpreter->>ChainRules: Check if Morph203 is active (IsMorph203(now))
ChainRules-->>EVMInterpreter: Return true/false
alt Morph203 is active
EVMInterpreter->>JumpTable: Use morph203InstructionSet
JumpTable-->>EVMInterpreter: Provide new opcode mappings
else
EVMInterpreter->>JumpTable: Use default instruction set
JumpTable-->>EVMInterpreter: Provide standard opcodes
end
EVMInterpreter-->>Caller: Execute instructions accordingly
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
🪧 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 (
|
![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/contracts.go (1)
193-197: Qianxuesen condition in ActivePrecompiles.Perfectly aligns with existing logic of returning addresses based on the chain rules. Ensure thorough testing for transitions between prior chain rules and Qianxuesen activation to avoid unexpected fallback states.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/vm/contracts.go(6 hunks)core/vm/instructions.go(1 hunks)core/vm/interpreter.go(1 hunks)core/vm/jump_table.go(1 hunks)params/config.go(4 hunks)
🔇 Additional comments (13)
core/vm/instructions.go (1)
458-478: Restore logic clarity and confirm intent.This conditional block selectively bypasses the hashing procedure for Qianxuesen by directly calling
GetHash. Ensure that the fallback hashing logic (involving chain ID and block number) properly reflects the legacy requirement for other forks. Also, confirm that skipping the chain ID logic for Qianxuesen aligns with the specification and does not introduce splits in blockhash behavior.core/vm/contracts.go (6)
23-23: Import looks good.
No issues spotted with adding"fmt"here.
32-32: Import of kzg4844 library is appropriate.
No immediate concerns. Just ensure that the external dependencies are pinned at known safe versions.
130-143: Ensure version consistency for Qianxuesen precompiled contracts.The newly introduced
PrecompiledContractsQianxuesenmap references known precompiles but includes new functionality for the KZG point evaluation. Verify that these addresses don't conflict with other versions and that the new address usage is documented.
160-160: New address slice for Qianxuesen.Storing addresses explicitly is consistent with existing patterns. Confirm that the order of addresses doesn’t cause confusion in the codebase or among other chain upgrades.
188-190: Appending Qianxuesen addresses in init.Initialization follows the same approach as other precompile sets. Ensure that you test these new addresses to confirm their presence in the final set of active precompiles.
1148-1210: Validate KZG point evaluation logic and error handling.
- Input length checks: Strictly validating the 192-byte input is good. However, consider whether partial or extra data might require more robust error reporting or clarifying logs.
- Versioned hash: The mismatch check quickly fails for incorrect commitment versions. Consider adding domain separation or additional logging to ease debugging in production.
- Proof verification: Any underlying library errors are fairly documented. Continue to maintain test coverage for corner cases (e.g., invalid proofs, mismatched claims).
- Return value: Returning a hardcoded hex-encoded value is unusual but presumably part of the specification. Verify that this is indeed the expected format and that no dynamic or actual claim data is missed.
Overall, this section is well-structured and keeps the code straightforward.
core/vm/interpreter.go (1)
77-78: Select Qianxuesen instruction set.Introducing another specialized jump table maintains the pattern for fork-based logic. Verify that any new instructions or gas cost changes within
qianxuesenInstructionSetare thoroughly covered by tests before mainnet deployment.core/vm/jump_table.go (2)
64-64: LGTM!The new instruction set variable follows the established pattern and naming convention.
70-75:❓ Verification inconclusive
Verify if BLOCKHASH modifications are needed in the instruction set.
The function reuses the Darwin instruction set without modifications. However, based on the PR objectives to "remove the restriction on pre-compile & restore results of opcode BLOCKHASH", you might need to modify the BLOCKHASH opcode's configuration in this instruction set.
Run the following script to check the BLOCKHASH opcode configuration in the Darwin instruction set:
🏁 Script executed:
#!/bin/bash # Description: Check if BLOCKHASH opcode is configured in the Darwin instruction set ast-grep --pattern $'func newDarwinInstructionSet() JumpTable { $$$ BLOCKHASH: { $$$ } $$$ }'Length of output: 106
Manual verification required: BLOCKHASH opcode configuration in Darwin instruction set
The
newQianxuesenInstructionSetfunction currently reuses the Darwin instruction set unchanged. Given the PR objective to "remove the restriction on pre-compile & restore results of opcode BLOCKHASH", it’s unclear if the BLOCKHASH opcode is being modified as intended. Our automated search did not yield clear evidence of any BLOCKHASH configuration modifications in the Darwin instruction set, though this result may be inconclusive.
- Action Item: Manually verify the BLOCKHASH opcode settings in the Darwin instruction set to confirm that the desired changes are applied.
--- 🏁 Script executed: ```shell [run_scripts] #!/bin/bash echo "Searching for 'BLOCKHASH:' occurrences in the repository to locate opcode configuration..." rg "BLOCKHASH:" -n . echo "Searching for 'newDarwinInstructionSet' function definition with context..." rg -C 5 "func newDarwinInstructionSet()" -n .Length of output: 1324
Verify BLOCKHASH Opcode Configuration Alignment
The
newQianxuesenInstructionSetfunction currently callsnewDarwinInstructionSet()without applying any modifications. Given the PR objective to "remove the restriction on pre-compile & restore results of opcode BLOCKHASH," this implementation does not appear to adjust the BLOCKHASH opcode configuration. The grep results confirm that BLOCKHASH is defined in the codebase (e.g., incore/vm/jump_table.goandcore/vm/opcodes.go), but no modifications specific to the new instruction set are present.
- Action Item: Manually verify whether the Darwin instruction set already satisfies the new requirements for BLOCKHASH or if an update is still needed in
newQianxuesenInstructionSet.params/config.go (3)
541-541: LGTM!The new field follows the established pattern for time-based fork configurations.
752-755: LGTM!The function follows the established pattern for time-based fork activation checks.
969-969: LGTM!The new flag follows the established pattern for fork activation flags in the Rules struct.
Also applies to: 995-995
ryanmorphl2
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
params/version.go (1)
25-27: Verify the version downgrade from 5.5.1 to 2.0.3.The version has been significantly downgraded, which could impact compatibility with existing systems. Please confirm if this is intentional and document any migration steps needed.
core/vm/contracts.go (1)
1148-1202: Enhance error handling in the KZG point evaluation precompile.The implementation is solid, but consider adding more descriptive error messages to help with debugging:
-var ( - errBlobVerifyInvalidInputLength = errors.New("invalid input length") - errBlobVerifyMismatchedVersion = errors.New("mismatched versioned hash") - errBlobVerifyKZGProof = errors.New("error verifying kzg proof") -) +var ( + errBlobVerifyInvalidInputLength = errors.New("invalid input length: expected 192 bytes") + errBlobVerifyMismatchedVersion = errors.New("mismatched versioned hash: commitment hash does not match") + errBlobVerifyKZGProof = errors.New("error verifying kzg proof: proof verification failed") +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/vm/contracts.go(6 hunks)core/vm/interpreter.go(1 hunks)params/config.go(4 hunks)params/version.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/vm/interpreter.go
🔇 Additional comments (3)
core/vm/contracts.go (2)
130-143: LGTM! The precompiled contracts map for Qianxuesen is well-structured.The implementation maintains consistency with previous contract maps and includes all necessary contracts, including the new KZG point evaluation contract.
1204-1210: LGTM! The kZGToVersionedHash implementation is correct.The function correctly implements the versioned hash calculation as per EIP-4844.
params/config.go (1)
541-541: LGTM! The Morph203 fork configuration is well-integrated.The implementation follows the established pattern for fork configuration:
- Adds Morph203Time field to ChainConfig
- Implements IsMorph203 check using isForkedTime
- Updates Rules struct to include IsMorph203 flag
Also applies to: 752-755, 969-969
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
params/config.go (1)
783-830: 🛠️ Refactor suggestionUpdate
CheckConfigForkOrderto include Morph203 time-based fork.The
CheckConfigForkOrdermethod should be updated to include time-based forks like Morph203 to ensure proper fork ordering.Apply this diff to add time-based fork checks:
func (c *ChainConfig) CheckConfigForkOrder() error { type fork struct { name string - block *big.Int + block *big.Int + time *uint64 optional bool // if true, the fork may be nil and next fork is still allowed } var lastFork fork for _, cur := range []fork{ {name: "homesteadBlock", block: c.HomesteadBlock}, {name: "daoForkBlock", block: c.DAOForkBlock, optional: true}, // ... existing block-based forks ... + {name: "darwinTime", time: c.DarwinTime, optional: true}, + {name: "morph203Time", time: c.Morph203Time, optional: true}, } { if lastFork.name != "" { - // Next one must be higher number - if lastFork.block == nil && cur.block != nil { + // Skip time-based fork comparisons + if lastFork.time != nil || cur.time != nil { + continue + } + // Next block must be higher number + if lastFork.block == nil && cur.block != nil { return fmt.Errorf("unsupported fork ordering: %v not enabled, but %v enabled at %v", lastFork.name, cur.name, cur.block) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/vm/interpreter.go(1 hunks)core/vm/jump_table.go(1 hunks)params/config.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/vm/interpreter.go
🔇 Additional comments (4)
core/vm/jump_table.go (1)
64-64: LGTM!The variable declaration follows the existing pattern for instruction sets.
params/config.go (3)
541-541: LGTM!The field follows the existing pattern for time-based forks and is properly documented.
752-755: LGTM!The method follows the existing pattern for time-based fork checks and reuses the
isForkedTimehelper function.
969-969: LGTM!The
Rulesstruct and method are correctly updated to include the Morph203 fork status.Also applies to: 995-995
core/vm/jump_table.go
Outdated
| // newMorph203InstructionSet returns the frontier, homestead, byzantium, | ||
| // contantinople, istanbul, petersburg, berlin, london, shanghai, curie, darwin and morph203 instructions. | ||
| func newMorph203InstructionSet() JumpTable { | ||
| instructionSet := newDarwinInstructionSet() | ||
| return instructionSet | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update the function comment to accurately reflect its implementation.
The function comment suggests that it includes instructions from multiple forks, but the implementation simply returns the Darwin instruction set without any modifications. Either:
- Add new instructions specific to Morph203, or
- Update the comment to clarify that it currently inherits all instructions from Darwin.
Apply this diff to fix the comment:
-// newMorph203InstructionSet returns the frontier, homestead, byzantium,
-// contantinople, istanbul, petersburg, berlin, london, shanghai, curie, darwin and morph203 instructions.
+// newMorph203InstructionSet returns the Darwin instruction set.
+// TODO: Add Morph203-specific instructions if needed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // newMorph203InstructionSet returns the frontier, homestead, byzantium, | |
| // contantinople, istanbul, petersburg, berlin, london, shanghai, curie, darwin and morph203 instructions. | |
| func newMorph203InstructionSet() JumpTable { | |
| instructionSet := newDarwinInstructionSet() | |
| return instructionSet | |
| } | |
| // newMorph203InstructionSet returns the Darwin instruction set. | |
| // TODO: Add Morph203-specific instructions if needed. | |
| func newMorph203InstructionSet() JumpTable { | |
| instructionSet := newDarwinInstructionSet() | |
| return instructionSet | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
params/config.go (1)
626-660: Update String() method to include Morph203.The
String()method ofChainConfigshould be updated to include the Morph203 time in its output for better debugging and logging.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, Engine: %v, Morph config: %v}", - c.ChainID, + c.ChainID, c.HomesteadBlock, c.DAOForkBlock, c.DAOForkSupport, c.EIP150Block, c.EIP155Block, c.EIP158Block, c.ByzantiumBlock, c.ConstantinopleBlock, c.PetersburgBlock, c.IstanbulBlock, c.MuirGlacierBlock, c.BerlinBlock, c.LondonBlock, c.ArrowGlacierBlock, c.ArchimedesBlock, c.ShanghaiBlock, c.BernoulliBlock, c.CurieBlock, c.DarwinTime, + c.Morph203Time, engine, c.Morph, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
params/config.go(8 hunks)
🔇 Additional comments (4)
params/config.go (4)
285-285: LGTM! The Morph203Time field follows the established pattern.The addition of
Morph203Timeas a pointer touint64is consistent with other time-based fork configurations likeDarwinTime.Also applies to: 543-543
754-757: LGTM! The IsMorph203 method is correctly implemented.The implementation follows the same pattern as other time-based fork checks, using the
isForkedTimehelper function.
990-990: LGTM! Rules struct and method updates are consistent.The
IsMorph203field has been added to theRulesstruct and is correctly initialized in theRulesmethod.Also applies to: 1016-1016
789-815:✅ Verification successful
Verify the fork ordering logic.
The fork ordering checks have been updated to include the Morph203 upgrade. However, we should verify that the timestamp-based ordering is correctly enforced.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any block-based forks defined after timestamp-based forks # This would help verify the fork ordering logic is correct # Search for block-based fork definitions after timestamp-based ones rg -A 5 'darwinTime.*timestamp.*optional.*true' params/config.go | rg 'block.*optional'Length of output: 89
Below is an additional verification script that you can run to double‑check that no block‑based fork definitions (i.e. those using the “block” field) appear after any timestamp‑based fork definitions (i.e. those using the “timestamp” field):
If you run this script and it confirms that the ordering is correct (i.e. no error messages and a success message is printed), then we can conclude that the fork ordering logic is correctly enforced, including the new Morph203 upgrade.
Fork Ordering Verification Complete
- Verified that all block‑based fork definitions (using “block”) occur before timestamp‑based definitions (using “timestamp”), per the updated configuration.
- The updated ordering with the Morph203 upgrade is correctly positioned.
🏁 Script executed:
[run_scripts] #!/bin/bash # Description: Verify that all block-based fork entries appear before timestamp-based fork entries in params/config.go. # This script extracts the line numbers for each type of fork and reports if any block-based entry appears after a timestamp-based one. # Get line numbers for block-based fork definitions block_lines=$(rg -n 'name:\s*".*Block"' params/config.go | cut -d: -f1) # Get line numbers for timestamp-based fork definitions time_lines=$(rg -n 'name:\s*".*Time"' params/config.go | cut -d: -f1) error_found=0 for t in $time_lines; do for b in $block_lines; do if [ "$b" -gt "$t" ]; then echo "ERROR: Block-based fork at line $b appears after timestamp-based fork at line $t" error_found=1 fi done done if [ "$error_found" -eq 0 ]; then echo "Fork ordering is correct: All block-based forks appear before timestamp-based forks." fiLength of output: 2172
Fork ordering logic is verified and correct.
- All block‑based forks in
params/config.goprecede the timestamp‑based forks, including the Morph203 upgrade.- No block‑based fork definitions were found after the timestamp‑based ones.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/vm/contracts.go (1)
127-139: Enhance the comment to describe changes from previous versions.The comment could be more descriptive about how Morph203's precompiled contracts differ from previous versions (e.g., Bernoulli, Archimedes) where some precompiles were disabled.
Apply this diff to improve the comment:
-// PrecompiledContractsMorph203 contains the default set of pre-compiled Ethereum -// contracts used in the Morph203 release. +// PrecompiledContractsMorph203 contains the default set of pre-compiled Ethereum +// contracts used in the Morph203 release. Unlike Archimedes and Bernoulli releases +// where some precompiles were disabled, Morph203 enables all standard precompiles +// including sha256, ripemd160, and blake2f.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/vm/contracts.go(4 hunks)
🔇 Additional comments (4)
core/vm/contracts.go (4)
38-38: LGTM!The error message is clear and descriptive, accurately indicating which precompiles are temporarily disabled.
156-156: LGTM!The slice declaration follows the established pattern and maintains consistency with other precompiled addresses declarations.
184-186: LGTM!The initialization logic is consistent with other precompiled addresses initializations and is correctly placed in the init function.
192-193: LGTM!The Morph203 case is correctly placed at the top of the switch statement, following the pattern of having the latest version first.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
params/config.go (1)
1-1020: Consider adding documentation for the Morph203 upgrade.While the implementation is correct, it would be helpful to add documentation explaining the purpose and impact of the Morph203 upgrade, particularly around the removal of pre-compilation restrictions and BLOCKHASH opcode changes mentioned in the PR objectives.
Add documentation above the
Morph203Timefield in theChainConfigstruct:+ // Morph203Time is the timestamp when the Morph203 upgrade goes into effect, removing + // pre-compilation restrictions and restoring BLOCKHASH opcode functionality. Morph203Time *uint64 `json:"morph203Time,omitempty"` // Morph203Time switch time (nil = no fork, 0 = already on morph203)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
params/config.go(10 hunks)
🔇 Additional comments (5)
params/config.go (5)
285-285: LGTM! Field additions follow existing patterns.The new
Morph203Timefield is consistently added to both mainnet and testnet chain configurations, following the same pattern as other time-based forks like Darwin.Also applies to: 316-316, 543-543
755-758: LGTM! Fork check method is correctly implemented.The
IsMorph203method follows the established pattern for time-based forks, using theisForkedTimehelper function.
789-816: LGTM! Fork ordering logic is properly updated.The fork ordering checks in
CheckConfigForkOrderhave been updated to include Morph203 as an optional time-based fork, maintaining consistency with the existing pattern.
636-637: LGTM! String representation is updated correctly.The
Stringmethod has been updated to include Morph203 information in the chain configuration output.
991-991: LGTM! Rules struct is updated appropriately.The
Rulesstruct has been updated to include theIsMorph203field, maintaining consistency with other fork flags.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
params/config.go (1)
808-834: Enhanced fork ordering validation handles mixed block/timestamp forks.The fork ordering validation has been improved to:
- Handle both block-based and timestamp-based forks
- Prevent timestamp-based forks from reverting to block-based ordering
- Maintain proper sequencing of forks
This enhancement ensures the integrity of the fork activation sequence.
Consider documenting the fork ordering rules in the codebase to help future maintainers understand the constraints around mixing block-based and timestamp-based forks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/blockchain_test.go(1 hunks)core/state_processor_test.go(1 hunks)core/vm/interpreter.go(1 hunks)core/vm/jump_table.go(0 hunks)core/vm/runtime/runtime.go(1 hunks)eth/gasprice/gasprice_test.go(1 hunks)params/config.go(14 hunks)
💤 Files with no reviewable changes (1)
- core/vm/jump_table.go
✅ Files skipped from review due to trivial changes (1)
- core/blockchain_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/vm/interpreter.go
🔇 Additional comments (5)
eth/gasprice/gasprice_test.go (1)
117-117: LGTM!The test configuration has been correctly updated to use
Morph203Timeinstead ofDarwinTime, maintaining consistency with the fork transition.core/vm/runtime/runtime.go (1)
75-75: LGTM!The EVM runtime configuration has been properly updated to use
Morph203Time, maintaining the same type and initialization pattern.core/state_processor_test.go (1)
61-61: LGTM!The state processor test configuration has been correctly updated to use
Morph203Time, maintaining consistency with the fork transition.params/config.go (2)
540-540: LGTM! Field update maintains backward compatibility.The
Morph203Timefield inChainConfigmaintains the same type (*uint64) and JSON tag structure as the previousDarwinTimefield, ensuring backward compatibility.
746-749: LGTM! Fork check method correctly implements timestamp-based validation.The
IsMorph203method properly implements timestamp-based fork validation using theisForkedTimehelper function, which is consistent with the transition from block-based to time-based fork activation.
Remove the restriction on pre-compile & restore results of opcode BLOCKHASH
1. Purpose or design rationale of this PR
...
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
Chores