Feat: pass coinbase to execution layer, and add coinbase to block context #731
Conversation
WalkthroughThis pull request updates the submodule branch in Changes
Sequence Diagram(s)sequenceDiagram
participant E as Executor
participant M as TmPubKeyMap
participant RC as RetryableClient
participant AC as AuthClient
E->>M: Lookup coinbase using tmKey
alt Coinbase Found
E->>RC: Call AssembleL2Block(ctx, number, coinbase, txs)
RC->>AC: Forward call with coinbase
AC-->>RC: Return assembled L2 block
RC-->>E: Return assembled L2 block
else Coinbase Not Found
E-->>E: Log error and abort process
end
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package zstd: could not load export data: no export data for "morph-l2/node/zstd"" ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 1
🔭 Outside diff range comments (2)
node/flags/flags.go (1)
304-367:⚠️ Potential issueFlag missing from Flags slice.
The newly added
Morph204Timeflag isn't included in theFlagsslice, meaning it won't be recognized by the CLI parser.Apply this diff to include the flag:
var Flags = []cli.Flag{ Home, L1NodeAddr, L1ChainID, L1Confirmations, L2EthAddr, L2EngineAddr, L2EngineJWTSecret, MaxL1MessageNumPerBlock, L2CrossDomainMessengerContractAddr, L2SequencerAddr, GovAddr, // sync optioins SyncDepositContractAddr, SyncStartHeight, SyncPollInterval, SyncLogProgressInterval, SyncFetchBlockRange, // db options DBDataDir, DBNamespace, DBHandles, DBCache, DBFreezer, DevSequencer, TendermintConfigPath, MockEnabled, ValidatorEnable, ChallengeEnable, // validator ValidatorPrivateKey, // derivation RollupContractAddress, DerivationStartHeight, DerivationBaseHeight, DerivationPollInterval, DerivationLogProgressInterval, DerivationFetchBlockRange, DerivationConfirmations, L1BeaconAddr, // batch rules UpgradeBatchTime, + Morph204Time, MainnetFlag, HoleskyFlag, // logger LogLevel, LogFormat, LogFilename, LogFileMaxSize, LogFileMaxAge, LogCompress, // metrics MetricsServerEnable, MetricsPort, MetricsHostname, }node/types/consensus_message.go (1)
171-176:⚠️ Potential issuePotential issue: HeightFromBlockContextBytes function needs update
The
HeightFromBlockContextBytesfunction checks for a strict length of 60 bytes, but with the new changes, the block context bytes can now be 80 bytes when the Miner address is included.- if len(blockContextBytes) != 60 { + if len(blockContextBytes) != 60 && len(blockContextBytes) != 80 { return 0, fmt.Errorf("wrong block context bytes length, input: %x", blockContextBytes) }
🧹 Nitpick comments (3)
ops/docker/Dockerfile.l1-beacon (1)
3-3: Added RUN date Command.
The addition ofRUN dateprints the current date during the Docker build. This can be useful for debugging or build timestamping. Please verify that this output is intentional and documented (or remove it if not required in production builds).ops/docker/docker-compose-4nodes.yml (1)
253-253: Commented-out Logging Configuration.
A new commented-out line## --log.level="consensus:debug,*:info"has been introduced. This appears to suggest a logging configuration option for debugging. Confirm whether you want to keep it for future use (with proper documentation on how to enable it) or remove it entirely to avoid confusion.node/core/batch.go (1)
345-347: Remove unused function isBatchUpgraded.This function is flagged as unused in the pipeline failures and appears to be leftover from the previous conditional logic that was simplified.
-func (e *Executor) isBatchUpgraded(blockTime uint64) bool { - return blockTime >= e.UpgradeBatchTime -}🧰 Tools
🪛 GitHub Actions: Node
[warning] 345-345: func
(*Executor).isBatchUpgradedis unused (unused)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
bindings/go.sumis excluded by!**/*.sumcontracts/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumnode/go.sumis excluded by!**/*.sumops/l2-genesis/go.sumis excluded by!**/*.sumops/tools/go.sumis excluded by!**/*.sumoracle/go.sumis excluded by!**/*.sumtx-submitter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
.gitmodules(1 hunks)Makefile(2 hunks)bindings/go.mod(1 hunks)contracts/go.mod(1 hunks)contracts/src/deploy-config/l1.ts(1 hunks)go-ethereum(1 hunks)node/core/batch.go(1 hunks)node/core/batch_seal.go(2 hunks)node/core/config.go(3 hunks)node/core/executor.go(2 hunks)node/flags/flags.go(1 hunks)node/go.mod(1 hunks)node/sequencer/mock/sequencer.go(2 hunks)node/types/consensus_message.go(2 hunks)node/types/retryable_client.go(1 hunks)ops/docker/Dockerfile.l1-beacon(1 hunks)ops/docker/docker-compose-4nodes.yml(1 hunks)ops/l2-genesis/deploy-config/devnet-deploy-config.json(2 hunks)ops/l2-genesis/go.mod(1 hunks)ops/l2-genesis/morph-chain-ops/genesis/config.go(1 hunks)ops/l2-genesis/morph-chain-ops/genesis/genesis.go(2 hunks)ops/tools/go.mod(1 hunks)oracle/go.mod(1 hunks)tx-submitter/go.mod(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Node
node/core/batch.go
[warning] 345-345: func (*Executor).isBatchUpgraded is unused (unused)
🔇 Additional comments (33)
node/go.mod (1)
13-13: Update of go-ethereum Dependency Version.
The dependency ongithub.com/morph-l2/go-ethereumhas been updated tov1.10.14-0.20250225020901-6fe0ee6ffedd, aligning with similar updates across other modules. Considering the PR’s objectives—to pass the coinbase to the execution layer and add coinbase to the block context—please verify that this new version provides the necessary support for those features.ops/tools/go.mod (1)
8-8: Updated go-ethereum Dependency Version in Tools Module.
The change togithub.com/morph-l2/go-ethereum v1.10.14-0.20250225020901-6fe0ee6ffeddensures consistency with the overall dependency updates. Double-check that this version meets all feature requirements (including handling coinbase parameters) for the tools module.contracts/go.mod (1)
9-9: Dependency Version Update in Contracts Module.
The update ofgithub.com/morph-l2/go-ethereumhere tov1.10.14-0.20250225020901-6fe0ee6ffeddbrings consistency across modules. Given the PR’s focus on coinbase integration, please confirm that this new version offers the necessary improvements and supports the added coinbase parameter in block context.ops/l2-genesis/go.mod (1)
9-9: Version updated for go-ethereum dependency.The dependency on
github.com/morph-l2/go-ethereumhas been updated to versionv1.10.14-0.20250225020901-6fe0ee6ffedd. This aligns with similar updates across multiple modules in this PR.tx-submitter/go.mod (1)
10-10: Version updated for go-ethereum dependency.The dependency on
github.com/morph-l2/go-ethereumhas been updated to versionv1.10.14-0.20250225020901-6fe0ee6ffedd. This aligns with similar updates across multiple modules in this PR.oracle/go.mod (1)
10-10: Version updated for go-ethereum dependency.The dependency on
github.com/morph-l2/go-ethereumhas been updated to versionv1.10.14-0.20250225020901-6fe0ee6ffedd. This aligns with similar updates across multiple modules in this PR.bindings/go.mod (1)
7-7: Version updated for go-ethereum dependency.The dependency on
github.com/morph-l2/go-ethereumhas been updated to versionv1.10.14-0.20250225020901-6fe0ee6ffedd. This aligns with similar updates across multiple modules in this PR.node/flags/flags.go (1)
229-233: New flag added for Morph 204 upgrade time.The new flag is properly defined and follows the same pattern as other flags in the file.
ops/l2-genesis/deploy-config/devnet-deploy-config.json (2)
24-25: Governance batch parameters significantly increasedThe
govBatchBlockIntervalandgovBatchTimeouthave been substantially increased (from 20 to 600 and from 600 to 1200 respectively). These changes will result in less frequent batch processing and longer timeouts.Could you confirm if this change was intentional and whether it could impact system responsiveness? The significant increase suggests a fundamental change in governance batch processing frequency.
46-51: New staking public keys addedThe addition of
l2StakingPkswith four public keys aligns with the PR objective of adding coinbase support. These keys likely correspond to thel2StakingAddressesdefined above.Make sure these public keys are properly secured in production environments, as they're sensitive cryptographic material.
Makefile (1)
3-3: Updated Ethereum target versionThe
ETHEREUM_TARGET_VERSIONhas been updated to a newer version, which aligns with the PR objective of updating dependency versions.contracts/src/deploy-config/l1.ts (1)
30-30: Updated batch header valueThe
batchHeaderhexadecimal value has been updated, which is likely related to the changes in batch processing mentioned in the PR description.Since this is a large hexadecimal value that's difficult to verify by visual inspection, can you confirm this value was generated correctly and matches the expected format for the updated system?
go-ethereum (1)
1-1: Submodule Commit Updated Correctly.The submodule pointer has been updated to commit
6039a12094aa91bc711d7296341f58ded34f3077, which aligns with the updated branchfeatures/coinbaseas specified in the PR objectives. Please ensure that the associated changes in the.gitmodulesand build configurations (e.g., Makefile) are thoroughly tested with this new commit..gitmodules (1)
4-4: Branch update for go-ethereum submoduleThe change from "main" branch to "features/coinbase" branch aligns with the PR's objective to pass coinbase to the execution layer. This change will ensure that the project uses the specific feature branch that contains the required coinbase functionality.
node/types/consensus_message.go (1)
108-125: Good implementation of coinbase in block contextThe implementation correctly appends the Miner address to the block context bytes when it's not empty. The comment has been updated to reflect the new structure including the Miner field (20 bytes).
A few observations:
- The conditional check for non-empty Miner address is appropriate
- The byte slice is extended to accommodate the additional 20 bytes
- The Miner address bytes are copied to the correct position in the extended array
This implementation supports the PR objective of adding coinbase to the block context.
ops/l2-genesis/morph-chain-ops/genesis/config.go (1)
131-131: Addition of Morph204Time configuration fieldThe new field
Morph204Timefollows the same pattern as the existingMorph203Timefield, with appropriate type (*uint64) and JSON tag. This addition allows for configuration of the Morph204 upgrade time.ops/l2-genesis/morph-chain-ops/genesis/genesis.go (2)
38-47: Good implementation of Morph204Time handlingThe new variable block declaration improves code readability and the initialization logic for
morph204Timefollows the established pattern formorph203Time. The implementation correctly checks if the configuration value is not nil before assigning it.
70-70: Properly extending chain configuration with Morph204TimeThe
Morph204Timefield is correctly integrated into theparams.ChainConfigstructure, maintaining consistency with how other upgrade times are configured.node/sequencer/mock/sequencer.go (2)
8-8: Necessary addition for new coinbase functionality.The import of the common package is required for using the common.Address type in the updated AssembleL2Block call.
43-43:❓ Verification inconclusive
New coinbase parameter added to AssembleL2Block.
The method call now includes a coinbase parameter (empty address), aligning with the updated method signature in node/types/retryable_client.go.
Consider whether an empty address is appropriate as the coinbase parameter or if a specific address should be used for the mock sequencer.
🏁 Script executed:
#!/bin/bash # Check for other usages of AssembleL2Block to understand expected coinbase values rg -A 1 "AssembleL2Block" --type goLength of output: 1183
Action: Verify the appropriateness of the default empty coinbase value in the mock sequencer.
After reviewing the code:
- In
node/types/retryable_client.go, the updated method signature forAssembleL2Blocknow requires an explicitcoinbaseparameter.- In
node/core/executor.go, a specific coinbase value is provided, whereas in the mock sequencer (node/sequencer/mock/sequencer.go), an empty address (common.Address{}) is used.Please verify whether passing an empty address in the mock context accurately reflects the desired behavior for tests. If the intention is to simulate a real sequencer, you might consider using a representative coinbase value instead.
node/core/batch.go (1)
181-181: Code simplification for batch size checking.The logic for determining if a batch exceeds the compressed size limit has been simplified by removing conditional checks related to batch upgrades.
node/types/retryable_client.go (2)
48-48: Updated method signature to include coinbase address.The AssembleL2Block method now takes a coinbase parameter of type common.Address, which is important for block assembly.
50-50: Properly forwarded coinbase parameter.The coinbase parameter is correctly passed to the underlying authClient.AssembleL2Block method.
node/core/config.go (4)
25-28: Well-structured upgrade configuration.The new UpgradeConfig struct provides a cleaner way to manage upgrade-related parameters by grouping them together.
31-37: Network-specific upgrade configurations.Predefined configurations for Mainnet and Holesky networks with Morph204Time initialized to 0. This approach allows for network-specific defaults.
47-47: Config struct field updated to use new UpgradeConfig.Changed from a direct uint64 field to a pointer to UpgradeConfig struct, allowing for more structured configuration management.
170-189: Enhanced CLI configuration handling.The upgrade configuration now supports:
- Setting network-specific configs (Mainnet/Holesky)
- Creating a custom config with values from CLI flags
- Proper logging of configuration values
This provides more flexibility while maintaining good defaults.
node/core/executor.go (2)
123-123: Configuration refactoring improvement.The change to use
config.UpgradeConfig.BatchTimeinstead of a direct field improves organization by grouping upgrade-related configurations together.
178-187:✅ Verification successful
Implementation of coinbase address retrieval for block assembly.
The code correctly retrieves the validator's address from
valsByTmKeyto use as coinbase and passes it toAssembleL2Block. This aligns with the PR objective of passing coinbase to the execution layer.
🏁 Script executed:
#!/bin/bash # Check if all AssembleL2Block calls now use the coinbase parameter rg "AssembleL2Block\(" -A 1 -B 1Length of output: 1071
Action Required: Confirm proper coinbase usage in production and mock contexts
The changes in
node/core/executor.gocorrectly retrieve the validator’s coinbase fromvalsByTmKeyand pass it toAssembleL2Block(). Our verification confirms that in production code—such as innode/types/retryable_client.go—the coinbase parameter is properly provided. Note that in the mock implementation (node/sequencer/mock/sequencer.go), the coinbase defaults to an empty address, which appears intentional for test scenarios.
- node/core/executor.go: Correctly assembles coinbase from validator data.
- node/types/retryable_client.go: Uses the coinbase parameter as expected.
- node/sequencer/mock/sequencer.go: Continues to pass an empty address, aligning with mock design.
node/core/batch_seal.go (4)
64-64: Parameter usage clarification.The
blockTimestampparameter has been replaced with an underscore, indicating it's no longer used in the function. This is good practice for documenting unused parameters.
72-75: Simplification of batch compression logic.The conditional checks for batch version have been removed in favor of directly using V2 methods, which simplifies the code.
77-82: Direct usage of V2 hash calculation.Similar to the compression logic, this simplifies the code by removing version checks and directly using V2 methods for data hash calculation.
99-115:✅ Verification successful
Simplified batch header creation.
The batch header creation has been streamlined to directly create a
BatchHeaderV1structure without conditional version checking, making the code cleaner and more maintainable.
🏁 Script executed:
#!/bin/bash # Check for any remaining references to conditionally creating different batch header versions rg "BatchHeaderV0.*?Bytes\(\)" --no-filenameLength of output: 147
Batch header creation simplified verified; conditional version check removed.
The changes in
node/core/batch_seal.gonow directly create aBatchHeaderV1without any conditional version checking logic. The shell script search did not uncover any lingering conditional references in this context—the remainingBatchHeaderV0.Bytes()implementation is separate and expected.
| @@ -128,7 +128,7 @@ go-ubuntu-builder: | |||
|
|
|||
| ################## devnet 4 nodes #################### | |||
|
|
|||
| devnet-up: submodules go-ubuntu-builder | |||
| devnet-up: go-ubuntu-builder | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Simplified dependency chain for devnet-up target
The dependency on submodules has been removed from the devnet-up target, which simplifies the build process but might affect dependency management.
This simplification means developers will need to manually update submodules before running this target. Consider adding a comment or documentation update if this change is intentional.
🏁 Script executed:
#!/bin/bash
# Check if there are any uninitialized or outdated submodules
git submodule status | grep -E '^\-|^\+'Length of output: 96
Attention: Update Documentation for Manual Submodule Refresh in devnet-up Target
The dependency on submodules has been removed from the devnet-up target—as evidenced by the outdated go-ethereum submodule in your verification—which means submodules now need to be updated manually. Please add a comment in the Makefile and update the relevant documentation (e.g., README) to remind developers that running devnet-up no longer auto-updates submodules.
- The output from
git submodule status | grep -E '^\-|^\+'shows an outdatedgo-ethereumsubmodule. - If the removal of automatic submodule updates is intentional, document this behavior to avoid confusion.
Summary by CodeRabbit