libp2p / go-libp2p-pubsub

The PubSub implementation for go-libp2p
https://github.com/libp2p/specs/tree/master/pubsub
Other
321 stars 185 forks source link

remove panic inducing depencency #338

Closed fxfactorial closed 4 years ago

fxfactorial commented 4 years ago

I use:

blklist, _ := libp2p_pubsub.NewTimeCachedBlacklist(15 * time.Minute)
libp2p_pubsub.WithBlacklist(blklist),
pubsub, err := libp2p_pubsub.NewGossipSub(ctx, p2pHost, options...)

and use the pubsub.BlacklistPeer(peer) but the upstream dep on whyrusleeping imo is bad because banning peers more than once, the same peer, causes a panic on add, so to use it right, I'd need to hold the ref to the blacklist, call contained before calling add, kinda too much of a hassle to accomdate this API.

func (tc *TimeCache) Add(s string) {
    _, ok := tc.M[s]
    if ok {
        panic("putting the same entry twice not supported")
    }

from timecache.go

Please implement blacklist without a panic causing lib

vyzo commented 4 years ago

This is actually very easy to fix: just add a Has check in the implementation of the timecache backed blacklist. Care for a patch?

vyzo commented 4 years ago

Or we might try fixing the upstream library directly to support multiple puts.

fxfactorial commented 4 years ago

I think fix upstream is better but can it be done quick ? I don’t know that upstream dep , how quick it can be updated

vyzo commented 4 years ago

fixing upstream a little more complicated I think.

fxfactorial commented 4 years ago

Right then we’re back at my op about soln and it’s only diff if I put it app layer or in your layer at libp2p, sounds like latter? If so , then ya , I’ll open PR

vyzo commented 4 years ago

It is better if we put it in the pubsub library; a pr would be very welcome!

fxfactorial commented 4 years ago

https://github.com/libp2p/go-libp2p-pubsub/pull/340