refactor(rpc): replace subscription cache with pre_confirmed tip deduper#3778
refactor(rpc): replace subscription cache with pre_confirmed tip deduper#3778EgeCaner wants to merge 13 commits into
Conversation
|
Claude finished @EgeCaner's task in 4m 30s —— View job Review:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/integ-multi-preconfirmed #3778 +/- ##
================================================================
Coverage ? 76.20%
================================================================
Files ? 438
Lines ? 39320
Branches ? 0
================================================================
Hits ? 29963
Misses ? 7205
Partials ? 2152 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude finished @EgeCaner's task in 4m 25s —— View job Review:
|
EgeCaner
force-pushed
the
feat/integ-multi-preconfirmed
branch
2 times, most recently
from
a4f2629 to
895ca24
Compare
EgeCaner
force-pushed
the
refactor/subscription-preconfirmed-deduper
branch
from
95c2078 to
f7c2d5f
Compare
|
Claude finished @EgeCaner's task in 5m 59s —— View job Review:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| head *core.Block, | ||
| ) error { | ||
| return s.processBlock(ctx, id, head, TxnAcceptedOnL2) | ||
| // Canonical blocks bypass the deduper: they are published once. |
There was a problem hiding this comment.
The boundary-block duplicate this commit removes the note for isn't actually stale — it's still reachable, and the old cache used to suppress it.
In the old code both the historical replay (sendEventWithoutDuplicate) and onNewHead went through the SubscriptionCache, keyed on block number. The boundary block N (= head at subscribe time) was cached during replay with TxnAcceptedOnL2, so a subsequent onNewHead(N) hit ShouldSend → false and was suppressed. In the new design, historical replay sends "directly without the deduper" and canonical blocks "bypass the deduper", so nothing suppresses a re-delivery of N.
That re-delivery is still possible because of the store→publish ordering in the syncer:
sync/sync.go:359 committedBlock.Persisted <- nil // DB head becomes N (time A)
sync/sync.go:381 s.newHeads.Send(block) // newHeads publishes N (time B)
If a subscription sets up in the A < T0 < T1 < B window (resolveBlockRange reads head N at T0, then SubscribeKeepLast(newHeads) registers at T1 before Send(N) fires at B), the KeepLast sub receives N, historical replay also covers up to N, and block N's events get emitted twice as ACCEPTED_ON_L2. Narrow race, but non-zero.
So the behaviour the earlier comment called "acceptable" is a real, if rare, regression vs. the old cache — not stale. I'd either restore a reworded note (the dup is now possible rather than suppressed, and deemed acceptable) or add a cheap guard skipping the boundary block in onNewHead when it was already replayed. Same applies to rpc/v10/subscription_events.go:143.
EgeCaner
force-pushed
the
feat/integ-multi-preconfirmed
branch
from
895ca24 to
fdfff54
Compare
EgeCaner
force-pushed
the
refactor/subscription-preconfirmed-deduper
branch
from
f7c2d5f to
9f94e4e
Compare
|
Claude finished @EgeCaner's task in 2m 45s —— View job Review:
|
| head *core.Block, | ||
| ) error { | ||
| return s.processBlock(ctx, id, head, TxnAcceptedOnL2) | ||
| // Canonical blocks bypass the deduper: they are published once. |
There was a problem hiding this comment.
The boundary-block duplicate this comment (commit 9f94e4ee6) reframes as gone isn't actually stale — it's still reachable, and the old cache used to suppress it.
In the old code both historical replay and onNewHead routed through the block-keyed SubscriptionCache. The boundary block N (= head read at subscribe time) was Put during replay with TxnAcceptedOnL2, so a later onNewHead(N) hit ShouldSend → existingValue == value → false and was suppressed (subscription_cache.go ShouldSend, old sendEventWithoutDuplicate). In the new design replay sends "directly without the deduper" and onNewHead "bypasses the deduper", so nothing suppresses a re-delivery of N.
The re-delivery is reachable via the syncer's store→publish gap:
sync/sync.go:359 committedBlock.Persisted <- nil // DB head becomes N
sync/sync.go:381 s.newHeads.Send(block) // newHeads publishes N
SubscribeKeepLast only delivers events published after subscribe (feed/feed.go:68), so if a subscription's resolveBlockRange→HeadsHeader reads N (after store) and its SubscribeKeepLast(newHeads) registers before Send(N) fires, replay covers up to N and the KeepLast sub later delivers N → block N's events emitted twice as ACCEPTED_ON_L2. Narrow race, but non-zero — and only the events subscription is affected (receipts/transactions have no onStart replay).
So the earlier "acceptable duplicate" note described real behaviour; the follow-up commit calling it stale is inaccurate. I'd restore a reworded note rather than delete it:
| // Canonical blocks bypass the deduper: they are published once. | |
| // Canonical blocks bypass the deduper: they are published exactly once. The single | |
| // boundary block at the historical handoff can still be delivered twice (replay covers | |
| // up to the head read at subscribe time, and the KeepLast newHeads sub may also deliver | |
| // it if it registers before sync publishes that block) — this rare duplicate is accepted. |
Alternatively add a boundary guard in onNewHead, but note it must reset on reorg (a naive head.Number <= replayedHead skip would wrongly drop blocks re-delivered after a reorg below N). Same applies to rpc/v9/subscription_events.go:152.
thiagodeev
left a comment
thiagodeev
left a comment
There was a problem hiding this comment.
LGTM.
Any ideais of how can we improve our code to make this not happen?
#3778 (comment)
rodrodros
force-pushed
the
feat/integ-multi-preconfirmed
branch
from
fdfff54 to
1e94b7d
Compare
2 participants
Replace the subscription cache with a per-subscription PreConfirmedDeduper. Because new heads (canonical blocks) arrive exactly once, only the pre_confirmed tip can produce duplicates (it's re-published in full on every delta), so we only need to dedup within the current tip round. During historical handoff, event subscriptions are bounded to canonical blocks, any pre_confirmed blocks missed in the gap will eventually arrive as canonical, so no cross-round caching is needed.