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

go-ipfs#8586 added EOL for subscriptions #92

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

Conversation

TobiaszCudnik
Copy link

@TobiaszCudnik TobiaszCudnik commented Jan 12, 2022

Minimal (and probably naive) implementation of ipfs/kubo#8586.

Changes

  • EOL handled in pubsub directly because of republish on startup
  • GC for topics in pubsub router
    • EOL channel for each subscription
    • bump the delay after repeated resolve/publish requests
  • added key formatting for logs
  • assumptions
    • lifetime of IPNS in DHT separate from lifetime in pubsub
    • DefaultSubscriptionLifetime (pubsub) higher than DefaultRecordEOL (namesys)

Logging

export GOLOG_LOG_LEVEL="warning,pubsub-valuestore=debug,net:pubsub=debug,ipns-repub=debug,ipns=debug,namesys=debug"

TODO

  • tests
  • config Ipns.ReproviderDuration

While I havent looked at the tests just yet, I had hard time figuring out how to pass the config from go-ipfs to go-libp2p-pubsub-router through go-namesys.

@aschmahmann
Copy link
Contributor

This needs some review (and perhaps further discussion in the linked IPFS issue). Having self-denotating topics (i.e. closing after 36hrs) is likely not the way to go here for GC.

@lidel or I should take a look or respond when some time frees up.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Ok, so my understanding is:

  • go-libp2p-pubsub-router provides Routing interface, which is pretty generic.
  • we happen to only use it in go-ipfs for "IPNS over PubSub" atm
    • it may be useful for other things in the future
    • we are worried that hardcoding IPNS-centric topic EOL will impact other use cases

I think a safe and scoped way is to allow defining optional EOL per namespace.

We use NewPubsubValueStore to initialize pubsub router in go-ipfs/core/node/libp2p/routing.go, and we could pass TTL per namespace there, do something like:

func PubsubRouter(mctx helpers.MetricsCtx, lc fx.Lifecycle, in p2pPSRoutingIn) (p2pRouterOut, *namesys.PubsubValueStore, error) {
	psRouter, err := namesys.NewPubsubValueStore(
		helpers.LifecycleCtx(mctx, lc),
		in.Host,
		in.PubSub,
		in.Validator,
		namesys.WithRebroadcastInterval(time.Minute),
+		namesys.WithUnusedSubscriptionTTL(36*time.Hour, "ipns"),
	)

So the idea is to have expiration logic engage only if key is from ipns namespace.

This way we would expire only unused IPNS topics, and have means of controlling TTL via go-ipfs-config (public gateways like eth.link that resolve many ipns names may want to expire subscriptions faster than desktop clients).

@aschmahmann mind eyeballing if the above is acceptable?
(my north star is to be able to enable ipns-over-pubsub by default)

pubsub.go Outdated Show resolved Hide resolved
@TobiaszCudnik
Copy link
Author

TobiaszCudnik commented Jan 20, 2022

Thanks for the review guys!

Having self-denotating topics (i.e. closing after 36hrs) is likely not the way to go here for GC.

The strategy is to re-publishing IPNS entries every DefaultRebroadcastInterval (in go-namesys/republisher/repub.go::Run), which will bump the subscriptions's EOL (in go-libp2p-pubsub-router/pubsub.go::Subscribe). Without being bumped the subscription will GC itself. This is tighter than manually unsubscribing.

[removed]

So the idea is to have expiration logic engage only if key is from ipns namespace.

Thats a valid point, I will pass the namespace down from go-ipfs to go-libp2p-pubsub-router, as suggested.

Edit: removed wrong info

@TobiaszCudnik
Copy link
Author

Ignore the part about publish / resolve above, Ive been debugging for too long and got it mixed up. The GC actually works well.

Changes:

  • TTL per namespace
  • tests
  • config entry Ipns.ReproviderDuration

Sibling PRs:

Ive used 1y for the default TTL to simplify the code and avoid nil pointers. Not sure if thats ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants