fix: handle previous-epoch payload timeliness committees - #8983
fix: handle previous-epoch payload timeliness committees#8983lodekeeper wants to merge 3 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical bug in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses an epoch-boundary bug by caching and serving payload timeliness committees for the previous epoch. While the overall implementation is clean and includes good unit test coverage, a critical vulnerability exists: the initialization logic in EpochCache.createFromState is incomplete. It fails to populate the previous-epoch committees when the cache is created from a state, which could lead to validation failures and Denial of Service at epoch boundaries after a node restart.
|
@gemini-code-assist good catch — you were right about I pushed commit
Validation rerun:
Thanks again for flagging it. |
|
That's great to hear, @lodekeeper! I'm glad the feedback was helpful and that you've addressed the initialization of |
|
Closing for now per latest scope decision (Option 2): continue debugging mixed-client finalization/peering until acceptance criteria are met, then reopen/retarget with consolidated evidence. |
|
@lodekeeper why did you close this? |
|
@nflaig I closed it because of the latest scope decision (Option 2): keep debugging mixed-client finalization/peering first and only reopen once acceptance criteria are met.\n\nYou're right that I should have replied to your question immediately — sorry about that delay.\n\nIf you prefer, I can reopen this now and keep it as draft while we continue the interop investigation. |
|
Reopening per maintainer request so other maintainers can review this scoped PTC fix while we continue mixed-client interop debugging in parallel. |
|
Interop update while this PR is open for scoped PTC review:\n\n- Re-validated that PTC regression remains fixed in mixed-client runs (0 occurrences of |
|
Option-2 interop update (new mitigation run):
Evidence: notes/epbs-devnet-0/runs/mitigation3-supernode-2x2-20260304T183130Z/SUMMARY.md |
|
Final scope/status update:
Per latest maintainer direction, treating this task as done-for-now on Lodestar side. I will only provide normal review follow-up on this PR if additional comments/questions arrive. |
| // Populate previous-epoch committees when creating cache from an in-memory state. | ||
| // This is needed for epoch-boundary lookups (slot N block validating slot N-1 attestation) | ||
| // after restart / fresh cache initialization. | ||
| if (!isGenesis && previousEpoch >= config.GLOAS_FORK_EPOCH) { |
There was a problem hiding this comment.
Good call — it is not strictly required.
I simplified this in fb9cbaeaac by removing isGenesis from the guard and keeping only previousEpoch >= config.GLOAS_FORK_EPOCH.
That keeps pre-fork behavior unchanged, and for genesis with fork-at-0 it just computes the same committee set (harmless) instead of special-casing.
There was a problem hiding this comment.
what's better is to do the following, similar to shuffling
const previousShuffling =
cachedPreviousShuffling ??
(isGenesis
? currentShuffling
: computeEpochShuffling(state, new Uint32Array(previousActiveIndicesAsNumberArray), previousEpoch));
There was a problem hiding this comment.
ah, maybe it was fine, let's re-add the is genesis check
There was a problem hiding this comment.
Good call — agreed. The final version now uses exactly that shuffling-style fallback for previousShuffling (cachedPreviousShuffling ?? (isGenesis ? currentShuffling : computeEpochShuffling(...))). This is present in #8991 (the PR carrying this fix to unstable), and #8983 was closed in favor of that cherry-pick path.
There was a problem hiding this comment.
Yep — agreed, and this ended up reflected in the follow-up path. This PR is now closed/superseded, but in the merged replacement (#8991) the genesis guard is present again in EpochCache.createFromState (previousShuffling = isGenesis ? currentShuffling : computeEpochShuffling(...)).
| // Populate previous-epoch committees when creating cache from an in-memory state. | ||
| // This is needed for epoch-boundary lookups (slot N block validating slot N-1 attestation) | ||
| // after restart / fresh cache initialization. |
There was a problem hiding this comment.
can we move this comment to previousPayloadTimelinessCommittees type
There was a problem hiding this comment.
Done in fb9cbaeaac — I moved that explanatory comment up to the previousPayloadTimelinessCommittees declaration so it documents the field directly.
|
@lodekeeper I cherry picked your changes in #8991 to bring them directly to our unstable branch. please review that PR to make sure it's correct with your implementation and if it is please close this PR, I will cherry-pick my commit to |
We need the PTC cached for previous epoch in case of epoch boundary lookups, eg. simple case is slot N block validating slot N-1 attestation ~~but also if there are deeper reorgs / missed slots.~~ (this isn't required as ptc messages are only valid for one slot). See #8983 for more details.
We need the PTC cached for previous epoch in case of epoch boundary lookups, eg. simple case is slot N block validating slot N-1 attestation ~~but also if there are deeper reorgs / missed slots.~~ (this isn't required as ptc messages are only valid for one slot). See #8983 for more details.
We need the PTC cached for previous epoch in case of epoch boundary lookups, eg. simple case is slot N block validating slot N-1 attestation ~~but also if there are deeper reorgs / missed slots.~~ (this isn't required as ptc messages are only valid for one slot). See ChainSafe#8983 for more details.
Summary
Fixes an epoch-boundary bug in payload timeliness committee lookup.
EpochCache.getPayloadTimelinessCommittee(slot)previously only served committees forepochCtx.epoch.At epoch boundaries, payload attestation validation can reference
slot - 1(previous epoch), which caused:This patch:
Changes
packages/state-transition/src/cache/epochCache.tsgetPayloadTimelinessCommittee(slot)lookup logicpackages/state-transition/test/unit/cache/epochCache.test.tsValidation
Local
pnpm --filter @lodestar/state-transition lintpnpm --filter @lodestar/state-transition test:unit -- test/unit/cache/epochCache.test.tsDevnet evidence (local)
Notes on remaining mixed-client instability
In my local mixed-client and Teku-only control runs, finalization remained unstable/absent due separate interop/topology behavior (outside this patch scope). This PR is scoped to the PTC root-cause fix only.
AI-assisted: drafted and validated with OpenClaw; final patch and test assertions reviewed by me.