Use From field in a PING packet when creating a peer table entry

[matthew1001]

PR description

This PR uses the From field of a PING packet when creating a local entry in the peer table, unless the From field is not available in which case it reverts to the existing behaviour of using the UDP source address.

Since (as far as I can tell) all PING packets include a From field (with 127.0.0.1 in the case where the user hasn't specified a custom value) the PR is coded to ignore a From field of 127.0.0.1 and use the UDP source address instead. The reality is that on a localhost system the UDP source will be 127.0.0.1 anyway but it provides a little extra protection from this change/fix causing issues on existing systems that might be working just fine using the UDP source address in peer's enode URL. The main aim of this fix is that if something has specified a custom value it is honoured by the PING recipient.

Fixed Issue(s)

Fixes #6224

[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

[matthew1001]

@fab-10 would you be happy to put a review on the PR?

[fab-10]

LGTM, just want to have an opinion from @macfarla @pinges

[matthew1001]

@macfarla , @pinges - would one of you be able to take a look at this as well to see what you think?

[pinges]

It would be nice to understand what the specification says about this field in the PING message.

[matthew1001]

The devp2p discv4 docs for PING (https://github.com/ethereum/devp2p/blob/master/discv4.md#ping-packet-0x01) seem to only say that it's a field in the packet:

packet-data = [version, from, to, expiration, enr-seq ...]
version = 4
from = [sender-ip, sender-udp-port, sender-tcp-port]
to = [recipient-ip, recipient-udp-port, 0]

and that the recipient should send a PONG packet in reply, without being specific about where to reply to:

When a ping packet is received, the recipient should reply with a [Pong](https://github.com/ethereum/devp2p/blob/master/discv4.md#pong-packet-0x02) packet. It may also consider the sender for addition into the local table. Implementations should ignore any mismatches in version.

The docs for PONG (https://github.com/ethereum/devp2p/blob/master/discv4.md#pong-packet-0x02) don't seem to say much about where a PONG packet has to reply to either:

Pong is the reply to ping.

ping-hash should be equal to hash of the corresponding ping packet. Implementations should ignore unsolicited pong packets that do not contain the hash of the most recent ping packet.

The enr-seq field is the current ENR sequence number of the sender. This field is optional.

Interestingly the ethereum discovery overview wiki (https://github.com/ethereum/devp2p/wiki/Discovery-Overview#discovery-v4) says this:

Certain specified packet fields such as the "From" field in a Ping are ignored by implementations.

It's not completely clear to me whether that means "must be ignored by" or "are often ignored by"? If it's the case that it should be ignored (and that this PR/issue isn't valid) I'm a bit confused as to the purpose of the --p2p-host option.

[pinges]

@matthew1001 That comment in the spec made me suspicious.
With your change I think we would run in to problems with nodes that do not correctly set the FROM field in the PING message.
Without your change I can imagine that we could run into problems if someone is running a few local nodes (including the bootnode) and wants to add an external node to the chain using that bootnode. The bootnode would send the 127.0.0.1 addresses to the external node, which would not work.
One thing that we could do is to run at least a mainnet node for some time that reports (metric?) how many PING messages do contain a different IP address than the one that we currently use (not counting 127.0.0.1).

[matthew1001]

With your change I think we would run in to problems with nodes that do not correctly set the FROM field in the PING message.

Yes I agree, it's difficult to know if fixing this is going to break existing setups that rely on the current behaviour.

I think running a mainnet node for a period of time like you describe would be a useful way of getting some more data to base a decision on. I'm not sure if I'm able to do that personally (I need to look into what I have available re. a public-facing system) but I think it would be a useful test.

I also wonder if there's any convention within Besu for a "fallback to previous behaviour" env-var or similar (something that doesn't require a first-class config option, but gives any user a way of reverting to the old behaviour if their environment doesn't suit the new behaviour). I think a first-class config option (e.g. something like --p2p-honor-from-addr) is probably too heavy weight just to provide a way of reverting the behaviour, particularly if the consensus that the new behaviour is correct.

Finally, maybe a look at what options Geth & Nethermind offer and what they do in code with that field would be useful input to the discussion?

[diega]

For Nethermind, based on NethermindEth/nethermind...PingMsgSerializer, I think they honor the From field of the message. For go-ethereum, it's a bit more tricky to say b/c the deserialization occurs in ethereum/go-ethereum...v4wire.go but using reflection forgery to match the order in the rlp list with the position of the fields of every struct but, in a nutshell, I think they also use the From from the packet

[matthew1001]

That's really useful context @diega

@pinges - do you think based on that we would be comfortable releasing the change in behaviour without an option to disable/revert to the old behaviour?

[matthew1001]

@pinges, wondered if you had any more thoughts based on the comment from @diega?

[matthew1001]

Merging for now as this would appear to be in keeping with how other clients work.

[matkt]

I see a huge amount of error message after merging main banch today . Is it related to this PR ?

{"@timestamp":"2024-01-19T11:31:39,831","level":"ERROR","thread":"vert.x-eventloop-thread-3","class":"ContextBase","message":"Unhandled exception","throwable":" java.lang.IllegalArgumentException: Invalid ip address.\n\tat org.hyperledger.besu.ethereum.p2p.peers.EnodeURLImpl$Builder.ipAddress(EnodeURLImpl.java:405)\n\tat org.hyperledger.besu.ethereum.p2p.peers.EnodeURLImpl$Builder.ipAddress(EnodeURLImpl.java:380)\n\tat org.hyperledger.besu.ethereum.p2p.discovery.PeerDiscoveryAgent.handleIncomingPacket(PeerDiscoveryAgent.java:312)\n\tat org.hyperledger.besu.ethereum.p2p.discovery.VertxPeerDiscoveryAgent.lambda$handlePacket$10(VertxPeerDiscoveryAgent.java:298)\n\tat io.vertx.core.impl.future.FutureImpl$3.onSuccess(FutureImpl.java:141)\n\tat io.vertx.core.impl.future.FutureBase.lambda$emitSuccess$0(FutureBase.java:54)\n\tat io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:173)\n\tat io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:166)\n\tat io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:470)\n\tat io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:413)\n\tat io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)\n\tat io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)\n\tat io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)\n\tat java.base/java.lang.Thread.run(Thread.java:1583)\n"}

[matthew1001]

Just having a look @matkt. Is this syncing a public node?

[matkt]

yes during checkpoint on mainnet, directly at the beginning

[matthew1001]

Just started a thread on discord if it's easier to discuss on there? https://discord.com/channels/905194001349627914/905205502940696607/1197868266635407410

[matthew1001]

I've raised #6439 which validates the From address as a valid IP before attempting to use it, and reverts to the previous behaviour of taking it from the UDP source IP if it is invalid.

[pinges]

LGTM