[Merged by Bors] - Revise EE peer penalites #3485
Conversation
paulhauner
added
ready-for-review
| ExecutionPayloadError::RejectedByExecutionEngine { .. } => true, | ||
| // An honest optimistic node may propagate blocks which are rejected by an EE, do not | ||
| // penalize them. | ||
| ExecutionPayloadError::RejectedByExecutionEngine { .. } => false, |
There was a problem hiding this comment.
I'm a little concerned about this being false when getting blocks over rpc. A malicious peer might intentionally keep sending us invalid payloads without it getting penalized. Does it make sense to have different penalties for gossip and rpc errors?
There was a problem hiding this comment.
We could have different penalties, but I'm not sure how critical it is. Those blocks would have to be signed by a proposer so the attack surface is fairly small. Can a peer just send us the same block again and again via RPC?
There was a problem hiding this comment.
Actually we can't even penalize peers for invalid payloads via RPC since honest, optimistic nodes may do that.
Non-faulty, optimistic nodes may send blocks which result in an INVALID response from an execution engine. To prevent network segregation between optimistic and non-optimistic nodes, transmission of an INVALID execution payload via the Req/Resp domain SHOULD NOT cause a node to be down-scored or disconnected. Transmission of a block which is invalid due to any consensus layer rules (i.e., not execution layer rules) MAY result in down-scoring or disconnection.
There was a problem hiding this comment.
Okay this makes sense. I think we should merge this PR and handle any potential dos issues in sync.
@divagant-martian I'm not completely sure this is a dos vector in block lookups. We should tackle it in a separate PR if it is.
There was a problem hiding this comment.
This is a kinda tricky one.
In principle, we could end up in an infinite loop in sync where we continuously re-download the same blocks from the same peer. its unlikey tho. If a malicious peer sends us this in sync, we should re-download and progress from another peer.
If some peer sends us this in an RPC request, i think its not too bad, we should mark the block as failed and not try again (assuming its an execution failure, not a downloading failure).
Agree the current logic is probably the best. If we see anything happen we may want to add a small penalty to some of these
paulhauner
added
ready-for-merge
|
bors r+ |
## Issue Addressed NA ## Proposed Changes Don't penalize peers for errors that might be caused by an honest optimistic node. ## Additional Info NA
bors
bot
changed the title
## Issue Addressed NA ## Proposed Changes Don't penalize peers for errors that might be caused by an honest optimistic node. ## Additional Info NA
Issue Addressed
NA
Proposed Changes
Don't penalize peers for errors that might be caused by an honest optimistic node.
Additional Info
NA