nimbus-eth2
nimbus-eth2 copied to clipboard
Fix REST /eth/v1/node/identity should return proper MultiAddresses which is not AnyAddress4/6.
This should fix https://github.com/status-im/nimbus-eth2/issues/6060
Unit Test Results
9 files ±0 1 115 suites ±0 30m 57s :stopwatch: + 3m 44s 4 244 tests ±0 3 897 :heavy_check_mark: ±0 347 :zzz: ±0 0 :x: ±0 16 926 runs ±0 16 528 :heavy_check_mark: ±0 398 :zzz: ±0 0 :x: ±0
Results for commit 56f10979. ± Comparison against base commit 0fa363e0.
:recycle: This comment has been updated with latest results.
This looks off in that this identity must match what both libp2p and discv5 uses - the rest API should just be a reflection of what libp2p identify reports and what we send to other peers in our ENR
identify
protocol is not enabled in nimbus
and not enabled in lighthouse
. And identity is actually match what both libp2p and discv5 using. AnyAddress
just got expanded explicitly.
Also all the corresponding information is obtained via libp2p and discovery5 interfaces. So previously we had
{
"p2p_addresses": [
"/ip4/0.0.0.0/tcp/9011/p2p/16Uiu2HAmQEmCoEfKQ7ZdPyw1vkMYPLfosfksRUJwssvyDzAosGgm"
],
"discovery_addresses": [
"/ip4/185.181.230.77/udp/9011/p2p/16Uiu2HAmQEmCoEfKQ7ZdPyw1vkMYPLfosfksRUJwssvyDzAosGgm"
]
}
Now we will have
{
"p2p_addresses": [
"/ip4/127.0.0.1/tcp/9011/p2p/16Uiu2HAmQEmCoEfKQ7ZdPyw1vkMYPLfosfksRUJwssvyDzAosGgm",
"/ip4/192.168.0.2/tcp/9011/p2p/16Uiu2HAmQEmCoEfKQ7ZdPyw1vkMYPLfosfksRUJwssvyDzAosGgm",
],
"discovery_addresses": [
"/ip4/185.181.230.77/udp/9011/p2p/16Uiu2HAmQEmCoEfKQ7ZdPyw1vkMYPLfosfksRUJwssvyDzAosGgm",
"/ip4/127.0.0.1/udp/9011/p2p/16Uiu2HAmQEmCoEfKQ7ZdPyw1vkMYPLfosfksRUJwssvyDzAosGgm",
"/ip4/192.168.0.2/udp/9011/p2p/16Uiu2HAmQEmCoEfKQ7ZdPyw1vkMYPLfosfksRUJwssvyDzAosGgm"
]
}
https://github.com/vacp2p/nim-libp2p/blob/03f67d3db5355fad9bac90d458ce7d01870bfd2e/libp2p/switch.nim#L218 - we perform identity requests as part of connection upgrade:
DBG 2024-03-29 00:04:01.317+01:00 identify: decoded message topics="libp2p identify" conn=16U*QAxRx2:6605f760bf19a48fcaad265f pubkey=some(s...0810)) addresses=/ip4/0.0.0.0/tcp/9001 protocols=/ipfs/id/1.0.0,/meshsub/1.1.0,/eth2/beacon_chain/req/status/1/ssz_snappy,/eth2/beacon_chain/req/ping/1/ssz_snappy,/eth2/beacon_chain/req/metadata/1/ssz_snappy,/eth2/beacon_chain/req/metadata/2/ssz_snappy,/eth2/beacon_chain/req/goodbye/1/ssz_snappy,/eth2/beacon_chain/req/beacon_blocks_by_range/2/ssz_snappy,/eth2/beacon_chain/req/beacon_blocks_by_root/2/ssz_snappy,/eth2/beacon_chain/req/blob_sidecars_by_root/1/ssz_snappy,/eth2/beacon_chain/req/blob_sidecars_by_range/1/ssz_snappy,/eth2/beacon_chain/req/light_client_bootstrap/1/ssz_snappy,/eth2/beacon_chain/req/light_client_updates_by_range/1/ssz_snappy,/eth2/beacon_chain/req/light_client_finality_update/1/ssz_snappy,/eth2/beacon_chain/req/light_client_optimistic_update/1/ssz_snappy observable_address=some(/ip4/82.220.110.199/tcp/12274) proto_version=ipfs/0.1.0 agent_version=nimbus signedPeerRecord=None
Notably, it looks like we perform it the wrong way given how addresses
is formed here
So what is doing current PR.
For p2p_addresses
.
- Expands all the addresses which are
0.0.0.0
or::
to corresponding interface addresses. - Obtains observed addresses (which should be received via
identify
). - Obtains addresses which we advertising as our NAT via NAT config option.
- Remove non-unique addresses.
For discovery_addresses
.
- Expands all the addresses which are
0.0.0.0
or::
to corresponding interface addresses. - Obtains observed addresses (which is actually working pretty good)
- Obtains addresses which we advertising as our NAT via NAT config option.
- Remove non-unique addresses.
So whatever we are doing we doing in the same way we should do, the only difference is that we using NAT configuration to get specified address (it will help when we not connected to any peer yet). And we expand AnyAddress
which of course do not need to be reported because when you establish connection with 0.0.0.0
you will actually connect to 127.0.0.1
.
This PR does not replacing any old logic, it add more sources of information and eliminates 0.0.0.0
reports.
Does https://github.com/vacp2p/nim-libp2p/pull/1099 do some of what this does?
Yep, it supposed to fix the issue, but i have not checked it yet.
This PR was superseded by #6422