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
base: unstable
Are you sure you want to change the base?
Conversation
addresses | ||
|
||
proc getObservedAddresses(node: BeaconNode): seq[MultiAddress] = | ||
# Following functionality depends on working `Identify/IdentifyPush` protocol |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
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 |
|
Also all the corresponding information is obtained via libp2p and discovery5 interfaces.
Now we will have
|
https://github.com/vacp2p/nim-libp2p/blob/03f67d3db5355fad9bac90d458ce7d01870bfd2e/libp2p/switch.nim#L218 - we perform identity requests as part of connection upgrade:
Notably, it looks like we perform it the wrong way given how |
So what is doing current PR.
For
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 |
This PR does not replacing any old logic, it add more sources of information and eliminates |
This should fix #6060