-
Notifications
You must be signed in to change notification settings - Fork 220
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
fix: ban peer when Merkle roots mismatch #3162
fix: ban peer when Merkle roots mismatch #3162
Conversation
b8fc1a6
to
6fd94a7
Compare
6fd94a7
to
39ddd33
Compare
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.
LGTM
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.
Looks good, this was much needed. Just one question
if self.config.sync_peers.contains(&node_id) { | ||
debug!( | ||
target: LOG_TARGET, | ||
"Not banning peer that is allowlisted for sync. Ban reason = {}", reason | ||
); | ||
return Ok(()); | ||
} |
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.
Should this logic rather not live in the connectivity manager? Here we don't ban a peer because it is allow listed, but I don't think we have the same logic with propagation, there that peer will get banned.
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.
This is the force sync peer setting specific to sync, not the seed peers. By default it's empty. So seed peers can get banned, but if you explicitly say "I want to force a sync only from peer X" we never ban them.
Waiting for approval before queuing |
PR queued successfully. Your position in queue is: 2 |
PR is on top of the queue now |
Description
Ban peer if block validation fails during block body sync
Motivation and Context
When this error is encountered, the peer sent an invalid block body given the current header chain.
DEBUG Block sync failed: Block validation failed: Block validation error: Mismatched MMR roots
The node should ban the peer. Previously, the node would encounter this, then retry the same peer,
rinse and repeat - preventing the node from syncing.
How Has This Been Tested?
Not explicitly tested - fairly basic change.
Checklist:
development
branch.