[PAN-2585] Prefer EnodeURL over Endpoint #1378
Conversation
Base Peer, DefaultPeer, DiscoveryPeer on EnodeURL rather than Endpoint
Previously some tests assumed that a NeighborsPacket created with a set of peers would return the same instances from getNodes(). This is no longer the case.
Remove some useless tests
Keep a concrete Endpoint reference within DiscoveryPeer
This reverts commit e3da50c.
| verify(controller, times(0)).bond(otherPeer); | ||
| verify(controller, times(1)).bond(otherPeer2); | ||
| verify(controller, times(0)).bond(eq(otherPeer)); | ||
| verify(controller, times(1)).bond(eq(otherPeer2)); |
There was a problem hiding this comment.
nit: I don't think we need the eq here. The default is equals comparison so you typically only use eq if you need to use other matchers for some params (because all params must use matchers if any do).
| + "(\\?discport=(?<discovery>\\d+))?$"; | ||
| private static final int NODE_ID_SIZE = 64; | ||
| private static final Pattern DISCPORT_QUERY_STRING_REGEX = | ||
| Pattern.compile("discport=([0-9]{1,5})"); |
There was a problem hiding this comment.
If we're being really strict this should probably be "(^|&)discport=([0-9]{1,4})" I think. So discport should either be the first arg or after a & to prevent matching enode://...@...:30303?notthediscport=1234
There was a problem hiding this comment.
Made this check stricter and added more tests. For now, I have the parsing fail if there are any additional params outside of a singular discport param. It's simpler to implement this way and if we need additional params for some reason we can add that later.
| public InetAddress getIp() { | ||
| return ip; | ||
| } | ||
|
|
||
| public InetAddress getInetAddress() { | ||
| return ip; | ||
| } |
There was a problem hiding this comment.
We probably shouldn't have both getIp and getInetAddress both returning the same thing.
There was a problem hiding this comment.
Good call - not sure how I missed this since the method is right below the one I added 😂
PR description
EnodeURLis a more complete representation of a node's address thanEndpointand many API's are being built aroundEnodeURL. To simplify the code base, this PR tries to remove and contain the use ofEndpointin favor ofEnodeURLby:Endpointinto the discovery package, as it is really an object that is specific to discovery message parsing.Peerclasses toDiscoveryPeergetEndpoint()fromPeerinterface.EnodeURLobjects so we can leverage theEnodeURL.Builderrather than duplicating logic across many constructors (removed a lot ofDefaultPeerconstructors).A few tangential changes are included as well:
DefaultPeertoEnodeURLand replaces the existing String-based URI parsing code.EnodeURL.getIp()returns anInetAddressandgetIpAsString()was added to support the cases where a string is required.EnodeURL.getListeningPort()returns a primitiveintrather than anInteger.DefaultCommandValuesas there's really no case where the p2p logic should assume a default port.PeerDiscoveryControllerto extract and deduplicate peers from neighbors packets before passing those peers on toRecursivePeerRefreshState.