Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Enhance lightpush protocol error handling #2722

Open
6 tasks
Tracked by #3085
NagyZoltanPeter opened this issue May 23, 2024 · 6 comments
Open
6 tasks
Tracked by #3085

feat: Enhance lightpush protocol error handling #2722

NagyZoltanPeter opened this issue May 23, 2024 · 6 comments
Assignees
Labels

Comments

@NagyZoltanPeter
Copy link
Contributor

NagyZoltanPeter commented May 23, 2024

Problem

Deriving from [Epic] Enhance light push protocol there is a certain need to enhance lightpush protocol error handling and request-responses to allow users properly answer to certain situation.

Suggested solution

Define proper Response RPC that reflects all possible edge cases.

  • service node does not have enough relay peer in this mesh for this pubsub topic
  • service node not subscrived to this pub sub topic
  • message pushed is invalid/malformed
  • timestamp out of range
  • (future) RLN proof is invalid (or could be same as above)
  • (future) this content topic is not served

For success case:

  • return number of peers to whom the message was forwarded.

Hint:

Because of the lack of proper error propagation from GossipSub layer we probably will need to duplicate some checks to filter out such conditions.
With extending the waku_relay interface it is possible to reuse GossipSub level validators that can check against the message whished to push.

Additional context

This is a breaking change, that must be tested and introduced with care and in alignment with other subsidiaries like js-waku/go-waku.

Acceptance criteria

  • Proposed RPC change must be introduced in RFC
  • REST Api adapton
  • Update REST-API definition repository
  • All lightpush tests are adopted
  • Tests for different edge cases added.
  • js-waku integration tests adopted to change and runs ok
@NagyZoltanPeter
Copy link
Contributor Author

@chair28980 : Please help me add/create proper epic label for this issue. Thank you.

@Ivansete-status Ivansete-status added the effort/weeks Estimated to be completed in a few weeks label May 28, 2024
@Ivansete-status Ivansete-status moved this to To Do in Waku May 28, 2024
@Ivansete-status
Copy link
Collaborator

@NagyZoltanPeter - Thanks for creating that!
I have the impression that we don't need to create a new RFC.
It suffices with what we have, string for error cases, and we should just fill in the error string correctly on each case, isn't it?

@NagyZoltanPeter
Copy link
Contributor Author

NagyZoltanPeter commented May 28, 2024

@NagyZoltanPeter - Thanks for creating that!
I have the impression that we don't need to create a new RFC.
It suffices with what we have, string for error cases, and we should just fill in the error string correctly on each case, isn't it?
Not exactly. We need to extend the protocol as discussed with @jm-clius. We need an ok answer with numbers of node messages relayed to.
For error cases I think we do better to make it confirm with other protocols, where we have exact error codes along with error messages. We start having to differentiate a lot of error cases that clients need to reflect on. I think proper error codes are better for such. Also@jm-clius mentioned that later we may want light push requests to serve different push needs, so it may be better to add placeholder now and later no clients need to be modified with a protobuf change.

I will add a PR on the existing light push RFC this evening.

@jm-clius
Copy link
Contributor

One more thing that will be great to be rid of is the unnecessarily nested PushRPC.

The protocol can simply be a LightpushRequest and LightpushResponse with request_id embedded into each. No need for neswting.

@NagyZoltanPeter
Copy link
Contributor Author

@Ivansete-status
update 27.08.24.:
Left to do:

  • work on libp2p - gossip layer to provide information about possible processing failures or validation results.
  • introduce non-intrusive observers to enable fast path for relaying.
  • some more test-case will be needed.

@Ivansete-status
Copy link
Collaborator

It is blocked until the new implementation in nim-libp2p is completed ( cc @NagyZoltanPeter )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Blocked
Development

No branches or pull requests

4 participants