fix: bound payload envelope seen cache - #9904
Conversation
![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: bb66eca801
ℹ️ 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".
| for (const input of this.payloadInputs.values()) { | ||
| if (this.payloadInputs.size <= MAX_PAYLOAD_ENVELOPE_INPUT_CACHE_SIZE) { | ||
| break; | ||
| } | ||
| this.evictPayloadInput(input, "cap"); |
There was a problem hiding this comment.
When range sync fills the 96-entry cap and another block is inserted, insertion-order eviction can remove an older-inserted canonical input even if it is recent and still awaiting its envelope or columns. The gossip envelope and payload-column handlers only call seenPayloadEnvelopeInputCache.get(), so after this eviction they ignore valid data with PAYLOAD_ENVELOPE_INPUT_MISSING; they do not recover through getOrReload() as the safety comment claims. This can stall payload processing during concurrent range sync or fork pressure. The cap must exclude active inputs or the gossip paths must reload them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this should not happen, the seen cache contains 96 items while gossip only accept messages in the last recent 32 slots, given DEFAULT_EARLIEST_PERMISSIBLE_SLOT_DISTANCE = 32
even if gossip fails due to this, this is by design, because it will fallback to UnknownBlockInputwhich callsgetOrReload()`
| source: PayloadEnvelopeInputSource.reload, | ||
| daOutOfRange, | ||
| }); | ||
| this.payloadInputs.set(blockRootHex, input); |
There was a problem hiding this comment.
On the DB path, reloadFromDb() yields while awaiting db.block.get(). A concurrent gossip, API, by-root, or range call can then add the same root and attach an envelope or columns, but this unconditional set() overwrites that live object with an empty reconstructed shell. Callers retaining the first object and callers reading the cache then operate on different inputs, potentially losing received data and duplicating payload processing. Recheck payloadInputs after the await and return the concurrently inserted entry instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
should not happen too, when the block comes again at either source it will be ignored due to forkchoice check
Performance Report✔️ no performance regression detected Full benchmark results
|
|
@lodekeeper please review, given the last error that you know #9489 (comment) |
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Reviewed against the #9489 failure I root-caused (insert-time pruneToMaxSize evicting a root mid-batch → cacheByRangeResponses throwing Missing PayloadEnvelopeInput).
Core fix — LGTM, this closes the #9489 hole. Range sync now resolves in-batch payload inputs from the batch-local payloadEnvelopes map (strong refs, seeded in the validatedBlocks loop, round-tripped across retries via Batch.state.payloadEnvelopes), so an in-batch entry is guaranteed present regardless of shared-cache eviction. Only the dangling parent still falls back to seenPayloadEnvelopeInputCache.get(), and that path is isFirstBatchInChain-only at sync-chain start when the cache is well below the 96 cap — no realistic eviction there. Right decoupling: range sync no longer depends on the shared cache for correctness.
On the two Codex P1s — I agree neither is a blocker, two small precision notes:
P1 / pruneToMaxSize (L296): the mechanism is real — eviction is insertion-order (Map.values()), not slot-order, so a concurrent backfill/reorg burst can shed an older-inserted but recent-slot gossip entry. And to be precise, the gossip handlers don't self-heal here: all three consumers (.get() at L492 column, L645 block post-processing, L1196 envelope) IGNORE/throw on miss — none call getOrReload. The recovery is the UnknownBlockInput fallthrough (which does getOrReload), not the gossip path itself. In practice this needs a below-head backfill piling ≥96 inserts after a live gossip entry, and it's recoverable via that fallthrough + req/resp — so bounded, agreed. Only ask: tweak the pruneToMaxSize comment to name the UnknownBlockInput fallthrough as the recovery path rather than implying the gossip handlers reload, so the next reader doesn't over-trust the shared cache.
P1 / reloadFromDb recheck (L240): agree it's effectively unreachable — the reloading map dedups getOrReload×getOrReload, any add() for a fork-choice-known root short-circuits before reloadFromDb, and the EMPTY-over-FULL overwrite is by design with consumers gating on hasPayloadHexUnsafe. A 2-line post-await const existing = this.payloadInputs.get(blockRootHex); if (existing) return existing; before the set() is optional insurance so a future add() caller can't invalidate the invariant — not required.
Nit: reloadFromDb uses Math.floor(Date.now()/1000) while downloadByRoot uses unfloored Date.now()/1000 — harmless.
Nice work bounding this without reintroducing the #9489 regression.
There was a problem hiding this comment.
One more spot I looked at, resolved — noting for the record, no action needed:
getMissingBlockDependency (unknownBlock.ts L516) drops the seen-cache parent-payload mismatch check. On a closer read this is fine: the authoritative invalidParentPayload branch (forkChoice.hasPayloadHexUnsafe(parentRootHex), L543-544) is retained, so a child referencing a wrong parent block hash is still rejected once the parent payload reveals. The removed parentPayloadInput.getBlockHashHex() comparison was only a pre-reveal shortcut over the now-non-authoritative seen cache, so dropping it is correct.
| return {kind: "invalidParentPayload", parentRootHex, parentBlockHashHex}; | ||
| } | ||
|
|
||
| const parentPayloadInput = this.chain.seenPayloadEnvelopeInputCache.get(parentRootHex); |
There was a problem hiding this comment.
changing this to "getOrReload" would make the consumers to change accordingly
but we don't really need it, it's kind of dead code
we checked this.chain.forkChoice.hasPayloadHexUnsafe(parentRootHex) above
so if it reaches here, it really means "parentPayload"
There was a problem hiding this comment.
Agreed, this matches what I flagged in the review: that trailing cache.get(parentRootHex) mismatch check was redundant, so dropping it is fine. By the time control reaches the parentPayload return, the hasPayloadHexUnsafe(parentRootHex) guard just above (returning invalidParentPayload) has already caught the revealed-but-mismatched case, so anything past it really is a parent payload. Since the line is gone, .get() vs .getOrReload() there is moot. No change needed.
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Approving. The core fix correctly closes the #9489 range-sync eviction hole — range sync now resolves in-batch payload inputs from the batch-local payloadEnvelopes map, so it no longer depends on the shared cache for correctness. CI is green and the seenTimestampSec float nit is addressed in both reloadFromDb and the anchor-state path.
The two remaining items from my earlier review are non-blocking and fine as follow-ups if at all: (1) the pruneToMaxSize comment could name the UnknownBlockInput fallthrough as the actual recovery path (the gossip handlers themselves IGNORE on miss), and (2) the optional post-await recheck in reloadFromDb — which we agreed is effectively unreachable given the reloading dedup + fork-choice gate. Nice work bounding this without reintroducing the #9489 regression.
nflaig
left a comment
nflaig
left a comment
There was a problem hiding this comment.
Looks pretty good, left some comments, but non-blocking to merge this
| const forkName = this.config.getForkName(block.message.slot); | ||
| if (!isForkPostGloas(forkName)) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
how can this be reached pre-gloas?
There was a problem hiding this comment.
This path doesn't read from the seen cache: reloadFromDb loads the block straight from seenBlockInputCache/db.block by whatever root getOrReload was called with, and only computes the fork after loading (getForkName(block.message.slot)). So a pre-gloas block reaches this guard whenever a caller passes such a root, and the guard then bails. Whether any real getOrReload caller actually passes a pre-gloas root vs this being purely defensive is your call @twoeths.
There was a problem hiding this comment.
there should be no pre-gloas flow calling this
it's still nice to have this check through, we need to query db block for subsequent statements anyway
There was a problem hiding this comment.
Makes sense, thanks @twoeths. Agreed, worth keeping as a cheap defensive guard since the db block load happens for the following statements anyway.
| expect(db.block.get).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("reconstructs the shell from the hot db when the gloas block is in fork choice", async () => { |
There was a problem hiding this comment.
"the shell" what does this mean?
There was a problem hiding this comment.
"shell" = the PayloadEnvelopeInput that reloadFromDb rebuilds from just the block via createFromBlock, i.e. the block skeleton with no execution payload envelope or data columns attached yet. This test checks that a cache miss on a known gloas block reconstructs that block-only entry from the hot db, so callers can then fill in the envelope/columns.
| /** | ||
| * Backstop cap for entries pruneBelowParent/pruneFinalized can't reach (non-canonical forks, | ||
| * EMPTY/PENDING entries). Evicts by INSERTION ORDER — the Map iterates oldest-inserted first — so a | ||
| * just-reloaded old-slot entry (set at the back) survives while genuinely stale forks/shells are shed. |
There was a problem hiding this comment.
"forks/shells are shed" I find the wording a bit strange
There was a problem hiding this comment.
Wording's yours to tweak @twoeths, but for context: "forks/shells" = the two kinds of entry this cap sheds first, stale non-canonical fork entries and the reconstructed block-only "shell" entries (same "shell" as the test thread). Something like "stale non-canonical or reconstructed-only entries are evicted first" might read clearer.
github-project-automation
Bot
moved this from In Progress
to Done
in Lodestar Team Coordination
4 participants
Motivation
Description
getOrReload()to the seen cache. If, just in case, some old payloads built on a pruned EMPTY PayloadEnvelopeInput, we can reload easily from dbgetOrReload()forUnknownBlockInputAI Assistance Disclosure