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 14s —— View job Code Review CompleteI've reviewed PR #3005 which enables force inclusion functionality, re-enables E2E tests, and adds comprehensive benchmarks. Summary of Changes
Detailed Review Findings✅ Strengths
Issues & Recommendations🔴 CriticalNone identified. 🟡 Moderate
🟢 Minor / Style
Test Coverage AssessmentRe-enabled Tests
Benchmark Scenarios
Coverage Grade: Excellent - benchmarks thoroughly test the optimization path introduced for force inclusion. Security Review
Performance ConsiderationsThe benchmarks specifically test the performance-critical concern with force inclusion:
Recommendation: Run these benchmarks on CI to track performance regressions over time. Code Quality
ConclusionThis is a well-executed PR that:
Recommendation: Approve with minor suggestions The identified issues are all minor (documentation, comments) and don't block merging. Consider addressing them in a follow-up if preferred. Action Items (Optional)
|
![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 55.57% 55.56% -0.01%
==========================================
Files 116 116
Lines 11477 11473 -4
==========================================
- Hits 6378 6375 -3
+ Misses 4400 4399 -1
Partials 699 699
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
julienrbrt
force-pushed
the
julien/test-fi
branch
from
6ffefb1 to
309d52e
Compare
| // listen to the wrong namespace, while we submit directly to the correct namespace. | ||
| func TestEvmSyncerMaliciousSequencerForceInclusionE2E(t *testing.T) { | ||
| t.Skip() // To re-enable after https://github.com/evstack/ev-node/issues/2965 | ||
| t.Skip() |
There was a problem hiding this comment.
@chatton, I don't seem to manage to make them connect. Am I missing something obvious?
There was a problem hiding this comment.
the setupMaliciousSequencer is returning the sequencer jwt token, not the full node one, inside it discards the fullnode jwt
jwtSecret, _, genesisHash, endpoints := setupCommonEVMTest(t, sut, true)
There was a problem hiding this comment.
diff --git a/test/e2e/evm_force_inclusion_e2e_test.go b/test/e2e/evm_force_inclusion_e2e_test.go
index 40592848..1c8722be 100644
--- a/test/e2e/evm_force_inclusion_e2e_test.go
+++ b/test/e2e/evm_force_inclusion_e2e_test.go
@@ -284,7 +284,7 @@ func setupMaliciousSequencer(t *testing.T, sut *SystemUnderTest, nodeHome string
t.Helper()
// Use common setup with full node support
- jwtSecret, _, genesisHash, endpoints := setupCommonEVMTest(t, sut, true)
+ jwtSecret, fullNodeJwtSecret, genesisHash, endpoints := setupCommonEVMTest(t, sut, true)
passphraseFile := createPassphraseFile(t, nodeHome)
jwtSecretFile := createJWTSecretFile(t, nodeHome, jwtSecret)
@@ -321,7 +321,7 @@ func setupMaliciousSequencer(t *testing.T, sut *SystemUnderTest, nodeHome string
sut.ExecCmd(evmSingleBinaryPath, seqArgs...)
sut.AwaitNodeUp(t, endpoints.GetRollkitRPCAddress(), NodeStartupTimeout)
- return genesisHash, jwtSecret, endpoints
+ return genesisHash, fullNodeJwtSecret, endpoints
}
|
The malicious test passing is blocked on #2963, as we get blocks from P2P. |
ref: #2965 (comment)
ref: #2982 (comment)