Our metrics related to dial errors were off. The original error was not
wrapped, so the caller function had no chance of picking it up.
Therefore the most common error, which is "TooManyPeers", was not
correctly counted.
The metrics were originally introduced in
https://github.com/ethereum/go-ethereum/pull/27621
I was thinking of various possible solutions.
- the one proposed here wraps both the new error and the origial error.
It is not a pattern we use in other parts of the code, but works. This
is maybe the smallest possible change.
- as an alternate, I could write a proper `errProtoHandshakeError` with
it's own wrapped error
- finally, I'm not even sure we need `errProtoHandshakeError`, maybe we
could just pass up the original error.
---------
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
As of now, Geth disconnects peers only on protocol error or timeout,
meaning once connection slots are filled, the peerset is largely fixed.
As mentioned in https://github.com/ethereum/go-ethereum/issues/31321,
Geth should occasionally disconnect peers to ensure some churn.
What/when to disconnect could depend on:
- the state of geth (e.g. sync or not)
- current number of peers
- peer level metrics
This PR adds a very slow churn using a random drop.
---------
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
Our previous success metrics gave success even if a peer disconnected
right after connection. These metrics only count peers that stayed
connected for at least 1 min. The 1 min limit is an arbitrary choice. We do
not use this for decision logic, only statistics.
Make UPnP more robust
- Once a random port was mapped, we try to stick to it even if a UPnP
refresh fails. Previously we were immediately moving back to try the
default port, leading to frequent ENR changes.
- We were deleting port mappings before refresh as a possible
workaround. This created issues in some UPnP servers. The UPnP (and PMP)
specification is explicit about the refresh requirements, and delete is
clearly not needed (see
https://github.com/ethereum/go-ethereum/pull/30265#issuecomment-2766987859).
From now on we only delete when closing.
- We were trying to add port mappings only once, and then moved on to
random ports. Now we insist a bit more, so that a simple failed request
won't lead to ENR changes.
Fixes https://github.com/ethereum/go-ethereum/issues/31418
---------
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
Here we are modifying the port mapping logic so that existing port
mappings will only be removed when they were previously created by geth.
The AddAnyPortMapping functionality has been adapted to work consistently
between the IGDv1 and IGDv2 backends.
This is for the implementation of Portal Network in the Shisui client.
Their handler needs access to the node object in order to send further
calls to the requesting node. This is a breaking API change but it
should be fine, since there are basically no known users of TALKREQ
outside of Portal network.
---------
Signed-off-by: thinkAfCod <q315xia@163.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
When resending the WHOAREYOU packet, a new nonce and random IV should not
be generated. The sent packet needs to match the previously-sent one exactly
in order to make the handshake retry work.
---------
Co-authored-by: Felix Lange <fjl@twurst.com>
It introduces a new variable to store the external port returned by the
addAnyPortMapping function and ensures that the correct external port is
returned even in case of an error.
---------
Co-authored-by: Felix Lange <fjl@twurst.com>
This fixes the handshake in a scenario where the remote end sends two unknown
packets in a row. When this happens, we would previously respond to both with
a WHOAREYOU challenge, but keep only the latest sent challenge. Transmission is
assumed to be unreliable, so any client that sends two request packets simultaneously
has to be prepared to follow up on whichever request leads to a handshake. With
this fix, we force them to do the handshake that we can actually complete.
Fixes#30581
Fixes#31093
Here we add some API functions on the UDPv5 object for the purpose of implementing
the Portal Network JSON-RPC API in the shisui client.
---------
Signed-off-by: Chen Kai <281165273grape@gmail.com>
This is a not-particularly-important "cleanliness" PR. It removes the
last remnants of the `x/exp` package, where we used the `maps.Keys`
function.
The original returned the keys in a slice, but when it became 'native'
the signature changed to return an iterator, so the new idiom is
`slices.Collect(maps.Keys(theMap))`, unless of course the raw iterator
can be used instead.
In some cases, where we previously collect into slice and then sort, we
can now instead do `slices.SortXX` on the iterator instead, making the
code a bit more concise.
This PR might be _slighly_ less optimal, because the original `x/exp`
implementation allocated the slice at the correct size off the bat,
which I suppose the new code won't.
Putting it up for discussion.
---------
Co-authored-by: Felix Lange <fjl@twurst.com>
I ran into this while trying to debug a discv5 thing. I tried to disable
DNS discovery using `--discovery.dns=false`, which doesn't work.
Annoyingly, geth started anyway and discarded the error silently. I
eventually found my mistake, but it took way longer than it should have.
Also including a small change to the error message for invalid DNS URLs
here. The user actually needs to see the URL to make sense of the error.
The test occasionally fails when network connectivity is bad or if it
hits the wrong server. We usually don't add tests with external network
dependency so I'm removing them.
Fixes#31220
This implements a basic mechanism to query the node's external IP using
a STUN server. There is a built-in list of public STUN servers for convenience.
The new detection mechanism must be selected explicitly using `--nat=stun`
and is not enabled by default in Geth.
Fixes#30881
---------
Co-authored-by: Felix Lange <fjl@twurst.com>
This is an alternative for #27407 with a solution based on gencodec.
With the PR, one can now configure like this:
```
# config.toml
[Node.P2P]
NAT = "extip:33.33.33.33"
```
```shell
$ geth --config config.toml
...
INFO [01-17|16:37:31.436] Started P2P networking self=enode://2290...ab@33.33.33.33:30303
```
Closes#23210
# Context
When deploying Geth in Kubernetes with ReplicaSets, we encountered two
DNS-related issues affecting node connectivity. First, during startup,
Geth tries to resolve DNS names for static nodes too early in the config
unmarshaling phase. If peer nodes aren't ready yet (which is common in
Kubernetes rolling deployments), this causes an immediate failure:
```
INFO [11-26|10:03:42.816] Starting Geth on Ethereum mainnet...
INFO [11-26|10:03:42.817] Bumping default cache on mainnet provided=1024 updated=4096
Fatal: config.toml, line 81: (p2p.Config.StaticNodes) lookup idontexist.geth.node: no such host
```
The second issue comes up when pods get rescheduled to different nodes -
their IPs change but peers keep using the initially resolved IP, never
updating the DNS mapping.
This PR adds proper DNS support for enode:// URLs by deferring resolution
to connection time. It also handles DNS failures gracefully instead of failing
fatally during startup, making it work better in container environments where
IPs are dynamic and peers come and go during rollouts.
---------
Co-authored-by: Felix Lange <fjl@twurst.com>
This fixes an issue where the disconnect message was not wrapped in a list.
The specification requires it to be a list like any other message.
In order to remain compatible with legacy geth versions, we now accept both
encodings when parsing a disconnect message.
---------
Co-authored-by: Felix Lange <fjl@twurst.com>
This PR modifies how the metrics library handles `Enabled`: previously,
the package `init` decided whether to serve real metrics or just
dummy-types.
This has several drawbacks:
- During pkg init, we need to determine whether metrics are enabled or
not. So we first hacked in a check if certain geth-specific
commandline-flags were enabled. Then we added a similar check for
geth-env-vars. Then we almost added a very elaborate check for
toml-config-file, plus toml parsing.
- Using "real" types and dummy types interchangeably means that
everything is hidden behind interfaces. This has a performance penalty,
and also it just adds a lot of code.
This PR removes the interface stuff, uses concrete types, and allows for
the setting of Enabled to happen later. It is still assumed that
`metrics.Enable()` is invoked early on.
The somewhat 'heavy' operations, such as ticking meters and exp-decay,
now checks the enable-flag to prevent resource leak.
The change may be large, but it's mostly pretty trivial, and from the
last time I gutted the metrics, I ensured that we have fairly good test
coverage.
---------
Co-authored-by: Felix Lange <fjl@twurst.com>
Changelog: https://golangci-lint.run/product/changelog/#1610
Removes `exportloopref` (no longer needed), replaces it with
`copyloopvar` which is basically the opposite.
Also adds:
- `durationcheck`
- `gocheckcompilerdirectives`
- `reassign`
- `mirror`
- `tenv`
---------
Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de>
This PR fixes two tests, which had a tendency to sometimes write to the `*testing.T` `log` facility after the test function had completed, which is not allowed. This PR fixes it by using waitgroups to ensure that the handler/logwriter terminates before the test exits.
closes#30505
`WriteToUDP` was never called, since `meteredUdpConn` exposed directly
all the methods from the underlying `UDPConn` interface.
This fixes the `discover/egress` metric never being updated.
## Issue
If `nextTime` has passed, but all nodes are excluded, `get` would return
`nil` and `run` would therefore not invoke `schedule`. Then, we schedule
a timer for the past, as neither `nextTime` value has been updated. This
creates a busy loop, as the timer immediately returns.
## Fix
With this PR, revalidation will be also rescheduled when all nodes are
excluded.
---------
Co-authored-by: lightclient <lightclient@protonmail.com>
The test specifies `ListenAddr: ":0"`, which means a random ephemeral
port will be chosen for the TCP listener by the OS. Additionally, since
no `DiscAddr` was specified, the same port that is chosen automatically
by the OS will also be used for the UDP listener in the discovery UDP
setup. This sometimes leads to test failures if the TCP listener picks a
free TCP port that is already taken for UDP. By specifying `DiscAddr:
":0"`, the UDP port will be chosen independently from the TCP port,
fixing the random failure.
See issue #29830.
Verified using
```
cd p2p
go test -c -race
stress ./p2p.test -test.run=TestServerPortMapping
...
5m0s: 4556 runs so far, 0 failures
```
The issue described above can technically lead to sporadic failures on
systems that specify a listen address via the `--port` flag of 0 while
not setting `--discovery.port`. Since the default is using port `30303`
and using a random ephemeral port is likely not used much to begin with,
not addressing the root cause might be acceptable.
enode.Node was recently changed to store a cache of endpoint information. The IP address in the cache is a netip.Addr. I chose that type over net.IP because it is just better. netip.Addr is meant to be used as a value type. Copying it does not allocate, it can be compared with ==, and can be used as a map key.
This PR changes most uses of Node.IP() into Node.IPAddr(), which returns the cached value directly without allocating.
While there are still some public APIs left where net.IP is used, I have converted all code used internally by p2p/discover to the new types. So this does change some public Go API, but hopefully not APIs any external code actually uses.
There weren't supposed to be any semantic differences resulting from this refactoring, however it does introduce one: In package p2p/netutil we treated the 0.0.0.0/8 network (addresses 0.x.y.z) as LAN, but netip.Addr.IsPrivate() doesn't. The treatment of this particular IP address range is controversial, with some software supporting it and others not. IANA lists it as special-purpose and invalid as a destination for a long time, so I don't know why I put it into the LAN list. It has now been marked as special in p2p/netutil as well.
Here we clean up internal uses of type discover.node, converting most code to use
enode.Node instead. The discover.node type used to be the canonical representation of
network hosts before ENR was introduced. Most code worked with *node to avoid conversions
when interacting with Table methods. Since *node also contains internal state of Table and
is a mutable type, using *node outside of Table code is prone to data races. It's also
cleaner not having to wrap/unwrap *enode.Node all the time.
discover.node has been renamed to tableNode to clarify its purpose.
While here, we also change most uses of net.UDPAddr into netip.AddrPort. While this is
technically a separate refactoring from the *node -> *enode.Node change, it is more
convenient because *enode.Node handles IP addresses as netip.Addr. The switch to package
netip in discovery would've happened very soon anyway.
The change to netip.AddrPort stops at certain interface points. For example, since package
p2p/netutil has not been converted to use netip.Addr yet, we still have to convert to
net.IP/net.UDPAddr in a few places.
