Include node identity in the P2P advertised client version.

[jimpo]

Fixes #6852.

Geth client versions are "Geth/NAME/VERSION/COMPILER", whereas this PR puts name at the front, like "NAME/Parity/VERSION/COMPILER". This is out of convenience and because I think it makes more sense that way, but I could change it to go after Parity if people prefer.

[parity-cla-bot]

It looks like @jimpo signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[5chdn]

I would love to see this working!

However, we need to check: Isn't the identity parameter used to override the node key? In this case, it should not be broadcasted as NAME. Maybe we should introduce a --node-key.

[jimpo]

@5chdn By node key, do you mean the P2P network authentication key? There already is a separate --node-key flag for that. The help string for --identity is Specify your node's name. (default: ).

[folsen]

We already read the identity flag into NetworkSettings.name but version is set in NetworkConfiguration. Do we have the NetworkSettings available where we actually report the version string, and if so should we use that rather than duplicating the arg to more places?

I'm slightly weary of doing this since I don't know what assumptions have been built around having a specific client_version on NetworkConfiguration. Maybe @arkpar can chime in?

[jimpo]

@folsen NetworkSettings is for RPC settings, whereas NetworkConfiguration is for devp2p, which is why I duplicated the name into both.

Regarding assumptions about client_version, definitely would like some review from someone who can answer that. As far as I can tell, it is only exposed in the Hello packet to peers and RPC endpoints returning peer information.

[andresilva]

LGTM. I'm not very familiar with the --identity flag, had a look and it seems as @jimpo suggested is only used in the Hello packet (after this PR) and in the parity_nodeName RPC. Would like someone else to also have a look.

Regarding "Parity/NAME/.." vs "NAME/Parity/.." I'm not sure about what I prefer but I'm fine with either.

[andresilva]

In #6852 it is claimed that this used to work (apparently with "Parity/NAME/.." format). But I checked out some old v1.7 tags and the code is similar to what we have now, just using version().

[5chdn]

In #6852 it is claimed that this used to work

If that ever used to work it must be way earlier, maybe 1.3

[dvdplm]

It seems like @arkpar s changes are not directly related to this?

I'm having a hard time following along in the code as there are plenty of version variants floating around, but I agree with @jimpo and @andresilva that this will solve the immediate issue.

I'm a little wary of adding more logic (albeit simple) to handle the version string, e.g. it's not obvious that peer_client_version() uses the same info (or does it?).

Ideally the fix should be in version() but that might have implications that I don't see.

[5chdn]

Seems to work.

curl --data '{"method":"parity_nodeName","params":[],"id":1,"jsonrpc":"2.0"}' -H "Content-Type: application/json" -X POST localhost:8545

{"jsonrpc":"2.0","result":"AfriTestsPr#8830-LoremIpsum","id":1}

This is out of convenience and because I think it makes more sense that way, but I could change it to go after Parity if people prefer.

Could you put it after the Parity/? I think this would be more consistent.

had a look and it seems as @jimpo suggested is only used in the Hello packet (after this PR) and in the parity_nodeName RPC.

Should this also be part of the official version string? I.e.,:

Parity/AfriTestsPr#8830-LoremIpsum/v1.12.0-nightly-9bd4bd6bb-20180613/x86_64-linux-gnu/rustc1.26.2

(Maybe limited to 8 chars)

[andresilva]

@5chdn I don't think we should add it to the version string, only peer announces. It's your node but it's our version 😉.

@dvdplm I think we can add a extra: Option<&str> to the version function and then pass the identity when computing client_version. This is what @jimpo was trying to avoid by putting the name as a prefix.

[dvdplm]

add a extra: Option<&str> to the version function

I think this would be wise.

[folsen]

@andresilva AFAICT the main usecase of this is to show your name on ethstats, which would make it necessary to report as version right?

[andresilva]

@folsen I meant more for things like parity -v. The geth nodes in ethstats.net don't seem to be showing any names (maybe could be that there are no nodes registered there with names). The parity nodes show the version string with a "//" e.g. "Parity//v1.8.12-unstable/x86_64-linux-gnu/rustc1.26.2", which is done here when implementing the client_version RPC. If we want to get the name in ethstats we also need to get the identity string into that method.

[jimpo]

@dvdplm peer_client_version returns the version string of the remote peer in a session, not the one we advertise.

I don't think the version() method should return with the same -- it's the version. But I can definitely modify the client_version to put the name after Parity.

[dvdplm]

@jimpo I hear you on the "version is the version" argument, although the same could be said for "client_version".
My general point was this: there's some confusing naming happening in several places – "identity", "node name", "version", "client version", "peer client version" – and even if we don't want to tackle all of that at once, perhaps keeping the logic grouped in one spot is helpful to future code readers.

[5chdn]

@dvdplm @andresilva @folsen needs a 2nd review

[dvdplm]

My grumbles remain but as I don't have anything better to suggest I'm fine with this.