fix(relayer): correct Solana ALT chunk count; bound EventManager retention - #3726
fix(relayer): correct Solana ALT chunk count; bound EventManager retention#3726droplet-rl wants to merge 3 commits into
Conversation
…ntion - getAddressLookupTableInstructions used Math.floor(len/32)+1, always appending one extra chunk; when the account count was an exact multiple of 32 (including 0) the final chunk was an empty slice, producing an ExtendLookupTable instruction with zero addresses that the on-chain ALT program rejects, failing relayer-refund-leaf execution. Use Math.ceil. - EventManager.events/blockHashes grew for the process lifetime with no eviction (remove() only fired on reorg 'removed' notifications and even then leaked the blockHashes bucket key). Fix remove() to drop the stale blockHash bucket, and prune event/blockHash records older than a re-org-safe retention window as the head advances so memory stays bounded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc6ed82a11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bucketBlockNumber = eventKeys.map((eventKey) => this.events[eventKey]?.blockNumber).find(isDefined); | ||
| if (!isDefined(bucketBlockNumber) || bucketBlockNumber < cutoff) { |
There was a problem hiding this comment.
When one provider continues reporting events more than 5,000 blocks ahead while another provider's stream is stalled or buffered, this eviction deletes the first provider's vote for every older event. When the delayed stream resumes, each old event is recorded as only its first vote and can never reach quorum because the leading provider will not report it again; both EventListener.onEvents and the SVM listener therefore permanently omit those deposits/fills until a restart and backfill. Retain incomplete-quorum records independently or otherwise reconcile late events rather than evicting them solely by the highest observed block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in 694a20e.
Anchoring eviction on the highest observed block was wrong: a provider lagging more than MAX_EVENT_RETENTION_BLOCKS would find the leading provider's vote already gone, record a fresh single-vote entry, and never reach quorum.
Eviction is now anchored on the slowest provider: EventManager tracks each provider's highest observed block and only evicts a record once every provider has advanced a full retention window past it, so a late vote can always land on an existing record.
I did not take the "retain incomplete-quorum records independently" option — those are exactly the unbounded set (a provider reporting events the others never will has no terminating condition), so exempting them reinstates the leak this PR set out to fix.
To keep the memory bound real, the anchor is clamped to MAX_PROVIDER_LAG_BLOCKS (5k) behind the head, capping retention at 10k blocks. A provider stalled beyond that stops holding retention open — unavoidable for any bounded-memory scheme — but it is now logger.warn'd (rate-limited to one warning per 5k blocks of head progression) instead of silently dropping votes.
Three tests added in test/EventManager.ts; the late-vote one fails against the previous head-anchored cutoff (verified by reverting the anchor locally) and passes now.
Pruning was anchored on the highest observed block, so a provider lagging more than MAX_EVENT_RETENTION_BLOCKS behind the head would have its late quorum votes dropped: the leading provider's vote was already evicted, the late vote recorded a fresh single-vote record, and the event could never reach quorum — silently omitting deposits/fills from both EventListener.onEvents and the SVM listener until a restart and backfill. Track each provider's highest observed block and anchor eviction on the slowest of them, so an event is only evicted once every provider has advanced a full retention window past it. The anchor is clamped to MAX_PROVIDER_LAG_BLOCKS behind the head so a stalled or dead provider can't hold retention open indefinitely; that case is now logged (rate-limited) rather than silently dropping votes. Tests cover eviction past the retention window, a lagging provider's late vote still reaching quorum, and the memory bound holding when a provider stalls for good. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex Addressed the P2 in 694a20e — please re-review. Preserve late quorum votes across the retention cutoff — valid, fixed. Anchoring eviction on the highest observed block meant a provider lagging more than
I did not take the "retain incomplete-quorum records independently" option: those are exactly the unbounded set (a provider reporting events the others never will has no terminating condition), so exempting them reinstates the leak this PR set out to fix. Instead the anchor is clamped to Three tests added to
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 694a20e83a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!(blockNumber > (this.providerBlockNumbers[provider] ?? 0))) { | ||
| return; // This provider hasn't advanced, so the cutoff can't have moved. | ||
| } | ||
| this.providerBlockNumbers[provider] = blockNumber; |
There was a problem hiding this comment.
When quorum is greater than one and a provider's FundsDeposited subscription stalls while its FilledRelay subscription continues, this single provider watermark advances from the healthy subscription. EventListener.ts:115-134 creates a separate watchEvent stream for every event descriptor but identifies all of them only by provider.name, so the cutoff can evict another provider's deposit vote before the delayed deposit callback arrives; that callback then starts a fresh one-vote record and the deposit is silently omitted, with no stalled-provider warning because the provider appears current. Fresh evidence after 694a20e is this per-event subscription split, so progress must be tracked per subscription or derived from a signal covering all watched logs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 6a1f7ef.
onEvents() does open one watchEvent stream per (address, event) pair while keying progress on provider.name alone, so a provider whose FilledRelay stream kept flowing advanced the watermark used to evict pending FundsDeposited votes — and, as you note, it never appeared stalled, so the warning never fired either.
Watermarks are now keyed per subscription (${address}-${event}), which is exactly the granularity of a viem watchEvent stream, and eviction only sweeps events belonging to the subscription that advanced. Progress on one stream can no longer evict another's pending votes, and the stalled-provider warning is likewise scoped per subscription.
Note this also removes a false-positive source: a legitimately quiet subscription (e.g. RelayedRootBundle) no longer looks stalled just because a busy one raced ahead. Its own records are held until its own providers advance, and since no new events arrive on a quiet stream that retention is naturally bounded.
Test Tracks provider progress per subscription covers it; it fails against 694a20e.
| const bucketBlockNumber = eventKeys.map((eventKey) => this.events[eventKey]?.blockNumber).find(isDefined); | ||
| if (!isDefined(bucketBlockNumber) || bucketBlockNumber < cutoff) { | ||
| eventKeys.forEach((eventKey) => delete this.events[eventKey]); | ||
| delete this.blockHashes[blockHash]; |
There was a problem hiding this comment.
In the SVM listener, logFromEvent assigns blockHash: "" to every log (RelayerSpokePoolListenerSVM.ts:38-62), so every Solana event lands in the same bucket. Once that bucket's oldest event crosses the cutoff, this branch treats the whole bucket as stale and deletes all its event keys, including events from the newest 5,000 slots that are still awaiting another provider's vote; a later vote then recreates a one-vote entry and never reaches quorum, silently dropping FundsDeposited or FilledRelay. Bucket SVM events by slot or evaluate each event independently instead of assuming every key in a block-hash bucket shares the sampled block number.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this was the more serious of the two — fixed in 6a1f7ef.
UNUSED_FIELDS in RelayerSpokePoolListenerSVM.ts does set blockHash: "", so every Solana event landed in one bucket; sampling the oldest still-present member and deleting the whole bucket would have wiped events from the newest slots that were still awaiting a second vote.
Two changes:
- Eviction is now per event, keyed on each event's own
blockNumber, instead of per bucket with a sampled block number. Bucket compaction happens afterwards in a single pass overblockHashes(filtering against aSetof evicted keys) so it stays linear rather than quadratic on a large bucket. - Logs with an empty
blockHashare no longer bucketed at all. The bucket exists solely to service re-org removals, which are keyed onblockHashand never fire for SVM — so an empty key was pure downside: it accumulated every event and would have dropped all of them on a singleremove().
Test Evicts events individually when logs share a blockHash bucket covers it; it fails against 694a20e.
Unrelated pre-existing issue I noticed while checking this, not touched here: logFromEvent also pins logIndex: 0, so getEventKey() collides for two same-named events in one Solana transaction (FundsDeposited--<signature>-0). Worth a separate fix — happy to open an issue if useful.
…r event Two further defects in the retention bound, both silently costing quorum: - Progress was tracked per provider, but EventListener.onEvents() opens a separate watchEvent stream per (address, event) pair. A provider whose FilledRelay stream kept flowing therefore advanced the watermark used to evict pending FundsDeposited votes, even though its deposit stream was stalled — and it never looked stalled, so no warning fired. Watermarks are now keyed per subscription, so one stream's progress can't evict another's pending votes. - Eviction operated on blockHash buckets, sampling one event's block number for the whole bucket. SVM logs carry no blockHash (UNUSED_FIELDS sets it to ""), so every Solana event shared a single bucket: once its oldest member aged out, the entire bucket was deleted, including events from the newest slots still awaiting a second vote. Eviction is now per event, keyed on each event's own block number, and logs without a blockHash are no longer bucketed at all — the bucket exists only to service re-org removals, which are keyed on blockHash and never fire for SVM. Tests cover both: cross-subscription isolation of provider progress, and individual eviction when every log shares a bucket. Both fail against 694a20e. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex Both findings were real; fixed in 6a1f7ef. Please re-review. P1 — Key SVM retention buckets by slot. P2 — Track progress per event subscription. Two tests added, one per finding; both fail against 694a20e and pass now (verified by checking the previous One adjacent pre-existing bug I did not touch, flagged on the inline thread: |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
This pass is against Worth flagging for anyone reading the thread: Not re-tagging the bot, since I already requested a re-review of Standing verification for One open item from that commit, unaddressed by design: |
Outcome
Checks
|
|
Acknowledged — nothing further needed here. One clarification on the
No lockfile change is in the diff, so there's nothing to revert or exclude. This is ready for human review. |
1 participant
Two independent robustness fixes in relayer utils:
Solana ALT off-by-one (
getAddressLookupTableInstructions).nInstructions = Math.floor(len / 32) + 1always appended one extra chunk; whenlutAddresses.lengthis an exact multiple of 32 (including 0) the finalslice(offset, offset)was empty, producing anExtendLookupTableinstruction with zero addresses, which the on-chain address-lookup-table program rejects — failing Solana relayer-refund-leaf execution whenever the leaf's account count lands on a multiple of 32. Changed toMath.ceil(len / 32).EventManager unbounded growth (
EventManager).eventsandblockHashesaccumulated one entry per observed log/block for the whole process lifetime with no eviction, andremove()(only triggered by reorg 'removed' notifications) deleted fromeventsbut left theblockHashes[blockHash]bucket behind.remove()now also drops the stale bucket, and a re-org-safe retention window (MAX_EVENT_RETENTION_BLOCKSbehind the highest observed block) prunes old records as the head advances, so memory stays bounded. The retention window is chosen to comfortably exceed any monitored chain's re-org depth so pruning can never drop an event still eligible for a 'removed' notification or a late quorum vote.Build (
tsc --build), prettier and eslint pass.