fix(discv5): get fork_id from Enr for all network stacks

[mablr]

Fix #18924

[mattsse]

lgtm

pending @emhane

[emhane]

@mablr could you pls find out if op-geth adheres to specs and sets the fork id key as 'opel' or if it still sets 'eth'? otherwise it may have trouble to connect to the bootnodes.

possibly we can just assume not every node will set it correctly and relax the passing criteria as if key = NetworkStackId::ETH or NetworkStackId::OP...just to be sure we don't disconnect the network

[mablr]

@emhane That's right, op-geth still sets the fork id key to "eth".
https://github.com/ethereum-optimism/op-geth/blob/ded9c88e417b1bc196ae1e6475078952dcd1cd53/eth/protocols/eth/discovery.go#L34-L37

On the side on reth I see you created a MustNotIncludeKeys filter, and it is configured to exclude ETH2 keys

reth/crates/net/discv5/src/config.rs

Lines 246 to 247 in 169a1fb

	let discovered_peer_filter = discovered_peer_filter
	.unwrap_or_else(|| MustNotIncludeKeys::new(&[NetworkStackId::ETH2]));

so according to what I understand the rest of keys are accepted.

Tested locally by spawning a op-geth and reth instances :

./build/bin/geth --discv5 --port 30305 --discovery.port 9202

and apparently reth accepts op-geth peer

[emhane]

thanks @mablr ! ETH is EL and ETH2 is CL.

ok so based on op-geth behaviour, let's do this
here

reth/crates/net/discv5/src/lib.rs

Lines 387 to 391 in 856ad08

	let Some(key) = self.fork_key else { return Err(Error::NetworkStackIdNotConfigured) };
	let fork_id = enr
	.get_decodable::<EnrForkIdEntry>(key)
	.ok_or(Error::ForkMissing(key))?
	.map(Into::into)?;

we want to do smthg like this

        let Some(key) = self.fork_key else { return Err(Error::NetworkStackIdNotConfigured) };
        let fork_id = enr
            .get_decodable::<EnrForkIdEntry>(key)
            .ok_or(enr
                .get_decodable::<EnrForkIdEntry>(NetworkStackId::ETH)
                .ok_or(Error::ForkMissing(key)?
            )?
            .map(Into::into)?;

the syntax in my example is a bit off but I think you get the point? would you mind implementing it? would be good if we had a trace log for the case when the configured key isn't found. we will also need to change the error message for Error::ForkMissing(key) to say that neither {key} nor 'eth' were found among the ENR kv-pairs. need to double check also that metrics are update correctly so they don't increment the count for e.g. 'opel' hits when actually it just found the 'eth' key.

[mablr]

Thank you very much @emhane for the pointers.

The metrics are incremented based on peer's Enr, I guess this is ok:

reth/crates/net/discv5/src/metrics.rs

Lines 125 to 137 in 169a1fb

	pub fn increment_once_by_network_type(&self, enr: &discv5::Enr) {
	if enr.get_raw_rlp(NetworkStackId::OPEL).is_some() {
	self.opel.increment(1u64)
	}
	if enr.get_raw_rlp(NetworkStackId::OPSTACK).is_some() {
	self.opstack.increment(1u64)
	}
	if enr.get_raw_rlp(NetworkStackId::ETH).is_some() {
	self.eth.increment(1u64)
	}
	if enr.get_raw_rlp(NetworkStackId::ETH2).is_some() {
	self.eth2.increment(1u64)
	}

[emhane]

nice! nitpicks remaining

[emhane]

Suggested change

	trace!(target: "net::discv5", "Fork id not found for key, trying 'eth'...");
	trace!(target: "net::discv5",
	key = String::from_utf8_lossy(key),
	"Fork id not found for key, trying 'eth'..."
	);

excuse me if wasn't clear, we want to keep the key kv-pair like it was!

[mablr]

no problem, I reverted it. Assuming decoding here is useful.

[emhane]

check out how it looks with the message and then the kv-pairs