fix: ban peer that advertises higher PoW than able to provide

[sdbondi]

Description

• Can only transition to HeaderSync if claimed chain metadata is
  advertised
• HeaderSync is now aware of the claimed ChainMetadata
• HeaderSync now assumes (invariant) that all sync peers have
  claimed a higher accumulated PoW and will ban them if the validated
  accumulated difficulty does not reach the claimed acc diff.
• Adds ban condition in determine_sync_status phase, if a peer is not
  able to improve on the local chain strength (because we know that in
  order to be selected for header sync it must have advertised a
  stronger chain)
• Ban if header sync completes but is less than the claimed PoW.
  This is not strictly necessary since they were still able
  to provide a stronger chain as per Nakamoto consensus, but could still
  indicate some malicious intent.
• If sync fails for all peers, the state machine continues rather than
  WAITING. This removes the disruption that false metadata can cause.
• fix select_sync_peers to include peers that claim to provide enough full
  blocks for our pruning horizon (fixes cucumber test)

Motivation and Context

A peer may disrupt a base node by sending false chain metadata. This PR ensures that if the remote peer cannot provide
the claimed chain, it will get banned and the base node will continue in listening state immediately.

How Has This Been Tested?

Ran sync cucumber tests
Manually, by creating a node that inflates it's advertised accumulated Pow by a small amount and checking that the node is banned by the syncing node.
not entirely related: add block sync RPC unit tests

[stringhandler]

Happy with the code for the most part, but I don't like the extra pruned_height check in Listening

[SWvheerden]

This looks good, but the attack is still possible. We need to check each peer to its provided meta_data not just what headers it can provide.

[SWvheerden]

What happens when we are the first node in our peer list to receive a block via sync? Won't we ban all our other peers?
We select our sync peers not only on who sent meta_data to us but also on whom we are connected.
So a peer might have provided accurate metadata and did not cause the activation of sync mode, but it can get banned here because some other peer provided fake metadata.

So I am thinking of this, what happens if a peer sends invalid metadata, but does not respond to the dial request. Your node won't sync to that peer because it thinks it's offline and keep it out of sync, but it will ban the other good nodes?

[sdbondi]

We need to check each peer to its provided meta_data not just what headers it can provide.

Not sure I follow, validation of headers (PoW) is the only way to determine if the peer is honest about its metadata claim

We select our sync peers not only on who sent meta_data to us but also on whom we are connected.

Good point, which is why I made this no longer the case. In order for the bn to enter the HeaderSync state, the peer MUST have made a claim to have a better block. This is an invariant of the HeaderSync state. So basically, I removed the code that dials all connected peers and now leave it up to the listening state to provide peers that have made a better metadata claim.

So I am thinking of this, what happens if a peer sends invalid metadata, but does not respond to the dial request.

Another good point. However, a peer is already connected when they make a metadata claim. A peer could time it to disconnect, which, as you say, would cause the header sync to fail. So to combat this, if a header sync fails the state machine immediately continues to listening state. This should cause no disruption (that is, a component listening for state events SHOULD NOT disrupted by the node entering the HeaderSync state - e.g mining should continue unless the chain has been confirmed to be behind)

EDIT: I believe the WAITING state after a failed header sync is the primary cause of disruptions, the change here is to follow a "all peers couldn't live up to their promises" header sync with going straight back to is_synced = true listening state as though nothing happened (and ban the peers that were naughty)

it will ban the other good nodes?

No it will only ban the node that was unable to back up its claim, due to the change that a claim has to be made to be included in HeaderSync sync_peers list.

[sdbondi]

I'm not too sure of the behaviour of upstream components (miner, maybe mempool, that's it?). I will look into and run a test on Sunday, but AFAIK it will not interrupt mining. The miner will only not begin if is_bootstrapped is false but after that will always continue on - will run a test to be sure, could be wrong.

EDIT: if necessary, another flag could be added is_synced that is set to false when the headers have proven a stronger PoW
EDIT2: Another potential change would be for header sync to select the active connection, rather than using dial_peer which will dial if necessary, which should never be - but I don't think this can be called an attack if there is no disruption caused.

[SWvheerden]

Awesome, then we cover this.

[sdbondi]

Pruned mode network only passed locally, seems flakey now? Will check

EDIT: passed every time 🤷 - rerunning on CI

[SWvheerden]

LGTM

[stringhandler]

Man this PR is getting stuck in the tumbler. I am going to merge it since the CI seems to have broken elsewhere