From 9812f990104c9b12fd2becbe49b576200ed554a4 Mon Sep 17 00:00:00 2001 From: darshankabariya Date: Sat, 18 Jul 2026 03:14:57 +0530 Subject: [PATCH] fix: clamp by range width, not absolute floor, to avoid clock-skew false clamp Adversarial review found the previous clamp compared the incoming range's lower bound to a fresh now()-derived selfLowerBound. Because the responder samples now() later than the initiator, that floor is always slightly higher, so the clamp fired even between EQUAL-window peers (store<->store), skipping a sub-second sliver at the bottom of the window and risking permanent store-fleet inconsistency for a message landing in it. Compare range WIDTH to our syncRange instead: width is independent of each peer's now() (both bounds subtract the same now+jitter), so equal windows never clamp and only a genuinely wider-window peer is clamped, erring toward reconciling slightly extra rather than dropping. Added an equal-window near-floor guard test (suite 38/38). Co-Authored-By: Claude Fable 5 --- .../waku/waku_store_sync/reconciliation.nim | 18 ++++++++----- tests/waku_store_sync/test_protocol.nim | 26 +++++++++++++++++++ 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/logos_delivery/waku/waku_store_sync/reconciliation.nim b/logos_delivery/waku/waku_store_sync/reconciliation.nim index 87f5fe3e6..d6731f355 100644 --- a/logos_delivery/waku/waku_store_sync/reconciliation.nim +++ b/logos_delivery/waku/waku_store_sync/reconciliation.nim @@ -155,17 +155,21 @@ proc preProcessPayload(self: SyncReconciliation, payload: RangesData): Opt[Range elif rangeType == RangeType.ItemSet: payload.itemSets.delete(0) else: - # This range straddles our window's lower bound: mark the portion below - # our window as skip so a peer with a wider window does not reconcile - # (and offer us) messages older than we retain. Only triggers on a - # window mismatch; equal windows never straddle. + # If the remote is reconciling a wider window than ours, clamp the + # bottom of this range to our window's width so a wider-window peer + # (e.g. a store node) does not offer us history older than we retain. + # We compare range WIDTH to our syncRange rather than absolute floors: + # width is independent of each peer's now(), so equal-window syncs + # (e.g. store<->store) never clamp, avoiding clock-skew false positives. let lowerBound = payload.ranges[i][0].a.time - if lowerBound < selfLowerBound: - let clampId = SyncID(time: selfLowerBound, hash: EmptyFingerprint) + let myRange = self.syncRange.nanos + if upperBound - lowerBound > myRange: + let clampTime = upperBound - myRange + let clampId = SyncID(time: clampTime, hash: EmptyFingerprint) let belowSkip = (payload.ranges[i][0].a .. clampId, RangeType.Skip) if rangeType == RangeType.ItemSet: - payload.itemSets[0].elements.keepItIf(it.time >= selfLowerBound) + payload.itemSets[0].elements.keepItIf(it.time >= clampTime) payload.ranges[i][0].a = clampId payload.ranges.insert(belowSkip, i) diff --git a/tests/waku_store_sync/test_protocol.nim b/tests/waku_store_sync/test_protocol.nim index aaf6da1c3..50d8b4bf5 100644 --- a/tests/waku_store_sync/test_protocol.nim +++ b/tests/waku_store_sync/test_protocol.nim @@ -198,6 +198,32 @@ suite "Waku Sync: reconciliation": # the old, below-window message is clamped out, never offered remoteNeeds.contains((serverPeerInfo.peerId, oldHash)) == false + asyncTest "equal-window peers reconcile a message near the window floor": + ## Guard for the clamp: two peers with the SAME sync range must not clamp + ## each other (the clamp compares range width to our window, not absolute + ## floors, so peer clock skew never triggers it), so a message near the + ## bottom of the shared window is still reconciled. + const equalRange = 1.hours + + server = await newTestWakuRecon( + serverSwitch, @[], @[], equalRange, idsChannel, localWants, remoteNeeds + ) + client = await newTestWakuRecon( + clientSwitch, @[], @[], equalRange, idsChannel, localWants, remoteNeeds + ) + + let + nearFloorTs = now() - 3_300_000_000_000 # 55 min ago: in-window, near floor + msg = fakeWakuMessage(ts = nearFloorTs, contentTopic = DefaultContentTopic) + hash = computeMessageHash(DefaultPubsubTopic, msg) + + client.messageIngress(hash, DefaultPubsubTopic, msg) + + let res = await client.storeSynchronization(Opt.some(serverPeerInfo)) + assert res.isOk(), res.error + + check remoteNeeds.contains((serverPeerInfo.peerId, hash)) == true + asyncTest "sync 2 nodes different hashes": server = await newTestWakuRecon( serverSwitch, @[], @[], DefaultSyncRange, idsChannel, localWants, remoteNeeds