[vulnerability] Disconnect outbound peers on the invalid chain

[yixiao5428]

Currently, the outbound peers on incompatible chains may use up outbound connection slots (the ProcessMessage() function in src/net_processing.cpp). If the block header is valid, but the block is known to be invalid, and the peer announces the same block as being on its active chain, the peer should be disconnected.

A possible solution is to check whether the first invalid header in mapBlockIndex is at the end, if not, disconnect the peer (after line 1981 in src/net_processing.cpp).

Similar fix from Bitcoin: bitcoin/bitcoin@37886d5.

Reported by 6004ed5feaa31ae9df36b5dbc60f0fa53255a5fb734334082c6d202405fc738c.

[ReverseControl]

@patricklodder @rnicoll Who is working on this? How can I help?

[ReverseControl]

@patricklodder @rnicoll @michilumin

This fix requires a significant amount of work to implement properly. See Bitcoins, #11490 ( bitcoin/bitcoin@d93fa26 ), where the merge description describes the kind of work I mean. For reference:

e065249 Add unit test for outbound peer eviction (Suhas Daftuar)
5a6d00c Permit disconnection of outbound peers on bad/slow chains (Suhas Daftuar)
c60fd71 Disconnecting from bad outbound peers in IBD (Suhas Daftuar)

Pull request description:

  The first commit will disconnect an outbound peer that serves us a headers chain with
 insufficient work while we're in IBD.

  The second commit introduces a way to disconnect outbound peers whose chains 
fall out of sync with ours:

  For a given outbound peer, we check whether their best known block (which is known
 from the blocks they announce to us) has  at least as much work as our tip.  If it doesn't, 
we set a 20 minute timeout, and if we still haven't heard about a block with as much work
 as our tip had when we set the timeout, then we send a single getheaders message, and
 wait 2 more minutes.  If after two  minutes their best known block has insufficient work,
 we disconnect that peer.

 We protect 4 of our outbound peers (who provide some "good" headers chains, ie a chain 
with at least as much work as our tip at some point) from being subject to this logic, to prevent 
excessive network topology changes as a result of this algorithm, while still ensuring that we have 
a reasonable number of nodes not known to be on bogus chains.

  We also don't require our peers to be on the same chain as us, to prevent accidental 
partitioning of the network in the event of a  chain split.  Note that if our peers are ever 
on a more work chain than our tip, then we will download and validate it, and then either 
reorg to it, or learn of a consensus incompatibility with that peer and disconnect.  This PR
 is designed to protect against peers that are on a less work chain which we may never try
 to download and validate.


Tree-SHA512: 2e0169a1dd8a7fb95980573ac4a201924bffdd724c19afcab5efcef076fdbe1f2cec7dc5f5d7e0a6327216f56d3828884f73642e00c8534b56ec2bb4c854a656

Subsequent to that, we will need to implement #11568 (bitcoin/bitcoin@4637f18), and then the mentioned above commit.

I tried implementing only the suggest fix in the commit, but substantial changes will need to be made to src/net.h , src/net.cpp, src/net_processing.cpp, and possibly to src/validation.cpp as well --- I cant remember by now, after chasing so many logical nibles of code that needs to included to make that small suggestion above work.

Unless someone's got a subtle idea to make it work, this is all I got for now.

[ReverseControl]

@patricklodder Given #2459, let's move this one as well to 1.14.5.