fix(sync): sync distant P2P data heads by range - #3421
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesSynchronization verification
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/sync/sync_service_test.go`:
- Around line 105-112: Move the getter.getByHeightCalls.Load() assertion below
the require.Eventually catch-up completion check in the sync test, while
preserving its existing limit and message. Keep the final height and range-call
assertions unchanged.
In `@types/p2p_envelope.go`:
- Around line 114-115: Update the height adjacency check in the surrounding
envelope validation logic to avoid incrementing p.Height(), which can wrap at
math.MaxUint64. Compare the trusted and untrusted heights using subtraction or
an equivalent overflow-safe condition, preserving adjacent-height hash
validation while treating overflow and non-adjacent candidates as the documented
non-adjacent path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1442f16e-c5ec-45bd-bb95-05ae5213589b
📒 Files selected for processing (3)
pkg/sync/sync_service_test.gotypes/p2p_envelope.gotypes/p2p_envelope_test.go
| if p.Height()+1 != untrusted.Height() { | ||
| return nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prevent height wraparound in the adjacency check.
p.Height()+1 wraps from math.MaxUint64 to zero. A candidate at height zero then takes the adjacent hash-validation path. This contradicts the documented non-adjacent behavior and can reject that provisional candidate based on LastDataHash.
Compare heights without incrementing the trusted height.
Proposed fix
- if p.Height()+1 != untrusted.Height() {
+ if untrusted.Height() <= p.Height() || untrusted.Height()-p.Height() != 1 {
return nil
}As per coding guidelines, “Prevent integer overflows … during validation.”
📝 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.
| if p.Height()+1 != untrusted.Height() { | |
| return nil | |
| if untrusted.Height() <= p.Height() || untrusted.Height()-p.Height() != 1 { | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@types/p2p_envelope.go` around lines 114 - 115, Update the height adjacency
check in the surrounding envelope validation logic to avoid incrementing
p.Height(), which can wrap at math.MaxUint64. Compare the trusted and untrusted
heights using subtraction or an equivalent overflow-safe condition, preserving
adjacent-height hash validation while treating overflow and non-adjacent
candidates as the documented non-adjacent path.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3421 +/- ##
==========================================
+ Coverage 62.37% 62.47% +0.10%
==========================================
Files 121 121
Lines 13467 13469 +2
==========================================
+ Hits 8400 8415 +15
+ Misses 4127 4113 -14
- Partials 940 941 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
interesting fix, its odd this was hit in production.
2 participants
Overview
Fix P2P data head verification so non-adjacent heads are accepted as provisional sync targets instead of triggering repeated bifurcation requests.
P2PData.Verifypreviously compared the trusted data hash withLastDataHashfor every candidate. That linkage is only meaningful for adjacent heights: a distant head points to its immediate predecessor, not the node's current local head. As a result, distant heads soft-failed verification and go-header fetched intermediate heights repeatedly before starting normal range sync (258GetByHeightcalls when catching up from height 1 to 64 in the regression).The verification now checks data-hash linkage only for adjacent heights. Non-adjacent heads can become provisional sync targets, after which the downloaded range is verified link by link. Adjacent candidates with an invalid
LastDataHashare still rejected.Validation
go test ./... -count=1go test ./pkg/sync -run TestDataSyncerDistantHeadUsesRangeSync -count=50go test -race ./pkg/sync -run TestDataSyncerDistantHeadUsesRangeSync -count=1go test ./types -run TestP2PDataVerifyAdjacentHeads -count=50go vet ./types ./pkg/syncgit diff --checkSummary by CodeRabbit