feat(evm): add force inclusion command

[julienrbrt]

Add force inclusion command for evm app.
Similar to evstack/ev-abci#295

Blocked on #2797

[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	Dec 4, 2025, 1:06 PM

[claude]

Claude finished @julienrbrt's task —— View job

---

Comprehensive Code Review - Force Inclusion Command Feature

I've completed a thorough review of this PR implementing the force inclusion command for EVM applications. Overall, this is a well-structured implementation with good test coverage. Below are my findings organized by category.

---

1. Code Quality & Architecture ⭐

Strengths

• Clean separation of concerns: The force inclusion server (apps/evm/server/force_inclusion.go) is well-isolated and properly implements the JSON-RPC 2.0 specification
• Modular design: New core/execution/context.go provides a clean abstraction for passing force-included transaction metadata through the execution pipeline
• Proper interface usage: The implementation correctly uses existing DA and execution interfaces without creating unnecessary abstractions

Issues

Critical: Typo in Error Message

Location: apps/evm/cmd/post_tx_cmd.go:107

return fmt.Errorf("forced inclusionnamespace cannot be empty")

Issue: Missing space between "inclusion" and "namespace"
Fix: Should be "forced inclusion namespace cannot be empty"

Moderate: Magic Number in StartUp Timing

Location: apps/evm/server/force_inclusion.go:103

case <-time.After(100 * time.Millisecond):
    return nil

Issue: The 100ms wait is a magic number without explanation. This appears to be a startup check, but there's no guarantee the server actually started successfully.
Recommendation:

1. Add a comment explaining why this timeout exists
2. Consider using a more robust startup verification (e.g., attempting a connection or checking server state)

Minor: Inconsistent Context Usage

Location: apps/evm/server/force_inclusion.go:255

ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)

Issue: Creates a new context from Background() instead of using the request context. This means client cancellations won't propagate.
Recommendation: Use r.Context() as the parent context to respect client cancellations

---

2. Error Handling & Resilience ⚠️

Good Practices

• Proper error wrapping with context using fmt.Errorf with %w
• Retry logic in execution/evm/execution.go with exponential backoff for payload status
• Graceful handling of invalid transactions (filtering rather than failing)

Issues

Moderate: No Maximum Request Body Size

Location: apps/evm/server/force_inclusion.go:152

body, err := io.ReadAll(r.Body)

Issue: No limit on request body size - potential DoS vector
Recommendation: Use http.MaxBytesReader to limit request body size:

r.Body = http.MaxBytesReader(w, r.Body, 1024*1024) // 1MB limit
body, err := io.ReadAll(r.Body)

Minor: Incomplete Error Context

Location: apps/evm/cmd/post_tx_cmd.go:86-92

txData, err = decodeTxFromFile(txInput)
if err != nil {
    return fmt.Errorf("failed to decode transaction from file: %w", err)
}

Issue: When file reading succeeds but decoding fails, the error doesn't include the file path, making debugging harder
Recommendation: Include the file path in the error message

---

3. Security 🔒

Strengths

• Transaction validation for force-included transactions (execution/evm/execution.go:254-283)
• JWT authentication for Engine API calls
• No sensitive data in logs (transaction hashes only, not content)

Issues

Critical: Pseudo Transaction Hash Collision Risk

Location: apps/evm/server/force_inclusion.go:299

txHash := fmt.Sprintf("0x%064x", daHeight)

Issue: Using DA height as the transaction hash creates potential collisions when multiple transactions are submitted at the same height. This isn't a security vulnerability per se, but could cause confusion in tracking.
Recommendation: Include additional entropy (e.g., transaction index or hash of transaction data):

txHash := fmt.Sprintf("0x%032x%032x", daHeight, hash(txData)[:16])

Moderate: No Rate Limiting

Location: apps/evm/server/force_inclusion.go (general)
Issue: No rate limiting on the force inclusion API. Since the node operator pays DA gas costs, this could be exploited to drain funds.
Recommendation: Implement rate limiting per IP or add authentication

Minor: Namespace Validation Could Be Stricter

Location: apps/evm/server/force_inclusion.go:56-58

if len(cfg.DA.GetForcedInclusionNamespace()) == 0 {
    return nil, errors.New("forced inclusion namespace is empty")
}

Issue: Only checks if namespace is empty, but doesn't validate format
Recommendation: Add validation for namespace format (hex string, correct length, etc.)

---

4. Performance & Resource Efficiency 🚀

Good Practices

• Proper use of buffered HTTP client with timeout
• Efficient transaction filtering in execution/evm/execution.go
• Context-based optimization to skip validation for mempool transactions

Issues

Minor: Inefficient String Concatenation in Hot Path

Location: execution/evm/execution.go:266,282

validTxs = append(validTxs, "0x"+hex.EncodeToString(tx))

Issue: String concatenation in a loop; could use strings.Builder for better performance
Impact: Low - only affects blocks with many transactions

Minor: Redundant JSON Unmarshal Attempt

Location: apps/evm/cmd/post_tx_cmd.go:202

if err := json.Unmarshal([]byte(input), &txJSON); err == nil {

Issue: Always attempts JSON unmarshal even when input is clearly a hex string (starts with "0x")
Recommendation: Check for "0x" prefix first to avoid unnecessary unmarshal attempt

---

5. Testing & Reliability ✅

Strengths

• Comprehensive unit tests in force_inclusion_test.go covering success, error cases, and edge cases
• Test for force-included transaction validation in force_included_test.go
• Context tests in context_test.go

Issues

Moderate: Missing Integration Tests

Issue: No end-to-end integration test that:

1. Starts the force inclusion server
2. Submits a transaction via JSON-RPC
3. Verifies it appears in a block at the correct epoch
   Recommendation: Add an integration test in test/integration/

Minor: Test Coverage Gaps (per Codecov)

Locations:

• core/execution/context.go: 0% coverage (6 lines missing)
• pkg/config/config.go: 0% coverage (2 lines missing)
  Note: The context.go functions are tested indirectly through execution tests, but explicit unit tests would be better

Minor: Mock DA Client Could Be More Realistic

Location: apps/evm/server/force_inclusion_test.go:20-34
Issue: Mock DA client returns fixed height - doesn't simulate realistic DA behavior
Recommendation: Make mock behavior configurable per test case

---

6. Documentation & Developer Experience 📚

Strengths

• Excellent README documentation in apps/evm/README.md (lines 107-172)
• Clear CHANGELOG entry
• Good code comments explaining the force inclusion mechanism
• Usage examples with curl and spamoor

Issues

Minor: Incomplete Documentation of RPC Proxying

Location: apps/evm/README.md
Issue: Documentation doesn't mention that other RPC methods are proxied to the execution RPC
Recommendation: Add a section explaining the proxy behavior and the --evm.eth-url flag requirement

Minor: No Documentation on Economic Implications

Location: apps/evm/README.md:111
Issue: While it mentions the node operator pays DA costs, it doesn't provide guidance on cost estimation
Recommendation: Add example cost calculations or link to DA layer pricing

---

7. Concurrency & Thread Safety 🔄

Good Practices

• Proper mutex usage in execution/evm/execution.go:415-430 for block hash updates
• Atomic operations for DA height in sequencers
• Safe context propagation

Issues

None Found

The concurrency patterns look solid. Good use of mutexes and atomic operations where needed.

---

8. Configuration & Deployment 🔧

Strengths

• Clear configuration through command-line flags
• Graceful server start/stop with context cancellation
• Proper timeout configuration

Issues

Moderate: Config Validation Timing

Location: Review comment mentions pkg/config/config.go changes must be reverted before merge
Issue: The feature is disabled by default (correct approach), but the review comment indicates config activation needs to wait for issue #2906
Status: Addressed in commit caa7fba according to review comments
Recommendation: Verify the config is properly disabled and document the dependency on #2906

---

Summary & Recommendations

Must Fix Before Merge (Critical)

1. Typo: Fix "inclusionnamespace" → "inclusion namespace" in post_tx_cmd.go:107

Should Fix Before Merge (High Priority)

1. Rate limiting: Add basic rate limiting to prevent DA cost exploitation
2. Request body size limit: Protect against DoS with http.MaxBytesReader
3. Context propagation: Use request context instead of context.Background() in RPC handler

Nice to Have (Medium Priority)

1. Integration tests: Add end-to-end test for force inclusion flow
2. Transaction hash: Improve pseudo-hash to avoid collisions
3. Startup verification: More robust server startup check than 100ms sleep
4. Documentation: Add economic implications and proxy behavior docs

Low Priority (Future Improvements)

1. Test coverage improvements for context.go
2. Minor performance optimizations (string concatenation, JSON detection)
3. More realistic mock DA client behavior

---

Overall Assessment

This is a well-implemented feature that adds important force inclusion capabilities to the EVM application. The code is generally clean, well-tested, and follows good Go practices. The main concerns are:

1. The typo (easy fix)
2. Missing rate limiting (security concern)
3. Request size limiting (DoS prevention)

Once these are addressed, this PR will be ready for production use.

Recommendation: Approve with requested changes (fix critical issues, then merge)

---

[codecov]

Codecov Report

❌ Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.65%. Comparing base (38e5579) to head (3a9144b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
block/internal/executing/executor.go	75.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2888      +/-   ##
==========================================
+ Coverage   65.59%   65.65%   +0.06%     
==========================================
  Files          86       87       +1     
  Lines        7906     7926      +20     
==========================================
+ Hits         5186     5204      +18     
- Misses       2155     2156       +1     
- Partials      565      566       +1     

Flag	Coverage Δ
combined	65.65% <92.00%> (+0.06%)	⬆️

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.

[julienrbrt]

this must be reverted before merge (need all of #2906 before activation)

[julienrbrt]

Re-disabled in caa7fba

[tac0turtle]

i dont fully follow the need for the server, can we send it without the server? I could be misunderstanding the flow

[tac0turtle]

oh i missed the docs, now i got it.

[julienrbrt]

That server is the RPC endpoint you can use to submit txs.
It basically wraps ethereum rpc but overrides eth_sendRawTransaction to post to DA instead of exec client mempool

[tac0turtle]

i think this should a standalone binary not embedded in a operating node.

[julienrbrt]

This is more annoying as you'd need to configure the da client twice (for submitting to da).
I do agree that I see no sane public node operator enabling this and subsidizing the DA submission cost.
However, for people running themselves their private full node, this has the advantage to be all in one.

Happy to split it in another standalone server if you want, but we'll need to add config and such there too.

[tac0turtle]

we can keep as is, if a user needs something else we can modify

[tac0turtle]

utACK