-
-
Notifications
You must be signed in to change notification settings - Fork 457
fix: ignore repeat proposal during block publishing #9235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: unstable
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -191,7 +191,10 @@ export function getBeaconBlockApi({ | |||||
| try { | ||||||
| await validateGossipBlock(config, chain, signedBlock, fork); | ||||||
| } catch (error) { | ||||||
| if (error instanceof BlockGossipError && error.type.code === BlockErrorCode.ALREADY_KNOWN) { | ||||||
| if ( | ||||||
| error instanceof BlockGossipError && | ||||||
| (error.type.code === BlockErrorCode.ALREADY_KNOWN || error.type.code === BlockErrorCode.REPEAT_PROPOSAL) | ||||||
| ) { | ||||||
| chain.logger.debug("Ignoring known block during publishing", valLogMeta); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The log message "Ignoring known block" is slightly inaccurate when the error is
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — applied in b68ea2b. The log is now |
||||||
| // Blocks might already be published by another node as part of a fallback setup or DVT cluster | ||||||
| // and can reach our node by gossip before the api. The error can be ignored and should not result in a 500 response. | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
REPEAT_PROPOSALis raised for any second proposal by the same proposer/slot, not just benign duplicates of an already-known root. By returning success for this code path,publishBlock()now silently drops blocks that may be a different root (equivocation or failover mismatch) before the publish/import section runs, so the caller receives a false-positive success even though this block was neither broadcast nor processed. Consider only suppressing when the same block root is already known/in-flight, and keep a surfaced error for non-identical repeat proposals.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point — worth discussing the tradeoff.
REPEAT_PROPOSALdoes fire purely on(slot, proposerIndex)(seenBlockProposers.tstracks no root), so the suppression is semantically wider thanALREADY_KNOWN. The cases it collapses:Same root, race window. API publish arrives after gossip validation has run for the same block (adds to
seenBlockProposersatblock.ts:248) but before fork-choice import finishes.ALREADY_KNOWNatblock.ts:59returns null;REPEAT_PROPOSALatblock.ts:68fires. This is the case issuePOST eth/v2/beacon/blocksreturningBLOCK_ERROR_REPEAT_PROPOSALin multi-node setup #9228 describes.Distinct root (equivocation / failover mismatch / VC bug). Same (slot, proposer), different body.
In both cases the block is rejected by gossip validation before the
publishBlock/import section — so nothing is broadcast or imported either way. The only observable difference for the caller is 500 vs 2xx.Reliably distinguishing (1) from (2) would need
SeenBlockProposersto track seen roots per(slot, proposer), which is a structural cache change beyond the scope of this targeted fix. Thereason: error.type.codemetadata now in the log (commitb68ea2b, per @gemini-code-assist suggestion) does surface the REPEAT_PROPOSAL case to operators so case (2) is still observable.Happy to tighten if @nflaig prefers — options I see:
reasonlog field for observability (current PR),SeenBlockProposerswith aseenRootsByProposer: Map<Slot, Map<ValidatorIndex, Set<RootHex>>>and only suppress whenseenRootsByProposer.get(slot)?.get(proposerIndex)?.has(blockRoot)is true, surfacing otherwise,