Skip to content

pool, agent: StrictPeers checking for remote host matching#80

Merged
shazow merged 7 commits intomasterfrom
strict-peers-2
Oct 4, 2019
Merged

pool, agent: StrictPeers checking for remote host matching#80
shazow merged 7 commits intomasterfrom
strict-peers-2

Conversation

@shazow
Copy link
Copy Markdown
Member

@shazow shazow commented Oct 1, 2019

  • Add ethnode.ParseNodeURI(...), similar to url.Parse(...). Closes ethnode: Add NewEnodeURI(uri string) (*EnodeURI, error) #76.
  • UpdateResponse.ActivePeers are now enode:// strings instead of just NodeIDs. This should be safe for previous versions because they only looked at len(ActivePeers) rather than actual values.
  • In the future, UpdateResponse.InvalidPeers might also be changed to enode:// strings for consistency, though thinking about it more it might not make sense since invalid peers are "disconnected" so the pool doesn't necessarily know about their prior host/port etc. Maybe just change them to an enode:// string with an empty host? (Updated code to parse defensively so this is a less-breaking change in the future.)
  • Added hostname checking to StrictMode after being normalized with ParseNodeURI. Closes agent: Add host/port checking to StrictPeer mode #79

@shazow shazow marked this pull request as ready for review October 3, 2019 02:59
@shazow shazow requested a review from ryanschneider October 3, 2019 02:59
// of IPs, so we avoid stripping out hosts.
if hostname := (*url.URL)(u).Hostname(); hostname == "localhost" {
return ""
} else if ip := net.ParseIP(hostname); ip.IsUnspecified() || ip.IsLoopback() {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check helps normalize things like "[::]", "0.0.0.0", "127.0.0.1", etc into an empty host for more effective comparison.

I think this is a Good Idea, but I could also see an argument for having a more explicit CompareRemote(string) bool function.

// StaticPool is a dummy implementation of a pool service that always returns
// from the same set of host nodes. It does not do any signature checking.
type StaticPool struct {
Nodes []store.Node
Copy link
Copy Markdown
Member Author

@shazow shazow Oct 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was another example where it was too tempting/easy to accidentally have side effects/mutations on the internal slice, messing up some test scenarios.

Copy link
Copy Markdown
Collaborator

@ryanschneider ryanschneider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@shazow shazow merged commit 140f59c into master Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

agent: Add host/port checking to StrictPeer mode ethnode: Add NewEnodeURI(uri string) (*EnodeURI, error)

2 participants