search

ExchangeUnion / xud

Exchange Union Daemon 🔁 ⚡️
https://exchangeunion.com
GNU Affero General Public License v3.0
115 stars 44 forks source link

GET_NODES periodically #402

Closed kilrau closed 5 years ago

kilrau commented 6 years ago

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

offerm commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

moshababo commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

EDIT: OUTDATED, latest summary below

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

Notes:

Agreed? Anything to add? @sangaman @moshababo

sangaman commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Any new ideas on how to query deltas of new nodes only NOT using timestamps? @moshababo @sangaman

sangaman commented 6 years ago

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 commented 6 years ago

Let's do it

rsercano commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

offerm commented 5 years ago

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?

On Sun, 2 Dec 2018 at 21:35 Sercan Özdemir notifications@github.com wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ExchangeUnion/xud/issues/402#issuecomment-443535320, or mute the thread https://github.com/notifications/unsubscribe-auth/AJQ0cvUxlHIHPH7VzD39xDoKLIZHNcYNks5u1CuagaJpZM4WOQAt .

kilrau commented 5 years ago

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 commented 5 years ago

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

kilrau commented 5 years ago

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

rsercano commented 5 years ago

Hello @kilrau,

sangaman commented 5 years ago

I also don't see the need for auto_connect_peers.

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

kilrau commented 5 years ago
  • 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