PubsubValueStore.PutValue receives a context.Context but does not respect it. In particular, we have:

which uses the PubsubValueStore's context instead of the PutValue context when calling the very expensive bootstrapPubsub function.

Additionally, bootstrapPubsub could be much less expensive if:

was moved into the go func right below it.

After speaking with @Stebalien my current plan is to start by moving the Provide into the go func, and then look into moving bootstrapping into PubSub via the Discovery (https://github.com/libp2p/go-libp2p-discovery) interface, as indicated by:

@vyzo @raulk Any thoughts or issues with this?

Related to: libp2p/go-libp2p-kad-dht#151.

I'm starting to think we should use a DHT record service instead of a datastore. The record service would end up owning the validator and would handle all record updates/validation internally.

in GossipSub, there is a MessageSize limit. Someone told me to use this package to avoid bandwith waste.

If for some reason the subscription dies or gets cancelled without being managed by PubsubValueStore.Cancel(key) then we can't resubscribe until we first call PubSubValueStore.Cancel(key).

If instead of just cancelling the subscription as above we triggered an internal cancellation function that would likely be better.

Currently, the local user can use this to send out an invalid record. This isn't a problem from a security standpoint (nobody will accept the record) but we still shouldn't allow it.

This is an extraction of the IPNS over pubsub module as a general-purpose ValueStore. It can be upgraded to a full-blown router with go-libp2p-routing-helpers (which, before you get scared away by all the code, are thoroughly tested).

Recent pubsub changes make it so that you can only join a topic once per instance of go-lib2p-pubsub.PubSub. If you provide a PubSub instance to this pubsub router, and then in a different goroutine try to join the same topic, you will get an error.

The latest changes to the way go-libp2p-pubsub (which personally I like) means you need to implement topic management yourself (as you must do now) or have go-libp2p-pubsub implement internal topic management, that ensures when you call Join on a topic, you dont actually attempt to create multiple topic handlers.

It is entirely plausible that one might want to also process messages coming in on the topic subscription both in this library, and other ones.

Personally speaking I have made the following changes to this library:

// MinimalPubsub allows us to provide the bare minimum pubsub functionality
// to the router, while still using the same pubsub instance in other goroutines.
// This is primarily done to allow callers to not have to worry about topic management
// due to topics only allowing a single "joiner".
type MinimalPubsub interface {
	RegisterTopicValidator(topic string, val pubsub.Validator, opts ...pubsub.ValidatorOpt) error
	Join(topic string, opts ...pubsub.TopicOpt) (*pubsub.Topic, error)
}

// NewPubsubValueStore constructs a new ValueStore that gets and receives records through pubsub. Ignore the `zap.Logger` argument, my fork uses the zap logging library directly
func NewPubsubValueStore(ctx context.Context, host host.Host, ps MinimalPubsub, validator record.Validator, logger *zap.Logger, opts ...Option) (*PubsubValueStore, error) {...}

Then in my custom ipfs node, I wrote a shim around go-libp2p-pubsub.PubSub that handles shared access to a pubsub topic by multiple goroutines (ie, go-libp2p-pubsub-router subscribed to a topic, and some arbitrary service using the same pubsub instance is performing processing and messages coming in on the topic)

Currently the PubsubValueStore assumes Pubsub is distributing a single Key-Value pair which is updating by utilizing a best function that is passed in via the Validator interface.

This looks like:

bestVal, updated := best(currentVal, receivedVal)
if updated {
   data[key]=bestVal
   Publish(bestVal)
}

Instead it would be better to use an interface that allows for:

updatedState, delta := merge(currentState, receivedState)
if delta != nil {
   data[key]=updatedState
   Publish(delta)
}

Given that the PubsubValueStore assumes Get and Put of full data records (as opposed to deltas) it's probably fine to assume that the types of the state and deltas are the same. However, if we wanted we could also add a diff function that takes the states and computes a delta.

Some example merge functions:

• Best (as we have now)
• Union (if the record type is a set take the union of the set)
• BestN (do a union, but only keep the best N records)

Having this (and landing #33) would make an implementation of the suggestion at libp2p/go-libp2p#694 (comment) fairly straightforward.

It uses a context with no deadline, which does not operate well with DHT providing:

goroutine 13313 [9m0s]:
github.com/libp2p/go-libp2p-kad-dht.(*IpfsDHT).Provide [0xc0001888f0, 0x1a534a0, 0xc0151884c0, 0xc002cf0ed0, 0x24, 0xc0003d2601, 0x0, 0x0]
	/root/gopath/pkg/mod/github.com/libp2p/[email protected]/routing.go:422
github.com/libp2p/go-libp2p-pubsub-router.bootstrapPubsub.func1 [0x7f2feb060d90, 0xc0001888f0, 0x1a534a0, 0xc0151884c0, 0xc002cf0ed0, 0x24, 0xc0181b8f50, 0x47]
	/root/gopath/pkg/mod/github.com/libp2p/[email protected]/pubsub.go:338
created by github.com/libp2p/go-libp2p-pubsub-router.bootstrapPubsub []
	/root/gopath/pkg/mod/github.com/libp2p/[email protected]/pubsub.go:337

This router doesn't currently persist the records it finds to disk. We should consider doing that but this may depend on transaction support.

I have noticed that a lot of the libp2p library has changed the dependences of github.com/libp2p/go-libp2p-core to github.com/libp2p/go-libp2p/core . But why just only this one doesn't?
Hope this dependence can be update. Thanks.

This seems to be getting progressively worse over time...some tests are hanging and timing out waiting for propagation, see e.g. this run:

https://github.com/libp2p/go-libp2p-pubsub-router/runs/4035361341?check_suite_focus=true

If we call PutValue for a key we're not subscribed to, we won't record the record locally (which means it may not resolve through GetValue).