allow waiting for enough peers in gossipsub

[mvdan]

(see commit message)

[mvdan]

FYI @adlrocha @masih @willscott for the time.Sleep TODOs in go-legs and indexer-reference-provider.

[mvdan]

@willscott another question is how to expose this via go-legs. Right now it calls NewGossipSub, which creates a GossipSubRouter and feeds it to NewPubSub. Unfortunately, that is then hidden behind an unexported field in PubSub.rt, so neither go-legs nor its downstreams can access it to call methods like WaitEnoughPeers.

One option would be to add another method, like:

func (*PubSub) Router() PubSubRouter

Thoughts, @vyzo?

[mvdan]

Yet another option is to leave it up to the downstream to poll EnoughPeers in a loop, since that's already part of the PubSubRouter interface. That would certainly work, but then we'd be stuck with polling and sleeping forever, without the ability to do better like the TODO says. So I think supporting a standard method like "wait for enough peers" is worthwhile.

I'm new to this module and its design, though, so maybe there's a better design to be had here.

[vyzo]

This should be part of the pubsub interface, not the router; all routers implement enough peers.
Also, thread safety...

[vyzo]

Actually it should be in the topic interface.

[vyzo]

and there is also machinery for topic related events, which can make implementation of this much nicer.

[vyzo]

and there is already an option that more or less does what you want, it's called WithReadiness.

[mvdan]

and there is already an option that more or less does what you want, it's called WithReadiness.

I saw that, but it doesn't do anything at all in our case, because we're not using WithDiscovery.
Maybe we could teach that option to also work without WithDiscovery? It's unclear to me what API would be nicest.

Actually it should be in the topic interface.

That's fine by me; I was just following EnoughPeers. I can rewrite this PR if we can agree on what API to add or modify.

[vyzo]

then open an issue to discuss and agree on an api.

…

On Thu, Sep 23, 2021, 13:57 Daniel Martí ***@***.***> wrote: and there is already an option that more or less does what you want, it's called WithReadiness. I saw that, but it doesn't do anything at all in our case, because we're not using WithDiscovery. Maybe we could teach that option to also work without WithDiscovery? It's unclear to me what API would be nicest. Actually it should be in the topic interface. That's fine by me; I was just following EnoughPeers. I can rewrite this PR if we can agree on what API to add or modify. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#452 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAI4SRHWXSUQHCDAGWCDETUDMBXZANCNFSM5EOWP34A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[mvdan]

then open an issue to discuss and agree on an api.

Done: #454

[vyzo]

thanks.

…

On Thu, Sep 23, 2021, 15:08 Daniel Martí ***@***.***> wrote: then open an issue to discuss and agree on an api. Done: #454 <#454> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#452 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAI4SUR5FWUUDEFPPXCUYTUDMKCRANCNFSM5EOWP34A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[mvdan]

I've updated the code to no longer be racy, I think - the added WithReadiness logic inside Publish now uses the eval channel to call the ready func, which then calls EnoughPeers. Can you take another look?

[vyzo]

i would say this is good enough.

However, please don't change the gossipsub tests to remove the sleep, we want to test mesh formation with heartbeats.

[mvdan]

Hm, I'm not sure I understand removing the sleeps from tests. Sleeping for two seconds or sleep-polling until the equivalent condition is met seems basically the same, just that sleeping makes the tests slower. I'm happy to revert those changes though, as I'm not the maintainer.

If I'm not touching any of the existing tests, then it sounds like I need to add a new test to cover this feature.

[vyzo]

it's not the same condition; one allows the mesh to form, the other waits for number of peers. Yes, please add a new test for the readiness condition.

…

On Thu, Oct 28, 2021, 19:43 Daniel Martí ***@***.***> wrote: Hm, I'm not sure I understand removing the sleeps from tests. Sleeping for two seconds or sleep-polling until the equivalent condition is met seems basically the same, just that sleeping makes the tests slower. I'm happy to revert those changes though, as I'm not the maintainer. If I'm not touching any of the existing tests, then it sounds like I need to add a new test to cover this feature. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#452 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAI4SSCQDZ5OB5LNBXTEA3UJGDSJANCNFSM5EOWP34A> .

[mvdan]

All done, I think.

[vyzo]

looks pretty good - we just need to avoid unnecessary ticker allocations once we've got enough peers.

[vyzo]

thank you!