feat: penalize peers for forwarding REJECTED gossip messages - #10059
Merged
Merged
Conversation
Contributor
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #10059 +/- ##
=========================================
Coverage 52.72% 52.73%
=========================================
Files 848 848
Lines 59137 59129 -8
Branches 4350 4350
=========================================
Hits 31180 31180
+ Misses 27900 27892 -8
Partials 57 57 🚀 New features to boost your workflow:
|
spiral-ladder
previously approved these changes
Sep 14, 2026
spiral-ladder
left a comment
spiral-ladder
left a comment
Member
There was a problem hiding this comment.
seems good, should we be more tolerant of certain op pool actions like voluntary_exit?
matthewkeil
moved this from In Progress
to Awaiting Author
in Lodestar Team Coordination
nflaig
reviewed
Sep 14, 2026
wemeetagain
reviewed
Sep 15, 2026
| [GossipType.payload_attestation_message]: {default: PeerAction.MidToleranceError}, | ||
| [GossipType.execution_payload_bid]: { | ||
| default: PeerAction.HighToleranceError, | ||
| byCode: {[ExecutionPayloadBidErrorCode.INVALID_SIGNATURE]: PeerAction.MidToleranceError}, |
Member
There was a problem hiding this comment.
Should invalid signatures be fatal?
Member
Author
There was a problem hiding this comment.
invalid signature for beacon_block is fatal as in #10087
it seems too much for other topics, but doing that would helpful to disconnect some spammers as we see in mainnnet
@lodekeeper research how other clients behave on this matter
Contributor
There was a problem hiding this comment.
I would not use Lodestar PeerAction.Fatal for invalid signatures generally. Fatal is immediate ban; the analogous #10087 block path is LowToleranceError, which disconnects/bans after repeated offenses.
So my recommendation is:
LowToleranceErrorfor block-like/heavy gossip and for attestation/sync invalid signatures.MidToleranceErrorfor invalid signatures on cheap/op-pool topics such asvoluntary_exit, BLS-to-exec, execution payload bid, and proposer preferences.HighToleranceErrorfor other cheap-topic rejects.
I pushed that as a signed patch to my fork because I cannot push to the PR branch directly: lodekeeper@fbcb83dbf7. Verification passed: pnpm build, targeted gossipValidatorFn.test.ts, and pnpm lint.
Contributor
There was a problem hiding this comment.
Checked the client behavior and I agree with the shape you suggested: bad block signatures deserve the strongest repeated-offense penalty, but applying that equally to every gossip topic is too aggressive.
What I found:
- Lighthouse rejects and low-tolerance penalizes invalid attestation/sync signatures; light-client invalid update paths are much more tolerant.
- Prysm returns pubsub
ValidationRejectfor invalid signatures on block/sync/voluntary-exit validation paths, but I did not find a separate immediate peer-manager fatal action in those validators. - Teku returns
InternalValidationResult.rejectfrom the equivalent gossip validators for invalid signatures, again without an obvious immediate-ban layer in the validator itself.
So I would keep Lodestar on per-topic scoring: low tolerance for block-like/heavy topics plus attestation/sync invalid signatures, mid tolerance for cheap-topic invalid signatures, and high tolerance for other cheap-topic rejects. I pushed a tested patch here because I cannot write to te/penalize_on_rejected_messages directly: lodekeeper@fbcb83dbf7
Contributor
|
@spiral-ladder follow-up on the dismissed review body about op-pool tolerance: yes, I agree we should be more tolerant for cheap/op-pool topics like The policy I recommended in the inline threads is per-topic rather than all-topic fatality: keep block-like/heavy topics on low tolerance, move attestation/sync invalid signatures to low tolerance because other clients treat those as strongly invalid, but keep cheap op-pool topics on high-tolerance defaults with invalid signatures only at mid tolerance. I pushed a tested patch to my fork since I cannot write to the PR branch directly: lodekeeper@fbcb83dbf7 |
twoeths
commented
Sep 17, 2026
twoeths
left a comment
twoeths
left a comment
Member
Author
There was a problem hiding this comment.
the latest consensus from team is to always apply FATAL for all INVALID_SIGNATURE codes because it may cause chain split, even for oppool topics like voluntary_exit, they could be bundled to a beacon block later
I also added INVALID_SIGNATURE codes for slashing topics.
Contributor
|
@twoeths thanks for pushing this — checked the diff ( This isn't blanket Fatal for every reject: the topic-level That distinction resolves my original pushback ( On "bundled into a block later" specifically — worth being precise about what backstops what, since this is the codebase's actual current behavior either way: No objection to |
Member
|
@lodekeeper does this look good now? please approve or leave final remarks |
lodekeeper
approved these changes
Sep 17, 2026
lodekeeper
left a comment
lodekeeper
left a comment
Contributor
There was a problem hiding this comment.
Re-checked the final diff after the signature-specific Fatal updates. The mapping is now scoped the way I would want it: generic REJECTs keep the topic-level tolerance, while every concrete invalid-signature code is Fatal, including the op-pool topics and the new slashing-specific codes. That matches the deterministic-signature rationale from the earlier thread, and the slashing code split avoids treating all slashing invalidity as signature-invalid. No further remarks from me.
wemeetagain
approved these changes
Sep 17, 2026
github-project-automation
Bot
moved this from Awaiting Author
to Done
in Lodestar Team Coordination
nflaig
added a commit
that referenced
this pull request
Sep 20, 2026
Since #10059 the validator wrapper reports the peer for every REJECT by topic and code, Fatal for an invalid proposer signature on beacon_block, so the handler-level LowToleranceError report was a second, weaker penalty for the same rejection.
ensi321
added a commit
that referenced
this pull request
Sep 28, 2026
Conflicts resolved: - execution/engine/http.ts: keep both deserializeExecutionPayloadBodyV2 and deserializeInclusionList imports - metrics/lodestar.ts: drop backfillSync metrics removed upstream (#10147), keep engine_getInclusionListV1 metrics - network/gossip/topic.ts: adopt required sszType param (upstream), keep MAX_SIGNED_INCLUSION_LIST_SIZE cap - state-transition shuffling.test.ts: accept upstream deletion (#9829), dropping the computeInclusionListCommittee unit tests with it Semantic fix: add inclusion_list to the gossip REJECT peer-action table from #10059 (mid tolerance, fatal on invalid signature) and export inclusion list errors from the chain errors barrel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
|
🎉 This PR is included in v1.49.0 🎉 |
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
6 participants
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.
Motivation
Description
gossipValidatorFnpart of #9925
AI Assistance Disclosure