waku-org / go-waku

Go implementation of Waku v2 protocol
https://waku.org/
Other
122 stars 42 forks source link

bug: publish messages without being subscribed #1207

Open richard-ramos opened 3 months ago

richard-ramos commented 3 months ago

As described in https://github.com/status-im/status-go/issues/5759#issuecomment-2305221720 by @igor-sirotin The code as it is right now would allow publishing a message while not being subscribed to a pubsub topic.

I suggest either checking if a subscription to the pubsub topic before publishing and return an error or maybe attempt to subscribe with this code. I kinda prefer to return an error instead since then the user of the library can choose to subscribe or not depending on the error (and also because publishing without being subscribed does look like an incorrect usage of gowaku)

//Check if gossipsub subscription already exists for pubSubTopic
        if !w.IsSubscribed(pubSubTopic) {
            _, err := w.subscribeToPubsubTopic(cFilter.PubsubTopic)
            if err != nil {
                //TODO: Handle partial errors.
                w.log.Error("failed to subscribe to pubsubTopic", zap.Error(err), zap.String("pubsubTopic", cFilter.PubsubTopic))
                return nil, err
            }
        }
kaichaosun commented 2 months ago

I found this piece of code which check similar logic, wondering why is it not working in this case? https://github.com/waku-org/go-waku/blob/master/waku/v2/protocol/relay/waku_relay.go#L285

igor-sirotin commented 2 months ago

wondering why is it not working in this case?

I think it works 🙂 Check the logs I attached in https://github.com/status-im/status-go/issues/5759#issuecomment-2305818102 - could not send message: not enough peers to publish.

The question is, should we maybe auto-subscribe to topic in such case.

kaichaosun commented 2 months ago

should we maybe auto-subscribe to topic in such case.

It seems we need a wrapper API for auto subscribe. what do you think? @richard-ramos

chaitanyaprem commented 2 months ago

should we maybe auto-subscribe to topic in such case.

It seems we need a wrapper API for auto subscribe. what do you think? @richard-ramos

it may not be so straight-forward, because when you subscribe to a pubsubtopic doesn't mean you have peers connected wrt topic. So, doing it as part of publish may not solve the issue rather mislead further.

either we subscribe and wait to have atleast 1 peer for topic(which itself is not gauranteed msg will be reliably propagated). or return an error so that caller can subscribe to topic and wait for it to be healthy before publishing. wondering if we ca use topic-health to decide whether to publish or not rather than min-peers. wdyt @richard-ramos

richard-ramos commented 2 months ago

I agree with Prem's observations about creating a wrapper not being straightforward. Subscribing to the principle of least surprise, i'd rather have the publish function return an error and handle it appropriately.

wondering if we ca use topic-health to decide whether to publish or not rather than min-peers

IMO yes. Using topic health is a pending topic we have for go-waku / status-go integration, so perhaps this is a good moment to work on this task!