Skip to content
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

Map peers by ip only (ignoring port unless on loopback ip) #2540

Merged
merged 7 commits into from
Feb 18, 2019

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Feb 8, 2019

We have chunks of logic in our peering code that explicitly ignores the port when looking at peer addresses.
But we still store peers, both in the db and in our connected peers map based on ip:port.
This appears to cause some unnecessary complexity as we sometimes iterate over them all, comparing the ip and ignoring the port.

This PR attempts to clean this up and consolidate the "do we include the port or ignore the port?" logic based on whether our peer is on the "loopback" address or not.
i.e. If we are running tests or running a usernet locally then we care about the port.
But in all other cases we ignore the port and just treat each ip as having a single node on a single port.

Note we still store the peer ip and port, we just don't take the port into account when creating the key for it in the db or the connected peers map.

Thoughts on this?
It runs locally with tests fine. It also appears to run happily against mainnet.

TODO -

  • discuss if this makes sense and is worth spending more time on
  • introduce a PeerAddr struct so we can encapsulate the key creation logic in one place (currently duplicated in both peers.rs and store.rs)
  • leverage the existing internal SockAddr and rename it PeerAddr (turning into a big can of worms)
  • investigate our api endpoints that take peer addresses (see below)

http://127.0.0.1:3413/v1/ shows "get peers/a.b.c.d” in the list, but http://127.0.0.1:3413/v1/a.b.c.d doesn’t work. It has to include the port to get results: http://127.0.0.1:3413/v1/a.b.c.d:port. @antiochp Is this fixed with the changes you recently made around peers/ip addresses/etc?

@ignopeverell
Copy link
Contributor

Makes sense to me!

@antiochp antiochp changed the title [DNM] Map peers by ip only (ignoring port unless on loopback ip) Map peers by ip only (ignoring port unless on loopback ip) Feb 14, 2019
@antiochp
Copy link
Member Author

@ignopeverell @yeastplume @hashmap this is ready for final review.

  • Consolidated and reworked all our SocketAddr/SockAddr handling.
  • Renamed SockAddr -> PeerAddr and moved it into p2p/types.
  • Moved all custom "do we include the port or not" logic to inside PeerAddr.
  • We now consistently only allow one peer per ip address unless we are using the loopback address (for local testing).
    • This is enforced in the store/db (peers keyed via PeerAddr).
    • Also enforced in the connected peers map in the same way.

This was a bigger change than I initially anticipated, but I think its worth it.

Currently testing public and private nodes on mainnet and things appear to be running smoothly so far.

@antiochp antiochp self-assigned this Feb 14, 2019
@antiochp antiochp added this to the 1.0.2 milestone Feb 14, 2019
@antiochp
Copy link
Member Author

Look at all those peers -

20190214 17:51:35.979 DEBUG grin_servers::grin::seed - 
monitor_peers: on 0.0.0.0:3414, 
56 connected (47 most_work). all 10796 = 
425 healthy + 9 banned + 10361 defunct

10,000 defunct peers...

🙈

@antiochp
Copy link
Member Author

GET /peers/<a.b.c.d> works now with or without the port explicitly provided -

curl 127.0.0.1:3413/v1/peers/34.207.234.131    
{"addr":"34.207.234.131:3414","capabilities":{"bits":15},"user_agent":"MW/Grin 1.0.1","flags":"Healthy","last_banned":0,"ban_reason":"None","last_connected":1550246724}

Working on getting POST /peers (ban/unban) working the same way..

@antiochp
Copy link
Member Author

These now work -

# ip only
curl -X POST 127.0.0.1:3413/v1/peers/34.207.234.131/ban 
curl -X POST 127.0.0.1:3413/v1/peers/34.207.234.131/unban 

# ip and port
curl -X POST 127.0.0.1:3413/v1/peers/34.207.234.131:3414/ban
curl -X POST 127.0.0.1:3413/v1/peers/34.207.234.131:3414/ban  

@antiochp antiochp merged commit 23cb9e2 into mimblewimble:master Feb 18, 2019
@antiochp antiochp deleted the peers_by_ip_addr branch February 18, 2019 12:15
@nikito7
Copy link

nikito7 commented Feb 21, 2019

If we change default port 3414 its break peer count limits

@antiochp
Copy link
Member Author

@nikito7 can you explain what you mean here? We effectively ignore the port when identifying peers, we don't assume they are on 3414.

If you have a peer running on 10.0.0.1:4414 we treat it as identical to a peer running on 10.0.0.1:3414 and the limits will still apply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants