Feat: pass coinbase from consensus node #179
Conversation
WalkthroughThis pull request removes coinbase validation related to the fee vault from the consensus L2 engine and introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as L2ConsensusAPI
participant Engine
participant Miner
participant Worker
participant Config as ChainConfig
Client->>API: AssembleL2Block(params with coinbase)
API->>Engine: Forward request with coinbase, transactions
Engine->>Miner: BuildBlock(parentHash, timestamp, coinbase, transactions)
Miner->>Worker: prepareWork(genParams with coinbase)
Worker->>Config: IsMorph204(timestamp)?
Config-->>Worker: true/false
Worker->>Miner: Update coinbase assignment based on Morph204 check
sequenceDiagram
participant Worker
participant ChainConfig
Worker->>ChainConfig: IsMorph204(now)
alt Morph204 active
ChainConfig-->>Worker: true
Worker->>Worker: Set coinbase from genParams
else Morph204 inactive
ChainConfig-->>Worker: false
Worker->>Worker: Skip coinbase assignment
end
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (1)
ethclient/authclient/engine.go (1)
15-15: Client API signature properly updated to include coinbase parameter.The AssembleL2Block method has been correctly updated to accept the coinbase parameter and pass it to the engine API. This completes the chain of modifications needed to pass the coinbase from the consensus node through to the block creation process.
Consider adding documentation comments to explain the purpose and expected format of the coinbase parameter to help API users.
-// AssembleL2Block assembles L2 Block used for L2 sequencer to propose a block in L2 consensus progress +// AssembleL2Block assembles L2 Block used for L2 sequencer to propose a block in L2 consensus progress +// The coinbase parameter specifies the address that will receive the mining rewards for the block.Also applies to: 27-28
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
consensus/l2/consensus.go(0 hunks)eth/catalyst/api_types.go(1 hunks)eth/catalyst/gen_l2blockparams.go(4 hunks)eth/catalyst/l2_api.go(2 hunks)eth/catalyst/l2_api_test.go(3 hunks)ethclient/authclient/engine.go(2 hunks)miner/miner.go(1 hunks)miner/worker.go(1 hunks)params/config.go(2 hunks)
💤 Files with no reviewable changes (1)
- consensus/l2/consensus.go
🔇 Additional comments (10)
params/config.go (1)
541-541: Field addition for Morph204 upgrade checkpointThe addition of
Morph204Timefield to the ChainConfig struct allows for a time-based activation of the coinbase passing feature.eth/catalyst/api_types.go (1)
41-45:✅ Verification successful
Added Coinbase field to AssembleL2BlockParams struct
The addition of the
Coinbasefield to the AssembleL2BlockParams struct allows passing the coinbase address from the consensus node, which aligns with the PR objective.Make sure to update any JSON serialization/deserialization code for this struct if needed.
🏁 Script executed:
#!/bin/bash # Check if the generated code for this struct has been updated find . -name "gen_l2blockparams.go" -exec cat {} \; | grep -i "coinbase"Length of output: 253
Coinbase Field Addition Verified
The addition of the
Coinbasefield to theAssembleL2BlockParamsstruct now correctly propagates into the generated JSON serialization/deserialization code. The verification script confirms that the Coinbase field is being encoded and decoded as expected, matching thejson:"coinbase"tag.
- File:
eth/catalyst/api_types.go(Lines 41-45) – The struct now includes the Coinbase field.- Generated code: The output from
gen_l2blockparams.goconfirms proper handling of Coinbase in both encoding and decoding.No further updates are required.
miner/worker.go (1)
171-172: Conditional coinbase setting based on Morph204 activationThis change adds a feature gate for setting the coinbase based on the Morph204 upgrade activation status. This ensures the coinbase can only be set from the consensus node when the Morph204 feature is active.
The condition properly checks both for the Morph204 activation time and that the coinbase address is not empty.
eth/catalyst/l2_api_test.go (3)
114-115: Updated BuildBlock call with empty coinbase parameterTests have been updated to pass an empty address instead of nil to match the updated method signature.
173-174: Updated BuildBlock call with empty coinbase parameterTests have been updated to pass an empty address instead of nil to match the updated method signature.
226-227: Updated BuildBlock call with empty coinbase parameterTests have been updated to pass an empty address instead of nil to match the updated method signature.
eth/catalyst/gen_l2blockparams.go (1)
9-9: Looks good: Coinbase field implementation is correctly handled.The addition of the Coinbase field to the AssembleL2BlockParams struct is properly implemented for both JSON marshaling and unmarshaling. The implementation correctly handles both required fields and the new optional Coinbase field.
Also applies to: 19-19, 24-24, 38-38, 49-51
eth/catalyst/l2_api.go (2)
67-67: Good: Enhanced logging with coinbase information.Adding the coinbase address to the log message provides better context for debugging and monitoring block assembly operations.
84-84: Properly integrated coinbase parameter in BuildBlock call.The coinbase parameter is correctly passed to the BuildBlock method, ensuring the specified coinbase address is used in the block creation process.
miner/miner.go (1)
170-178:✅ Verification successful
BuildBlock signature change is properly implemented.
The signature change to include the coinbase parameter is well implemented. This modification allows external callers to specify the coinbase address for block creation, which aligns with the PR objective of passing the coinbase from the consensus node.
You might want to verify that all existing call sites to this method have been updated to include the coinbase parameter to avoid breaking changes.
🏁 Script executed:
#!/bin/bash # Find all calls to BuildBlock to ensure they're updated with the new signature rg -A 2 "BuildBlock\(" --type goLength of output: 1599
Approved: BuildBlock signature update correctly integrates the coinbase parameter
The coinbase parameter has been properly added to the
Miner.BuildBlockmethod, and our verification confirms that all call sites (e.g., ineth/catalyst/l2_api.goandeth/catalyst/l2_api_test.go) now include the coinbase argument. Note that theBuildBlockfunction incmd/evm/internal/t8ntool/block.gois unrelated and does not require this change.
| func (c *ChainConfig) IsMorph204(now uint64) bool { | ||
| return isForkedTime(now, c.Morph204Time) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Consistent implementation for new fork activation check
The IsMorph204 method follows the established pattern for fork detection by leveraging the existing isForkedTime helper function, maintaining codebase consistency.
Note that you'll need to update the CheckConfigForkOrder method to include this new fork in the sequence to properly enforce fork ordering.
🏁 Script executed:
#!/bin/bash
# Check if Morph204Time has been added to the fork ordering check
grep -A 30 "CheckConfigForkOrder.*func" params/config.go | grep -B 5 -A 15 "for.*range.*fork" | grep "morph204Time"Length of output: 115
Action Required: Update Fork Ordering in CheckConfigForkOrder
The new fork activation check in IsMorph204 is implemented consistently using the existing isForkedTime helper. However, our verification indicates that CheckConfigForkOrder has not yet been updated to include Morph204Time. Please update the fork ordering logic in CheckConfigForkOrder (located in params/config.go) to incorporate this new fork, ensuring that fork activation is enforced in the intended sequence.
* handle system call * revert unnecessary changes * revert unnecessary changes * fix consensus * fix RewardStartedSlot * handle errors * clean * max gas * Refine the getSystemResult implementation * refactor start hook --------- Co-authored-by: curryxbo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
contracts/morphtoken/morph_token_test.go (1)
1-13: Basic test coverage for MorphToken's PacketData function.This test verifies that the
PacketData()function can successfully pack the data needed for themintInflationsfunction call. While the test ensures the function runs without errors, consider enhancing it with assertions that verify the expected structure of the packed data.You could improve this test by also verifying the structure of the returned data:
func TestPackData(t *testing.T) { - _, err := PacketData() + data, err := PacketData() require.NoError(t, err) + + // Verify the function selector (first 4 bytes) + require.Equal(t, "0x12345678", hexutil.Encode(data[:4]), "Function selector should match mintInflations") + + // Add additional assertions based on expected ABI encoding }consensus/l2/consensus.go (2)
24-27: DuplicaterewardEpochdefinition—prefer a single constant
rewardEpochis declared here and again as a local constant intracing.go.
Keep a single exported constant (e.g. inrcfg) to avoid future drift.
215-249: Minor robustness & clarity issues inStartHook
The constructed
blockContextuses the current header; for pre-tx hooks you usually want the parent header so theBaseFee,Time, and difficulty fields align with the state you just loaded.
vm.Config{Tracer: nil}disables tracing completely. If tracing is desired (e.g., fordebug_traceBlock), pass the appropriate tracer or make this configurable.Any failure inside the hook aborts block processing. Consider wrapping
evm.Callwith limited gas and returning a more contextual error (e.g.,%w while calling L2Staking).rollup/tracing/tracing.go (2)
344-347: Second definition ofrewardEpoch
rewardEpochis already defined as a package-level variable in
consensus/l2/consensus.go. Remove this duplicate or reference the shared
constant to avoid silent divergence.
774-777: Out-of-date TODO commentThe
Transactionsfield is now populated, so the explanatory comment no longer applies.
Please delete or update to prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (12)
consensus/clique/clique.go(1 hunks)consensus/consensus.go(1 hunks)consensus/ethash/consensus.go(1 hunks)consensus/l2/consensus.go(3 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)core/types/l2trace.go(1 hunks)rollup/rcfg/config.go(1 hunks)rollup/tracing/tracing.go(9 hunks)
✅ Files skipped from review due to trivial changes (2)
- core/types/l2trace.go
- rollup/rcfg/config.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
contracts/l2staking/l2staking_test.go (4)
contracts/morphtoken/morph_token_test.go (1)
TestPackData(9-12)contracts/l2staking/l2staking.go (1)
PacketData(32-42)contracts/morphtoken/morph_token.go (1)
PacketData(31-41)common/types.go (1)
HexToAddress(218-218)
contracts/morphtoken/morph_token.go (2)
accounts/abi/abi.go (2)
ABI(33-44)JSON(47-55)contracts/l2staking/l2staking.go (2)
Abi(20-30)PacketData(32-42)
🔇 Additional comments (12)
consensus/clique/clique.go (1)
564-566: Implementation of StartHook consensus method.The new
StartHookmethod provides a no-op implementation for the Clique consensus engine, as required by the consensus.Engine interface. This is consistent with the implementation pattern in other consensus engines and allows for specialized behavior in the L2 consensus engine.consensus/ethash/consensus.go (1)
593-595: Implementation of StartHook consensus method.The new
StartHookmethod provides a no-op implementation for the Ethash consensus engine, as required by the consensus.Engine interface. This is consistent with the implementation pattern in other consensus engines and allows for specialized behavior in the L2 consensus engine.core/state_processor.go (1)
95-98: Added consensus engine hook before transaction processing.This change implements the
StartHookcall to the consensus engine before processing transactions. It correctly halts processing if the hook returns an error, providing a clean integration point for the staking and inflation minting logic implemented in the L2 consensus engine.The integration approach is clean and maintains compatibility with different consensus engines through their no-op implementations of the
StartHookmethod.contracts/l2staking/l2staking_test.go (1)
10-13: Simple but effective test for PacketData function.The test correctly verifies that
PacketDatacan handle a basic address input without errors. Consider enhancing it by also asserting the expected format of the returned data to ensure the encoding is correct, not just error-free.consensus/consensus.go (1)
84-86: New StartHook method added to Engine interface.This addition creates a pre-transaction execution hook point in the consensus engine, which aligns with the PR objective of passing coinbase from consensus nodes. The method signature is clean and consistent with other Engine interface methods.
Note that adding a method to an interface requires all implementers to provide this method, so ensure all Engine implementations have been updated accordingly.
contracts/morphtoken/morph_token.go (3)
11-17: Thread-safe ABI initialization pattern.Good use of constants and sync.Once for thread-safe, lazy initialization of the ABI. The JSON data correctly defines the
mintInflationsfunction with no parameters.
19-29: Well-implemented Abi function.The Abi function follows established patterns for ABI handling in go-ethereum, with proper error wrapping and thread safety.
31-41: Clean implementation of PacketData.This function correctly prepares the calldata for the
mintInflationsfunction with appropriate error handling. The error messages are clear and include the underlying cause.contracts/l2staking/l2staking.go (3)
12-18: Well-defined ABI and state variables.The ABI JSON correctly defines the
recordBlocksfunction that takes an address parameter. The package-level variables for caching are properly initialized.
20-30: Thread-safe ABI initialization.Good implementation of thread-safe, lazy loading of the ABI using sync.Once. Error handling is thorough with proper wrapping.
32-42: Correct implementation of PacketData.This function properly handles ABI encoding for the
recordBlocksfunction with an address parameter. All errors are appropriately checked and wrapped with meaningful messages.rollup/tracing/tracing.go (1)
678-689: Execution result ordering fix looks goodStoring
ExecutionResultsat the real transaction index instead ofindex+1
removes the earlier off-by-one confusion and matches the slice pre-allocation.
| parentHeader := chain.GetHeaderByHash(header.ParentHash) | ||
| if parentHeader == nil { | ||
| return consensus.ErrUnknownAncestor | ||
| } | ||
| cx := chainContext{Chain: chain, engine: l2.ethone} | ||
| blockContext := core.NewEVMBlockContext(header, cx, l2.config, nil) |
There was a problem hiding this comment.
GetHeaderByHash does not exist on ChainHeaderReader
consensus.ChainHeaderReader exposes GetHeader(hash, number) but not GetHeaderByHash.
This will cause a compilation error.
-parentHeader := chain.GetHeaderByHash(header.ParentHash)
+parentHeader := chain.GetHeader(header.ParentHash, header.Number.Uint64()-1)If the richer interface is really required, assert the type first:
type headerByHash interface {
GetHeaderByHash(hash common.Hash) *types.Header
}
var parentHeader *types.Header
if r, ok := chain.(headerByHash); ok {
parentHeader = r.GetHeaderByHash(header.ParentHash)
} else {
parentHeader = chain.GetHeader(header.ParentHash, header.Number.Uint64()-1)
}| // chain context | ||
| type chainContext struct { | ||
| Chain consensus.ChainHeaderReader | ||
| engine consensus.Engine | ||
| } | ||
|
|
||
| func (c chainContext) Engine() consensus.Engine { | ||
| return c.engine | ||
| } | ||
|
|
||
| func (c chainContext) GetHeader(hash common.Hash, number uint64) *types.Header { | ||
| return c.Chain.GetHeader(hash, number) | ||
| } | ||
|
|
||
| func (c chainContext) Config() *params.ChainConfig { | ||
| return c.Chain.Config() | ||
| } |
There was a problem hiding this comment.
Missing GetHeaderByNumber breaks the core.ChainContext contract
core.NewEVMBlockContext expects the argument to satisfy core.ChainContext, which in Geth also requires
GetHeaderByNumber(uint64) *types.Header.
The current chainContext implements Engine and GetHeader but omits this method, so the code will not compile.
type chainContext struct {
Chain consensus.ChainHeaderReader
engine consensus.Engine
}
+// GetHeaderByNumber delegates to the underlying reader.
+func (c chainContext) GetHeaderByNumber(number uint64) *types.Header {
+ return c.Chain.GetHeader(common.Hash{}, number) // or cast to a richer interface if available
+}
+📝 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.
| // chain context | |
| type chainContext struct { | |
| Chain consensus.ChainHeaderReader | |
| engine consensus.Engine | |
| } | |
| func (c chainContext) Engine() consensus.Engine { | |
| return c.engine | |
| } | |
| func (c chainContext) GetHeader(hash common.Hash, number uint64) *types.Header { | |
| return c.Chain.GetHeader(hash, number) | |
| } | |
| func (c chainContext) Config() *params.ChainConfig { | |
| return c.Chain.Config() | |
| } | |
| // chain context | |
| type chainContext struct { | |
| Chain consensus.ChainHeaderReader | |
| engine consensus.Engine | |
| } | |
| // GetHeaderByNumber delegates to the underlying reader. | |
| func (c chainContext) GetHeaderByNumber(number uint64) *types.Header { | |
| return c.Chain.GetHeader(common.Hash{}, number) // or cast to a richer interface if available | |
| } | |
| func (c chainContext) Engine() consensus.Engine { | |
| return c.engine | |
| } | |
| func (c chainContext) GetHeader(hash common.Hash, number uint64) *types.Header { | |
| return c.Chain.GetHeader(hash, number) | |
| } | |
| func (c chainContext) Config() *params.ChainConfig { | |
| return c.Chain.Config() | |
| } |
| if err := env.getSystemResult(env.state.Copy(), block, parent); err != nil { | ||
| return nil, fmt.Errorf("failed to execute StartHook: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
State divergence risk – StartHook runs on a throw-away copy
env.getSystemResult(env.state.Copy(), …) executes the hook on a copy of the state, so the
mutations (minting, rewards) are lost before transaction execution and proof generation.
This will desynchronise the trace from the real block.
Either:
-if err := env.getSystemResult(env.state.Copy(), block, parent); err != nil {
+if err := env.getSystemResult(env.state, block, parent); err != nil {or apply the diff from the copy back to env.state (more complex).
📝 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.
| if err := env.getSystemResult(env.state.Copy(), block, parent); err != nil { | |
| return nil, fmt.Errorf("failed to execute StartHook: %w", err) | |
| } | |
| if err := env.getSystemResult(env.state, block, parent); err != nil { | |
| return nil, fmt.Errorf("failed to execute StartHook: %w", err) | |
| } |
| if zktrieTracer.Available() && !env.ZkTrieTracer[addrStr].Available() { | ||
| env.ZkTrieTracer[addrStr] = state.NewProofTracer(trie) | ||
| } | ||
|
|
There was a problem hiding this comment.
Possible nil-pointer dereference when accessing env.ZkTrieTracer
env.ZkTrieTracer[addrStr] is read before the key is guaranteed to exist.
If the map’s element type is a pointer, calling .Available() on the zero
value will panic.
-if zktrieTracer.Available() && !env.ZkTrieTracer[addrStr].Available() {
- env.ZkTrieTracer[addrStr] = state.NewProofTracer(trie)
-}
+if zktrieTracer.Available() {
+ if tracer, ok := env.ZkTrieTracer[addrStr]; !ok || (tracer != nil && !tracer.Available()) {
+ env.ZkTrieTracer[addrStr] = state.NewProofTracer(trie)
+ }
+}📝 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.
| if zktrieTracer.Available() && !env.ZkTrieTracer[addrStr].Available() { | |
| env.ZkTrieTracer[addrStr] = state.NewProofTracer(trie) | |
| } | |
| if zktrieTracer.Available() { | |
| if tracer, ok := env.ZkTrieTracer[addrStr]; !ok || (tracer != nil && !tracer.Available()) { | |
| env.ZkTrieTracer[addrStr] = state.NewProofTracer(trie) | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eth/catalyst/gen_l2_sd.go (1)
1-1: Note about generated code.This file is generated by gencodec. If further changes to the schema are needed, remember to modify the source definition file rather than this generated output.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (3)
eth/catalyst/api_types.go(2 hunks)eth/catalyst/gen_l2_sd.go(4 hunks)eth/catalyst/l2_api.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- eth/catalyst/l2_api.go
- eth/catalyst/api_types.go
🔇 Additional comments (2)
eth/catalyst/gen_l2_sd.go (2)
19-19: The Miner field integration looks good.The
Minerfield (coinbase address) has been correctly added to theSafeL2Datastruct's marshaling process with the appropriaterequiredtag, and is properly assigned from the source struct during marshaling.Also applies to: 28-28
46-46: Proper validation for the required Miner field.The unmarshal logic correctly:
- Defines the Miner field as a pointer to allow null detection
- Checks if the field is present in the JSON data
- Returns a descriptive error if missing
- Correctly assigns the value when present
This ensures that the coinbase/miner address is always provided in the JSON data, maintaining data integrity for the consensus node communication.
Also applies to: 58-61
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