kubo icon indicating copy to clipboard operation
kubo copied to clipboard

Update pubsub and add default validator

Open vyzo opened this issue 2 years ago • 9 comments

See #9665

This updates go-libp2p-pubsub to v0.9.2 and adds a default validator to protect against message cycles when the effective network diameter exceeds the span of the timecache.

vyzo avatar Mar 01 '23 19:03 vyzo

Some complaints from go-checks aboug go.mod not being tidy ... but that is! Seems it is the examples however.

vyzo avatar Mar 01 '23 19:03 vyzo

So the TestMessageSeenCacheTTLTest test fails because the behaviour of the seen cache has changed -- it doesn't eagerly aggregate any more and it cleans up with an background goroutine that runs once a minute.

We either have to wait a minute for cleanup or we need to do some hack to change the default duration or wait for a minute. This is controlled by a private variable in pubsub, and there is no way to set it currently. We could patch pubsub to make that variable public so that we can test, but this is quite ugly.

Also note, the test tests the wrong thing, i.e. the explicit behaviour of the original last seen cache implentation which has now changed.

Options:

  • wait for a minute for the cache to clear (makes the tests very slow)
  • change pubsub to export the timecache background sweep interval variable and set it to something low (say once per second). Requires a change in pubsub and new release.
  • disable the test. I don't like disabling tests, but this seems like it is testing the wrong thing and there are unit tests in pubsub for the behaviour of the cache, so this doesn't add much more than integration coverage.

I don't like waiting for minutes, and I think this is test is not very useful atm, so I am disabling for now.

vyzo avatar Mar 01 '23 20:03 vyzo

~Hrm, I don't like disabling tests either, so let's try with 1min waits (and see whether we should fix that interval).~

Edit: I don't have the patience for this, disabling for now.

vyzo avatar Mar 01 '23 20:03 vyzo

Thinking more about this test situation -- i think this test simply doesn't belong. It violates abstraction by programming against a specific implementation of the cache which is now gone.

It has to be rewritten and move into pubsub where we have better control of such things; also note it might be completely reduntant given how we have unit tests for the caches there.

vyzo avatar Mar 01 '23 20:03 vyzo

@Jorropo : what are the next steps here for getting this merged?

BigLep avatar Mar 09 '23 14:03 BigLep

2023-03-09 maintainer conversation on what to do about this situation.

Reminder for all: https://github.com/ipfs/kubo/blob/master/docs/config.md#pubsub is experimental and not on by default.

Historically this feature has caused a lot of pain because Kubo isn't making an explicit contract. One is accepting libp2p defaults. In addition, attempting to maintain compatibility between js-ipfs and Kubo has been a challenge.

This is a feature that hasn't made sense to have in Kubo itself.
This is not something we can create a generic solution for. We instead need to empower users to take more ownership over their pubsub needs. We plan to do this by deprecating (and then removing) from Kubo, but providing a soft-landing for users with a working example on how they can get setup on their own. We can also point to solutions that exist now that didn't previously which may be better than the historical staus quo of using pubsub in Kubo (e.g., https://fireproof.storage/ ) This PR is not going to get merged into master because it's untenable to update CI to account for the change. This could break other smaller webapp users who rely on the existing functionality that is working.

There are two sets of actions that need to be taken:

For Kubo and its users in general. These will get fully tracked in the issue below:

  • [X] @Jorropo Create placeholder issue about "[Tracking issue] deprecate/remove pubsub support (and provide alternative)". This should include justification / commentary like we have above. https://github.com/ipfs/kubo/issues/9717
  • [x] @Jorropo Close https://github.com/ipfs/kubo/issues/6621 (pointing to the issue above)
  • [X] @Jorropo Create a topic in discuss.ipfs.tech (https://discuss.ipfs.tech/t/help-kubo-maintainers-about-usecases-for-the-http-pubsub-api/16097)
    • We are planning to remove pubsub (link to issue)
    • "Please comment below to share your usecase and why you have used this in Kubo."
  • PR moving pubsub from experimental to deprecated (referencing issue above). This should have a changelog update. Hide/remove all the docs about this as well since we don't want anyone else putting weight on this.
  • TBD (but likely @Jorropo ): post 0.20 / IPFS Thing, do the migration path / soft landing so we can fully remove pubsub from Kubo. This is likely a full-example in libp2p/go-libp2p-pubub that is validated as part of CI

Specific actions concerning Ceramic:

  • [x] @BigLep Notify Ceramic about this, get into their slack channel, and get a meeting scheduled to discuss how to unblock them.
    • Message posted in Slack: https://filecoinproject.slack.com/archives/C01V5AWPF97/p1678402974122419
  • [ ] @Jorropo / @lidel (minimum), @biglep (bonus) : Meet with Ceramic to understand their architecture and how we can unblock them in the short term (e.g., can they build off a branch with this PR?)

BigLep avatar Mar 09 '23 16:03 BigLep

@Jorropo created https://github.com/ipfs/kubo/issues/9717 which is being used to track the generic tasks for deprecating pubsub in Kubo. We can still use the checklist above to cover the ceramic-specific steps (or those can move to https://github.com/ipfs/kubo/issues/9665 )

BigLep avatar Mar 10 '23 18:03 BigLep

Cermaic meeting scheduled for 2023-03-21 at 17UTC

BigLep avatar Mar 17 '23 23:03 BigLep

triage discussion

  • we've removed interop with js-ipfs but no pubsub interop exist with helia-pubsub, nor we agreed to maintain validator/caching logic in sync
  • if we are ok with making this kubo-specific, we should write down (in config.md ?) what exactly happens
  • most likely blocked until we pick up https://github.com/ipfs/kubo/issues/9717 and decide what to do next

lidel avatar Aug 07 '23 13:08 lidel