Make fork id the default and try to recover the DiscoveryPeer for incoming connections from the PeerTable

[pinges]

PR description

This PR introduces these changes:

• Make "--filter-on-enr-fork-id" default to true. That means that Besu by default will request the ENR from peers it has bonded with, which usually contains the fork id. The fork id then is used to find out whether that peer is on the same chain.
• When other peers are initiating a handshake with us we now will check our PeerTable to see whether we have bonded with that peer. If the peer is found in the PeerTable we can check if the fork id is available to find out whether that peer is on the same chain.

Only if the fork id indicates that the peer is on the same chain we will try to establish a connection with that peer.

Currently Besu tries to connect to each peer in the PeerTable every 30 seconds. If a peer responds with an ENR that contains the fork id that peer will only be tried if that fork id passes the check. Checking the fork id at that point will safe Besu from doing a handshake, exchange the HelloMessage, and potentially exchanging status messages with a peer before finding out that the peer is on a different chain. Metrics produced during the testing of these changes indicate that on mainnet about 70-75% of peers are failing the fork id check.

Fixed Issue(s)

#5272

[github-actions]

• I thought about documentation and added the doc-change-required label to this PR if updates are required.
• I thought about the changelog and included a changelog update if required.
• If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

[macfarla]

a few comments and def needs a changelog entry

[macfarla]

I'm happy with this PR but would like @fab-10 or someone else from chupa to take a look before it gets merged

[fab-10]

LGTM, just minor logs opt and a question about the rate of connection attempts

[fab-10]

to avoid doing the computation when debug level is not enabled, in the addArgument you need to use a lambda. I cannot make a suggestion since there are deleted lines, but the code should look like:

.addArgument(status::networkId)
.addArgument(() -> getPeerOrPeerId(peer))

[pinges]

I've updated all the places where we do log information to use lambdas where possible.

[fab-10]

Suggested change

	LOG.atDebug()
	.setMessage("{} has matching network id ({}), but non-matching fork id: {}")
	.addArgument(getPeerOrPeerId(peer))
	.addArgument(networkId)
	.addArgument(forkId)
	.log();
	LOG.atDebug()
	.setMessage("{} has matching network id ({}), but non-matching fork id: {}")
	.addArgument(() -> getPeerOrPeerId(peer))
	.addArgument(networkId)
	.addArgument(forkId)
	.log();

[pinges]

s.a.

[fab-10]

Suggested change

	LOG.atDebug()
	.setMessage("{} has matching network id ({}), but non-matching genesis hash: {}")
	.addArgument(getPeerOrPeerId(peer))
	.addArgument(networkId)
	.addArgument(status.genesisHash())
	.log();
	LOG.atDebug()
	.setMessage("{} has matching network id ({}), but non-matching genesis hash: {}")
	.addArgument(() -> getPeerOrPeerId(peer))
	.addArgument(networkId)
	.addArgument(status::genesisHash)
	.log();

[pinges]

s.a.

[fab-10]

Suggested change

	.addArgument(getPeerOrPeerId(peer))
	.addArgument(() -> getPeerOrPeerId(peer))

[pinges]

s.a.

[fab-10]

Suggested change

	.addArgument(message.getConnection())
	.addArgument(message::getConnection)

[pinges]

s.a.

[fab-10]

If I understood this correctly, this is called every 30 seconds, how many peers can be attempted every time?
Asking to see if we are doing too many attempts, and if there is a need to put some limits, so we try at max tot connections at time.

[pinges]

This PR is reducing the number of peers we are trying to connect to to the ones that have a valid fork id or did not send a fork id.
I agree that we should look into limiting this, e.g. if we do have reached max peers. But that is for another time :-)

[pinges]

I've created a new task to make sure that we can have a look at optimising that!
https://app.zenhub.com/workspaces/team-revenant-5e6accf93892a67e1d7a7f34/issues/gh/hyperledger/besu/6442

[macfarla]

one thing I spotted in the changelog but it doesn't necessarily need to be fixed in your PR

[macfarla]

Suggested change

- The time that can be spent selecting transactions during block creation is not capped at 5 seconds for PoS and PoW networks, and for PoA networks, at 75% of the block period specified in the genesis, this to prevent possible DoS in case a single transaction is taking too long to execute, and to have a stable block production rate, but it could be a breaking change if an existing network used to have transactions that takes more time to executed that the newly introduced limit, if it is mandatory for these network to keep processing these long processing transaction, then the default value of `block-txs-selection-max-time` or `poa-block-txs-selection-max-time` needs to be tuned accordingly.

- The time that can be spent selecting transactions during block creation is now capped at 5 seconds for PoS and PoW networks, and for PoA networks, at 75% of the block period specified in the genesis, this to prevent possible DoS in case a single transaction is taking too long to execute, and to have a stable block production rate, but it could be a breaking change if an existing network used to have transactions that takes more time to executed that the newly introduced limit, if it is mandatory for these network to keep processing these long processing transaction, then the default value of `block-txs-selection-max-time` or `poa-block-txs-selection-max-time` needs to be tuned accordingly.