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

Update pubsub and add default validator #9684

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update pubsub and add default validator #9684

wants to merge 4 commits into from

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented Mar 1, 2023

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
Copy link
Contributor Author

vyzo commented Mar 1, 2023

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

@vyzo
Copy link
Contributor Author

vyzo commented Mar 1, 2023

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
Copy link
Contributor Author

vyzo commented Mar 1, 2023

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.

@BigLep BigLep mentioned this pull request Mar 1, 2023
@vyzo
Copy link
Contributor Author

vyzo commented Mar 1, 2023

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.

Comment on lines +19 to +25
type P2PPubSubIn struct {
fx.In

Repo repo.Repo
Host host.Host
Discovery discovery.Discovery
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the point of P2PPubSubIn ? Couldn't you use arguments on the anonymous functions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its 3 of them, this is more tidy and it costs nothing.

@BigLep
Copy link
Contributor

BigLep commented Mar 9, 2023

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

@BigLep BigLep linked an issue Mar 9, 2023 that may be closed by this pull request
3 tasks
@BigLep
Copy link
Contributor

BigLep commented Mar 9, 2023

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:

Specific actions concerning Ceramic:

@BigLep
Copy link
Contributor

BigLep commented Mar 10, 2023

@Jorropo created #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 #9665 )

@BigLep
Copy link
Contributor

BigLep commented Mar 17, 2023

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

@lidel
Copy link
Member

lidel commented Aug 7, 2023

triage discussion

@Jorropo Jorropo removed their assignment Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🛑 Blocked
Development

Successfully merging this pull request may close these issues.

Pubsub flood due to same message propagated multiple times
4 participants