feat: prune PayloadEnvelopeInput when syncing#9509
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the pruning of the gloas payload-envelope cache. The pruning logic is moved from prepareNextSlot.ts to importBlock.ts and scheduled to run in the next event loop. Additionally, pruneBelowParent in seenPayloadEnvelopeInput.ts is updated to only evict when the payload status is FULL and all data has been computed, preventing premature eviction during syncing or column gathering. Corresponding unit tests have been added to verify these new conditions. There are no review comments, and I have no further feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Performance Report✔️ no performance regression detected Full benchmark results
|
|
@lodekeeper please review given the context you have here #9489 (comment) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #9509 +/- ##
=========================================
Coverage 52.58% 52.58%
=========================================
Files 848 848
Lines 60875 60875
Branches 4483 4483
=========================================
Hits 32013 32013
Misses 28800 28800
Partials 62 62 🚀 New features to boost your workflow:
|
![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: 4a2cfa088c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| callInNextEventLoop(() => { | ||
| const newHeadParent = this.forkChoice.getBlockHexDefaultStatus(newHead.parentRoot); | ||
| if (newHeadParent) { | ||
| this.seenPayloadEnvelopeInputCache.pruneBelowParent(newHeadParent); |
There was a problem hiding this comment.
When syncing through a long unfinalized Gloas range, this new per-import prune runs once for every imported block. pruneBelowParent() calls getAllAncestorBlocks(...), which walks from the parent back toward the finalized boundary, so importing N unfinalized blocks does O(N²) ancestor scans in the same non-finality syncing case this change targets and can stall sync on CPU. Please throttle this or track the last pruned point so each import only scans the newly eligible portion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid concern, and correct in direction. During non-finality the finalized boundary is frozen, so getAllAncestorBlocks walks parent→finalized with depth growing 1→N across imports → Σk = O(N²) node visits + a fresh ProtoNode[] allocation each import. In the empty-branch case this PR targets, those ancestors fail the FULL && hasComputedAllData() guard, so the walk evicts nothing — it's pure re-scan overhead over the same prefix.
Magnitude: negligible for a few epochs (N≈100–300 → tens of thousands of cheap ops). It only bites under prolonged non-finality (thousands of unfinalized blocks) — which is exactly the scenario class this change targets, so worth bounding eventually.
Concrete bounded fix: swap getAllAncestorBlocks for the lazy iterateAncestorBlocks generator and keep a lastPrunedSlot cursor, breaking early once the walk reaches already-pruned depth → aggregate O(N).
One correctness caveat with a pure cursor: an ancestor that was EMPTY when first walked but turns FULL later (payload revealed mid-sync) sits below the cursor and would never be re-visited by pruneBelowParent. pruneFinalized is the backstop (it evicts everything below the finalized slot on the next finalization), so the retained set stays bounded by the non-finality window even with the cursor. If you want precise eviction without leaning on finalization, an evict-on-FULL-transition hook is the alternative, but that's a larger change.
Non-blocking from my side for the immediate devnet fix — fine as a follow-up. Your call on whether to fold it in here.
|
@twoeths reviewed — this is the right direction for the devnet regression, and I'd take it over #9489. Why it's correct: moving the prune out of Test coverage looks good: positive eviction (FULL + computed below parent → evicted), plus the two retention cases (not-yet-FULL, not-computed-all-data) and the parent-slot edge. One non-blocking perf follow-up: the Codex P2 about the O(N²) ancestor walk is valid in direction for prolonged non-finality — details + a concrete bounded fix in the inline thread. I'd treat it as a follow-up, not a merge blocker for the devnet fix. Scope caveat (carry-over from #9489): #9509 bounds the cache via import-driven pruning, so it does not cover the original #9073 shape where the cache grows while no blocks import / no fork-choice progression. That OOM path should stay a separate follow-up rather than going back to unsafe insert-time eviction. |
I think this is a valid concern |
|
Agreed. To pin down where it's tracked: that shape lives in #9073, which stays open after this lands — #9509 references #9489 but doesn't close #9073. Not a blocker or a reason to revert #9509, though:
So: merge #9509 for the devnet regression, keep #9073 open for the no-progression follow-up. |
|
Agreed. Looked into this briefly with @matthewkeil yesterday. We concluded that this PR is good to patch the immediate issue but does not fix all cases (the obvious one being non-finality with a long-running consistently-minority fork, another being long, long backwards syncing case). And that directionally, this one seems better than #9489 which is a bit of a blunt instrument with nasty behavior in the backwards syncing case slightly longer than the cache size. |
|
Thanks @wemeetagain (and @matthewkeil) — matches the read here. #9509 is the right immediate patch (import-driven eviction, no insert-time data loss), and #9489's insert-time cap is the blunt instrument with the nasty behavior in the backwards-sync-longer-than-cache case. I've captured your two cases — non-finality with a long-running minority fork, and long backwards/backfill sync — as follow-up in #9073 (the tracking issue for the envelope cache's missing size-cap / sync pruning). Both point at the same fix: a |
|
@lodekeeper is there anything left to get this merged? if not please approve |
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
LGTM — approving. Verified end-to-end:
- CI all green; mergeable, only the review was outstanding.
- The fix is sound: pruning moves from
prepareNextSlot(which doesn't run during sync, so the cache grew unbounded) intoimportBlockso it runs on every import, and the newpayloadStatus === FULL && hasComputedAllData()guard inpruneBelowParentprevents evicting EMPTY/PENDING envelopes still needed for payload-by-root sync (#9475). - The only open review thread (Codex's P2 on the per-import ancestor walk) is a non-blocking follow-up — the walk is bounded by the non-finality window and
pruneFinalizedis the backstop; the lazy-iterator + cursor optimization can land later. - The broader #9073 shape (cache growth while no blocks are imported) stays tracked separately, per the thread with @ensi321 / @wemeetagain.
Nice fix @twoeths.
|
Approved 👍 — but one heads-up: it still shows Nothing else is left, though: CI's all green, the Codex P2 (per-import ancestor walk) is a non-blocking follow-up (bounded by the non-finality window, |
|
🎉 This PR is included in v1.44.0 🎉 |
5 participants
Motivation
pruneBelowParent()when the node is synced, ie in "PrepareNextSlot", this may cause OOM when the node is syncing and unfinality network as inglamsterdam-devnet-5Description
pruneBelowParent()afterimportBlock()this is an alternative to #9489
Created with the help of Claude