fix: marking peer as dumb on failed responses#21316
fix: marking peer as dumb on failed responses#21316mralj merged 5 commits intomerge-train/spartanfrom
Conversation
| // Or send us needed txs | ||
| // If block response is invalid, demote the peer back to dumb if it was smart. | ||
| // This happens when the peer pruned its proposal and can no longer serve index-based requests. | ||
| if (!this.isBlockResponseValid(response)) { |
There was a problem hiding this comment.
We have NOT_FOUND check above, maybe we should do the same here? If response is invalid it doesn't mean that peer is dumb now. Maybe it's a malicious response.
There was a problem hiding this comment.
@deffrian @mrzeszutko let's penalise peer in case of archiveRootsMatch is false
There was a problem hiding this comment.
@deffrian @mrzeszutko actually let me think bit more on this, not sure it's as simple as this
There was a problem hiding this comment.
I agree here with amp
TL;DR; if archive is not Fr.zero and there is a mismatch - we should penalise, otherwise ok.
P.S.
I agree with the code analysis, but I think the implementation might be better.
| * - Response archive is Fr.ZERO (peer pruned proposal, legitimate): marks peer dumb, returns true. | ||
| * - Non-zero archive mismatch (malicious response): penalises + marks dumb, returns true. | ||
| */ | ||
| private handleArchiveRootFromResponse(peerId: PeerId, response: BlockTxsResponse): boolean { |
There was a problem hiding this comment.
Being nitpicky, this now reads weird, you have
if(this.handleArchiveRootFromResponse)
And if it returns false then it singals everytihg being ok, otherwise it's not ok.
Why not rename it to responseArchiveRootIsBad or something like that?
So it reads better and returns true if bad
There was a problem hiding this comment.
refactored - did not really like just changing the name as the method had some side effects apart of the mismatch check
deffrian
left a comment
deffrian
left a comment
There was a problem hiding this comment.
I'm still not convinced that we should demote peers to dumb if they send us invalid response. But it's a nitpick
BEGIN_COMMIT_OVERRIDE feat: add ETHEREUM_HTTP_TIMEOUT_MS env var for viem HTTP transport (#20919) fix(archiver): filter tagged log queries by block number (#21388) fix(node): handle slot zero in getL2ToL1Messages (#21386) feat(sequencer): redistribute checkpoint budget evenly across remaining blocks (#21378) fix: fall back to package.json for CLI version detection (#21382) chore: Removed multiplier config (#21412) chore: Removed default snapshot url config (#21413) chore: Read tx filestores from network config (#21416) fix(node): check world state against requested block hash (#21385) feat(p2p): use l2 priority fee only for tx priority (#21420) feat(p2p): reject and evict txs with insufficient max fee per gas (#21281) revert "feat(p2p): reject and evict txs with insufficient max fee per gas (#21281)" (#21432) chore: Reduce log spam (#21436) fix(tx): reject txs with invalid setup when unprotecting (#21224) fix: orchestrator enqueue yield (#21286) chore(builder): check archive tree next leaf index during block building (#21457) fix: scenario deployment (#21428) chore: add claude skill to read network-logs (#21495) chore: update claude network-logs skill (#21523) feat(rpc): add package version to RPC response headers (#21526) chore(prover): silence "epoch to prove" debug logs (#21527) chore(sequencer): do not log blob data (#21530) fix: dependabot alerts (#21531) docs(p2p): nicer READMEs (#21456) fix(archiver): guard getL1ToL2Messages against incomplete message sync (#21494) fix(sequencer): await syncing proposed block to archiver (#21554) feat(ethereum): check VK tree root and protocol contracts hash in rollup compatibility (#21537) fix: marking peer as dumb on failed responses (#21316) fix(kv-store): make LMDB clear and drop operations atomic across sub-databases (#21539) feat(world-state): add blockHash verification to syncImmediate (#21556) chore(monitor): print out l2 fees components (#21559) chore: rm faucet (#21538) chore: remove old merkle trees (#21577) feat: Implement commit all and revert all for world state checkpoints (#21532) chore: skip flaky browser acir tests in CI (#21596) fix: Better detection for epoch prune (#21478) chore: logging (#21604) fix: Don't update state if we failed to execute sufficient transactions (#21443) END_COMMIT_OVERRIDE
3 participants
Summary
When a peer prunes a block proposal, it can no longer serve tx requests by index. Previously, if such a peer was already marked "smart", it stayed smart indefinitely — causing repeated failed queries and eventual penalty escalation to "bad" status. This PR demotes smart peers back to dumb when they can no longer serve index-based requests, and distinguishes legitimate pruning (
NOT_FOUND) from actual failures.markPeerDumb()toIPeerCollection/PeerCollection— removes peer from the smart setclearPeerData()toITxMetadataCollection/MissingTxMetadataCollection— removes stale "peer has tx X" associations on demotionhandleFailResponseFromPeer(): demote on bothFAILURE/UNKNOWN(with penalty) andNOT_FOUND(without penalty, since pruning is a legitimate state)decideIfPeerIsSmart(): replaceisBlockResponseValid()with an explicit archive root mismatch check andhandleArchiveRootMismatch()which distinguishes three cases:archiveRootisFr.ZERO(peer pruned proposal, legitimate) → demote to dumb, no penaltyarchiveRootis non-zero but mismatches ours (malicious response) → penalise (LowToleranceError) + demote to dumbFixes A-512