Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: unstable
Choose a base branch
from

Conversation

cheatfate
Copy link
Contributor

This should fix #6060

Copy link

github-actions bot commented Mar 28, 2024

Unit Test Results

         9 files  ±0    1 115 suites  ±0   30m 57s ⏱️ + 3m 44s
  4 244 tests ±0    3 897 ✔️ ±0  347 💤 ±0  0 ±0 
16 926 runs  ±0  16 528 ✔️ ±0  398 💤 ±0  0 ±0 

Results for commit 56f1097. ± Comparison against base commit 0fa363e.

♻️ This comment has been updated with latest results.

addresses

proc getObservedAddresses(node: BeaconNode): seq[MultiAddress] =
# Following functionality depends on working `Identify/IdentifyPush` protocol
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens in the absence of such working and enabled Identify/IdentifyPush protocol inside nim-libp2p?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addresses will not be added to the list, so it will be missing, like it happens right now.

@arnetheduck
Copy link
Member

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

@cheatfate
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@arnetheduck
Copy link
Member

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

@cheatfate
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants