fix: ban header sync peer if no headers provided

[sdbondi]

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

[SWvheerden]

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

[sdbondi]

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.

[SWvheerden]

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:

 [c::bn::header_sync] WARN  Invalid remote peer behaviour: Remote peer 1790934e3b56607a2f1fc281f6 advertised a better chain but did not return headers
[c::bn::header_sync] WARN  Banned sync peer because Peer misbehaviour: Invalid remote peer behaviour: Remote peer 1790934e3b56607a2f1fc281f6 advertised a better chain but did not return headers

[sdbondi]

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