Conversation
ryanschneider
left a comment
There was a problem hiding this comment.
I think the overall logic looks good but not sure about some of the specifics of enode comparisons, see comments below.
agent/agent.go
Outdated
| // enodeURItoID takes an enode:// URI string and returns the NodeID component. | ||
| func enodeURItoID(uri string) string { | ||
| if len(uri) <= 8+128 { | ||
| return uri |
There was a problem hiding this comment.
does this work for the mock URIs used in the tests like "enode://bar@127.0.0.1:30303"? Why not use url.Parse?
There was a problem hiding this comment.
Also seems like a dupe of code in PeerInfo.EnodeID() so should maybe be refactored into a common helper function?
There was a problem hiding this comment.
Yea, actually I don't think I even need this since ActivePeers is just IDs. I think I was using this helper for something else but it got factored out.
The reason why I avoided url.Parse here is because it returns the wrong result without a scheme (ie. if it's just the ID).
There was a problem hiding this comment.
Actually just realized that this might not work at all since ActivePeers is just IDs. Hmm, need to rethink the approach.
There was a problem hiding this comment.
I removed it altogether for now, so we're only comparing enodeIDs.
Might be good enough to launch this feature in v2.3 but I'd really like to get the host comparisons in so we can use it to replace any accidentally-externally-routed peers with proper internally-routed peers.
ethnode/rpc.go
Outdated
| return false | ||
| } | ||
| if uriA.Hostname() == "::" || uriB.Hostname() == "::" { | ||
| if uriA.Port() != uriB.Port() { |
There was a problem hiding this comment.
Are we sure we want to equate on port numbers? Won't the ports always mismatch if we are comparing remote- and locally- initiated connections? That is, if I connect to nodeB, the remote address will be :30303 but if they connect to me the remote address will be an ephemeral port number.
Also, since this uses url.Parse like I recommended for enodeToId maybe we should have a NewEnodeURI(uri string) (*EnodeURI, error) method that returns a EnodeURI struct with fields like ID, Address, and Port, and possibly helpers like IsWildcard()?
There was a problem hiding this comment.
(Oops misclicked on resolve)
Are we sure we want to equate on port numbers?
Good question! The way it's being used right now, it's only comparing the reported remote addresses from the pool vs local peers' remote addresses, so it's always remote vs remote (external ports).
The reason I wanted to equate port numbers at all is for the scenario if a node goes down, then comes back up on a different port for some reason, that should force an invalidation -> disconnect -> eventual reconnect next time it's returned from the pool.
But you're right, it's risky making this a public helper because it's a foot-gun if people aren't careful about that. At minimum, I'll move it to be a private helper where it's being used.
Also, since this uses url.Parse like I recommended for enodeToId maybe we should have a
NewEnodeURI(uri string) (*EnodeURI, error)method that returns aEnodeURIstruct with fields likeID,Address, andPort, and possibly helpers likeIsWildcard()?
I like this idea. Not sure if I'll fit it into this PR, but definitely 👍.
Opened accompanying issue for a future improvement based on it: #76
|
After a bit of prototyping, my plan here is to make a sorta-breaking-but-coincidentally-compatible change to UpdateResponse such that For consistency, I'd also like to change the |
Add
vipnode agent --strict-peersflag which will disconnect from any peers that aren't returned from the pool updates as "active" (aka peers that are registered with the pool and have sent an update within the past 120s).Fixes #70