Skip to content

Send no backlog when the requested sequence number is past its end - #4714

Open
smypmsa wants to merge 2 commits into
OffchainLabs:masterfrom
smypmsa:guard-requested-seqnum-past-backlog-end
Open

Send no backlog when the requested sequence number is past its end#4714
smypmsa wants to merge 2 commits into
OffchainLabs:masterfrom
smypmsa:guard-requested-seqnum-past-backlog-end

Conversation

@smypmsa

@smypmsa smypmsa commented Jul 27, 2026

Copy link
Copy Markdown

Anything that reads a sequencer feed — a nitro node, or a relay passing the feed on — tells the broadcaster which sequence number it wants to start from, using the Arbitrum-Requested-Sequence-Number header. If it asks for a number past the last message the broadcaster still holds, it is already up to date, so the right answer is to send it nothing. Instead it is sent the entire backlog, and the further ahead it claims to be, the more it gets. The header is unauthenticated, so any client can trigger this.

The cause is that backlog.Lookup is a plain index lookup, and it fails the same way whether the number is too high or too low. clientconnection.go treats any failure as "send everything from the head", which is right for a number that is too low and backwards for one that is too high.

This used to work. #883 specified that the relay sends cached messages "equal to or greater than requested sequence number", and the old sequencenumbercatchupbuffer returned nothing when the request was past the end, with a test covering the 2**64 - 1 case. The WebSocket refactor in #1930 replaced that buffer and dropped both the check and the test.

What changed:

  • Before looking the number up, compare it against the end of the backlog. If it is past the end, send nothing. backlog gains a Tail() accessor for this, alongside the existing Head().
  • Remember the end of the backlog as the last thing sent. Otherwise the catch-up that runs when the first live message arrives sees "nothing sent yet", decides the whole backlog is a gap, and sends it anyway. Storing the requested number instead would be worse — it can be arbitrarily high, and the client would then be dropped from every message that follows.

Nothing changes for a client that sends no header, asks for a number below the start of the backlog, or asks for one inside it.

TestBroadcasterRequestedSequenceNumber connects a real client and checks exactly which sequence numbers come back, for five cases: no header, before the start, inside, at the end, and past the end. The last two fail on master and pass here.

I also ran a relay built from this branch alongside an unpatched one, both on a public Orbit chain feed. Asking for 2**64 - 1 returned 1,103 stale messages (~8 MB) before and none now, and the unpatched relay logged two sending the entire backlog instead warnings where this one logged none.

🤖 Generated with Claude Code

A client that connects with an Arbitrum-Requested-Sequence-Number after the
end of the backlog is already ahead of everything the broadcaster has, so it
should be sent nothing. Instead it was sent the entire backlog: backlog.Lookup
is an index probe that fails for a number above the tail exactly as it fails
for one below the head, and the lookup failure fell back to the backlog head.

Bound the requested sequence number against the end of the backlog before the
lookup, and record that end as the last sequence number sent so that the catch
up after registration does not treat the whole backlog as a gap and resend it
through backlog.Get. The end of the backlog is recorded rather than the
requested number, which can be arbitrarily far ahead and would then drop every
message sent to the client.

This restores the behaviour specified in OffchainLabs#883, which the old
sequencenumbercatchupbuffer implemented explicitly and tested. The guard and
its coverage were dropped in the WebSocket library refactor in OffchainLabs#1930.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@CLAassistant

CLAassistant commented Jul 27, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment thread wsbroadcastserver/clientconnection.go Outdated
@ganeshvanahalli

Copy link
Copy Markdown
Contributor

Nice fix — the steady-state boundary behavior and the test design both check out. One race window worth guarding before merge:

Tail().End() can transiently read 0 while the backlog is non-empty, which misroutes requests into the new past-the-end branch.

backlog.Append publishes a still-empty tail segment before filling it, in two places:

  • initial creation: the fresh newBacklogSegment() is stored into b.head/b.tail before segment.append(...) runs;
  • segment rollover (every SegmentLimit messages): b.tail.Store(segment) on the empty nextSegment happens before segment.append(...).

backlogSegment.End() returns 0 for an empty segment, and IsBacklogSegmentNil only checks pointer nil-ness, so a client connecting in that window computes backlogEnd = 0. The enclosing guard already requires segment.Start() < requestedSeqNum, so every such request satisfies requestedSeqNum > 0 and takes the past-the-end branch: no backlog is written and LastSentSeqNum is stored as 0. When the next live message N arrives, the catch-up calls Get(1, N-1), which clamps start to head.Start() and sends the entire backlog. Two consequences:

  • a request inside the backlog regresses vs. master in this window (master's Lookup goes through lookupByIndex, which already contains the requested entry and is unaffected by the empty tail): the client receives nothing until the next broadcast, then the whole backlog including messages below its request;
  • a 2**64 - 1 request that hits the window is sent the whole backlog via the catch-up — the exact amplification this PR closes.

It also makes the new comment slightly inaccurate: "a concurrent Append can only move it forward" — the observable Tail().End() goes e.g. 100 → 0 → 101 across a rollover.

The window is tiny (between two adjacent statements in Append) and self-heals on the next broadcast, so this is minor — but the guard is cheap: treat backlogEnd == 0 as "end unknown" and fall through to the existing Lookup path, which restores exact pre-PR behavior inside the window while keeping the fix everywhere else:

if backlogEnd != 0 && uint64(cc.requestedSeqNum) > backlogEnd {

(backlogEnd == 0 with a non-empty backlog can otherwise only mean a backlog whose last message is sequence number 0, where falling through to Lookup is also fine.)

🤖 Posted with Claude Code

@smypmsa

smypmsa commented Aug 3, 2026

Copy link
Copy Markdown
Author

@ganeshvanahalli thanks, fixing.

backlog.Append publishes a tail segment before appending to it, so a
segment read in that window is empty and End reports zero for it. A
client connecting then computed a backlog end of zero, took the past the
end branch, was sent none of the backlog, and had zero recorded as the
last sequence number sent. The catch up on the next broadcast then
treated the whole backlog as a gap and sent it, which is the
amplification the past the end check exists to prevent. Fall through to
Lookup when the end reads as zero instead, which is what a client
connecting in that window was sent before the check existed.

Also log the past the end case at warn rather than debug, matching the
warning the condition this check replaces used to emit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@smypmsa
smypmsa requested a review from ganeshvanahalli August 3, 2026 20:52

@ganeshvanahalli ganeshvanahalli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
@smypmsa After receiving another review and design-approved This PR will be pulled into our internal version, and will be closed once that repo is merged back to the public repo.

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.

4 participants