Use the IP address given in the ping #301
Conversation
|
|
There seem to be a lot of open issues with this, are you sure that it makes sense to merge these PRs? |
|
Yeah, the assignment wasn't meant as a indication of merging, but as a way to solicit feedback. You are right that in the current state it's not ready to merge
They get ignored, but might indicate that just changing to the external IP address when saving the peer might not be sufficient |
| t.send(from, fromID, pongPacket, &pong{ | ||
| To: makeEndpoint(from, req.From.TCP), | ||
| ReplyTok: mac, | ||
| Expiration: uint64(time.Now().Add(expiration).Unix()), | ||
| }) | ||
|
|
||
| // Ping back if our last pong on file is too far in the past. | ||
| n := wrapNode(enode.NewV4(req.senderKey, from.IP, int(req.From.TCP), from.Port)) | ||
| n := wrapNode(enode.NewV4(req.senderKey, senderFrom, int(req.From.TCP), int(req.From.UDP))) |
There was a problem hiding this comment.
Do you need to handle the case if req.From is nil for the port?
There was a problem hiding this comment.
Yeah so this is where I dont yet fully understand when it would be nil for me know how to handle it? Under what circumstances would it be?
Wouldn't the bonding/verification of pings address this concern? |
That is odd. Do you think that it's a show stopper in getting this merged? Or would a follow up post-alphanet PR suffice? IMO, a follow up one is fine. |
Follow up question: Are you getting pongs from nodes outside of your network? When I set up an isolated validator running on it's own machine, I was seeing a lot of discovery messages from random computers. Perhaps that is what you are seeing? |
+1, I've experienced this as well in the past. |
It would if we ping'd at the passed IP address, which this PR currently does not
I don't think it is a show-stopper, though it would be valuable to have a theory why this happens, which I do not.
Yes, I believe there must be nodes that just ping all IP addresses effectively? Or we possibly ping out to other bootnodes |
Description
In the upstream v4 version of the discovery protocol, geth will use the source IP of the UDP connection to advertise the endpoint to other peers. However, when a node is behind a NAT, the source IP is not guaranteed to be the external IP under which that peer would be available. This PR naively will use the passed IP address to save it to its peer table and advertise to other peers. For a bit more background, also see celo-org/celo-monorepo#3877
It should be noted that two other changes should be made:
Sister-PR to celo-org/celo-monorepo#3952
Tested
Fixes celo-org/celo-monorepo#3877