-
Notifications
You must be signed in to change notification settings - Fork 265
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
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 |
Does vacp2p/nim-libp2p#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 |
This should fix #6060