test: enable force inclusion, re-enable fi e2e and add fi benchmarks #3005
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Summary of ChangesHello @julienrbrt, I'm Gemini Code Assist1! 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
🧠 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 AssistThe 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
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 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
|
|
Claude finished @julienrbrt's task in 2m 16s —— View job Comprehensive Code ReviewI've completed a thorough review of the force inclusion enablement changes across 3 files. Overall, the changes are well-implemented and achieve the stated goal of enabling and validating the force inclusion functionality. Below are my findings organized by category. SummaryThis PR successfully enables the force inclusion feature by:
The changes are straightforward and effective. I've identified several areas for improvement below. Critical Issues1. Hard-coded Private Key in Benchmark (Security)Location: execution/evm/filter_bench_test.go:18 const (
benchPrivateKey = "cece4f25ac74deb1468965160c7185e07dff413f23fcadb611b05ca37ab0a52e"Issue: Hard-coded private key in source code, even for testing purposes. Impact: While this is a test-only key, hard-coding private keys violates security best practices and could be flagged by security scanners. If this key was ever used elsewhere or copied to production code, it would be a critical vulnerability. Recommendation: Generate the key dynamically in tests or use a well-documented test-only constant with clear warnings. High Priority Issues2. Missing Test Coverage for Config Validation (Testing)Location: pkg/config/config.go:295-299 The PR removes the temporary error block that prevented force inclusion, enabling this validation: 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)
}
}Issue: No unit tests cover this validation path. The codecov report shows 0% patch coverage with 2 missing lines - these are likely the validation lines. Recommendation: Add unit tests for config validation:
3. Inconsistent Flag Naming Convention (Code Quality)Location: test/e2e/evm_force_inclusion_e2e_test.go:92, 113, 219, 374 As noted in the review comments, there's inconsistency between "--evnode.node.aggregator=true", // evnode prefix
"--evm.jwt-secret-file", jwtSecretFile, // evm prefixIssue: Mixed flag naming conventions reduce code maintainability and can confuse users. Recommendation: Standardize on one prefix convention. Based on CLAUDE.md guidance, Medium Priority Issues4. Benchmark Helper Function Naming (Code Quality)Location: execution/evm/filter_bench_test.go:109-114 func benchName(n int) string {
if n >= 1000 {
return fmt.Sprintf("%dk", n/1000)
}
return fmt.Sprintf("%d", n)
}Issue: Generic function name Recommendation: Rename to 5. Magic Numbers in Benchmark Setup (Code Quality)Location: execution/evm/filter_bench_test.go:53, 203, 230 data := make([]byte, 16) // Why 16?
maxGas := uint64(21000 * 500) // Why 500?
maxBytes := uint64(110 * 500) // Why 110 and 500?Issue: Magic numbers without explanation make benchmarks harder to maintain. Recommendation: Add constants or comments explaining the values: const (
benchmarkTxDataSize = 16 // Arbitrary data payload for realistic tx size
benchmarkGasPerTx = 21000 // Standard ETH transfer gas
benchmarkHalfBatchSize = 500 // Gas limit allowing ~half of tx batch
benchmarkAvgTxSize = 110 // Average transaction size in bytes
)6. Potential for Invalid Transaction Generation (Reliability)Location: execution/evm/filter_bench_test.go:96-101 if includeInvalid && i%10 == 0 {
// Every 10th transaction is invalid (random garbage)
txs[i] = make([]byte, 100)
if _, err := rand.Read(txs[i]); err != nil {
b.Fatalf("failed to generate random data: %v", err)
}
}Issue: Generating 100 bytes of random data might accidentally create valid RLP or transaction data (extremely unlikely but theoretically possible). Recommendation: Use deliberately malformed data that's guaranteed to fail parsing, such as 7. E2E Test Timeout Values (Reliability)Location: test/e2e/evm_force_inclusion_e2e_test.go:172, 272, 495 }, 30*time.Second, 1*time.Second, "Forced inclusion transaction not included")
}, 40*time.Second, 1*time.Second, "Forced inclusion tx not synced to full node")
for i := 0; i < 120; i++ { // 60 seconds total
time.Sleep(500 * time.Millisecond)Issue: Timeout values vary across tests (30s, 40s, 60s) without clear rationale. These could be too long for fast CI or too short for slow environments. Recommendation: Extract timeout constants based on expected behavior: const (
forceInclusionTimeout = 30 * time.Second // Based on epoch=2 and DA block time
syncTimeout = 40 * time.Second // Extra buffer for P2P sync
maliciousBehaviorDetectionTimeout = 60 * time.Second
)Low Priority / Observations8. Verbose E2E Test Documentation (Positive)Location: test/e2e/evm_force_inclusion_e2e_test.go:366-400 The
Observation: This is exemplary test documentation. Consider this pattern for other complex E2E tests. 9. Benchmark Coverage (Positive)Location: execution/evm/filter_bench_test.go The benchmark suite thoroughly covers:
Observation: Comprehensive benchmark coverage will help identify performance regressions. 10. Error Logging in FilterTxs (Performance)Location: execution/evm/execution.go:861-865 (from grep context) c.logger.Debug().
Int("tx_index", i).
Err(err).
Str("tx_hex", "0x"+hex.EncodeToString(tx)).
Msg("filtering out invalid transaction (gibberish)")Observation: The debug logging includes full transaction hex, which could be large for blob transactions. This is acceptable for debug level but consider adding a max length or using a hash for very large transactions. Performance Considerations11. UnmarshalBinary OverheadLocation: execution/evm/execution.go:858-869 The FilterTxs function now conditionally calls Analysis:
Recommendation: Consider running the new benchmarks in CI to track performance over time and detect regressions. Security Analysis12. Namespace ValidationLocation: pkg/config/config.go:295-299, 310-320 The validation properly checks:
Analysis: Security validation is appropriate. The check prevents invalid namespace configurations that could cause runtime errors or unexpected behavior. 13. Forced Inclusion Security ModelLocation: test/e2e/evm_force_inclusion_e2e_test.go:401-556 The malicious sequencer test validates that:
Analysis: This is a critical security property. The test properly validates censorship resistance. Question: What happens in production when a sync node detects malicious behavior? Does it halt, log and continue, or attempt recovery? This should be documented. Documentation14. Missing Benchmark DocumentationLocation: execution/evm/filter_bench_test.go Issue: The benchmark file lacks a package-level comment explaining:
Recommendation: Add package documentation: // Package evm benchmarks test the performance impact of force inclusion on transaction filtering.
//
// Force inclusion requires unmarshaling and validating all transactions to extract gas limits,
// which adds overhead compared to mempool transactions that are pre-validated.
//
// Run benchmarks:
// go test -bench=. -benchmem ./execution/evm/
//
// Compare before/after:
// go test -bench=BenchmarkFilterTxs_CompareUnmarshalOverhead -benchmemGo Idioms & Best Practices15. Use of b.Helper() (Positive)Location: execution/evm/filter_bench_test.go:26, 36, 81, etc. All helper functions properly call Observation: Good Go testing practice. 16. Benchmark Loop Usage (Positive)Location: execution/evm/filter_bench_test.go:131, 157, etc. Benchmarks correctly use Observation: Modern Go benchmark idiom. Recommendations SummaryMust Fix:
Should Fix: Nice to Have: Final AssessmentCode Quality: ✅ Good
Security: ✅ Adequate
Testing:
Performance: ✅ Good
Overall: This PR successfully enables force inclusion and provides strong E2E validation. The main gap is unit test coverage for the config validation changes. Once that's addressed, this will be production-ready. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3005 +/- ##
==========================================
- Coverage 57.98% 57.97% -0.01%
==========================================
Files 110 110
Lines 10522 10523 +1
==========================================
Hits 6101 6101
- Misses 3770 3771 +1
Partials 651 651
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ref: #2965 (comment)
ref: #2982 (comment)