internal/ethapi: restore eth_protocolVersion method

[MariusVanDerWijden]

restores the eth_protocolVersion RPC call which was broken due to a regression
during the snap refactor.

[holiman]

So, for ethereum protocol version this node supports -- does this return 64 (eth protocol version) or 1 (chainId) ?
What is the motivation for having this?

[MariusVanDerWijden]

It returns the lowest eth protocol version that this node supports I guess.
I just restored the functionality from before.
https://eth.wiki/json-rpc/API#eth_protocolversion
It seems to be part of the json-rpc spec so I don't know if we could drop it without changing the spec.
However I had the idea to move this from the eth namespace to the net namespace as it has more to do with the wire protocol and should not really be in the same space as eth_sign or eth_sendTransaction

[holiman]

Hm, I wonder if this is ever actually used... Something that would be more useful, IMO, would be net_caps that would return the protocol capabiities, e.g. ["eth/63", "eth/64", "eth/65", "snap/1"]. I wonder in what situation an RPC caller would ever want to know just the eth version?

Testing on an older rleease (1.9.12):

[user@work go-ethereum]$ ./build/bin/geth --datadir /tmp/foo  --nodiscover --maxpeers 0 console
...
INFO [12-30|13:56:02.676] Initialising Ethereum protocol           versions="[65 64 63]" network=1 dbversion=7
...
> eth.protocolVersion
"0x41"

So it returns 65, which is the preferred eth protocol version.

Since the next release is a big one, I would rather drop this method (and possibly add a net.caps() or net.capabilities() method)

[holiman]

@ryanschneider do you have any metrics from Infura about if the eth_protocolVersion is ever used?

[ryanschneider]

@holiman it is used, way more often than I would've expected to be honest, but it's not clear from the context what the user is trying to do with it. It seems like it's often used in conjunction with web3_clientVersion, so if I had to guess it's being called by one of the standard frameworks at session startup to try to guess which client the framework is speaking to on the backend, but that's pure conjecture on my part.

[ryanschneider]

@holiman as mentioned on Discord it's also used by vipnode to determine the client type, so probably used by other packages for similar functionality: https://github.com/vipnode/vipnode/blob/c90952223f23198d2b6f1f9082f8dd09abd34cc8/ethnode/rpc.go#L133

[holiman]

But...what are they doing..??

	protocol, err := strconv.ParseInt(protocolVersion, 0, 32)
	if err != nil {
		return nil, err
	}
	// FIXME: Can't find any docs on how this protocol value is supposed to be
	// parsed, so just using anecdotal values for now.
	if agent.Kind == Parity && protocol == 1 {
		agent.IsFullNode = false
	} else if agent.Kind == Geth && protocol == 10002 {
		agent.IsFullNode = false
	}

They somehow try to tell whether it's a fullnode, based on the protocol version.. But I don't understand what 10002 is all about. It would make more sense if they had 2 (for les/2) or 3 for (les/3), but ... I have no idea.

I suspect: Whatever they're doing, they're doing it wrong

[ryanschneider]

Ya I agree it looks like it's somewhat experimental/exploratory code, but the main issue was that if the RPC failed then vipnode agent fails to start (I had to cherry pick this PR into my branch to get it to work again). I might try to PR a fix (I don't think Shazow is actively maintaining the project any more though), I'm mainly just mentioning it as a data point that people still use this RPC (for almost definitely incorrect reasons though ;) ).

[ryanschneider]

@holiman since this RPC is definitely being used my suggestion would be to follow the same general course as the --rpc.*flags and deprecate the RPC now but wait until 1.11 to remove it. You can add a sync.Once to log at WARN that it's deprecated on first call of the RPC.

This will give any consumers of this RPC time to refactor without any immediate breakage.

[karalabe]

This was not a regression, it was removed deliberately.

The old API exposed the highest version of eth the node can speak. I don't really see what useful info this can hold:

• Geth speaks 66, 65, 64, and I think maybe even 63 currently. If I return 66, what does that tell a user? Maybe attempting to data mine a Geth version, but even that is weak at best.
• Returning N does not mean that everything below is supported. I.e. We're starting to drop older versions more aggressively, so 66 will still be returned for 63-to-66, 64-to-66, ..., and even only-66 too.
• Returning N does not mean that that is the version used on the wire. Peers negotiate the highest supported version, but if that is 65, 64 or 63, it depends on the capabilities of both peers. Returning 66 does not mean anything when it comes to the wire.
• Upper level users should absolutely not care what the P2P protocol uses. It's something that should be treated as a black box. We also don't expose the devp2p RLPx version and nobody complained, or the discovery version. Things become even murkier with snap because now a full node uses eth/66 and snap/1 in combination, relying on both. So what does it mean to return 66?

I can't really fathom a single use case where this API call is useful for anything.

[karalabe]

Btw, originally the point of this API call was for ethstats client to be able to retrieve and publish the protocol running so that client devs can more easily see what other PoC clients are doing. This was useful pre-Olympic.

[ryanschneider]

I totally get that the RPC doesn't make sense, I'm just advocating for deprecating it and removing it rather than removing it in the same version that's a required upgrade for Berlin since it's unclear who's currently using it and what all removing it might unexpectedly break.

[halasla]

restores the eth_protocolVersion RPC call which was broken due to a regression during the snap refactor.