feat(node/test): connection drop test + restart test

[theochap]

Description

This PR adds back the connection drop test and the restart test inside our e2e testing suite

See #2554 and #2553

[claude]

Claude finished @theochap's task —— View job

---

PR Review: Connection Drop Test

I've completed my review of the connection drop test implementation. Here's my comprehensive feedback:

Todo List:

• Read and analyze the new test file
• Review code quality and Go best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage and effectiveness
• Provide comprehensive feedback

---

📋 Code Quality & Best Practices

✅ Strengths:

• Clear, descriptive function name and comments
• Proper use of goroutines with sync.WaitGroup for concurrent testing
• Good error handling with t.Require() assertions
• Appropriate imports and package structure
• Uses the established testing framework (devtest.SerialT)

⚠️ Areas for Improvement:

1. Comment Mismatch: Line 36 has time.Sleep(2 * time.Minute) but the comment says "Wait for 1 minute" - should be updated for consistency.

2. Variable Naming: The variable gt parameter could be more descriptive (though this may be a pattern in the codebase).

🐛 Potential Bugs & Issues

1. Race Condition Risk: The goroutine captures node by reference in the loop (go func(node *dsl.L2CLNode)). While this is handled correctly with explicit parameter passing, it's good practice.

2. Hard-coded Sleep Duration: The 2-minute sleep is quite long for a test and could make the test suite slow. Consider if this could be reduced or made configurable.

3. Peer Check Logic: Lines 54-63 iterate through peers to find the sequencer connection. This could potentially have a false positive if peer IDs are reused or change during the test.

🚀 Performance Considerations

Concerns:

• Test Duration: 2-minute sleep per node means this test could take significant time with multiple nodes
• Concurrent Execution: Good use of goroutines to test multiple nodes in parallel, which helps mitigate the long duration
• Resource Usage: The test creates and manages multiple nodes concurrently, which is appropriate for an e2e test

Recommendations:

• Consider adding a timeout mechanism to prevent indefinite hanging
• The sleep duration might be configurable via environment variable for CI vs local testing

🔒 Security Concerns

Low Risk Assessment:

• No apparent security vulnerabilities in the test code itself
• Properly scoped within the test package
• Uses established devstack testing patterns
• No hardcoded credentials or sensitive data

🧪 Test Coverage & Effectiveness

✅ Good Coverage:

• Tests the critical path of connection loss and recovery
• Verifies both disconnection and reconnection states
• Validates that sync functionality works after reconnection
• Uses appropriate assertions for state validation

Potential Improvements:

1. Test Isolation: Consider testing edge cases like:

   • Multiple connection drops in succession
   • Partial network failures
   • Connection drops during sync operations

2. Assertions: The test could benefit from more granular assertions about the sync state before/after reconnection.

3. Cleanup: Consider adding explicit cleanup routines to ensure test isolation.

📝 Minor Suggestions

1. Line 36: Update comment to match the actual sleep duration:

   // Wait for 2 minutes
   time.Sleep(2 * time.Minute)

2. Consider Error Context: The error messages are good, but could include more context about expected vs actual state.

3. Magic Numbers: The values 50 in line 47 could be extracted as named constants for clarity.

---

🎯 Overall Assessment

This is a well-implemented e2e test that effectively validates the connection drop and recovery functionality. The code follows Go best practices and integrates well with the existing testing infrastructure. The main concerns are around test performance (long duration) and a minor comment inconsistency.

Recommendation: ✅ Approve with minor suggestions

The test successfully addresses the core requirement of ensuring kona-nodes can reconnect and sync after connection drops, which is crucial for network resilience.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.1%. Comparing base (c42e2bb) to head (3ad0938).
⚠️ 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]

Pull Request Overview

This PR re-adds a connection drop test to the e2e testing suite to verify that kona-nodes can properly reconnect to the sequencer and resync after network disconnections.

• Implements a test that simulates network disconnection and reconnection scenarios
• Tests multiple validator nodes concurrently to ensure consistent behavior across different nodes
• Validates that nodes can recover and resync with the sequencer after connection drops

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

The package name 'node' doesn't match the directory structure 'tests/node/common'. The package should be 'common' to match the directory name.

Suggested change

	package node
	package common

[Copilot]

Passing &node to the goroutine is incorrect because 'node' is already a pointer (*dsl.L2CLNode) from the range loop. This creates a double pointer (**dsl.L2CLNode) which will cause compilation errors. It should be just 'node' without the address operator.

Suggested change

	}(&node)
	}(node)