nimbus-eth2 icon indicating copy to clipboard operation
nimbus-eth2 copied to clipboard

Fix REST /eth/v1/node/identity should return proper MultiAddresses which is not AnyAddress4/6.

Open cheatfate opened this issue 10 months ago • 7 comments

This should fix https://github.com/status-im/nimbus-eth2/issues/6060

cheatfate avatar Mar 28 '24 16:03 cheatfate

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.

github-actions[bot] avatar Mar 28 '24 18:03 github-actions[bot]

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

arnetheduck avatar Mar 29 '24 08:03 arnetheduck

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.

cheatfate avatar Mar 29 '24 10:03 cheatfate

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"
  ]
}

cheatfate avatar Mar 29 '24 11:03 cheatfate

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

arnetheduck avatar Mar 29 '24 19:03 arnetheduck

So what is doing current PR. For p2p_addresses.

  1. Expands all the addresses which are 0.0.0.0 or :: to corresponding interface addresses.
  2. Obtains observed addresses (which should be received via identify).
  3. Obtains addresses which we advertising as our NAT via NAT config option.
  4. Remove non-unique addresses.

For discovery_addresses.

  1. Expands all the addresses which are 0.0.0.0 or :: to corresponding interface addresses.
  2. Obtains observed addresses (which is actually working pretty good)
  3. Obtains addresses which we advertising as our NAT via NAT config option.
  4. 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.

cheatfate avatar Mar 29 '24 20:03 cheatfate

This PR does not replacing any old logic, it add more sources of information and eliminates 0.0.0.0 reports.

cheatfate avatar Mar 29 '24 20:03 cheatfate

Does https://github.com/vacp2p/nim-libp2p/pull/1099 do some of what this does?

tersec avatar Jul 04 '24 05:07 tersec

Yep, it supposed to fix the issue, but i have not checked it yet.

cheatfate avatar Jul 04 '24 08:07 cheatfate

This PR was superseded by #6422

cheatfate avatar Jul 11 '24 17:07 cheatfate