Add verifier to e2e tests #325
Conversation
Only start the L1 instance if it isn't already up. This allows for verifier nodes to boot up independently if no sequencer node is already running.
WalkthroughThe pull request introduces enhancements to the end-to-end (e2e) testing framework for a blockchain system. The changes focus on improving the 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 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 (7)
e2e/e2e.go (3)
66-66: Setup script execution.
The helper script looks beneficial for environment setup. Consider logging the script's output for troubleshooting.
113-115: Prevent race conditions with "time.Sleep".
Sleeping for one second is a quick fix for waiting on L1 readiness. Consider using a more deterministic wait such as polling readiness or health checks.
129-131: Decouple environment.
Storing environment variables in Env might conflict with user OS environment. If you need custom environment variables, consider scoping them to the command only.e2e/clients.go (1)
56-59: Wrap errors with consistent context.
Using “block by number: %v” is helpful. You might consider including the block number for quicker debugging.- return nil, fmt.Errorf("block by number: %v", err) + return nil, fmt.Errorf("block by number (%d): %w", number, err)e2e/e2e_test.go (1)
61-63: Wait for node setup.
A 1-second sleep can be flaky in busy CI or slower machines. Consider a readiness check approach.integrations/integrations.go (1)
296-298: Document intent more clearly.
This comment block clarifies that L1 can run or not. Expanding these docs helps future maintainers understand the design choice.e2e/stack_test.go (1)
101-116: Enhance verifier sync test with better diagnostics and configurabilityThe verifier sync test implementation is functional but could be improved for better maintainability and debugging capabilities.
Consider these improvements:
+const ( + defaultSyncTimeout = 10 * time.Second + defaultSyncInterval = time.Second +) + func verifierSync(t *testing.T, stack *e2e.StackConfig) { sequencerBlock, err := stack.MonomerClient.BlockByNumber(stack.Ctx, nil) require.NoError(t, err) + t.Logf("Sequencer at block height %d with hash %s", + sequencerBlock.Header().Number.Uint64(), + sequencerBlock.Header().Hash().String()) - // Wait 10 seconds for the verifier node to sync with the sequencer node - for i := 0; i < 10; i++ { + timeout := time.After(defaultSyncTimeout) + ticker := time.NewTicker(defaultSyncInterval) + defer ticker.Stop() + + for { + select { + case <-timeout: + t.Fatalf("Verifier failed to sync with sequencer after %v", defaultSyncTimeout) + case <-ticker.C: verifierBlock, err := stack.VerifierClient.BlockByHash(stack.Ctx, sequencerBlock.Header().Hash()) - if verifierBlock != nil && err == nil { - t.Log("Verifier node can sync with the sequencer node") + if err != nil { + t.Logf("Verifier sync attempt failed: %v", err) + continue + } + if verifierBlock != nil { + t.Logf("Verifier successfully synced to block height %d", + verifierBlock.Header().Number.Uint64()) return } - - time.Sleep(time.Second) + t.Log("Verifier returned nil block, retrying...") } - require.Fail(t, "verifier node did not sync with the sequencer node") + } }The improvements include:
- Configurable timeout and interval constants
- More detailed logging of sync progress and failures
- Use of proper Go timer patterns with
select- Better error handling and diagnostics
- Clearer failure messages
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
e2e/clients.go(1 hunks)e2e/e2e.go(4 hunks)e2e/e2e_test.go(4 hunks)e2e/stack_test.go(1 hunks)integrations/integrations.go(2 hunks)
🔇 Additional comments (16)
e2e/e2e.go (8)
12-12: Prefer narrowing import usage.
The addition of "time" as a named import is fine, but always ensure it’s used consistently and is necessary where added.
32-32: VerifierClient field helpful for multi-node setups!
Adding "VerifierClient" extends the testing stack's capabilities for spinning up verifier nodes. Good addition.
58-58: Double-check parallel processes.
Ensure that monogenCmd finishes execution before launching subsequent tasks, to avoid concurrency pitfalls.
71-71: Sequencer app command clarity.
Renaming it to "sequencerAppCmd" helps to disambiguate from other nodes. This is a good readability improvement.
100-103: Handle sequencer startup errors gracefully.
Starting the sequencer command is crucial. If it crashes repeatedly, consider implementing a retry mechanism or more detailed error logs.
105-111: Introduce context-based termination checks.
These lines ensure that we capture context cancellation. This robust approach prevents goroutine leaks if the test stops abruptly.
116-128: Verifier node command.
Copying logic from sequencer node is consistent, but ensure concurrency aspects are well managed since both nodes run simultaneously.
134-135: Proper cleanup of verifier process.
Similar to the sequencer, deferring error checks helps ensure we handle unexpected terminations. Keep an eye on concurrency side effects.
e2e/clients.go (1)
61-64: Newly added BlockByHash method.
This aligns well with the existing pattern. Great for verifying blocks by hash. Keep consistent error message format.
e2e/e2e_test.go (4)
41-44: New test “Verifier Sync”
This is a crucial test for ensuring the verifier node properly syncs with the sequencer. Great addition to the coverage.
86-92: Verifier client creation.
Instantiating a second client is straightforward. Always verify that both the L2 and verifier endpoints do not conflict in ports or addresses.
93-96: Verifier BFT client addition.
Starting and verifying the BFT client ensures consistent integration testing. Keep an eye out for potential resource leaks.
127-127: VerifierClient field
Linking the newly created verifier client to the StackConfig is a cohesive step in your expanded e2e tests.
integrations/integrations.go (2)
42-42: New RPC import.
The added “github.com/ethereum/go-ethereum/rpc” import is consistent with other usage patterns.
299-305: Close L1 client if already running.
Allows verifier node to boot independently. Good safeguard to avoid re-initializing an active L1 node.
e2e/stack_test.go (1)
101-116: 🛠️ Refactor suggestion
Consider adding block content verification
The current implementation only verifies that the verifier can retrieve the block by hash, but doesn't verify the block contents.
Consider adding block content verification:
func verifierSync(t *testing.T, stack *e2e.StackConfig) {
sequencerBlock, err := stack.MonomerClient.BlockByNumber(stack.Ctx, nil)
require.NoError(t, err)
+ // Helper function to get block details for comparison
+ getBlockDetails := func(block types.Block) (hash common.Hash, number uint64, txCount int) {
+ header := block.Header()
+ return header.Hash(), header.Number.Uint64(), len(block.Transactions())
+ }
+
+ seqHash, seqNum, seqTxCount := getBlockDetails(sequencerBlock)
+
// Wait 10 seconds for the verifier node to sync with the sequencer node
for i := 0; i < 10; i++ {
verifierBlock, err := stack.VerifierClient.BlockByHash(stack.Ctx, sequencerBlock.Header().Hash())
if verifierBlock != nil && err == nil {
- t.Log("Verifier node can sync with the sequencer node")
+ verHash, verNum, verTxCount := getBlockDetails(verifierBlock)
+
+ // Verify block contents match
+ require.Equal(t, seqHash, verHash, "Block hash mismatch")
+ require.Equal(t, seqNum, verNum, "Block number mismatch")
+ require.Equal(t, seqTxCount, verTxCount, "Transaction count mismatch")
+
+ t.Logf("Verifier synced and verified block %d with %d transactions",
+ verNum, verTxCount)
return
}
time.Sleep(time.Second)
}
require.Fail(t, "verifier node did not sync with the sequencer node")
}✅ Verification successful
Block content verification is essential - similar pattern found in codebase
Based on the search results, there's evidence in the codebase (opdevnet_test.go) showing that block verification is indeed performed in other tests, specifically comparing state roots between verifier and sequencer nodes. This confirms that the original review comment's suggestion for adding block content verification is valid and follows existing patterns in the codebase.
The suggested changes in the review comment would improve test reliability by:
- Comparing block hashes
- Verifying block numbers match
- Checking transaction counts
- Providing more detailed logging
This aligns with the codebase's existing verification patterns while adding comprehensive block content checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any block verification functions in the codebase that could be reused
# Search for functions that compare blocks
rg -A 5 'func.*Block.*Compare|func.*Compare.*Block'
# Search for block verification patterns
ast-grep --pattern 'func $name($block1, $block2) {
$$$
$block1.Header()
$$$
$block2.Header()
$$$
}'
Length of output: 165
Script:
#!/bin/bash
# Let's search for existing block verification patterns in test files
rg -g '**/*test.go' -A 5 'verifier.*Block|block.*verif|compare.*block|block.*compare'
# Let's also check for any block verification in the main code
rg -g '**/*.go' -g '!**/*test.go' -A 5 'verifier.*Block|block.*verif|compare.*block|block.*compare'
# Let's look for any block header comparison patterns
rg -g '**/*.go' -A 5 'Header\(\).*Equal|Equal.*Header\(\)'
# Let's check how blocks are typically compared in the codebase
ast-grep --pattern 'require.Equal($t, $block1, $block2'
Length of output: 1719
natebeauregard
force-pushed
the
nkb.verifier-e2e-2
branch
from
c465f05 to
511914b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e/e2e.go (1)
Line range hint
71-111: Improve error context in sequencer command execution.While the command setup is correct, the error message could be more descriptive.
- return fmt.Errorf("run sequencer app: %v", err) + return fmt.Errorf("failed to start sequencer node (appName=%s): %w", appName, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
e2e/e2e.go(4 hunks)e2e/e2e_test.go(4 hunks)e2e/stack_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/e2e_test.go
🔇 Additional comments (5)
e2e/stack_test.go (1)
101-117: Verify test integration in the test suite
Ensure this verifier sync test is properly registered in the test suite.
✅ Verification successful
The search results show that verifierSync is registered in e2e/e2e_test.go as a test case. Let me verify the complete test registration context to ensure it's properly integrated.
Test is properly integrated in the test suite
The verifierSync test is correctly registered in the e2e test suite as part of the e2eTests slice, with a dedicated test case named "Verifier Sync". The test is executed through the main TestE2E function which runs all registered test cases. The test has proper access to all required dependencies through the StackConfig including the verifier client, making it a well-integrated part of the test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the verifierSync test is registered in the test suite
# Search for test registration
rg -A 5 "verifierSync" --type go
# Look for test execution patterns
rg -A 10 'func Test.*\bVerifier\b.*\(t \*testing\.T\)' --type go
Length of output: 634
Script:
#!/bin/bash
# Check the complete test registration context
cat e2e/e2e_test.go
Length of output: 4151
e2e/e2e.go (4)
18-18: LGTM!
The URL package import is correctly added and is used for L1 reachability checks.
32-32: LGTM!
The VerifierClient field is appropriately added to support verifier node testing, maintaining consistency with the existing client field patterns.
58-58: LGTM!
The error assignment syntax is correctly updated while maintaining proper error handling.
Also applies to: 66-66
122-142: Improve error handling and verify port configurations.
-
The error message improvement suggestion from the past review is still valid.
-
The verifier uses specific ports that should be verified for availability:
- RPC: 26667
- Engine: 9010
- Op-node: 9012
#!/bin/bash
# Description: Check if the ports required by the verifier are already in use
echo "Checking port availability..."
for port in 26667 9010 9012; do
if nc -z localhost $port 2>/dev/null; then
echo "Warning: Port $port is already in use"
else
echo "Port $port is available"
fi
done
Adds a verifier node to the e2e tests. A new e2e test was created to assert that the verifier node can correctly sync to the sequencer.
Summary by CodeRabbit
New Features
Bug Fixes
Tests