fix: ban header sync peer if no headers provided #3297
Conversation
A peer can advertise a higher chain metadata and then return no headers. This case is likely mallicious. In this PR, this is handled by banning the peer allowing the node to attempt a sync with another peer.
There was a problem hiding this comment.
This does look like it fixes the one bug I found that could break the network: #3277
We need some more changes here.
You added if headers.is_empty() { return err
but line 411 also contains a if headers.is_empty() { which makes that unreachable now.
I am more of the opinion we should change find_chain_split to return the first header you both have, then you can check that that is valid and is the same the peer advertised.
Is there another reason that can happen that a peer can return 0 headers
|
You could be right, I did rush this one a bit. We should not be syncing from a peer that has the same height as us. Listening should ensure that, but perhaps we should do a get_chain_metadata RPC call before syncing - I don't think that sending the current shared tip header has any value, they must send a valid header that links to the fork tip in any case. |
|
Yeah, I checked this, this PR broke header sync. If you are exactly 1000 blocks behind, you ban the peer and don't sync. You get this if you are 1000 blocks behind: |
|
Thats not great - need to bring back tests for this protocol. That seems like a bug on the sending side because the find_chain_split call should return RPC not found rather than returning no headers. the empty header case (below this change) was either indicating that we are ahead of the peer or in sync already, so we should not have used the peer in the first place. edit: // in BlockchainDatabase::find_headers_after_hash
if count == 0 {
return Ok(Some((i, Vec::new())));
}edit2: ^ this is not the issue |
Description --- This reverts commit 570e222. Motivation and Context --- #3297 (comment) Having thought about it a bit - header sync is designed to be self contained. It does not have the information required to ban a peer that "lied" in listening mode. Suggest that we pass through the chain metadata and peer that 'kicked' the base node into sync into header sync so that the peer can be banned if it fails to follow through. We should also not prevent mining when in header sync until a split and better chain have been comfirmed by header sync. How Has This Been Tested? --- Existing tests pass
Description --- Adds ban in header sync if peer has indicated that they have a better chain but is unable to provide headers Motivation and Context --- A peer can advertise a higher chain metadata and then return no headers. This case is likely malicious. In this PR, this is handled by banning the peer allowing the node to attempt a sync with another peer. How Has This Been Tested? --- Manually
…3297) (tari-project#3306) Description --- This reverts commit 570e222. Motivation and Context --- tari-project#3297 (comment) Having thought about it a bit - header sync is designed to be self contained. It does not have the information required to ban a peer that "lied" in listening mode. Suggest that we pass through the chain metadata and peer that 'kicked' the base node into sync into header sync so that the peer can be banned if it fails to follow through. We should also not prevent mining when in header sync until a split and better chain have been comfirmed by header sync. How Has This Been Tested? --- Existing tests pass
Description
Adds ban in header sync if peer has indicated that they have a better chain but is unable to provide headers
Motivation and Context
A peer can advertise a higher chain metadata and then return no
headers. This case is likely malicious. In this PR, this is handled
by banning the peer allowing the node to attempt a sync with another peer.
How Has This Been Tested?
Manually