feat: new filter protocol increment - subscribe request handling

[jm-clius]

Background

This is a first increment towards the server-side implementation of the filter protocol changes proposed in vacp2p/rfc#562 with wire protocol in waku-org/waku-proto#16.

It only covers basic subscriber request handling.

Note that this is still an incomplete feature and isolated in code. Once feature-complete, the old filter implementation will be removed and replaced by this one (with simultaneous merging of the RFC). Test cases are also still mainly focused on happy-path cases, to keep this increment as simple as possible.

The major outstanding features are:

• message handling within the filter service node
• a client implementation

Dogfooding can likely start without a client implementation in nwaku, as nwaku is most often configured as a service node only.

Next steps

Further steps, roughly in order:

• message handling within filter service node
• a subscriptions sweeper to handle subscription maintenance (e.g. removing stale subscriptions, managing capacity, etc.)
• test cases that cover error and boundary conditions
• improved metrics and logging
• client implementation

[rymnc]

Can we feature flag this, similar to rln? Would save some CI time. It would also need to be under the EXPERIMENTAL flag, defined

nwaku/Makefile

Lines 98 to 104 in 43b3f1c

	EXPERIMENTAL ?= false
	EXPERIMENTAL_PARAMS ?= $(EMPTY)
	ifeq ($(EXPERIMENTAL), true)
	RLN := true
	endif

[jm-clius]

Can we feature flag this, similar to rln? Would save some CI time. It would also need to be under the EXPERIMENTAL flag, defined

This is currently not integrated anywhere or part of any CI, while incrementing towards the feature. The idea is that this implementation will completely replace the existing filter implementation within the next week or two, so there may be little value in having a separate build or flag (the existing filter flags will be used), although we could consider doing it if both implementations need to co-exist for a while (hopefully not).

[LNSD]

I see some details but overall makes sense to me. Feel free to take my comments as part of the upcoming increments.

[LNSD]

This is a tricky scenario: Shall we accept partially a subscription that "overflows" the maximum number of subscriptions?

In my opinion, we shouldn't. But here, despite erroring out, we are inserting in the subscriptions map. To avoid that situation, I would do the following:

• After checking the contentTopics.len == 0, I would add another check comparing the upper bound of the content topics length against a limit (e.g. MAX 20 subscriptions per query).
• Before L56 (constructing the filter criteria set), I would calculate the remaining "subscription slots" from the wf.subscriptions (a proc that returns this would be great) and check if the seq[ContentTopic] length is larger than the remaining slots. In that case, I would error out.

Once the request is sanitized, then I would do the insertion without worrying of running out of subscription slots.

[jm-clius]

In my opinion, we shouldn't.

Agreed! I also considered this scenario and think a request should either be fully honoured or fail (partially honouring a request is a nightmare in error handling from both client and service node's perspective)

here, despite erroring out, we are inserting in the subscriptions map

Wait, I don't see it or am missing something obvious. 😅 We return in L63 without doing the insert in L68 and L69. Good idea to return earlier though.

e.g. MAX 20 subscriptions per query

Good point. Will also clarify this as a recommendation so that we get reasonable filter requests from the beginning.

I would calculate the remaining "subscription slots

As mentioned above, sounds reasonable to me too to return earlier here, although I don't quite see that we're currently exceeding max criteria per peer subscription. Will address in a follow-up (the subscriptions map is likely to be split to a separate module soon in any case, as I can imagine we may want to make it a bit more complex to e.g. build a reverse index, add timestamps, a sweeper, etc.)

[LNSD]

Wait, I don't see it or am missing something obvious. We return in L63 without doing the insert in L68 and L69. Good idea to return earlier though.

My bad, I misread this part. I thought that mgetOrPut was modifying already the table's value. True. You bail out before setting the value back to the table.

[LNSD]

Following my previous comment, the problem is that we permit surpassing the MaxSubscriptions limit. But we don't do anything to prevent or fix the situation.

For security reasons, it is preferable to apply a prevention approach (like the one I described before) than a post-fix.

[jm-clius]

I agree re early checking (e.g. proc that quickly check if a new request can be honoured given current subscriptions/criteria-per-subscription capacity). Not sure though I see how we

permit surpassing the MaxSubscriptions limit

if we return immediately once we reach the == limit?

I will add the early check in any case in a follow-up though.

[LNSD]

if we return immediately once we reach the == limit?

I will add the early check in any case in a follow-up though.

I believe that doing a precondition check is the safest approach. So you avoid some necessary operations (and you save some CPU cycles).

[LNSD]

While this is less harmful than the subscription, I would still sanitize the "unsubscribe" request (e.g., limiting the length of the topic string, limiting the number of content topics, for example, to a maximum of 20, etc.).

I we accept a big list of topics, 90% of the time, the customers will use the maximum.

[jm-clius]

Good point. Will add to follow-up!

[alrevuelta]

lgtm! some minor comments/ideas.

[alrevuelta]

any reason why pubsubTopic is optional?

[jm-clius]

It's an optional in the request RPC since it's only mandatory when following the SUBSCRIBE workflow. I made the choice to do all request consistency checking within the utility function belonging to that request. In this case it is mandatory to have a pubsubTopic, so subscribe returns a bad request error if optional is none.

[alrevuelta]

just an idea:

• make FilterCriterion a table pubsubtopic->contenttopic
• easier to reason perhaps since contenttopic is a "subtopic" of gossipsub topic
• less likely that we store duplicates since someone subscribing to ctopic_1, ctopic_2 in gtopic_1 will require: (gtopic_1, ctopic_1), (gtopic_1, ctopic_2).
• faster access time perhaps?
• but the most important, easier to access subscriptions
• one type less

# renamed FilterCriterion to PeerSubscriptions
type
   PeerSubscriptions* = Table[PubsubTopic, HashSet[ContentTopic]]

and then WakuFilter

 WakuFilter* = ref object of LPProtocol
    subscriptions*: Table[PeerID, PeerSubscriptions]
    peerManager: PeerManager

wdyt?

[jm-clius]

Mmm. This is indeed how we previously modelled it - a map of peer IDs to a map of pubsub topics to content topics - but I made the (perhaps not final) decision to flatten this as much as possible. Reasoning: the subscription is actually against a contentTopic, so in this model the "real" subscription would be nested two levels deep - we just (mandatorily) require each contentTopic to be qualified against a specific pubsubTopic. Explicitly modelling the pubsub topic separately requires a bit more maintenance. In that case it would be invalid, for example, to subscribe/unsubscribe with filter criteria that contains only a pubsubTopic or for a subscription where a peer's Peer Subscription has an empty content topic set against any pubsub topic. We can build in rules around this, of course, but importantly we want to move away from a model where pubsubTopic is necessary as a qualifier at all.

I am planning possible changes to this model, though, as we may need something of a reverse map/index when looking up peers for a specific subscription. In this case the reverse index may be modelled as you suggested. Will still think about this, as you make a good point.

[alrevuelta]

not very likely to happen in reality, but if the connection is closed by the counterparty right before, this will throw an exception. I just verified empirically when modifying px.

https://github.com/waku-org/nwaku/blob/master/tests/v2/test_waku_peer_exchange.nim#L247

[jm-clius]

Thanks will address!

[alrevuelta]

related to the other comment, using two maps this would be something like this. like the wf.subscriptions[peerId][pubsubTopic] to address the ctopic.

if wf.subscriptions.hasKey(peerId):
  if wf.subscriptions[peerId].hasKey(pubsubTopic):
    for cTopic in contentTopics:
      wf.subscriptions[peerId][pubsubTopic].add(cTopic)
....

[jm-clius]

True. But to e.g. count number of total subscriptions and enforce limit you'd need to count the values of the second map - i.e. sum of content topics for all pubsub topics (easily doable of course, but slightly more roundabout when modelled).

[rymnc]

LGTM