Support multiple all eth protocol versions announced in HelloMessage

[mattsse]

Describe the feature

as highlighted here:
https://t.me/paradigm_reth/1611

we're announcing support for eth66/67 by default in Hello but only check for a single version in the eth Status handshake.

reth/crates/net/eth-wire/src/ethstream.rs

Lines 101 to 106 in e0dbcae

	if status.version != resp.version {
	return Err(EthHandshakeError::MismatchedProtocolVersion {
	expected: status.version,
	got: resp.version,
	}
	.into())

the status message should also accept older versions if announced.

TODO

if we add this to Status directly, we can no longer derive Rlp
so perhaps we add a list of versions as a separate parameter to the handshake function

wdyt @Rjected

Additional context

No response

[Rjected]

I think this is because we always use EthVersion::Eth67 in the Default impl of Status:

reth/crates/net/eth-wire/src/types/status.rs

Lines 152 to 158 in e0dbcae

	impl Default for Status {
	fn default() -> Self {
	Status {
	version: EthVersion::Eth67 as u8,
	chain: Chain::Named(ethers_core::types::Chain::Mainnet),
	total_difficulty: U256::from(17_179_869_184u64),
	blockhash: MAINNET.genesis_hash(),

Instead, this should be selected depending on the highest version shared eth capability. Ideally this would be fully solved by #791 in a way that lets us preserve the current behavior, but for now, adding a parameter in handshake seems like the best solution

[mattsse]

Instead, this should be selected depending on the highest version shared eth capability.

for the handshake this should check that the version of the remote peer's Status message is supported.

[Rjected]

for the handshake this should check that the version of the remote peer's Status message is supported.

right, yeah

[jinsankim]

I'd like to take this one.

[jinsankim]

Question: Do we need to send EthMessage per a list of versions? I think we should do but I don't have confidence.

reth/crates/net/eth-wire/src/ethstream.rs

Lines 59 to 64 in a588f79

	// we need to encode and decode here on our own because we don't have an `EthStream` yet
	// The max length for a status with TTD is: <msg id = 1 byte> + <rlp(status) = 88 byte>
	let mut our_status_bytes = BytesMut::with_capacity(1 + 88);
	ProtocolMessage::from(EthMessage::Status(status)).encode(&mut our_status_bytes);
	let our_status_bytes = our_status_bytes.freeze();
	self.inner.send(our_status_bytes).await?;

[onbjerg]

Thanks! I've assigned you to the issue. If you feel stuck, open up a draft PR with your questions and someone will help you. If you are prevented from solving the issue, let me know and I will unassign you 😄

As for your question, I will defer to @mattsse and @Rjected

[gakonst]

A few questions:

1. Does Geth announce support for multiple protocol versions too? Looks like yes, incl eth/68.
2. Their handshake does not seem to take a protocol version, but the peer does have a negotiated version
3. And it seems like they try to do a handshake for each version

So is the idea that since we don't support multiple protocols yet, we'd just "loosen" the check from "if our version is not the same as theirs" to "if our version is eth/67 and their version is eth/66 or eth/67 we still mark it as an OK handshake"?

(cc @Rjected @mattsse a bit confused)

[Rjected]

So is the idea that since we don't support multiple protocols yet, we'd just "loosen" the check from "if our version is not the same as theirs" to "if our version is eth/67 and their version is eth/66 or eth/67 we still mark it as an OK handshake"?

yes, that's the short term fix, and the long term fix is supporting multiple protocol versions, which will look a lot like how geth does it.

[gakonst]

But should we be announcing eth/66? We're not responding to GetNodeData for example, so won't that get us penalized?

Should we just not advertise support for eth/66, and properly do the multiple caps? Most of the network is on latest geth anyway, so they all have eth/67, I doubt that we'd lose a lot of peers that way?

[Rjected]

Should we just not advertise support for eth/66, and properly do the multiple caps? Most of the network is on latest geth anyway, so they all have eth/67, I doubt that we'd lose a lot of peers that way?

After some research, I'm leaning much more toward implementing support for multiple capabilities, and don't think peers with eth/66, but not eth/67 are a significant share of the network. Looking at ethernodes, the most used version of geth is 1.10.25. The only older version with significant network share, 1.10.23, is at about 6%.

eth/67 was implemented in ethereum/go-ethereum#24093, around June of 2022, for 1.10.19. The PR even notes the following:

(3) OpenEthereum and Erigon stopped serving fast sync data already even at the expense of a protocol violation, eth67 is just catching up with the general direction

So most nodes should support eth/67

[gakonst]

Yeah, so I think let's just stop advertising eth/66, and when we can prioritize, go for the multiple caps?

[jinsankim]

Need feedback.

Alternative idea to original issue, passing a list of versions to handshake:

• Could we overwrite SharedCapability::Eth.version, from UnauthedP2PStream.handshake(), to status.version?

Rationale to my alternative idea:

• geth negotiates eth protocol and it selects highest version.
  https://github.com/ethereum/go-ethereum/blob/d0a4989a8def7e6bad182d1513e8d4a093c1672d/cmd/devp2p/internal/ethtest/helpers.go#L130-L132
• We also already have the same logic at
  

  reth/crates/net/eth-wire/src/p2pstream.rs

  Lines 562 to 567 in a55e7fd

  	if version.is_none() ||
  	(version.is_some() && peer_capability.version > *version.expect("is some; qed"))
  	{
  	shared_capabilities.insert(peer_capability.name.clone(), peer_capability.version);
  	shared_capability_names.insert(peer_capability.name);
  	}

HDYT?