Skip to content

feat: reserve-expanding sync eligibility for redistribution game (SWIP-44) - #5633

Open
aloknerurkar wants to merge 8 commits into
masterfrom
feat/per-bin-sync-rate
Open

aloknerurkar wants to merge 8 commits into
masterfrom
feat/per-bin-sync-rate

Conversation

@aloknerurkar

@aloknerurkar aloknerurkar commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

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.

  • pkg/rate: rewritten as a sliding window of fine-grained buckets. The rate decays linearly and reaches zero exactly one window after the last Add.
  • pkg/puller:
    • DefaultHistRateWindow 15m → 5m.
    • One rate per bin; the global rate and SyncRate() are removed.
    • New IsReserveSynced(depth): true when no bin at or above depth is historically syncing.
    • New SyncRateWithinRadius() / SyncRateOutsideRadius().
  • pkg/storer: the reserve worker decreases the radius only when IsReserveSynced(radius) is true, so syncing outside the radius no longer blocks a decrease.
  • pkg/storageincentives: the agent checks IsReserveSynced(committedDepth) right before sampling at that depth (SWIP-44). IsFullySynced on /redistributionstate reports the physical storage radius.
  • Status protocol: no wire change. PullsyncRate now carries the within-radius rate.
  • /status API: adds pullsyncRateOutsideRadius for the local node, for diagnostics.
  • The pullsync_rate metric keeps its name and now reports the within-radius rate.

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

  • This PR contains code that has been generated by an LLM.
  • I have reviewed the AI generated code thoroughly.
  • I possess the technical expertise to responsibly review the code generated in this PR.

Comment thread pkg/rate/rate.go
Comment thread pkg/rate/rate.go Outdated
Comment thread pkg/rate/rate.go Outdated
@aloknerurkar
aloknerurkar force-pushed the feat/per-bin-sync-rate branch from 333a253 to 02b222c Compare October 26, 2026 11:06
@aloknerurkar

Copy link
Copy Markdown
Contributor Author

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 — fixed

Issue: syncPeerBin did p.binRate[bin].Add(count), where bin is the peer's bin, PO(peer, chunk). IsReserveSynced(depth) and the within/outside radius rates compare that index against our own radius and committed depth, i.e. they assume PO(base, chunk). With peer proximity p:

  • peer bin b < p lands in our bin b
  • b > p lands in our bin p
  • b == p lands somewhere deeper than p

As a result:

  • Peers below our radius: pullsync stores only chunks within our radius (IsWithinStorageRadius), but chunks synced from peers at R-1/R-2 were recorded below R. IsReserveSynced(R) ignored them, so the radius could be decreased while the reserve was still filling. Master counted them, because it kept a single total rate.
  • Sister peer (po == R) with doubling: its bins were inverted with respect to the committed depth.

The existing test passed only because the mock pullsync doesn't filter by radius.

Change:

  • The puller now records each count under our bin (localBin(peer.po, bin)). When the peer bin equals the peer's proximity, our bin is only known to be deeper, so the count goes to the top bin and every depth check sees it.
  • Because pullsync never stores below the radius, the "outside radius" rate was always ~0. Removed SyncRateOutsideRadius, the /status field pullsyncRateOutsideRadius and its OpenAPI entry; pkg/api and openapi/ are back to identical with master.
  • TestSyncRates is table-driven: peer below the radius, sister peer (same and deeper bin), neighbor (shallower and deeper bin).

2. Doubled node could play a sister-neighborhood round with an unsynced radius bin — fixed

Issue: 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 handleSample only checked bins ≥ committedDepth. A wrong sample hash leads to a (probabilistic) stake freeze. Master was safe because it required the total rate to be zero.

Change: handleSample fetches the round anchor (ReserveSalt, the same currentRoundAnchor used for selection) before the sync check, and checks sync at min(committedDepth, PO(overlay, anchor)). The anchor is passed into makeSample, so it is still fetched once. Without doubling, behavior is unchanged. TestAgent covers a sister-neighborhood anchor both while the radius bin is still syncing and once it is synced.

3. isFullySynced on /redistributionstate differs from the play check — not changed

Intentional. The play check now depends on the round anchor (see 2), which isn't known at phase start. isFullySynced reports whether bins at and above the storage radius are synced. The per-round decision and depth are logged ("skipping round because reserve is not synced").

4A. Rate window equal to the peer recalc interval — fixed

Issue: a failed GetCursors is retried on the next topology change or 5m tick. With a 5m window, the rate could expire before the retry and read as synced.

Change: DefaultHistRateWindow is back to 10m (master's value). That outlives a single missed retry and stays below the 15m reserve wakeup. Syncing over chunks we already hold (count == 0) reading as synced is intended: the signal is newly stored chunks, because cursor progress can be inflated by peers.

4B. False "synced" right after a radius decrease — not changed

SetRadius notifies the puller through kademlia (SetStorageRadius → notifyPeerSig), and syncing of the new bin starts straight away rather than on the next 5m tick. The gap is the first Sync round trip (seconds), and it exists on master too.

5A. Rate sums the current partial bucket on top of the window — not changed

Intentional, so a count stays in the rate for at least one full window. The over-estimate is 0–3.3% depending on position within the bucket, and only affects the reported value; all decisions test Rate() > 0.

5B. Wall-clock jumps — not changed

Same as master, which also used UnixMilli. A backward step only drops buckets newer than the new time; the rate reads 0 only if the step is at least the full window, and the next Add during syncing restores it.

6. OpenAPI version / nullable — resolved by 1

The field was removed, so there is no API change.

7. pullsync_rate metric narrowed — resolved by 1

With rates recorded under our bin, the within-radius rate is the total historical rate, the same as the metric reported on master.

Comment thread pkg/storer/reserve.go Outdated
Comment thread pkg/storageincentives/agent.go Outdated
@aloknerurkar
aloknerurkar force-pushed the feat/per-bin-sync-rate branch from 02b222c to 31f10c9 Compare October 27, 2026 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants