test: enable force inclusion, re-enable fi e2e and add fi benchmarks

[julienrbrt]

ref: #2965 (comment)
ref: #2982 (comment)

[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 22, 2026, 11:59 AM

[gemini-code-assist]

Summary of Changes

Hello @julienrbrt, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on enabling and validating the 'force inclusion' functionality within the system. It removes a previous block in the configuration validation that prevented the feature from being active and subsequently re-enables the comprehensive suite of end-to-end tests designed to verify its correct operation across various EVM components.

Highlights

• Force Inclusion Feature Activation: The 'forced inclusion' feature has been activated by removing a temporary error message that previously prevented its use in pkg/config/config.go. The configuration now correctly validates the ForcedInclusionNamespace.
• E2E Tests Re-enabled: All end-to-end tests related to EVM force inclusion, including sequencer, full node, and malicious sequencer scenarios, have been re-enabled in test/e2e/evm_force_inclusion_e2e_test.go by removing t.Skip() directives.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[claude]

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

---

Code Review Complete ✅

I've completed a comprehensive review of PR #3005. The changes correctly enable the force inclusion feature and add thorough benchmarking coverage. Overall, this is well-structured code that achieves its goals effectively.

---

Summary

This PR makes three key changes:

1. Enables force inclusion by removing the validation block in pkg/config/config.go
2. Re-enables E2E tests for force inclusion functionality
3. Adds comprehensive benchmarks for FilterTxs performance with/without force inclusion

---

Detailed Findings

1. Configuration Changes (pkg/config/config.go)

✅ Positive:

• Clean removal of the temporary error block that was preventing force inclusion
• Properly validates the forced inclusion namespace using the existing validateNamespace function
• Maintains consistent error handling with other namespace validations

⚠️ Issues:

Security/Correctness Issue - Missing Test Coverage

Severity: Medium

The codecov report shows 0% patch coverage for the re-enabled validation code at lines 295-298. This validation logic should be covered by tests.

Location: pkg/config/config.go:295-298

if len(c.DA.GetForcedInclusionNamespace()) > 0 {
    if err := validateNamespace(c.DA.GetForcedInclusionNamespace()); err != nil {
        return fmt.Errorf("could not validate forced inclusion namespace (%s): %w", c.DA.GetForcedInclusionNamespace(), err)
    }
}

Recommendation: Add unit tests for config validation covering:

• Valid forced inclusion namespace
• Invalid forced inclusion namespace (should error)
• Empty forced inclusion namespace (should skip validation)

Fix this →

---

2. Benchmark Implementation (execution/evm/filter_bench_test.go)

✅ Positive:

• Comprehensive benchmark coverage testing multiple scenarios
• Well-organized with clear naming conventions
• Uses b.Helper() appropriately for helper functions
• Good use of b.ResetTimer() and b.ReportAllocs()
• Tests both fast path (no forced txs) and slow path (with forced txs)
• Includes edge cases like invalid transactions and limit enforcement

⚠️ Issues:

Security Issue - Hardcoded Private Key

Severity: High

Location: execution/evm/filter_bench_test.go:18

const (
    benchPrivateKey = "cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e"
    benchToAddress  = "0x944fDcD1c868E3cC566C78023CcB38A32cDA836E"
    benchChainID    = "1234"
)

Issue: While this is for benchmarking only, hardcoded private keys in source code are a security anti-pattern. If this key is reused elsewhere or if someone copies this pattern for production code, it could lead to security vulnerabilities.

Recommendations:

1. Add a clear comment warning that this key is ONLY for benchmarking and must NEVER be used in production
2. Consider generating a random key at benchmark runtime instead
3. Add a comment with the address derived from this key to make it obvious it's a test key

Suggested fix:

const (
    // SECURITY WARNING: These keys are ONLY for benchmarking purposes.
    // NEVER use these keys in production code or with real funds.
    // Address derived from benchPrivateKey: 0x[computed address]
    benchPrivateKey = "cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e"
    benchToAddress  = "0x944fDcD1c868E3cC566C78023CcB38A32cDA836E"
    benchChainID    = "1234"
)

Fix this →

Code Quality Issue - Incomplete Client Initialization

Severity: Low

Location: execution/evm/filter_bench_test.go:25-32

The createBenchClient function creates an EngineClient with only the store field populated. The comment says "It only needs the logger to be set" but the logger is not actually set.

func createBenchClient(b *testing.B) *EngineClient {
    b.Helper()
    baseStore := dssync.MutexWrap(ds.NewMapDatastore())
    store := NewEVMStore(baseStore)
    client := &EngineClient{
        store: store,
    }
    return client
}

Recommendations:

1. Update the comment to accurately reflect what fields are populated
2. Verify that FilterTxs doesn't panic with missing fields (logger, etc.)
3. Consider initializing a noop logger if needed

Suggested fix:

// createBenchClient creates a minimal EngineClient for benchmarking FilterTxs.
// Only the store field is populated; FilterTxs doesn't require other fields.
func createBenchClient(b *testing.B) *EngineClient {
    b.Helper()
    baseStore := dssync.MutexWrap(ds.NewMapDatastore())
    store := NewEVMStore(baseStore)
    client := &EngineClient{
        store: store,
    }
    return client
}

Performance Issue - Repeated Transaction Generation

Severity: Low

Location: execution/evm/filter_bench_test.go - Multiple benchmark functions

Each benchmark regenerates transactions on every iteration inside b.Loop(). For accurate performance measurement, transaction generation should happen during setup (before b.ResetTimer()), not during the measured loop.

Current pattern:

for _, count := range txCounts {
    b.Run(benchName(count), func(b *testing.B) {
        txs := generateTransactionBatch(b, count, 21000) // ✅ Good - outside loop
        b.ResetTimer()
        b.ReportAllocs()
        
        for b.Loop() {
            _, err := client.FilterTxs(ctx, txs, 0, 0, false) // ✅ Good - reuses txs
            if err != nil {
                b.Fatalf("FilterTxs failed: %v", err)
            }
        }
    })
}

Actually, looking more carefully, this is already correct - transactions are generated once before b.ResetTimer(). Good job! 👍

---

3. E2E Test Changes (test/e2e/evm_force_inclusion_e2e_test.go)

✅ Positive:

• Re-enables two critical E2E tests: TestEvmSequencerForceInclusionE2E and TestEvmFullNodeForceInclusionE2E
• Removes t.Skip() calls appropriately
• Third test (TestEvmSyncerMaliciousSequencerForceInclusionE2E) kept skipped with "TODO fix before merge" - good practice
• Bug fixes in setupFullNodeWithForceInclusionCheck: corrects flag prefix from evnode to rollkit and changes p2p.seeds to p2p.peers

⚠️ Issues:

Documentation Issue - Skipped Test

Severity: Medium

Location: test/e2e/evm_force_inclusion_e2e_test.go:402

func TestEvmSyncerMaliciousSequencerForceInclusionE2E(t *testing.T) {
    t.Skip() // TODO fix before merge

The test has excellent documentation (lines 366-401) explaining what it tests, but the skip message doesn't explain why it's skipped or what needs to be fixed.

Recommendation: Add a more descriptive skip message or a GitHub issue reference:

t.Skip("TODO: Fix malicious sequencer detection logic - see issue #XXXX")

Or if there's no issue yet, create one to track this work.

Minor Issue - Flag Consistency

Severity: Low

Location: test/e2e/evm_force_inclusion_e2e_test.go:352-362

The bug fix changes flags from --evnode.* to --rollkit.* prefix. While this appears correct based on the change from seeds to peers, it's worth verifying that:

1. All other tests use consistent flag prefixes
2. The flag normalization function in pkg/config/config.go:344-350 handles both prefixes correctly

This is likely fine, but worth a quick verification.

---

Architecture & Design Review

Force Inclusion Feature Architecture

The implementation follows a clean separation of concerns:

• Config layer: Validates namespace configuration
• Execution layer: Handles transaction filtering and validation
• E2E layer: Validates end-to-end behavior including malicious scenarios

Positive observations:

• The FilterTxs method has a boolean flag hasForceIncludedTransaction that enables a slower validation path - this is a good performance optimization
• Benchmarks show the overhead of this validation path, which will help with capacity planning
• E2E tests cover normal sequencer, full node sync, and malicious sequencer scenarios

Performance Considerations

The benchmarks test transaction filtering at scales of 100, 1,000, and 10,000 transactions. Key insights:

• With forced inclusion disabled, UnmarshalBinary is skipped (fast path)
• With forced inclusion enabled, every transaction must be unmarshaled (slow path)
• The benchmarks quantify this overhead, which is valuable for understanding system capacity

Recommendation: Run these benchmarks on CI and track regression over time.

---

Testing & Reliability

Test Coverage Analysis

Strong coverage:

• ✅ Sequencer force inclusion (E2E)
• ✅ Full node force inclusion sync (E2E)
• ✅ Performance benchmarks for all FilterTxs code paths
• ✅ Invalid transaction handling in benchmarks

Missing coverage:

• ⚠️ Config validation for forced inclusion namespace (0% patch coverage)
• ⚠️ Malicious sequencer detection (test skipped)

Recommendations for Test Completion

1. Before merge:

   • Add unit tests for config validation (addresses codecov issue)
   • Either fix or document why TestEvmSyncerMaliciousSequencerForceInclusionE2E is skipped

2. Follow-up work:

   • Consider adding integration tests that sit between unit and E2E tests
   • Add tests for edge cases: empty forced inclusion namespace, very large transactions, concurrent submissions

---

Go-Specific Code Quality

Excellent Practices Observed:

• ✅ Proper use of b.Helper() in benchmark helpers
• ✅ Consistent error handling with wrapped errors
• ✅ Good use of table-driven benchmarks
• ✅ Context propagation in async operations
• ✅ No goroutine leaks observed

Minor Style Notes:

• Comment on line 24 says "logger" but logger isn't set (addressed above)
• Consider extracting magic numbers (21000 gas, 500ms timeouts) to named constants

---

Security Review

Security Considerations:

1. Namespace Validation: ✅ Properly validates forced inclusion namespace format
2. Private Key Handling: ⚠️ Hardcoded key in benchmarks needs warning comment
3. Transaction Validation: ✅ Benchmarks test invalid transaction handling
4. Malicious Sequencer Detection: ⚠️ Test exists but is skipped (needs attention)

Critical Security Property:

The force inclusion feature implements a key censorship resistance mechanism. The skipped test (TestEvmSyncerMaliciousSequencerForceInclusionE2E) validates that honest full nodes can detect when sequencers maliciously omit forced transactions. This test should be enabled before considering the feature production-ready.

---

Recommendations Summary

Must Fix Before Merge:

1. ⚠️ Add unit tests for config validation (addresses 0% patch coverage issue)
2. ⚠️ Add security warning to hardcoded benchmark private key
3. ⚠️ Document why malicious sequencer test is skipped (issue reference or explanation)

Should Fix Before Merge:

4. Update comment in createBenchClient to accurately reflect fields populated
5. Consider running benchmarks in CI to track performance regressions

Follow-Up Work:

6. Enable and fix TestEvmSyncerMaliciousSequencerForceInclusionE2E (critical security property)
7. Extract magic numbers to named constants for better maintainability

---

Final Assessment

Overall Quality: Good ✅

This PR successfully enables the force inclusion feature with comprehensive testing and benchmarking. The code quality is high, with good Go idioms and well-structured tests. The main concerns are:

1. Missing test coverage for config validation (easily fixed)
2. One critical security test is skipped (needs attention before production)
3. Minor documentation improvements needed

The force inclusion feature appears architecturally sound with proper separation of concerns. Once the test coverage gaps are addressed, this will be production-ready.

---

[gemini-code-assist]

Code Review

This pull request enables the force inclusion feature. The changes in pkg/config/config.go correctly re-enable the validation logic for the forced inclusion namespace, and the changes in test/e2e/evm_force_inclusion_e2e_test.go re-enable the end-to-end tests for this feature by removing t.Skip() calls. The changes are straightforward and effectively achieve the goal of activating the force inclusion functionality and its associated tests. The pull request title also mentions the addition of force inclusion benchmarks, but no new benchmark files were found in the changes; this might be planned for a subsequent pull request.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.88%. Comparing base (5660535) to head (cab4a67).

Files with missing lines	Patch %	Lines
pkg/config/config.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3005      +/-   ##
==========================================
- Coverage   57.91%   57.88%   -0.03%     
==========================================
  Files         110      110              
  Lines       10526    10527       +1     
==========================================
- Hits         6096     6094       -2     
- Misses       3780     3782       +2     
- Partials      650      651       +1     

Flag	Coverage Δ
combined	57.88% <0.00%> (-0.03%)	⬇️

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.