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

GET_NODES periodically #402

Closed
kilrau opened this issue Aug 27, 2018 · 22 comments · Fixed by #770
Closed

GET_NODES periodically #402

kilrau opened this issue Aug 27, 2018 · 22 comments · Fixed by #770
Assignees
Labels
P1 top priority p2p Peer to peer networking question/tbd Further information or discussion needed
Milestone

Comments

@kilrau
Copy link
Contributor

kilrau commented Aug 27, 2018

This issue is to discuss 2 opposing approaches in how to share new peers (reminder: peer is a node I'm currently successfully connected to):

1

Manually send GET_NODES packet to learn about all peers of my peer (concern: nobody will ever do that, it should be automated). Also it's potentially a huge load every time since it's ALL peers.

2

My peer shares newly connected peers dynamically at run time every time it successfully connects to one (concern: spammy?)

I would define our overall goal as: "All xuds automatically find each other and connect to each other."

Just like BitTorrents DHT gets bootstrapped from some hard coded seeds (or a swarm) and the rest happens automatically. Hence, I think we should combine a) and b).

a) happens on startup
b) during run time

@kilrau kilrau added the p2p Peer to peer networking label Aug 27, 2018
@kilrau kilrau added this to the 1.0.0-alpha.2 milestone Aug 27, 2018
@kilrau kilrau mentioned this issue Aug 27, 2018
@kilrau kilrau added the question/tbd Further information or discussion needed label Aug 27, 2018
@offerm
Copy link
Contributor

offerm commented Aug 27, 2018

@kilrau If the goal is : "All xuds automatically find each other and connect to each other." we should assume that all nodes are already connected to all connectable nodes. TBD: is this scalable model

So when a new node (X) joins the graph it is doing so by connecting to one of the existing nodes (Y). Y should already be connected to most other nodes and should share their addresses to X. X will connect to these nodes itself so all other nodes become connected with X without the need for anything like a and b being done.

@sangaman
Copy link
Collaborator

a) Manually send GET_NODES packet to learn about all peers of my peer (concern: nobody will ever do that, it should be automated). Also it's potentially a huge load every time since it's ALL peers.
b) My peer shares newly connected peers dynamically at run time every time it successfully connects to one (concern: spammy?)

Manually querying for new nodes is the type of thing that would have to happen quite rarely, and while it can potentially be a big packet it's already something we do every time start up and I don't think it's a problem. My thinking is that, for nodes that stay online for long stretches of time, manually querying for new nodes is a way to simulate that start-up querying for nodes but without actually shutting down the server. But maybe we don't even need either of these options. We already do GET_NODES each time we start up, and if we're online, new nodes will hear about us.

TBD: is this scalable model

Not exactly, I think this is fine for a network on the order of dozens or hundreds of nodes. Much beyond that I'm not sure, and at some point we just exhaust the number of ports. That would definitely be a phase 2 sort of concern.

@kilrau kilrau self-assigned this Aug 27, 2018
@kilrau
Copy link
Contributor Author

kilrau commented Aug 28, 2018

TBD: is this scalable model : not much tbd needed - definitely not. But we hopefully have a good reason to start like this: we want the fastest way for orders to flow from node a to node b. For now the IP protocol finds the fastest route for us, hence we want a direct socket connection.

We have a plan to introduce something called super-nodes later on which are allowed to route orders because they can ensure certain things, like a good ping, ensure they don't temper with the order, don't front-run etc. It takes a bit more to figure this out. @offerm

So when a new node (X) joins the graph it is doing so by connecting to one of the existing nodes (Y). Y should already be connected to most other nodes and should share their addresses to X. X will connect to these nodes itself so all other nodes become connected with X without the need for anything like a and b being done.

Well a) is how "Y share their addresses to X" is realized. @offerm

if we're online, new nodes will hear about us.

This made me think. I have the feeling it won't work that well for nodes being online long-term since they have to rely on new nodes connecting to them. Anyhow, let's keep things PoC for now and we'll revisit this later after we have some testing data - moved to beta.

@kilrau kilrau modified the milestones: 1.0.0-alpha.2, 1.0.0-beta Aug 28, 2018
@kilrau
Copy link
Contributor Author

kilrau commented Sep 5, 2018

Any new opinions on 1) or 2) @sangaman @moshababo @michael1011 @offerm ?

@moshababo
Copy link
Collaborator

moshababo commented Oct 26, 2018

I think we have total 4 different approaches:

  1. Manually query GET_NODES from peers

I don't think it's relevant. everything should be done automatically.

  1. Schedule a periodic query GET_NODES from peers

Sounds like the easiest approach.

  1. Share newly connected peers with others

Could be spammy, I would rather keep the push messaging limited as possible, and favor the request/response model.

  1. don't query/share new peers. the newly connected nodes will find us.

If our node is reachable, I think this could work, it just put the responsibility on newly connected nodes. But I don't think we can assume that our node is reachable.

@sangaman
Copy link
Collaborator

I also prefer request/response over pushing newly discovered peers. Periodically querying for new nodes I'd agree is probably the best way to go.

@kilrau
Copy link
Contributor Author

kilrau commented Oct 27, 2018

EDIT: OUTDATED, latest summary below

Ok I agree on GET_NODES because less spammy. Summary of what is to be implemented in this issue:

  • periodically query GET_NODES from our peers (proposal: 24h)
  • add timestamp to GET_NODES packet so that peer can respond with new/deleted peers AFTER this timestamp

Notes:

  • save timestamp of last successful getNodes response from a peer
  • include timestamp in next query
  • if node was down and last timestamp per peer is > 24h, immediately send getNodes with this timestamp)

Agreed? Anything to add? @sangaman @moshababo

@kilrau kilrau modified the milestones: 1.0.0-alpha.2, 1.0.0-alpha.4 Oct 27, 2018
@sangaman
Copy link
Collaborator

The timestamp idea might conflict with how we handle nodes that have been offline for a while and come back online. Sometimes we want to hear about those nodes again provided they are online, because we may have exhausted reconnection attempts locally and cleared that node's connection addresses.

Stiil it might be a nice feature if we can make it work around that concern, and in that case I figure we'd also want it on the GET_NODES call we make after the handshake. So I'd be inclined to handle that issue separately from the scheduled GET_NODES calls.

@kilrau
Copy link
Contributor Author

kilrau commented Oct 27, 2018

Gotcha and makes sense. Just can't come up with anything else apart from timestamps how to easily get the "delta" of nodes, not the whole list every time. But agreed, then we handle it separately from this issue.

@kilrau kilrau changed the title Share newly connected peers dynamically at runtime GET_NODES periodically Oct 27, 2018
@sangaman
Copy link
Collaborator

Thinking about this again I don't have any bright ideas. I'm thinking that for now, it'd be ok just to query for everything. It's not a lot of data to transmit or anything, and we're not dealing with very large numbers of nodes yet. So I can implement just the periodic queries, and it can be improved later perhaps.

@kilrau kilrau added the P3 low priority label Nov 20, 2018
@kilrau
Copy link
Contributor Author

kilrau commented Nov 26, 2018

Let's do it

@rsercano
Copy link
Collaborator

rsercano commented Dec 2, 2018

This seems simple if I understand correctly, just to be at the same page, do we need a scheduler for this line ? Are there any concerns other than this ? @kilrau @sangaman

@sangaman
Copy link
Collaborator

sangaman commented Dec 2, 2018

I haven't thought about it too deeply yet. But yes if discover is enabled, we want to set a schedule to repeat sending the GetNodes packet. We also want to make sure to end the timer when we disconnect from that peer.

@rsercano
Copy link
Collaborator

rsercano commented Dec 2, 2018

Gotcha thanks, assigning to myself, hopefully will come with a PR asap.

@rsercano rsercano self-assigned this Dec 2, 2018
@offerm
Copy link
Contributor

offerm commented Dec 2, 2018 via email

@kilrau
Copy link
Contributor Author

kilrau commented Dec 3, 2018

Question : why do we need to ask the peer for new nodes? Why can't the peer push it when it discover a new node?

This is the option 1) and 2) I put out to debate above. Basically we went with 1) because if I'm connected to 100 nodes I'll hear about a new node 100 times without me being able to control these push messages. And they always come in bulks - potentially very spammy. Whereas when using the GET_NODES pull approach, we can fine tune this at some point to e.g. only ask a sub-set of peers and it will be in regular intervals.

@kilrau kilrau modified the milestones: 1.0.0-alpha.5, 1.0.0-alpha.6 Dec 5, 2018
@kilrau
Copy link
Contributor Author

kilrau commented Dec 6, 2018

Latest summary of what is to be implemented in this issue:

  • query GET_NODES on handshake with peer
  • periodically query GET_NODES from all our peers (proposal: 1h), store & connect to new peers
  • xud.conf option auto_connect_peers . false on default.

@kilrau
Copy link
Contributor Author

kilrau commented Dec 6, 2018

Concerns? Anything to add? @offerm @sangaman @rsercano

@kilrau kilrau added the P1 top priority label Dec 6, 2018
@rsercano
Copy link
Collaborator

rsercano commented Dec 8, 2018

Hello @kilrau,

  • we already got discover option to decide sending getNodes packet, why do we need auto_connect_peers ? Is it for scheduler ?

  • I guess connecting to the nodes which are sent by getNodes packet is already being handled here Is there anything to be done other than making getNodes work scheduled, and clearing timeout when peer disconnects ?

@sangaman
Copy link
Collaborator

I also don't see the need for auto_connect_peers.

As for your second bullet point, I think that's right.

@kilrau
Copy link
Contributor Author

kilrau commented Dec 11, 2018

  • we already got discover option to decide sending getNodes packet, why do we need auto_connect_peers ? Is it for scheduler ?

My bad, yep discover is enough.

  • I guess connecting to the nodes which are sent by getNodes packet is already being handled here Is there anything to be done other than making getNodes work scheduled,

Sounds right.

and clearing timeout when peer disconnects ?

Do you mean reset the timeout once a peer connects? If so, yes. Once a peer disconnects, the reconnect cycle with increasing timeouts as per #311 starts

rsercano pushed a commit to rsercano/xud that referenced this issue Dec 11, 2018
rsercano pushed a commit to rsercano/xud that referenced this issue Dec 18, 2018
@ghost ghost added the in progress label Dec 25, 2018
@kilrau kilrau modified the milestones: 1.0.0-alpha.6, 1.0.0-alpha.8 Jan 2, 2019
moshababo pushed a commit that referenced this issue Jan 4, 2019
* #402

* #402 made mentioned changes

* removed unrequested changes

* fixing test

* made discoverminutes option lower case

* fixed typo

* na

* put discover, discoverminutes into peer.open

* na

* triggering travis build

* moved Poolconfig into types to prevent circular import
@ghost ghost removed the in progress label Jan 4, 2019
@kilrau kilrau removed the P3 low priority label Jan 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 top priority p2p Peer to peer networking question/tbd Further information or discussion needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants