Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
nflaig
marked this pull request as draft
September 12, 2026 14:32
![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: b6fda77e14
ℹ️ 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".
| stateRoot: toRootHex(block.stateRoot), | ||
| timeliness: isTimely, | ||
| ptcTimeliness: this.isBlockPtcTimely(block, receiveDelaySec), | ||
| ptcTimeliness: this.isBlockPtcTimely(block, ptcReceiveDelaySec), |
There was a problem hiding this comment.
When unknown-block sync imports an early-seen sibling after its slot has ended, passing only the saved delay here does not preserve its timeliness: isBlockPtcTimely() also requires fcStore.currentSlot === block.slot. This occurs when the sibling becomes relevant through a descendant in the next slot, so it is still recorded as not PTC-timely and shouldApplyProposerBoost() misses the early equivocation. Derive the current-slot condition from the saved arrival time or pass a precomputed first-seen timeliness value.
Useful? React with 👍 / 👎.
Comment on lines
+145
to
+147
| for (const slot of this.firstSeenTimestampSecBySlot.keys()) { | ||
| if (slot < finalizedSlot) { | ||
| this.firstSeenTimestampSecBySlot.delete(slot); |
There was a problem hiding this comment.
If finality stalls, this cleanup never removes entries for later slots, so the new cache can grow by up to 4096 roots for every affected slot indefinitely. After a proposer has supplied two signature-verified siblings, additional repeat proposals skip signature verification, making it practical for an equivocating proposer and its peers to fill this allowance on each selected slot and steadily exhaust node memory during an extended non-finalizing period. Retain only the small recent-slot window in which PTC timeliness can still matter, or enforce a global bound.
Useful? React with 👍 / 👎.
nflaig
commented
Sep 12, 2026
| validBlobSidecars?: BlobSidecarValidation; | ||
| /** Seen timestamp seconds */ | ||
| seenTimestampSec?: number; | ||
| /** Seconds the block was first seen on gossip, if it was ignored there as a repeat proposal. Only affects PTC timeliness */ |
Member
Author
There was a problem hiding this comment.
Only affects PTC timeliness
so confusing that the ptc deadline was reused for this, also kinda arbitrary
Contributor
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A repeat proposal received over gossip is ignored on the wire but imported locally (#9805), bounded by the two signature-verified roots the seen cache keeps per proposer and slot (#9795). Since the cache fills on signature alone, a signed but invalid block burns one of the two slots, and a valid sibling arriving after that is dropped without ever reaching fork choice. If it was released before the PTC deadline,
should_apply_proposer_boostshould count it as an early equivocation andget_proposer_headshould see the equivocation, but we never learn about it. Raising the cap does not help, a proposer can send as many signed blocks as it takes.This keeps the bound on full imports and instead records when each repeat proposal was first seen. A sibling that later becomes relevant is fetched by sync anyway, through unknown parent or attestation lookups, and is now imported with its gossip arrival for the PTC timeliness flag rather than the download time. Validation work is only spent on siblings that gained attestation weight or a descendant, so it is bounded by what the attacker can get attested rather than by a constant.
SeenBlockProposers, capped per slot and pruned on finalizationForkChoice.onBlockand pass the first seen time throughprocessBlockfrom unknown block syncAttestation timeliness and proposer boost eligibility of a fetched block still use the import time, so the existing boost guard for downloaded blocks of a known proposer is unchanged. A valid timely sibling that never gains weight or a descendant stays out of fork choice, which by the reasoning in ethereum/consensus-specs#4807 is not a dangerous block.