feat(node/test): sequencer restart test

[theochap]

Description

Adds a restart test for the sequencer. Ensure that the unsafe head of the other nodes doesn't progress

Rebased on top of #2770

[claude]

Claude finished @theochap's task —— View job

---

Code Review Complete ✅

Review Checklist:

• Read and analyze the new sequencer restart test file
• Review dependency changes in go.mod and go.sum
• Assess code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage and implementation
• Provide constructive feedback and recommendations

---

Summary

This PR introduces a well-structured integration test for sequencer restart functionality. The test validates that when a sequencer is stopped, validator nodes stop advancing their unsafe head, and when the sequencer is restarted and reconnected, the nodes resume normal operation.

Code Quality & Best Practices ✅

Strengths:

• Clear test structure: The test follows a logical flow: setup → verify initial state → stop sequencer → verify stopped state → restart → verify resumed state
• Good separation of concerns: Uses dedicated utility functions from the test infrastructure
• Descriptive variable names: preCheckFuns, stopCheckFuns, postCheckFuns clearly indicate their purpose
• Proper error handling: Uses t.Require() for critical assertions
• Comprehensive logging: Good use of t.Logf() for debugging and monitoring test progress

Code organization:

• Test is appropriately placed in tests/node/restart/ directory
• Uses established patterns from the existing test infrastructure
• Follows Go testing conventions

Dependency Changes ✅

go.mod changes (lines 293):

replace github.com/ethereum-optimism/optimism => github.com/theochap/optimism v0.0.0-20250821131302-b9d84951d8a2

• This appears to be a temporary fork reference for development purposes
• Recommendation: Ensure this gets updated to point to the main Optimism repository before merging to main

go.sum: Standard dependency hash updates corresponding to go.mod changes.

Test Implementation Analysis ✅

Test Logic Flow:

1. Setup: Creates mixed OP-Kona network with sequencer and validator nodes
2. Pre-check: Verifies all nodes are advancing properly (types.CrossUnsafe, types.LocalSafe)
3. Stop Sequencer: Stops the first sequencer node
4. Validation: Confirms nodes are disconnected and not advancing (types.LocalUnsafe)
5. Restart: Restarts sequencer and reconnects to all nodes
6. Post-check: Verifies nodes resume advancing (types.LocalSafe, types.LocalUnsafe)

Key Test Assertions:

// Ensure disconnection
t.Require().NotEqual(peer.PeerID, seqPeerId, "expected node %s to be disconnected from sequencer %s", ...)

// Verify nodes stop advancing unsafe head
stopCheckFuns = append(stopCheckFuns, node.NotAdvancedFn(types.LocalUnsafe, 50))

// Verify nodes resume advancing after restart
postCheckFuns = append(postCheckFuns, node.AdvancedFn(types.LocalSafe, 10, 100), node.AdvancedFn(types.LocalUnsafe, 10, 100))

Potential Issues & Recommendations 🔍

1. Timing Dependencies (sequencer_restart_test.go:46,66):

   • The test uses hardcoded timeout values (50, 100) for checking node advancement
   • Recommendation: Consider making these configurable or using environment-specific values for different CI environments

2. Single Sequencer Assumption (sequencer_restart_test.go:22):

   sequencer := sequencerNodes[0]  // Always uses first sequencer

   • Recommendation: Add a comment explaining why only the first sequencer is tested, or consider testing multiple sequencers if the setup supports it

3. Manual Reconnection (sequencer_restart_test.go:56-60):

   • The test manually reconnects the sequencer to each node after restart
   • Question: Is this the expected behavior in production, or should nodes automatically reconnect? Consider adding a comment explaining this design choice.

4. Test Isolation (sequencer_restart_test.go:12):

   • Uses devtest.SerialT(gt) which suggests this test must run serially
   • Good practice: This is appropriate for integration tests that manage network state

Security Considerations ✅

• No security concerns identified: The test code only performs read operations and controlled start/stop of test nodes
• Test isolation: Proper use of test infrastructure prevents interference with other tests
• Resource cleanup: Implicit cleanup through the test framework

Performance Considerations ✅

• Test duration: The test includes multiple synchronization points which may make it slower, but this is appropriate for integration testing
• Resource usage: Creates a full mixed OP-Kona network which is resource-intensive but necessary for the test scenario
• Parallel execution: Uses SerialT appropriately to prevent resource conflicts

Test Coverage Assessment 📊

What's tested:

• ✅ Sequencer stop/start lifecycle
• ✅ Node disconnection verification
• ✅ Unsafe head advancement stopping when sequencer is down
• ✅ Normal operation resumption after restart
• ✅ P2P reconnection functionality

Potential additions for future consideration:

• Multiple sequencer restart scenarios
• Network partition/recovery scenarios
• Different restart timing scenarios
• Error handling when restart fails

Minor Suggestions 💡

1. Code documentation (sequencer_restart_test.go:12):

   // TestSequencerRestart verifies that validator nodes stop advancing their unsafe head
   // when the sequencer is stopped and resume normal operation when restarted.
   func TestSequencerRestart(gt *testing.T) {

2. Variable naming (sequencer_restart_test.go:40):
   Consider renaming nodePeers to currentPeers for clarity:

   currentPeers := node.Peers()

Conclusion 🎯

This is a well-implemented integration test that effectively validates the sequencer restart functionality. The code follows established patterns, has good error handling, and tests the critical aspects of the restart process. The main considerations are around timing dependencies and ensuring the temporary dependency fork is addressed before merging.

Overall Assessment: ✅ Approved - Ready for merge after addressing the dependency fork reference.

---

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.9%. Comparing base (bc3155e) to head (dfe5095).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.