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

Question: expected behavior on empty mesh peers? #211

Closed
ZenGround0 opened this issue Oct 14, 2019 · 12 comments
Closed

Question: expected behavior on empty mesh peers? #211

ZenGround0 opened this issue Oct 14, 2019 · 12 comments
Labels
kind/support A question or request for support req:filecoin

Comments

@ZenGround0
Copy link

I've noticed that the following situation can occur during gossipsub publish:

  1. There are peers in the pubsub topic map for this topic this ok is true
  2. This router is in the mesh for the topic this ok is true
  3. The router has not added any peers to the mesh gmap and tosend are empty

This leads to the counterintuitive result that peers that are not in the mesh will end up publishing, but peers that are in the mesh (without peers) will not end up publishing.

Is this behavior expected?

@vyzo
Copy link
Collaborator

vyzo commented Oct 15, 2019

If there are no peers in the mesh, there is nothing to publish ... except we might have floodsub peers.
That latter may be a bug, otherewise it is expected behaviour.

Regardless, we may want some kind of event notification for when we have peers so that the application knows when to start publishing.

@ZenGround0
Copy link
Author

Regardless, we may want some kind of event notification for when we have peers so that the application knows when to start publishing.

That would be amazing for my use case

@aschmahmann
Copy link
Contributor

aschmahmann commented Oct 16, 2019

Regardless, we may want some kind of event notification for when we have peers so that the application knows when to start publishing.

#184 allows the Publish function to block on waiting for a bootstrapped condition to be met (e.g. connected to a certain number of peers).

@ZenGround0 is that sufficient? It feels much better then an event, since a node could lose all its peer connections and then we would need to fire a "we don't have enough peers event" and manage that (worry about what happens if we drop an event, etc.).

@vyzo
Copy link
Collaborator

vyzo commented Oct 16, 2019

But that's for peers that are not subscribed, no? We need something for peers in the mesh.

@aschmahmann
Copy link
Contributor

@vyzo
Copy link
Collaborator

vyzo commented Oct 16, 2019

We can still emit events regardless of blocking publish -- so that the application can decide exactly when it can start publishing.
Blocking is a little rude for many use cases.
It is a slightly different use case than the pubsub-router.

@aschmahmann
Copy link
Contributor

Whether (and how) the publish is blocking is determined by options passed into publish function. Adding more features doesn't really hurt except by adding more confusion and potential footguns.

As I mentioned above, if you do this by receiving an "I'm ready" event you also need to receive "I'm not ready anymore" event. However, depending on how the event system is used/constructed it might be possible that one of these events ends up getting dropped or not processed in time. There are also potentially some atomicity questions that while they aren't currently handled are possible to handle with the Publish options interface that events would make very difficult to handle.

@ZenGround0 is the solution in the current PR sufficient for your use case, or is there a reason that an event approach would be better/more useful to you?

@ZenGround0
Copy link
Author

@aschmahmann's PR works great for our use case.

@aschmahmann
Copy link
Contributor

@ZenGround0 question about how you're using pubsub to make sure we don't accidentally miss something. The current "publish when ready" code only runs if a discovery service has been passed into pubsub. The reason is that if you're not ready we'd like to actively discover peers to make you ready.

My understanding is that you are using a DHT for peer discovery, but instead of using the approach at https://github.com/libp2p/go-libp2p-discovery/blob/998fc35855101b7a32b303e190051e2020c62989/routing.go#L18 (which uses provider records to leverage a DHT record as a rendezvous point) you are using a separate DHT which assumes that all (or many) of the clients also subscribe/publish to the same topics.

To accommodate this use case do you have a preference for A) wrapping the DHT in a discovery interface that just ignores the topic name, or B) having the readiness check work even if no discovery mechanism is passed in (this might just be an option instead of the default)?

@ZenGround0
Copy link
Author

My preference is B. We could make A work if it is a lot better on your end.

@aschmahmann
Copy link
Contributor

After talking it over with @Stebalien we think option A would likely make more sense and is a less confusing API. There are a number of possible ways you could wrap the DHT for peer discovery, and you guys may even be inclined to change them over time.

A couple easy options on how you might want to do this:

  1. Make Advertise do nothing, and FindPeers do a random walk before returning an empty channel (see the linked DHT based rendezvous example above for some details)
  2. Make Advertise do nothing, and FindPeers just sleeps for a while (or indefinitely)

If you need any help figuring out how to do this lmk, but it should be pretty straightforward (especially if you decide to go with the option 2 😛)

@aschmahmann
Copy link
Contributor

Closed by #184

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/support A question or request for support req:filecoin
Projects
None yet
Development

No branches or pull requests

3 participants