fix(syncing): skip forced txs checks for p2p blocks #2922
Conversation
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 introduces a temporary fix to optimize block processing by conditionally skipping forced transaction checks. Specifically, these checks will now only be applied to blocks sourced from the Data Availability layer, while blocks received via the peer-to-peer network will bypass this verification step. This adjustment is a stopgap measure designed to improve syncing behavior until a more robust background verification mechanism for P2P blocks is implemented in a future update. Highlights
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
|
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task in 2m 6s —— View job Comprehensive Code ReviewI've analyzed PR #2922 which introduces a temporary fix to skip forced transaction checks for P2P blocks. Here's my detailed review across multiple dimensions: Summary of ChangesThe PR modifies Key changes:
1. Code Quality & Architecture✅ Strengths:
a) Inconsistent Error Handling Pattern (High Priority) At lines 595-601, there's a problematic error handling pattern flagged by Gemini Code Assist: if err := s.verifyForcedInclusionTxs(currentState, data); err != nil {
s.logger.Error().Err(err).Uint64("height", nextHeight).Msg("forced inclusion verification failed")
if errors.Is(err, errMaliciousProposer) {
s.cache.RemoveHeaderDAIncluded(headerHash)
return err
}
}Issue: Non-
Impact: A node could process a block without properly verifying forced inclusion requirements, potentially accepting blocks that should be rejected. Recommendation: Return all errors from if event.Source == common.SourceDA {
if err := s.verifyForcedInclusionTxs(currentState, data); err != nil {
s.logger.Error().Err(err).Uint64("height", nextHeight).Msg("forced inclusion verification failed")
s.cache.RemoveHeaderDAIncluded(headerHash)
return err
}
}b) Architectural Concern: Dual Code Paths The PR creates divergent verification paths for DA vs P2P blocks:
While this is marked as temporary, it introduces a trust model inconsistency where P2P blocks are assumed valid without verification. This is acceptable for a temporary fix but should be closely tracked. c) State Mutation Removed (Minor) The change from 2. Error Handling & ResilienceAdditional Observations:
3. Security ImplicationsTemporary Security Degradation:
Mitigation Context:
Recommendation:
4. Performance & Resource Efficiency✅ Positive Impact:
5. Testing & Reliability❌ Critical Gap: Zero Test Coverage for New Behavior:
Recommended Test Cases: // Test: P2P blocks skip forced inclusion verification
func TestTrySyncNextBlock_P2PSkipsForcedInclusion(t *testing.T) {
// Setup syncer with forced inclusion configured
// Create event with Source = common.SourceP2P
// Verify verifyForcedInclusionTxs is not called (use mock)
}
// Test: DA blocks still verify forced inclusion
func TestTrySyncNextBlock_DAVerifiesForcedInclusion(t *testing.T) {
// Setup syncer with forced inclusion configured
// Create event with Source = common.SourceDA
// Verify verifyForcedInclusionTxs is called (use mock)
}
// Test: Transient errors in forced inclusion are properly handled
func TestTrySyncNextBlock_ForcedInclusionTransientErrorRetries(t *testing.T) {
// Setup syncer with DA source event
// Mock verifyForcedInclusionTxs to return transient error
// Verify event is rescheduled via cache.SetPendingEvent
}6. Documentation & Developer Experience✅ Good:
Suggested Comment: // Verify forced inclusion transactions if configured.
// TEMPORARY: Skip for P2P blocks until background verification is implemented (see #2891).
// This is safe because:
// 1. DA blocks (source of truth) are still fully verified
// 2. P2P blocks will be re-verified in the background once #2891 merges
// 3. Any malicious P2P blocks will be caught when DA confirmation arrives
// Related: https://github.com/evstack/ev-node/issues/2906
if event.Source == common.SourceDA {
...
}Additional ObservationsMinor Code Quality Issues:
Final VerdictConditional Approval with Required Changes The PR achieves its stated goal of temporarily skipping forced inclusion checks for P2P blocks to improve sync performance. However, it has critical issues that must be addressed: Must Fix Before Merge:
Should Fix (High Priority): Post-Merge Actions: The core logic is sound for a temporary fix, but the error handling bug and lack of test coverage make this risky to merge as-is. |
![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 correctly implements the intended fix of skipping forced transaction checks for P2P blocks as a temporary measure. The changes are straightforward and align with the PR's objective.
However, I've identified a pre-existing issue in the error handling for verifyForcedInclusionTxs. Currently, errors other than errMaliciousProposer are logged but otherwise ignored, allowing block processing to continue even if verification fails due to a transient error. This could lead to incorrect state. I've suggested a change to ensure any verification failure causes the block sync to be retried, which improves the robustness of the syncer.
| if err := s.verifyForcedInclusionTxs(currentState, data); err != nil { | ||
| s.logger.Error().Err(err).Uint64("height", nextHeight).Msg("forced inclusion verification failed") | ||
| if errors.Is(err, errMaliciousProposer) { | ||
| s.cache.RemoveHeaderDAIncluded(headerHash) | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
The current error handling for verifyForcedInclusionTxs only returns an error if it's errMaliciousProposer. Other errors, such as a transient failure to retrieve data from the DA layer, are logged but then ignored. This allows block processing to continue without proper verification, which could be problematic.
Any error from verifyForcedInclusionTxs should cause trySyncNextBlock to fail. This will allow the caller (processHeightEvent) to handle the error appropriately, such as rescheduling the event for a retry, which is the default behavior for non-critical errors. This makes the syncer more robust against transient issues.
I suggest simplifying the error handling to always return the error, which is also more consistent with how errors from validateBlock are handled just above.
if err := s.verifyForcedInclusionTxs(currentState, data); err != nil {
s.logger.Error().Err(err).Uint64("height", nextHeight).Msg("forced inclusion verification failed")
s.cache.RemoveHeaderDAIncluded(headerHash)
return err
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2922 +/- ##
==========================================
- Coverage 63.72% 63.62% -0.11%
==========================================
Files 90 90
Lines 8571 8572 +1
==========================================
- Hits 5462 5454 -8
- Misses 2529 2536 +7
- Partials 580 582 +2
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:
|
|
|
||
| // only save to p2p stores if the event came from DA | ||
| if event.Source == common.SourceDA { | ||
| if event.Source == common.SourceDA { // TODO(@julienrbrt): To be reverted once DA Hints are merged (https://github.com/evstack/ev-node/pull/2891) |
There was a problem hiding this comment.
turns out the todo is on the wrong condition, but we'll get it :D (should have been L594)
Overview
Temporary fix until #2891.
After #2891 the verification for p2p blocks will be done in the background.
ref: #2906