feat: ensure p2p DAHint within limits#3128
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
📝 WalkthroughWalkthroughThis change adds conditional validation of P2P DA height hints: if any hint exceeds the current DA height by more than 200, the syncer fetches the latest DA height (with a 500ms timeout) to filter out unreasonable hints, logging and falling back if the fetch fails. Changes
Sequence DiagramsequenceDiagram
participant P2P as P2P Network
participant Syncer
participant DAClient as DA Client
participant DARetriever
P2P->>Syncer: Emit event with DA height hints
Syncer->>Syncer: compute currentDAHeight (atomic load)
Note over Syncer: If any hint > currentDAHeight + 200 -> validation needed
alt Validation needed
Syncer->>DAClient: GetLatestDAHeight() (500ms timeout)
alt Fetch successful
DAClient-->>Syncer: latestDAHeight
Syncer->>Syncer: filter out hints > latestDAHeight
else Fetch failed
Syncer-->>Syncer: log warning, disable strict validation
end
end
loop For each remaining hint
Syncer->>Syncer: skip if hint <= daRetrieverHeight
Syncer->>DARetriever: queue hint for priority DA retrieval
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @alpe's task in 2m 17s —— View job Code Review
Good targeted fix. The approach — lazy fetch of latest DA height only when a hint is suspiciously far ahead — is the right trade-off between security and RPC load. A few items to address: Bug: Missing test for the validation bypass on DA client error
There's no test covering the error path. Consider adding one that asserts the fallback behavior explicitly, so the contract is documented and regressions are caught. Fix this → Missing test: hint exceeds drift but is actually valid
A complete test matrix needs a case where hint exceeds the threshold but Warn log noise during initial sync
Consider Integer overflow edge case (minor)
if h > currentDAHeight+daHintMaxDrift {If if currentDAHeight+daHintMaxDrift < currentDAHeight || h > currentDAHeight+daHintMaxDrift {Or use a subtraction-based check: if h > daHintMaxDrift && h-daHintMaxDrift > currentDAHeight {Timeout already applied (no action needed)The coderabbitai suggestion about bounding the ChangelogThe entry at Summary: The core logic is correct and the staged validation approach avoids excessive RPC traffic. The two missing test paths (error fallback + hint-exceeds-drift-but-valid) are the main asks before merging. The warn log noise is a usability issue worth addressing. |
| currentDAHeight := s.daRetrieverHeight.Load() | ||
|
|
||
| // Only fetch the latest DA height if any hint is suspiciously far ahead. | ||
| const daHintMaxDrift = uint64(200) |
There was a problem hiding this comment.
this is an arbitrary value to ensure that we do not hit DA height request on every message
| } | ||
|
|
||
| if needsValidation && daHeightHint > latestDAHeight { | ||
| s.logger.Warn().Uint64("da_height_hint", daHeightHint). |
There was a problem hiding this comment.
This may be really noisy during syncing, when we are receiving p2p blocks very far ahead.
I think the 200 is too reasonable, or the log line should be a debug log instead of warning.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3128 +/- ##
=======================================
Coverage 60.76% 60.76%
=======================================
Files 113 113
Lines 11672 11693 +21
=======================================
+ Hits 7092 7105 +13
- Misses 3777 3785 +8
Partials 803 803
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
block/internal/syncing/syncer_test.go (1)
717-772: Add one explicit test for latest-height fetch failure fallback.Please add a case where
GetLatestDAHeightreturns an error and assert the intended fallback behavior (current code disables strict validation in that branch). This prevents regressions in the failure-mode contract.Also applies to: 787-829
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/syncer_test.go` around lines 717 - 772, Add a new test variant of TestProcessHeightEvent_RejectsUnreasonableDAHint where the mock DA client’s GetLatestDAHeight returns an error (e.g., mockDAClient.EXPECT().GetLatestDAHeight(...).Return(uint64(0), errors.New("fail")).Maybe()), then call s.processHeightEvent(...) and assert the fallback behavior: because strict validation is disabled on DA fetch error, the hint should be accepted (verify s.daRetriever.PopPriorityHeight() returns the hinted height rather than 0). Use the same setup flow (NewSyncer, initializeState, set store height) and reference the existing test function name, mockDAClient.GetLatestDAHeight, s.processHeightEvent, and s.daRetriever.PopPriorityHeight to locate where to add the case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@block/internal/syncing/syncer.go`:
- Around line 641-647: The call to s.daClient.GetLatestDAHeight(ctx) in the hot
path (inside processHeightEvent handling where needsValidation is true) must be
bounded by a short timeout to avoid stalling; replace the long-lived ctx with a
short-lived context (use context.WithTimeout, e.g., a few hundred milliseconds
or a configured short duration), defer the cancel, and pass the derived ctx to
s.daClient.GetLatestDAHeight; keep the existing error handling (s.logger.Warn...
and needsValidation = false) unchanged so behavior remains the same on timeout
or error.
---
Nitpick comments:
In `@block/internal/syncing/syncer_test.go`:
- Around line 717-772: Add a new test variant of
TestProcessHeightEvent_RejectsUnreasonableDAHint where the mock DA client’s
GetLatestDAHeight returns an error (e.g.,
mockDAClient.EXPECT().GetLatestDAHeight(...).Return(uint64(0),
errors.New("fail")).Maybe()), then call s.processHeightEvent(...) and assert the
fallback behavior: because strict validation is disabled on DA fetch error, the
hint should be accepted (verify s.daRetriever.PopPriorityHeight() returns the
hinted height rather than 0). Use the same setup flow (NewSyncer,
initializeState, set store height) and reference the existing test function
name, mockDAClient.GetLatestDAHeight, s.processHeightEvent, and
s.daRetriever.PopPriorityHeight to locate where to add the case.
|
Can we add a changelog as well please :) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 29: The CHANGELOG entry contains an extra space before the period in the
sentence "Validate P2P DA height hints against the latest known DA height to
prevent malicious peers from triggering runaway catchup ."; remove the stray
space so the sentence ends "runaway catchup." (Update the exact line text in
CHANGELOG.md to eliminate the space before the period.)
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdblock/internal/syncing/syncer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- block/internal/syncing/syncer.go
| - Store pending blocks separately from executed blocks key. [#3073](https://github.com/evstack/ev-node/pull/3073) | ||
| - Fixes issues with force inclusion verification on sync nodes. [#3057](https://github.com/evstack/ev-node/pull/3057) | ||
| - Add flag to `local-da` to produce empty DA blocks (closer to the real system). [#3057](https://github.com/evstack/ev-node/pull/3057) | ||
| - Validate P2P DA height hints against the latest known DA height to prevent malicious peers from triggering runaway catchup . [#3128](https://github.com/evstack/ev-node/pull/3128) |
There was a problem hiding this comment.
Fix spacing before period.
There's an extra space before the period in "catchup ." — it should be "catchup."
✏️ Proposed fix
-- Validate P2P DA height hints against the latest known DA height to prevent malicious peers from triggering runaway catchup . [`#3128`](https://github.com/evstack/ev-node/pull/3128)
+- Validate P2P DA height hints against the latest known DA height to prevent malicious peers from triggering runaway catchup. [`#3128`](https://github.com/evstack/ev-node/pull/3128)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Validate P2P DA height hints against the latest known DA height to prevent malicious peers from triggering runaway catchup . [#3128](https://github.com/evstack/ev-node/pull/3128) | |
| - Validate P2P DA height hints against the latest known DA height to prevent malicious peers from triggering runaway catchup. [`#3128`](https://github.com/evstack/ev-node/pull/3128) |
🧰 Tools
🪛 LanguageTool
[grammar] ~29-~29: Ensure spelling is correct
Context: ...malicious peers from triggering runaway catchup . [#3128](https://github.com/evstack/ev...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 29, The CHANGELOG entry contains an extra space before
the period in the sentence "Validate P2P DA height hints against the latest
known DA height to prevent malicious peers from triggering runaway catchup .";
remove the stray space so the sentence ends "runaway catchup." (Update the exact
line text in CHANGELOG.md to eliminate the space before the period.)
2 participants
Overview
Prevent malicious da hints
Summary by CodeRabbit