Repository navigation
feat: reserve-expanding sync eligibility for redistribution game (SWIP-44) - #5633
Conversation
bb86a9c to
2e57090
Compare
2e57090 to
e64fbf3
Compare
333a253 to
02b222c
Compare
|
An independent AI review of this PR was shared with me directly. Below are its findings and what changed for each, so other reviewers can follow along. Changes are in 02b222c. 1. Sync rate recorded under the peer's bin instead of ours — fixedIssue:
As a result:
The existing test passed only because the mock pullsync doesn't filter by radius. Change:
2. Doubled node could play a sister-neighborhood round with an unsynced radius bin — fixedIssue: with stake height = doubling, the contract selects the node when PO(overlay, anchor) ≥ committedDepth − height. When PO(overlay, anchor) == storage radius, every sampled chunk is in our bin at the storage radius, yet Change: 3.
|
02b222c to
31f10c9
Compare
martinconic
left a comment
There was a problem hiding this comment.
Please update PR description
sbackend123
left a comment
There was a problem hiding this comment.
Looks good!
Couple non-blocking comments.
The implementation diverges from SWIP-44 in ways that might be worth a follow-up amendment to the SWIP:
- the SWIP pseudocode checks store.StorageRadius(), while this PR (correctly, I think) uses the committed depth;
- sister-neighbourhood rounds (depth = min(D, PO(overlay, anchor))) are not covered by the SWIP;
- Definition 8 defines syncing(p) as an active pullsync goroutine, while here it is "newly stored chunks within the rate window".
SWIP test case 4 (sub-depth sync running concurrently with sampling) is not covered, and the agent tests mock isReserveSynced, so there is no puller -> agent path under test. Not a blocker, just noting it.
|
|
||
| window := r.now().UnixMilli() / r.windowSize | ||
| r.windows[window] += count | ||
| id := r.now().UnixMilli() / r.bucketSize |
There was a problem hiding this comment.
nit: UnixMilli() drops the monotonic reading, so a wall-clock step hides counts. A backward step of even ~1m makes a recent burst read as 0, and a forward step of a full window clears everything. Same as master, agreed, but since the package is being rewritten anyway and the result now gates sampling, it is cheap to measure from an epoch captured in New (r.now().Sub(r.epoch)), which keeps SetTimeFunc working in tests.
Co-authored-by: sbackend123 <sofia.sakharova@ethswarm.org>
Checklist
Description
Replaces the single global historical sync rate with per-bin rates and decides "synced" from the bins that matter for a given depth.
The old pkg/rate carried the previous window over into the current one, so after the last chunk arrived it took 16–30 minutes (15m window) to read zero, depending on where in the window syncing stopped. Any syncing below the radius also counted against the node.
Open API Spec Version Changes (if applicable)
SwarmCommon.yaml: added the optional pullsyncRateOutsideRadius field to StatusSnapshotResponse.
Related Issue (Optional)
Follows up on review feedback on #5568 (SWIP-44) about the goroutine-count sync signal.
AI Disclosure