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 28s —— View job Comprehensive Code ReviewThis PR successfully enables the force inclusion feature by removing the temporary block in configuration validation and re-enabling the complete E2E test suite. I've completed a thorough review covering code quality, security, performance, testing, and documentation. Summary of Changes✅ What Changed:
Code Quality & Architecture ⭐Strengths:
Observations:
Error Handling & Resilience ⭐Strengths:
Observations:
Security 🔒Strengths:
Key Security Property Validated: Potential Concern - Coverage Gap: Looking at the changes: if len(c.DA.GetForcedInclusionNamespace()) > 0 {
if err := validateNamespace(c.DA.GetForcedInclusionNamespace()); err != nil { // ← line 296
return fmt.Errorf("could not validate forced inclusion namespace (%s): %w", c.DA.GetForcedInclusionNamespace(), err) // ← line 297
}
}Recommendation: Add a unit test in
Example test case needed: func TestConfig_ValidateInvalidForcedInclusionNamespace(t *testing.T) {
cfg := DefaultConfig()
cfg.DA.ForcedInclusionNamespace = "invalid-namespace-format"
err := cfg.Validate()
require.Error(t, err)
require.Contains(t, err.Error(), "could not validate forced inclusion namespace")
}Performance & Resource Efficiency 🚀Strengths:
Benchmark Insights:
Minor Observation:
Testing & Reliability ✅Strengths:
E2E Test Architecture Excellence: // Sequencer listens to "wrong-namespace" (won't see forced txs)
"--evnode.da.forced_inclusion_namespace", "wrong-namespace",
// Full node listens to "forced-inc" (will detect violations)
"--evnode.da.forced_inclusion_namespace", "forced-inc",
// Submit directly to DA on "forced-inc" namespace
result := daClient.Submit(ctx, [][]byte{txBytes}, -1, []byte("forced-inc"), nil)This setup cleanly simulates a censoring sequencer without requiring actual malicious code. Testing Observations:
Documentation 📚Strengths:
Minor Enhancement Opportunity: // GetForcedInclusionNamespace returns the namespace for forced inclusion transactions.
// Returns empty string if force inclusion is not configured (feature is optional).
func (d *DAConfig) GetForcedInclusionNamespace() string {
return d.ForcedInclusionNamespace
}Flag Naming Consistency (Review Comment Thread)Context: Review discussion between @randygrok and @julienrbrt about flag naming:
Analysis: Looking at test/e2e/evm_force_inclusion_e2e_test.go, all flags use the "--evnode.node.aggregator=true"
"--evnode.da.forced_inclusion_namespace", "forced-inc"
"--evnode.p2p.listen_address"The codebase uses Conclusion: Current flag usage is correct and consistent with the evnode prefix. The comment suggests potential future refactoring to standardize away from rollkit naming, but that's outside the scope of this PR. CI Status
Final RecommendationsHigh Priority
Nice to Have
Verdict: ✅ Approve with Minor Test Coverage FixThis PR successfully enables a critical censorship-resistance feature with excellent test coverage and thoughtful performance benchmarking. The code quality is high, security properties are well-validated, and the architecture is sound. The only blocker is the test coverage gap - please add a unit test for the forced inclusion namespace validation error path before merging. Outstanding work on the comprehensive E2E tests - the malicious sequencer detection test is particularly well-designed and demonstrates deep understanding of the security model. |
![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.91% -0.08%
==========================================
Files 110 110
Lines 10522 10523 +1
==========================================
- Hits 6101 6094 -7
- Misses 3770 3778 +8
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:
|
julienrbrt
force-pushed
the
julien/test-fi
branch
from
d0bab1c to
287ccc4
Compare
ref: #2965 (comment)
ref: #2982 (comment)