test: add e2e tests for force inclusion (part 2)

[julienrbrt]

Forgot to push this other AI created test in this PR: #2964.
A bit verbose qua comments for my liking, but i've left it as it makes debugging easy.

[github-actions]

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Jan 13, 2026, 1:52 PM

[claude]

Claude finished @julienrbrt's task in 2m 17s —— View job

---

Code Review Complete

I've reviewed the force inclusion e2e tests added in this PR. Overall, the tests are well-structured and comprehensive, covering important security scenarios. Below is my detailed feedback organized by category.

---

Summary

Strengths:

• Comprehensive test coverage for force inclusion scenarios (sequencer-only, full node sync, malicious sequencer detection)
• Well-documented test logic with clear explanations of what's being tested
• Good use of helper functions to reduce code duplication
• Proper test isolation with separate directories and cleanup

Areas for Improvement:

• Some resource management issues (unclosed clients, potential goroutine leaks)
• Inconsistent error handling patterns
• Code duplication that could be refactored
• Some security concerns with file permissions and hardcoded values

---

1. Code Quality & Architecture

Strengths:

• Good modular design: Helper functions like enableForceInclusionInGenesis, submitForceInclusionTx, and setup functions follow existing patterns
• Clear separation of concerns: Setup logic is separated from test logic
• Consistent with codebase patterns: Follows the same structure as other e2e tests in the repo

Issues:

File: test/e2e/evm_force_inclusion_e2e_test.go

Issue 1: File permissions (line 45)

err = os.WriteFile(genesisPath, newData, 0644)

The file is written with world-readable permissions (0644). For consistency with security best practices and the codebase (see base_test.go:54 which uses 0600 for sensitive files), consider using more restrictive permissions.

Recommendation: Use 0600 unless there's a specific reason the genesis file needs to be world-readable.

Issue 2: Code duplication in setup functions

The setup functions setupSequencerWithForceInclusion (lines 76-124) and the inline sequencer setup in TestEvmFullNodeForceInclusionE2E (lines 192-233) share significant amounts of duplicated code. This violates DRY principles and makes maintenance harder.

Recommendation: Extract common sequencer setup logic into a reusable helper function that accepts configuration parameters (like force inclusion namespace, port, etc.).

Issue 3: Magic numbers throughout the tests

Multiple hardcoded values appear without explanation:

• Line 98, 209, 301: enableForceInclusionInGenesis(t, nodeHome, 2) - epoch value 2
• Lines 157, 174, 277, 443: Various timeout durations
• Line 483: Loop count 15 for advancing chain
• Line 536: Consecutive stop count 10

Recommendation: Define these as named constants at the package level with explanatory comments:

const (
    ForceInclusionTestEpoch = 2  // DA blocks before force inclusion must occur
    ForceInclusionTimeout = 30 * time.Second
    ChainAdvanceTransactions = 15  // Number of txs to advance past epoch boundary
    StopDetectionThreshold = 10  // Consecutive checks before confirming node stopped
)

Issue 4: Verbose comments (as author noted)

Lines 371-406 contain an extremely detailed comment block for TestEvmSyncerMaliciousSequencerForceInclusionE2E. While thorough documentation is good, this could be condensed. The test name and key steps are descriptive enough. Consider moving architectural details to package-level documentation or a separate doc file.

---

2. Error Handling & Resilience

Issues:

Issue 5: Missing error check (line 146)

client, err := ethclient.Dial(seqEthURL)
require.NoError(t, err)
defer client.Close()

The client.Close() doesn't check for errors. While this is a test, ignoring errors in cleanup can hide issues.

Recommendation:

defer func() {
    if err := client.Close(); err != nil {
        t.Logf("Failed to close client: %v", err)
    }
}()

This pattern appears multiple times (lines 146, 244, 248, 426, 430).

Issue 6: Inconsistent error messages

Line 205: require.NoError(t, err, "failed to init sequencer", output)
Line 338: require.NoError(t, err, "failed to init full node", output)

The error messages don't follow a consistent capitalization pattern. Some start with lowercase, some with uppercase. Go convention is lowercase for error messages.

Issue 7: Overly broad require checks

Line 71: require.Nil(t, res["error"], "RPC returned error: %v", res["error"])

Using require.Nil for JSON fields is less idiomatic than checking the actual type. If res["error"] doesn't exist in the map, this will pass, which might not be the intent.

Recommendation:

if errField, ok := res["error"]; ok && errField != nil {
    require.Fail(t, "RPC returned error: %v", errField)
}

---

3. Security

Issues:

Issue 8: HTTP client without timeout (line 62)

resp, err := http.Post(fiUrl, "application/json", bytes.NewBuffer(jsonData))

The HTTP client uses the default http.Post which has no timeout, potentially causing tests to hang indefinitely if the server doesn't respond.

Recommendation:

ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
req, err := http.NewRequestWithContext(ctx, http.MethodPost, fiUrl, bytes.NewBuffer(jsonData))
require.NoError(t, err)
req.Header.Set("Content-Type", "application/json")
resp, err := http.DefaultClient.Do(req)

Issue 9: Unvalidated JSON response (lines 68-72)

var res map[string]interface{}
err = json.NewDecoder(resp.Body).Decode(&res)
require.NoError(t, err)
require.Nil(t, res["error"], "RPC returned error: %v", res["error"])
require.NotNil(t, res["result"], "RPC result is nil")

The code doesn't validate the structure of the JSON response before accessing fields. This could panic if the response structure is unexpected.

Issue 10: Potential sensitive data in logs

Throughout the tests, transaction hashes and other potentially sensitive data are logged. While this is a test environment, consider whether this establishes a pattern that might leak into production code.

---

4. Performance & Resource Efficiency

Issues:

Issue 11: Resource leak - unclosed DA client (line 457)

defer blobClient.Close()

The blobClient is properly closed, but the daClient created on line 459 doesn't have an explicit close/cleanup mechanism. Check if DAClient implements io.Closer or has cleanup methods.

Issue 12: Inefficient polling loop (lines 502-555)

for i := 0; i < 120; i++ {
    time.Sleep(500 * time.Millisecond)
    // ... checks ...
}

This polling loop runs for up to 60 seconds with fixed 500ms intervals. It performs multiple RPC calls per iteration which could be optimized.

Recommendation: Use require.Eventually with appropriate polling intervals, or increase the sleep duration to reduce RPC call frequency.

Issue 13: Sequential transaction sending in loop (lines 483-488)

for i := 0; i < 15; i++ {
    txExtra := evm.GetRandomTransaction(t, TestPrivateKey, TestToAddress, DefaultChainID, DefaultGasLimit, &nonce)
    err = seqClient.SendTransaction(context.Background(), txExtra)
    require.NoError(t, err)
    time.Sleep(400 * time.Millisecond)
}

This loop could benefit from batch sending or at least removing the sleep if the goal is to quickly advance the chain. The 400ms sleep seems arbitrary.

Issue 14: Context usage inconsistency

Line 152, 255, 437, 485: context.Background() is created multiple times instead of reusing a test-scoped context. While not a major issue, it's less clean.

Recommendation: Create a single test context at the beginning and reuse it:

ctx := context.Background()
// Use ctx throughout

---

5. Testing & Reliability

Issues:

Issue 15: All tests are skipped (lines 127, 180, 407)

t.Skip() // To re-enable after https://github.com/evstack/ev-node/issues/2965

All three tests are skipped. While the issue reference explains why, this means the PR adds untested code. Ensure the tests are re-enabled once #2965 is resolved.

Issue 16: Non-deterministic test behavior

The malicious sequencer test (lines 406-563) relies on timing assumptions and may be flaky:

• Lines 536: Requires 10 consecutive unchanged height checks
• Lines 483-488: Advances chain with fixed number of transactions
• The grace period behavior is timing-dependent

Recommendation: Consider adding configurable timeouts and making the test more deterministic by directly querying DA state rather than relying on timing.

Issue 17: Missing negative test cases

The tests cover happy paths and one malicious scenario, but missing:

• What happens if force inclusion server is unavailable?
• What happens if DA is unavailable?
• What happens with malformed transactions?
• What happens with concurrent force inclusion submissions?

Issue 18: Test dependencies

The tests depend on external components (DA layer, sequencer, full node) which could make them brittle. Consider adding retries or more robust waiting mechanisms.

---

6. Documentation & Developer Experience

Strengths:

• Excellent inline comments explaining complex logic
• Clear test names that describe what's being tested
• Good use of logging to aid debugging

Issues:

Issue 19: Missing package-level documentation

The file lacks a package comment explaining the force inclusion feature being tested. Developers unfamiliar with the feature would benefit from a high-level overview.

Recommendation: Add a package comment at the top explaining:

• What force inclusion is
• Why it's important for security
• How these tests validate the feature

Issue 20: Unclear helper function naming

setupMaliciousSequencer (line 283) - the name doesn't clearly indicate that "malicious" means "listening to wrong namespace". Consider setupSequencerWithWrongNamespace or adding a doc comment explaining what makes it malicious.

Issue 21: Inconsistent logging style

Some logs use emoji (line 537: ✅), some don't. Some use title case, some lowercase. Standardize logging style for consistency.

---

7. Dependencies (go.mod/go.sum changes)

The PR adds github.com/rs/zerolog v1.34.0 as a direct dependency in test/e2e/go.mod. This is only used for:

Line 468:

zerolog.Nop(),

Issue 22: Unnecessary direct dependency

Adding a logging library just to pass zerolog.Nop() to one function seems heavyweight. Check if there's a way to pass a nil logger or use a lighter abstraction.

Recommendation: Check if block.NewDAClient accepts nil for the logger parameter, or if there's an existing logger in the test context you can reuse.

---

Recommendations Summary

High Priority:

1. Fix resource leaks (unclosed clients/contexts) - test/e2e/evm_force_inclusion_e2e_test.go:146, 244, 248, 426, 430
2. Add timeout to HTTP client - test/e2e/evm_force_inclusion_e2e_test.go:62
3. Review zerolog dependency necessity - test/e2e/go.mod
4. Re-enable tests once Expose gas in execution interface #2965 is resolved

Medium Priority:
5. Extract common setup code to reduce duplication
6. Define magic numbers as named constants
7. Improve error handling consistency
8. Make tests more deterministic (reduce timing dependencies)

Low Priority:
9. Improve file permissions (0644 → 0600)
10. Standardize logging and error message formatting
11. Add package-level documentation
12. Consider additional edge case coverage

---

Conclusion

The tests provide valuable coverage for force inclusion functionality, especially the malicious sequencer detection scenario. The code is generally well-written and follows existing patterns in the codebase. The main concerns are resource management, some security best practices, and test determinism.

Once the resource leaks are fixed and the tests are re-enabled (after #2965), this will be a solid addition to the test suite.

---

Tasks

• Read the new test file and understand force inclusion e2e test implementation
• Review existing e2e test patterns for comparison
• Analyze code quality, test structure, and Go idioms
• Check error handling, resource management, and test isolation
• Provide comprehensive code review feedback

---

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.83%. Comparing base (ddf8f31) to head (af3ce3e).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2970      +/-   ##
==========================================
+ Coverage   57.75%   57.83%   +0.07%     
==========================================
  Files          97       97              
  Lines        9303     9303              
==========================================
+ Hits         5373     5380       +7     
+ Misses       3326     3319       -7     
  Partials      604      604              

Flag	Coverage Δ
combined	57.83% <ø> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.