WIP:Handle system call #178
Conversation
WalkthroughThe pull request introduces a new pre-transaction hook called Changes
Sequence Diagram(s)sequenceDiagram
participant SP as StateProcessor
participant E as Engine (Consensus Implementation)
SP->>E: Retrieve parent header
SP->>E: Call StartHook(chain, header, state)
E-->>SP: Return nil/error
alt Error Occurs
SP->>SP: Handle error and exit process
else Successful
SP->>SP: Continue with state processing
end
sequenceDiagram
participant TE as TraceEnv
participant EVM as EVM Environment
participant DB as StateDB
TE->>TE: Initialize transaction context & call tracer
TE->>EVM: Execute transaction message with tracing
EVM-->>TE: Return updated state or error
TE->>TE: Merge bytecode, proof data, and update traces
TE-->>TE: Return nil on success
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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: 2
🧹 Nitpick comments (1)
consensus/l2/consensus.go (1)
290-296: Remove commented-out codeThis commented-out code appears to be leftover from development and doesn't provide any value in the final implementation.
-//type XX struct { -// consensus.ChainHeaderReader -//} -// -//func (l2 *Consensus) Engine() consensus.Engine { -// return l2.ethone -//}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
consensus/clique/clique.go(1 hunks)consensus/consensus.go(1 hunks)consensus/ethash/consensus.go(1 hunks)consensus/l2/consensus.go(4 hunks)core/state_processor.go(1 hunks)rollup/rcfg/config.go(1 hunks)rollup/tracing/tracing.go(1 hunks)
🔇 Additional comments (7)
consensus/ethash/consensus.go (1)
593-595: New consensus hook added but implementation is empty.This adds a new hook method
StartHookto the Ethash consensus engine, but the implementation is a simple placeholder that always returns nil. Based on its position betweenPrepareandFinalize, and the PR title mentioning system calls, this appears to be a hook point for handling system calls before transaction processing.Is this hook intended to handle system calls? Consider clarifying with a comment explaining its purpose for future maintainers.
rollup/rcfg/config.go (1)
36-40: New predefined addresses added for L2 staking and system operations.These new address constants follow the established pattern for predefined L2 contract addresses. The
SystemAddressappears to be related to the system call handling mentioned in the PR title.Please ensure these addresses match the contracts they're intended to interact with, especially if they're already deployed in any environment.
consensus/clique/clique.go (1)
564-566: Empty StartHook implementation added to Clique consensus.This adds the same
StartHookmethod to the Clique consensus engine as was added to Ethash. The consistent interface across consensus implementations is good, but consider adding a brief comment explaining the purpose of this hook.Is this hook needed for Clique consensus, or is it just for interface compatibility? If it's not expected to be used with Clique, consider adding a comment to clarify.
core/state_processor.go (1)
95-102: The engine.Finalize call was removed but no replacement is visible.Looking at the surrounding code, I notice that while you've added the
StartHookcall, the code no longer appears to callp.engine.Finalizeto finalize the block. This might be intentional, but it's a significant change that should be documented.Was the removal of the
engine.Finalizecall intentional? If so, is that functionality now handled elsewhere, or has it been deliberately removed?consensus/consensus.go (1)
84-86: Clear and well-structured addition of StartHook methodThe new StartHook method in the Engine interface provides a clear hook point for consensus engines to execute logic before transactions are applied. The parameter name change from "preHeader" to "parentHeader" better describes its purpose.
consensus/l2/consensus.go (2)
39-56: Well-structured implementation of chainContext typeThe chainContext helper type is well-designed, providing a clean abstraction for containing both a ChainHeaderReader and an Engine with appropriate methods.
221-238: Implementation of StartHook with system call handlingThe StartHook method properly implements the interface method added to Engine by creating an EVM context and executing a system call to the L2StakingAddress. Error handling is thorough.
The TODO comment on line 225 suggests that tracing functionality may be added later, which would likely connect to the new getSystemResult method in the tracing package.
Can you verify if the system address and L2StakingAddress constants in rcfg package are properly defined? This will ensure the system call functionality works as expected.
core/state_processor.go
Outdated
| parentHeader := p.bc.GetHeaderByHash(block.ParentHash()) | ||
| if parentHeader == nil { | ||
| // TODO | ||
| } | ||
| err := p.engine.StartHook(p.bc, header, parentHeader, statedb) | ||
| if err != nil { | ||
| return nil, nil, 0, err | ||
| } |
There was a problem hiding this comment.
Incomplete error handling for missing parent header.
The code retrieves the parent header and calls the new StartHook with appropriate error handling, but there's a TODO comment for the case when the parent header is nil. This should be addressed before the PR is finalized.
You should implement proper error handling for the case when the parent header is nil, rather than just leaving a TODO comment. Consider returning an appropriate error like consensus.ErrUnknownAncestor or similar.
if parentHeader == nil {
- // TODO
+ return nil, nil, 0, consensus.ErrUnknownAncestor
}📝 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.
| parentHeader := p.bc.GetHeaderByHash(block.ParentHash()) | |
| if parentHeader == nil { | |
| // TODO | |
| } | |
| err := p.engine.StartHook(p.bc, header, parentHeader, statedb) | |
| if err != nil { | |
| return nil, nil, 0, err | |
| } | |
| parentHeader := p.bc.GetHeaderByHash(block.ParentHash()) | |
| if parentHeader == nil { | |
| return nil, nil, 0, consensus.ErrUnknownAncestor | |
| } | |
| err := p.engine.StartHook(p.bc, header, parentHeader, statedb) | |
| if err != nil { | |
| return nil, nil, 0, err | |
| } |
rollup/tracing/tracing.go
Outdated
| env.Proofs[addrStr] = wrappedProof | ||
| txStorageTrace.Proofs[addrStr] = wrappedProof | ||
| env.pMu.Unlock() | ||
| } | ||
|
|
||
| zkTrieBuildStart := time.Now() | ||
| proofStorages := structLogger.UpdatedStorages() | ||
| for addr, keys := range proofStorages { | ||
| if _, existed := txStorageTrace.StorageProofs[addr.String()]; !existed { | ||
| txStorageTrace.StorageProofs[addr.String()] = make(map[string][]hexutil.Bytes) | ||
| } | ||
|
|
||
| env.sMu.Lock() | ||
| trie, err := state.GetStorageTrieForProof(addr) | ||
| if err != nil { | ||
| // but we still continue to next address | ||
| log.Error("Storage trie not available", "error", err, "address", addr) | ||
| env.sMu.Unlock() | ||
| continue | ||
| } | ||
| zktrieTracer := state.NewProofTracer(trie) | ||
| env.sMu.Unlock() | ||
|
|
||
| for key := range keys { | ||
| addrStr := addr.String() | ||
| keyStr := key.String() | ||
| value := state.GetState(addr, key) | ||
| isDelete := bytes.Equal(value.Bytes(), common.Hash{}.Bytes()) | ||
|
|
||
| txm := txStorageTrace.StorageProofs[addrStr] | ||
| env.sMu.Lock() | ||
| m, existed := env.StorageProofs[addrStr] | ||
| if !existed { | ||
| m = make(map[string][]hexutil.Bytes) | ||
| env.StorageProofs[addrStr] = m | ||
| } | ||
| if zktrieTracer.Available() && !env.ZkTrieTracer[addrStr].Available() { | ||
| env.ZkTrieTracer[addrStr] = state.NewProofTracer(trie) | ||
| } | ||
|
|
||
| if proof, existed := m[keyStr]; existed { | ||
| txm[keyStr] = proof | ||
| // still need to touch tracer for deletion | ||
| if isDelete && zktrieTracer.Available() { | ||
| env.ZkTrieTracer[addrStr].MarkDeletion(key) | ||
| } | ||
| env.sMu.Unlock() | ||
| continue | ||
| } | ||
| env.sMu.Unlock() | ||
|
|
||
| var proof [][]byte | ||
| var err error | ||
| if zktrieTracer.Available() { | ||
| proof, err = state.GetSecureTrieProof(zktrieTracer, key) | ||
| } else { | ||
| proof, err = state.GetSecureTrieProof(trie, key) | ||
| } | ||
| if err != nil { | ||
| log.Error("Storage proof not available", "error", err, "address", addrStr, "key", keyStr) | ||
| // but we still mark the proofs map with nil array | ||
| } | ||
| wrappedProof := types.WrapProof(proof) | ||
| env.sMu.Lock() | ||
| txm[keyStr] = wrappedProof | ||
| m[keyStr] = wrappedProof | ||
| if zktrieTracer.Available() { | ||
| if isDelete { | ||
| zktrieTracer.MarkDeletion(key) | ||
| } | ||
| env.ZkTrieTracer[addrStr].Merge(zktrieTracer) | ||
| } | ||
| env.sMu.Unlock() | ||
| } | ||
| } | ||
| getTxResultZkTrieBuildTimer.UpdateSince(zkTrieBuildStart) | ||
|
|
||
| tracerResultTimer := time.Now() | ||
| callTrace, err := callTracer.GetResult() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get callTracer result: %w", err) | ||
| } | ||
| getTxResultTracerResultTimer.UpdateSince(tracerResultTimer) | ||
|
|
||
| env.ExecutionResults[index] = &types.ExecutionResult{ | ||
| From: sender, | ||
| To: receiver, | ||
| AccountCreated: createdAcc, | ||
| AccountsAfter: after, | ||
| L1DataFee: (*hexutil.Big)(result.L1DataFee), | ||
| Gas: result.UsedGas, | ||
| Failed: result.Failed(), | ||
| ReturnValue: fmt.Sprintf("%x", returnVal), | ||
| StructLogs: vm.FormatLogs(structLogger.StructLogs()), | ||
| CallTrace: callTrace, | ||
| } | ||
| env.TxStorageTraces[index] = txStorageTrace | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Incomplete implementation of getSystemResult method
The method has several critical issues that need to be addressed:
- Variables
fromandtoare used (lines 288-304) but never initialized msgis referenced on line 307 but never definedresultis used on line 341 but not assigned anywhereindexis used on line 499 without being definedtxStorageTraceis used on line 511 but not initialized
Additionally, many lines are commented out, suggesting this is work in progress.
Consider implementing the missing pieces or properly handling the undefined variables. If this is intended to be a stub for future implementation, add a TODO comment to clarify and ensure missing variables have default values to prevent runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
rollup/tracing/tracing.go (1)
293-448: Refactor or finalize thegetSystemResultstub.
This large method is a placeholder for system-level operations and includes commented-out code. If you plan to keep it long-term, consider splitting it into smaller, focused functions or removing stale comments. Add a clear TODO or implement the final logic to avoid confusion.consensus/l2/consensus.go (2)
221-238: Parameterize the gas limit inStartHook.
Hardcoding a limit of210000might work now but could need adjustment. Expose or configure it to handle dynamic trends or to facilitate testing.
289-297: Remove or clarify commented-out code.
This block appears unused. Consider removing if no longer relevant or add aTODOlabel if it’ll be revived.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (4)
consensus/ethash/consensus.go(1 hunks)consensus/l2/consensus.go(4 hunks)core/types/l2trace.go(2 hunks)rollup/tracing/tracing.go(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- consensus/ethash/consensus.go
🔇 Additional comments (11)
rollup/tracing/tracing.go (7)
84-85: Check concurrency safety for newly introduced field.
Storing the StartHook result on a per-TraceEnvbasis looks fine. However, ensure thatStartHookResultis only set and read in a thread-safe manner, given that transactions may be traced in parallel.
125-125: Initialization seems appropriate.
SettingStartHookResulttonilhere is logical, given it will be overwritten later bygetSystemResult. No issues noted.
188-191: Ensure changes fromgetSystemResultare intentionally discarded or persisted.
env.getSystemResult(env.state.Copy(), block)uses a copied state. If StartHook must persist changes, consider referencingenv.statedirectly or merging the resulting state modifications into the main StateDB.
194-196: Reasonable concurrency adjustment.
Capping the number of threads at the number of transactions is a solid choice to minimize overhead.
214-214: Good addition of txHash to logging context.
This improves debuggability and traceability.
227-227: Loop structure is correct.
Iterating over transactions here is a straightforward approach.
284-291: Wrapper logic looks straightforward.
This helper function cleanly converts[][]byteto[]hexutil.Bytes; no apparent issues.core/types/l2trace.go (2)
23-23: New field successfully captures StartHook data.
AddingStartHookResulttoBlockTraceis consistent with the tracing enhancements.
119-119: Validate removal ofCodeSizefromAccountWrapper.
Ensure dependent code no longer relies onCodeSize. Verify references and usage in your codebase or tests to avoid runtime errors.consensus/l2/consensus.go (2)
39-43: Robust approach to unify chain context.
chainContextneatly ties together the chain reader and engine references.
45-55: Interface methods look consistent.
Implementations forEngine(),GetHeader(), andConfig()are straightforward.
rollup/tracing/tracing.go
Outdated
| @@ -167,7 +171,7 @@ func CreateTraceEnv(chainConfig *params.ChainConfig, chainContext core.ChainCont | |||
| log.Error("Proof for coinbase not available", "coinbase", coinbase, "error", err) | |||
| // but we still mark the proofs map with nil array | |||
| } | |||
| env.Proofs[key] = types.WrapProof(proof) | |||
| env.Proofs[key] = wrapProof(proof) | |||
There was a problem hiding this comment.
Synchronize access to env.Proofs.
This assignment occurs without a preceding lock, but env.Proofs is guarded by pMu. To avoid data races in concurrent scenarios, wrap it in a mutex lock, for example:
+ env.pMu.Lock()
+ defer env.pMu.Unlock()
env.Proofs[key] = wrapProof(proof)📝 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.
| env.Proofs[key] = wrapProof(proof) | |
| env.pMu.Lock() | |
| defer env.pMu.Unlock() | |
| env.Proofs[key] = wrapProof(proof) |
curryxbo
force-pushed
the
handleSystemTx
branch
from
1249d4e to
0e1f0ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
consensus/l2/consensus.go (1)
221-238: Potential error-handling enhancement in StartHook.
- The
StartHooklogic correctly initializes the EVM and calls therecordBlocksfunction in the designated staking contract.- Consider verifying that
parentHeaderis not nil to avoid passing a nil Coinbase tostakingAbi.Pack.- You may want to account for the possibility of partial/higher gas usage, or capture logs from the call to ensure the system contract call is fully succeeded.
Here is a small example to add a
nilcheck forparentHeader:func (l2 *Consensus) StartHook(chain consensus.ChainHeaderReader, header, parentHeader *types.Header, state *state.StateDB) error { + if parentHeader == nil { + return errors.New("parentHeader is nil, cannot retrieve Coinbase") + } cx := chainContext{Chain: chain, engine: l2.ethone} ...rollup/tracing/tracing.go (3)
125-125: Nil initialization is optional.Explicitly setting
StartHookResulttonilis fine, but if not set anywhere else, Go defaults tonil.
284-439: Placeholder system call logic in getSystemResult.
- The placeholder is clearly marked. Ensure final implementation handles possible errors during contract calls or system updates, e.g., revert reasons.
- Logging and tracer usage look consistent with how other transactions are traced.
- If the system call modifies state (once fully implemented), verify that using a copiable
state *state.StateDBaligns with the intended flow of changes.Consider returning a structured result or capturing logs from system-level calls, so you can differentiate between partial reverts and fully successful calls.
691-692: TODO reminder for Transaction field.A small note to handle any type mismatches for the block’s transactions if tracing expects a specialized structure.
Do you need help formulating how to convert the existing
types.Transactioninto your required block trace format?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (4)
consensus/ethash/consensus.go(1 hunks)consensus/l2/consensus.go(4 hunks)core/types/l2trace.go(1 hunks)rollup/tracing/tracing.go(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/types/l2trace.go
- consensus/ethash/consensus.go
🧰 Additional context used
🧬 Code Definitions (1)
consensus/l2/consensus.go (1)
contracts/l2staking/l2staking.go (10) (10)
L2StakingMetaData(46:49)_(21:21)_(22:22)_(23:23)_(24:24)_(25:25)_(26:26)_(27:27)_(28:28)_(29:29)
🔇 Additional comments (12)
consensus/l2/consensus.go (8)
11-12: Imports look good.No immediate concerns; the introduced packages (
l2stakingandcore) are consistent with the usage shown later in the code.
15-15: Import for VM usage.The addition of
"github.com/morph-l2/go-ethereum/core/vm"is aligned with the newly introduced EVM calls.
17-17: Logical import for rollup config.The import of
"github.com/morph-l2/go-ethereum/rollup/rcfg"is necessary for referencingrcfg.SystemAddressandrcfg.L2StakingAddress.
39-43: Introduction of chainContext struct.This wrapper neatly embeds both a
ChainHeaderReaderand aconsensus.Engine.
45-47: Getter for Engine.No issues. The method simply returns the embedded
enginefield.
49-51: Header retrieval logic.Straightforward pass-through to
c.Chain.GetHeader.
53-55: Chain config pass-through.Retrieves the chain config from the embedded
ChainHeaderReader.
57-57: Type assertion ensures compliance with consensus.Engine.This confirms that
&Consensus{}indeed satisfies theEngineinterface.rollup/tracing/tracing.go (4)
85-85: New field for StartHookResult is appropriate.Storing the system-level operation result in
StartHookResultcentralizes the pre-transaction EVM execution outcome.
194-195: Thread count adjustment.Good approach to limit concurrency to the lesser of CPU count or transaction count.
739-740: Enhanced block trace includes StartHookResult.
- Adding
StartHookResulttoBlockTraceensures the pre-transaction hook results are visible.- As noted, the
Transactionsfield remains to be implemented based on custom definitions.Also applies to: 755-756, 761-762
184-191:✅ Verification successful
Sequential logic for StartHook.
Invoking the system-level logic (
getSystemResult) before processing transactions is appropriate. However, be aware that callingenv.state.Copy()yields a separate state snapshot that might diverge from the final state if StartHook modifies state in a meaningful way. Ensure this is the intended behavior.Would you like to confirm that changes made in
getSystemResultdo not need to persist inenv.statefor subsequent transactions? If so, here’s a script to search the codebase for references toenv.stateafterwards, to see if immediate writes might be lost:
🏁 Script executed:
#!/bin/bash rg "env\.state\b" -A 10 -B 10Length of output: 12581
State Snapshot Behavior for StartHook Confirmed
The call to
env.getSystemResult(env.state.Copy(), block)is consistent with the code’s overall design. The grep output shows that:
- A fresh copy of
env.stateis taken for transaction-specific tasks (e.g., injobs <- &txTraceTask{statedb: env.state.Copy(), index: i}).- The main
env.stateis later updated (e.g., viaenv.state.SetTxContext(tx.Hash(), i)) without relying on any modifications fromgetSystemResult.This indicates that any changes made within
getSystemResultare intentionally confined to the copied state, and it is expected that they do not persist into the main state used for subsequent transactions. Please review this design decision to ensure that no side-effects are meant to be transferred from the StartHook execution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rollup/tracing/tracing.go (2)
285-440: Implement system-level StartHook placeholder.
ThegetSystemResultmethod outlines a structure for pre-transaction operations, collecting proofs and logs. If this remains a stub, document future tasks with TODOs.
762-765: Remove or update placeholder note for Transactions field.
This comment conflicts with line 759, whereTransactionsis already set. Consider deleting or revising.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
rollup/tracing/tracing.go(6 hunks)
🔇 Additional comments (5)
rollup/tracing/tracing.go (5)
84-85: Initialize a dedicated field for system hook results.
Storing the result inStartHookResultis clear and maintains separation for the system call's data.
125-125: Set StartHookResult to nil by default.
Initializing withnilensures the result is only set ifgetSystemResultactually runs.
189-193: Invoke getSystemResult before transactions.
Placing system-level operations ahead of normal transactions is logical. Verify any dependencies that might rely on these results.
757-757: Include StartHookResult in the final BlockTrace.
Good job exposing the StartHook’s outcome in the block trace.
759-759: Add transactions to block trace.
PopulatingTransactionshere keeps the trace data consistent.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
rollup/rcfg/config.go (1)
46-50: 💡 Verification agent🧩 Analysis chain
Verify slot collision with L1BlobBaseFeeSlot
The
RewardStartedSlotandL1BlobBaseFeeSlotare both defined to use storage slot 6, which could lead to unintended consequences. Please verify this is intentional or fix the slot number.RewardStartedSlot = common.BigToHash(big.NewInt(6)) // New fields added in the Curie hard fork - L1BlobBaseFeeSlot = common.BigToHash(big.NewInt(6)) + L1BlobBaseFeeSlot = common.BigToHash(big.NewInt(10)) // Changed to avoid collision with RewardStartedSlot CommitScalarSlot = common.BigToHash(big.NewInt(7)) BlobScalarSlot = common.BigToHash(big.NewInt(8)) IsCurieSlot = common.BigToHash(big.NewInt(9))
🏁 Script executed:
#!/bin/bash # Check for potential storage slot collisions across the codebase echo "Searching for storage slots with value 6..." rg -A 1 -B 1 "common\.BigToHash\(big\.NewInt\(6\)\)" --glob "*.go"Length of output: 591
Critical Conflict Detected: Update L1BlobBaseFeeSlot to a Unique Storage Slot
The grep output confirms that both
RewardStartedSlotandL1BlobBaseFeeSlotare currently assigned usingbig.NewInt(6). This collision could lead to unintended consequences. Please update the slot forL1BlobBaseFeeSlot(e.g., change it tobig.NewInt(10)as shown below) to ensure each storage slot is unique.
- File:
rollup/rcfg/config.go- Lines Affected: 46–50
RewardStartedSlot = common.BigToHash(big.NewInt(6)) // New fields added in the Curie hard fork - L1BlobBaseFeeSlot = common.BigToHash(big.NewInt(6)) + L1BlobBaseFeeSlot = common.BigToHash(big.NewInt(10)) // Changed to avoid collision with RewardStartedSlot CommitScalarSlot = common.BigToHash(big.NewInt(7)) BlobScalarSlot = common.BigToHash(big.NewInt(8)) IsCurieSlot = common.BigToHash(big.NewInt(9))
🧹 Nitpick comments (11)
contracts/morphtoken/morph_token_test.go (1)
9-12: Enhance test coverage by validating the returned dataThe test confirms that
PacketData()doesn't return an error, but doesn't verify that the returned data is correct. Consider enhancing it to validate the expected byte encoding from the ABI packing operation.func TestPackData(t *testing.T) { - _, err := PacketData() + data, err := PacketData() require.NoError(t, err) + // Verify the packed data matches the expected encoding for mintInflations + expected := []byte{0x92, 0x44, 0x21, 0x73} // Function selector for mintInflations + require.Equal(t, expected, data[:4]) }contracts/l2staking/l2staking_test.go (1)
10-13: Enhance test coverage by validating the returned dataThe test confirms that
PacketData()doesn't return an error, but doesn't verify that the returned data is correct. Consider enhancing it to validate the expected byte encoding and using a more realistic address for testing.func TestPackData(t *testing.T) { - _, err := PacketData(common.HexToAddress("0x01")) + testAddr := common.HexToAddress("0x71c7656ec7ab88b098defb751b7401b5f6d8976f") + data, err := PacketData(testAddr) require.NoError(t, err) + // Verify the function selector for "recordBlocks" + expectedSelector := []byte{0xa7, 0x15, 0xd7, 0xc3} // First 4 bytes are function selector + require.Equal(t, expectedSelector, data[:4]) + // Further verification could check if the address is properly encoded in the data }contracts/morphtoken/morph_token.go (3)
11-11: Add comments explaining the ABI purpose and mintInflations functionThe ABI definition lacks documentation about what the
mintInflationsfunction does and how it relates to the system call handling mentioned in the PR title.-const jsonData = `[{"inputs":[],"name":"mintInflations","outputs":[],"stateMutability":"nonpayable","type":"function"}]` +// jsonData defines the ABI for the MorphToken contract's mintInflations function, +// which is called by the system to mint inflation rewards. +const jsonData = `[{"inputs":[],"name":"mintInflations","outputs":[],"stateMutability":"nonpayable","type":"function"}]`
19-29: Add documentation for the Abi functionThe
Abi()function would benefit from documentation describing its purpose, thread safety, and return values.+// Abi returns the parsed ABI for the MorphToken contract. +// It ensures the ABI is parsed only once using sync.Once. +// Returns the ABI and any error encountered during parsing. func Abi() (*abi.ABI, error) { loadOnce.Do(func() { tokenABI, err := abi.JSON(strings.NewReader(jsonData)) if err != nil { - loadErr = fmt.Errorf("failed to parse ABI: %w", err) + loadErr = fmt.Errorf("failed to parse MorphToken ABI: %w", err) return } morphTokenABI = &tokenABI }) return morphTokenABI, loadErr }
31-41: Add documentation for the PacketData functionThe
PacketData()function needs documentation explaining its purpose in the context of system calls and how it's expected to be used.+// PacketData prepares the transaction data for calling the mintInflations function +// on the MorphToken contract. This is used for system-initiated token inflation. +// Returns the ABI-encoded call data or an error if packing fails. func PacketData() ([]byte, error) { a, err := Abi() if err != nil { return nil, fmt.Errorf("failed to get ABI: %w", err) } data, err := a.Pack("mintInflations") if err != nil { return nil, fmt.Errorf("failed to pack data: %w", err) } return data, nil }rollup/rcfg/config.go (3)
36-38: Improve documentation for MorphTokenAddressConsider adding more detailed documentation explaining the purpose of the MorphToken contract and how it relates to system call handling.
- // MorphTokenAddress is the address of the morph token contract + // MorphTokenAddress is the address of the MorphToken contract which handles + // token inflation through system-initiated calls. This contract is called + // via the mintInflations function during transaction processing. MorphTokenAddress = common.HexToAddress("0x5300000000000000000000000000000000000013")
39-42: Add detailed documentation for L2StakingAddress and RewardStartedSlotThe documentation for L2StakingAddress is minimal, and RewardStartedSlot lacks documentation entirely. Explain how these are used in system calls and their relationship to each other.
- // L2StakingAddress is the address of the l2 staking contract + // L2StakingAddress is the address of the L2 staking contract which handles + // staking operations and rewards. This contract is called via the recordBlocks + // function during system-initiated transactions. L2StakingAddress = common.HexToAddress("0x5300000000000000000000000000000000000015") + // RewardStartedSlot is the storage slot that indicates whether rewards have + // been started for the current block. This is used in the pre-transaction hook. RewardStartedSlot = common.BigToHash(big.NewInt(6))
43-45: Improve documentation for SystemAddressAdd more context about what the system address is used for and how it relates to the system call handling mechanism.
- // SystemAddress is the address of the system + // SystemAddress is the address used as the sender for system-initiated calls. + // When transactions are sent from this address, they are treated as system calls + // and are processed differently than regular user transactions. SystemAddress = common.HexToAddress("0x5300000000000000000000000000000000000021")contracts/l2staking/l2staking.go (1)
12-12: Consider making the ABI configurable rather than hardcoding it.The ABI is currently hardcoded as a string constant. While this works, it might be less maintainable if the contract interface changes in the future. Consider making this configurable or loading it from a file for better flexibility.
consensus/l2/consensus.go (2)
237-242: Consider making the gas limit configurable.The hard-coded gas limit of 210000 for the
evm.Calloperation might be inflexible. Consider making this a configurable parameter, especially if gas requirements might change with contract updates.
306-312: Remove commented-out code.There's a block of commented-out code at the end of the file that appears to be leftover from development. This should be removed to maintain code cleanliness.
-//type XX struct { -// consensus.ChainHeaderReader -//} -// -//func (l2 *Consensus) Engine() consensus.Engine { -// return l2.ethone -//}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (10)
consensus/clique/clique.go(1 hunks)consensus/consensus.go(1 hunks)consensus/ethash/consensus.go(1 hunks)consensus/l2/consensus.go(4 hunks)contracts/l2staking/l2staking.go(1 hunks)contracts/l2staking/l2staking_test.go(1 hunks)contracts/morphtoken/morph_token.go(1 hunks)contracts/morphtoken/morph_token_test.go(1 hunks)core/state_processor.go(1 hunks)rollup/rcfg/config.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- consensus/consensus.go
- consensus/ethash/consensus.go
- consensus/clique/clique.go
- core/state_processor.go
🧰 Additional context used
🧬 Code Definitions (2)
contracts/morphtoken/morph_token.go (1)
contracts/l2staking/l2staking.go (5) (5)
Abi(20-30)loadOnce(16-16)jsonData(12-12)loadErr(17-17)PacketData(32-42)
consensus/l2/consensus.go (3)
rollup/rcfg/config.go (4) (4)
L2StakingAddress(40-40)RewardStartedSlot(41-41)SystemAddress(44-44)MorphTokenAddress(37-37)contracts/l2staking/l2staking.go (1) (1)
PacketData(32-42)contracts/morphtoken/morph_token.go (1) (1)
PacketData(31-41)
🔇 Additional comments (3)
contracts/l2staking/l2staking.go (2)
20-30: Good use of singleton pattern for ABI loading.The use of
sync.Onceto ensure the ABI is only parsed once is a good practice. The error handling is also appropriate, wrapping the original error with context.
32-42: The data packing function follows good practices.The function properly handles errors from both ABI retrieval and data packing operations, with clear error messages that will facilitate debugging.
consensus/l2/consensus.go (1)
41-58: Well-structured chainContext implementation.The
chainContexttype and its methods are well-implemented, providing a clean interface for interacting with the chain reader and engine.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
consensus/l2/consensus.go (2)
309-315: Remove commented-out code.This commented-out code appears to be a development artifact that serves no purpose in the production codebase. Removing it will improve code readability and maintainability.
-//type XX struct { -// consensus.ChainHeaderReader -//} -// -//func (l2 *Consensus) Engine() consensus.Engine { -// return l2.ethone -//}
241-245: Consider adding a comment explaining the purpose of the system call.Adding documentation about what this system call to the L2StakingAddress accomplishes would improve code maintainability and help other developers understand the intent.
systemAddress := vm.AccountRef(rcfg.SystemAddress) + // Call L2Staking contract to record block production by the validator _, _, err = evm.Call(systemAddress, rcfg.L2StakingAddress, stakingCallData, 210000, common.Big0) if err != nil { return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
consensus/l2/consensus.go(4 hunks)
🔇 Additional comments (4)
consensus/l2/consensus.go (4)
235-235: Implement or clarify the tracer TODO comment.The TODO comment lacks context about what needs to be implemented for the tracer. Either implement the tracer functionality or provide more details about what is expected here.
246-246: Add validation for rewardEpoch to prevent division by zero.The code calculates
parentHeader.Time / rewardEpochandheader.Time / rewardEpochwithout checking ifrewardEpochis zero, which could lead to a panic. Although it's initialized as 86400, adding a safety check would prevent potential issues if the value is modified elsewhere.// Add this at the beginning of the StartHook function + if rewardEpoch == 0 { + return fmt.Errorf("rewardEpoch cannot be zero") + }
223-257: Good implementation of the StartHook system call mechanism.The StartHook implementation properly handles system calls before transaction processing. It correctly:
- Checks if rewards are started
- Retrieves the parent header
- Creates the appropriate EVM context
- Makes system calls to the staking contract
- Handles reward epoch boundaries
This is a clean implementation that follows proper error handling patterns.
41-57: Good implementation of the chainContext wrapper.The chainContext implementation correctly wraps the ChainHeaderReader and Engine interfaces, providing the necessary methods for interacting with the blockchain data. The structure is clean and follows Go idioms.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
consensus/l2/consensus.go (1)
246-246:⚠️ Potential issueAdd validation for rewardEpoch to prevent division by zero.
The code calculates
parentHeader.Time / rewardEpochandheader.Time / rewardEpochwithout checking ifrewardEpochis zero, which could lead to a panic. Although it's initialized as 86400, adding a safety check would prevent potential issues if the value is modified elsewhere.// Add this at the beginning of the StartHook function + if rewardEpoch == 0 { + return fmt.Errorf("rewardEpoch cannot be zero") + }
🧹 Nitpick comments (2)
consensus/l2/consensus.go (2)
236-245: Consider extracting magic numbers into named constants.The hard-coded gas limit value of 210000 is used for both contract calls. Consider extracting this into a named constant to improve maintainability.
+const ( + // DefaultSystemCallGasLimit represents the standard gas limit for system contract calls + DefaultSystemCallGasLimit = 210000 +) // Then replace both occurrences: - _, _, err = evm.Call(systemAddress, rcfg.L2StakingAddress, stakingCallData, 210000, common.Big0) + _, _, err = evm.Call(systemAddress, rcfg.L2StakingAddress, stakingCallData, DefaultSystemCallGasLimit, common.Big0) // And: - _, _, err = evm.Call(systemAddress, rcfg.MorphTokenAddress, callData, 210000, common.Big0) + _, _, err = evm.Call(systemAddress, rcfg.MorphTokenAddress, callData, DefaultSystemCallGasLimit, common.Big0)
223-257: Document the StartHook function more thoroughly.While the function has a brief comment, it would benefit from more detailed documentation explaining its purpose, parameters, and return values. This is especially important for hooks in consensus engines.
-// StartHook implements calling before start apply transactions of block +// StartHook is called before applying transactions of a block. It handles system calls +// that need to run before transaction processing, such as staking rewards and token inflation. +// +// Parameters: +// - chain: The chain reader for accessing blockchain data +// - header: The current block header being processed +// - state: The current state database for the block +// +// Returns an error if any of the system calls fails, which will abort block processing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
consensus/l2/consensus.go(3 hunks)core/state_processor.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/state_processor.go
🧰 Additional context used
🧬 Code Definitions (1)
consensus/l2/consensus.go (3)
rollup/rcfg/config.go (4) (4)
L2StakingAddress(40-40)RewardStartedSlot(41-41)SystemAddress(44-44)MorphTokenAddress(37-37)contracts/l2staking/l2staking.go (1) (1)
PacketData(32-42)contracts/morphtoken/morph_token.go (1) (1)
PacketData(31-41)
🔇 Additional comments (7)
consensus/l2/consensus.go (7)
11-18: Import updates look good for the new system call functionality.The new imports for L2 staking, Morph token, core VM, and rollup configuration are appropriate for the system call handling functionality being added.
23-26: Numeric constants are well-documented.The updated global variables are clearly documented and
rewardEpochis appropriately defined with a value of 86400 (representing seconds in a day).
41-57: Good implementation of chainContext wrapper.The new chainContext type and its methods are well-designed for encapsulating chain access. The methods properly delegate to the underlying chain and engine objects.
59-59: Good practice using compile-time interface check.Using
var _ = consensus.Engine(&Consensus{})ensures at compile time that the Consensus struct properly implements the Engine interface.
223-235: StartHook implementation needs tracer implementation.The StartHook method is well-structured, but the TODO comment for the tracer needs to be addressed.
The TODO comment at line 235 lacks context about what needs to be implemented for the tracer. Based on past review comments, you should either implement the tracer functionality or provide more details about what's expected. A typical EVM tracer in Go Ethereum would implement methods such as
CaptureEnter,CaptureTxStart, etc., to hook into EVM execution events.What are the latest best practices for implementing EVM tracers in Go Ethereum?
236-245: Properly implemented L2Staking system call.The code correctly prepares the call data for the L2Staking contract's recordBlocks function and executes the call with appropriate error handling.
247-254: Good implementation of epoch boundary token minting.The code correctly implements the epoch boundary check and calls the MorphToken contract to mint inflation rewards when an epoch boundary is crossed. The error handling has been properly fixed based on previous review comments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
consensus/l2/consensus.go (3)
242-243: Consider using a more appropriate gas limit for contract calls.Using
params.MaxGasLimit(which is 0x7fffffffffffffff) for contract calls is excessive and could lead to performance issues. Consider using a more reasonable gas limit that aligns with the actual computational needs of the contract function.- _, _, err = evm.Call(systemAddress, rcfg.L2StakingAddress, stakingCallData, params.MaxGasLimit, common.Big0) + _, _, err = evm.Call(systemAddress, rcfg.L2StakingAddress, stakingCallData, 210000, common.Big0)
251-252: Use consistent gas limits for similar operations.The gas limit for the second contract call should match the approach used for the first call. If you implement the gas limit change suggested above, make sure to apply it consistently here as well.
- _, _, err = evm.Call(systemAddress, rcfg.MorphTokenAddress, callData, params.MaxGasLimit, common.Big0) + _, _, err = evm.Call(systemAddress, rcfg.MorphTokenAddress, callData, 210000, common.Big0)
225-228: Add a comment explaining therewardStartedcheck logic.The code checks if
rewardStartedequals 1, but there's no comment explaining what this state variable represents or why this check is necessary. Adding a clear explanation would improve maintainability.+ // Check if rewards functionality is active in the system rewardStarted := state.GetState(rcfg.L2StakingAddress, rcfg.RewardStartedSlot).Big() if rewardStarted.Cmp(common.Big1) != 0 { + // Skip reward processing if rewards are not yet activated return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
consensus/l2/consensus.go(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
consensus/l2/consensus.go (5)
consensus/consensus.go (1) (1)
ChainHeaderReader(32-47)rollup/rcfg/config.go (2) (2)
L2StakingAddress(40-40)MorphTokenAddress(37-37)contracts/l2staking/l2staking.go (1) (1)
PacketData(32-42)contracts/morphtoken/morph_token.go (1) (1)
PacketData(31-41)params/protocol_params.go (1) (1)
MaxGasLimit(24-24)
🔇 Additional comments (4)
consensus/l2/consensus.go (4)
235-235: Address the TODO comment for tracer implementation.The TODO comment lacks context about what needs to be implemented. Either implement the tracer functionality or provide more details about what is expected here.
246-246: Add validation for rewardEpoch to prevent division by zero.The code calculates
parentHeader.Time / rewardEpochandheader.Time / rewardEpochwithout checking ifrewardEpochis zero, which could lead to a panic. Although it's initialized as 86400, adding a safety check would prevent potential issues if the value is modified elsewhere.
223-257: LGTM! Well-structured hook implementation for pre-transaction operations.The
StartHookimplementation follows a clean pattern:
- Checks if reward functionality is active
- Retrieves the parent header
- Creates the EVM context
- Calls L2Staking contract to record blocks
- Checks for reward epoch boundaries and mints tokens if needed
This structure properly separates concerns and handles errors appropriately.
41-57: Good implementation of the chainContext wrapper.The chainContext type correctly implements the necessary methods to bridge between the chain reader and engine components. This is a clean abstraction that follows Go idioms for interface implementation.
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
Bug Fixes
Refactor