chore: [WIP-for opinions] move push logic to lightpush

[shash256]

Description

For gathering thoughts on whether to do this to resolve the unnecessary indirection created by the pushHandler, as per @jm-clius suggestion.
cc: @SionoiS @NagyZoltanPeter
If we agree to go ahead with this, will modify the tests and whatever else is impacted by this change

Changes

removed the pushHandler pattern in waku_node.nim by adding a reference to wakuRelay into the Lightpush module and moving all push logic to this module itself.

[github-actions]

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2712-rln-v1

Built from 9807c71

[github-actions]

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2712-rln-v2

Built from 9807c71

[NagyZoltanPeter]

Description

For gathering thoughts on whether to do this to resolve the unnecessary indirection created by the pushHandler, as per @jm-clius suggestion. cc: @SionoiS @NagyZoltanPeter If we agree to go ahead with this, will modify the tests and whatever else is impacted by this change

Changes

removed the pushHandler pattern in waku_node.nim by adding a reference to wakuRelay into the Lightpush module and moving all push logic to this module itself.

Well, what do we gain with tightening coupling between lightpush and relay services?
I think such a decoupling layer can help keep flexibility in implementation of different services and even testing.

I think the biggest advance with light puhs and relay would be to propagate more information to the users about the success of the action. Like how many relay peers got the message or if we could require a minimum number of peers for success, etc.

[SionoiS]

This means that to test WakuLightPush you need WakuRelay. Please, no.

IMO, only acceptable options are;

• some kind of interface (more boilerplate)
• a channel (not yet functional in Chronos)
• a callback (current solution)

[jm-clius]

Right, good points @SionoiS and @NagyZoltanPeter.

I would say that if lightpush is specified as something of a libp2p API specifically for Waku Relay, the inclusion of Waku Relay becomes an implicit dependency (in the same way as it is necessarily included for the Relay REST API, which is the most similar module to what lightpush provides). The problem with having layers of indirection, i.e. the lightpush server action being a callback defined and managed by the Waku node, is that it relegates a core part of the protocol implementation to node setup code. Now that we are planning to expand significantly the responsibilities of the lightpush server when pushing a message (starting with adding RLN proofs on behalf of light clients), most of the protocol will live outside the protocol module.

I do accept though that it may be premature to flatten the structure and introduce more composition for the lightpush node. @shash256 I think a good alternative might then just be to at least define the pushHandler for adding RLN proofs in a separate helper module rather than keeping building protocol logic into the ever-growing waku_ node module.

[gabrielmer]

Mmm I agree that Lightpush has Relay as a dependency so there is already an intrinsic coupling between both.
I'm personally in favor of maintaining their code as decoupled as possible until we see and take advantage of the benefits that the refactor would bring

[SionoiS]

I would say that if lightpush is specified as something of a libp2p API specifically for Waku Relay, the inclusion of Waku Relay becomes an implicit dependency

I disagree, Lightpush can and should work without Relay the least assumption about node configuration the better. In the most basic sense, the user is pushing a message to a node, if Relay is enabled the message is published, if Archive is enabled it is stored. If none are enabled then we disallow Lightpush at config time (same with REST).

The problem with having layers of indirection, i.e. the lightpush server action being a callback defined and managed by the Waku node, is that it relegates a core part of the protocol implementation to node setup code.

As I stated in my comment, this is the only solution that we can use while maintaining composability AND separation.
Can we ask the IFT to prioritize this async channel implementation? That would give us more tools at least.

Now that we are planning to expand significantly the responsibilities of the lightpush server when pushing a message (starting with adding RLN proofs on behalf of light clients), most of the protocol will live outside the protocol module.

In this case I would expect Lightpush to "ask" RLN for proofs and then "ask" Relay to publish the message (composability, separation & single responsibility).
May require a refactor IDK how the RLN component is structured.

[jm-clius]

Lightpush can and should work without Relay the least assumption about node configuration the better. In the most basic sense, the user is pushing a message to a node,

A fair way of abstracting lightpush as a protocol. My point is more general in that composability might be necessary if a protocol is indeed defined as dependent on another protocol. My feeling is that we've started moving in this direction for Lightpush. I think your point that it may be premature/unnecessary to introduce this coupling is valid and that lightpush is only a resource restricted way to ask a node to process a message in a variety of configurable ways, not necessarily to publish to a relay network (although I'd revise the spec in future to make this point clearer).

With the above model RLN-proofs-as-a-service becomes a specific push handler for Lightpush.

I would expect Lightpush to "ask" RLN for proofs and then "ask" Relay to publish the message (composability, separation & single responsibility).

If Lightpush is doing the asking, it's composing protocols :). But indeed a separate push handler can do this already with minimal changes to existing code.

[NagyZoltanPeter]

Lightpush can and should work without Relay the least assumption about node configuration the better. In the most basic sense, the user is pushing a message to a node,

A fair way of abstracting lightpush as a protocol. My point is more general in that composability might be necessary if a protocol is indeed defined as dependent on another protocol. My feeling is that we've started moving in this direction for Lightpush. I think your point that it may be premature/unnecessary to introduce this coupling is valid and that lightpush is only a resource restricted way to ask a node to process a message in a variety of configurable ways, not necessarily to publish to a relay network (although I'd revise the spec in future to make this point clearer).

With the above model RLN-proofs-as-a-service becomes a specific push handler for Lightpush.

I would expect Lightpush to "ask" RLN for proofs and then "ask" Relay to publish the message (composability, separation & single responsibility).

If Lightpush is doing the asking, it's composing protocols :). But indeed a separate push handler can do this already with minimal changes to existing code.

I think some level of abstraction is needed even if there is a tight connection on logic level between services.
We do need it for the future flexibility to let independently evolve the services and have a space where we can implement special treatments.
Maybe not the callback mechanism is the best I can agree, but still we need something.
One current example we recently added
- Added message size check before relay for lightpush ([#2695](https://github.com/waku-org/nwaku/issues/2695)) ([9dfdfa27](https://github.com/waku-org/nwaku/commit/9dfdfa27))
in 0.28.0 upcoming.
This means that we do check things twice just for being able to say some meaningful to lightpush user, even we know relay will run its validators anyway. So some level of layering the concept is needed, as you meantion RLN-as-a-service concept also needs such. Do we really want to let lightpush protocol know about relay / rln_relay / message validators... maybe even more?

[SionoiS]

What if we move the callbacks from wakunode into each service/component.

egg. RLN component would have a callback that return proofs, Autoshard component callback return shard of content topic, etc...

This way we don't clutter wakunode. The only setup would be to add the callbacks each components requires.

[fryorcraken]

One reminder (I believe you are all on the same page), is that we will push for libwaku in Status app, and we will want to support store and light push as a service in this context. So indeed, best to be wary of pushing too much glue code in waku node instead of libwaku

[jm-clius]

What if we move the callbacks from wakunode into each service/component.

Yes, I think that's a reasonable mid-way. I would separate these callbacks into a separate module for each component - e.g. waku_lightpush/callbacks - to prevent circular imports. The callbacks would have to import the waku_node.

[jm-clius]

@shash256 if we're not going to continue down this avenue, perhaps we can close this draft?

[shash256]

Seems like we have some form of consensus to move callbacks from wakunode into each component. This has been done for light push in #2768 . So closing this PR for now.