waku-org / nwaku

Waku node and protocol.
Other
201 stars 53 forks source link

bug: #3165

Open vpavlin opened 3 weeks ago

vpavlin commented 3 weeks ago

Problem

When using js-waku and go-waku as clients and nwaku as a service node, We (I and @richard-ramos) noticed inconsistent behaviour with the genration in contentTopic

The flow is:

  1. Publish using lightPush from js-waku on contentTopic: /0/qaku/1/persist/json
  2. Subscribe to filter using go-waku on contentTopic: /qaku/1/persist/json
  3. Observe that messages are not pushed to the fitler node

After a lot of looking we realized the reason is the missing generation number which is optional (https://github.com/waku-org/specs/blob/b38f248b4fb56a442f609c605b9ab65471b5afed/standards/core/relay-sharding.md?plain=1#L195), but IMO the default should be added by the service node if not provided

Impact

Medium - it can be workarounded by using (or not using) the generation number across app implementations

To reproduce

As mentioned above

Expected behavior

Service node (nwaku) adds default generation number (0) if it is not included in the contentTopic

Screenshots/logs

If applicable, add screenshots or logs to help explain your problem.

nwaku version/commit hash

js-waku 0.0.29 go-waku v0.8.1-0.20240921011719-821481fec446 nwaku - waku.test and waku.sandbox fleets

CC @SionoiS @Ivansete-status

SionoiS commented 3 weeks ago

🤔 If we define /0/qaku/1/persist/json as equivalent to /qaku/1/persist/json why would the nwaku node add the 0, it's redundant.

It's true that the message should be relayed thought! On the other hand, the content topic generation should not affect the routing.

Can you check that all nodes use the same pubsub topic? This is weird...

SionoiS commented 3 weeks ago

Could it be that no impl. treat /0/qaku/1/persist/json as equivalent to /qaku/1/persist/json ?

I can't find the code that would do that for nwaku and also we use content topic as string in a lot of places so were F.

Who thought using string everywhere would be fine. Type systems exist for a reason... 😠