Enable starting monomer apps in verifier mode #320
Conversation
Introduces the --monomer.sequencer-mode flag to start the Monomer application in sequencer mode. Otherwise, the Monomer application will be started in verifier mode.
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
opdevnet/opdevnet_test.go (1)
178-188: Enhance the Synchronization Loop for RobustnessThe loop waiting for the verifier node to sync to block 10 could be improved by adding a timeout context or by checking for context cancellation to make it more robust and responsive to test interruptions.
Consider refactoring the loop as follows:
for { select { case <-ctx.Done(): t.Fatalf("Context cancelled before verifier synced to block 10") default: verifierBlockNum := verifierL2Backend.BlockChain().CurrentHeader().Number if verifierBlockNum.Uint64() >= 10 { break } time.Sleep(time.Second) } }integrations/integrations.go (1)
57-57: Enhance flag documentation for clarity.While the flag implementation is correct, consider enhancing the description to be more explicit about the implications of each mode:
-cmd.Flags().Bool(flagSequencerMode, false, "enable sequencer mode for the Monomer node") +cmd.Flags().Bool(flagSequencerMode, false, "enable sequencer mode for the Monomer node (default: verifier mode). In sequencer mode, the node runs batcher and proposer services")Also applies to: 82-82
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/docs/build/create-an-app-with-monomer.md(1 hunks)docs/docs/build/interact.md(1 hunks)e2e/e2e.go(1 hunks)integrations/integrations.go(3 hunks)opdevnet/op.go(1 hunks)opdevnet/opdevnet_test.go(1 hunks)
🔇 Additional comments (5)
docs/docs/build/create-an-app-with-monomer.md (1)
48-59: Clarify Instructions for Running in Sequencer and Verifier Modes
The documentation correctly specifies how to start the application in sequencer and verifier modes. Ensure that users are aware that the --monomer.sequencer-mode flag explicitly sets the application to sequencer mode, and omitting it defaults to verifier mode.
docs/docs/build/interact.md (1)
13-13: Update Command to Reflect New Sequencer Mode Flag
The command now includes the --monomer.sequencer-mode flag, which is necessary to start the devnet in sequencer mode. This update aligns with the recent changes and provides clear instructions.
e2e/e2e.go (1)
74-76: Include Sequencer Mode Flag in E2E Test Setup
Adding the --monomer.sequencer-mode flag ensures that the application starts in sequencer mode during end-to-end testing, which is essential for the test scenarios.
opdevnet/op.go (1)
147-151: LGTM: Clean implementation of verifier mode.
The conditional block effectively implements verifier mode by skipping batcher and proposer services when SequencerEnabled is false. This ensures clean separation of concerns and prevents unnecessary resource usage.
Let's verify the mode separation:
✅ Verification successful
Based on the results, I can generate the final response as the verification is complete. The search results show:
Verified: Mode separation is properly implemented
The codebase shows clean separation between sequencer and verifier modes:
- Configuration is properly handled via
SequencerEnabledflag - Test files demonstrate correct verifier mode setup (
opdevnet_test.go) - Documentation clearly explains both modes and their usage
- No unexpected coupling between verifier operations and sequencer services found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that batcher and proposer services are properly isolated
# Test: Search for any direct dependencies between verifier mode and sequencer services
# Expect: No direct coupling between verifier operations and sequencer services
# Check for proper separation of verifier/sequencer concerns
rg -A 5 "SequencerEnabled|sequencer.*mode|verifier.*mode"
# Check for proper initialization of services
ast-grep --pattern 'if $cond {
$$$
return nil
}'
Length of output: 4393
integrations/integrations.go (1)
307-307: LGTM: Proper flag value propagation.
The flag value is correctly propagated to the OP configuration's SequencerEnabled field.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
opdevnet/opdevnet_test.go (2)
162-174: Consider removing unnecessary secrets for verifierThe verifier configuration includes batcher and proposer secrets (lines 164-165) which aren't needed since the verifier doesn't perform batching or proposing operations.
Consider passing empty or dummy secrets for the verifier:
verifierOpConfig, err := opdevnet.BuildOPConfig( deployConfig, - secrets.Batcher, - secrets.Proposer, + eth.SignerEmpty{}, // dummy signer for batcher + eth.SignerEmpty{}, // dummy signer for proposer l1GenesisBlock, l1Deployments.L2OutputOracleProxy,
179-190: Enhance sync verification robustness and readabilityConsider the following improvements to the sync verification logic:
- Use descriptive constants for magic numbers
- Implement exponential backoff for more efficient waiting
- Provide more descriptive error messages
+ const ( + targetSyncBlock = 10 // Block number to verify state root + maxWaitIterations = 30 // Maximum number of iterations to wait for sync + baseWaitTime = time.Second + ) - const waitBlocks = 30 - for i := 0; i < waitBlocks; i++ { - if verifierL2Backend.BlockChain().CurrentHeader().Number.Uint64() >= 10 { + for i := 0; i < maxWaitIterations; i++ { + currentBlock := verifierL2Backend.BlockChain().CurrentHeader().Number.Uint64() + if currentBlock >= targetSyncBlock { // Assert that the verifier and sequencer state roots at block 10 are equal - require.Equal(t, verifierL2Backend.BlockChain().GetHeaderByNumber(10).Root, l2Backend.BlockChain().GetHeaderByNumber(10).Root) + require.Equal(t, + verifierL2Backend.BlockChain().GetHeaderByNumber(targetSyncBlock).Root, + l2Backend.BlockChain().GetHeaderByNumber(targetSyncBlock).Root, + "State root mismatch at block %d", targetSyncBlock, + ) break - } else if i == waitBlocks-1 { - t.Fatalf("verifier only synced to block %v", verifierL2Backend.BlockChain().CurrentHeader().Number) + } else if i == maxWaitIterations-1 { + t.Fatalf("Verifier sync timeout: reached block %d, expected >= %d after %d iterations", + currentBlock, targetSyncBlock, maxWaitIterations) } - time.Sleep(time.Second * time.Duration(l1Config.BlockTime)) + // Exponential backoff with base wait time + waitTime := baseWaitTime * time.Duration(1<<uint(i)) + if waitTime > time.Duration(l1Config.BlockTime)*time.Second { + waitTime = time.Duration(l1Config.BlockTime)*time.Second + } + time.Sleep(waitTime) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/docs/build/create-an-app-with-monomer.md(1 hunks)docs/docs/build/interact.md(1 hunks)e2e/e2e.go(1 hunks)integrations/integrations.go(3 hunks)opdevnet/opdevnet_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/e2e.go
- docs/docs/build/create-an-app-with-monomer.md
- docs/docs/build/interact.md
- integrations/integrations.go
🔇 Additional comments (3)
opdevnet/opdevnet_test.go (3)
81-81: LGTM: URL reachability check is well-placed
The L1 URL reachability check is appropriately placed after the L1 node setup and uses the correct assertion method.
Line range hint 89-102: LGTM: Sequencer node setup is properly configured
The sequencer node setup follows best practices with:
- Proper L2 node initialization
- Correct JWT configuration
- Appropriate URL configurations
- Well-placed reachability check
140-177: LGTM: Verifier node setup is well-structured
The verifier node setup is properly implemented with:
- Distinct ports for each service
- Correct reuse of JWT secret
- Explicit disabling of sequencer mode
Introduces the
--monomer.sequencer-modeflag to start the Monomer application in sequencer mode. Otherwise, the Monomer application will be started in verifier mode.Summary by CodeRabbit
Documentation
New Features