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

bring back the ability to specify peer in lightpush.send #1695

Open
vpavlin opened this issue Oct 27, 2023 · 3 comments
Open

bring back the ability to specify peer in lightpush.send #1695

vpavlin opened this issue Oct 27, 2023 · 3 comments
Labels
enhancement New feature or request

Comments

@vpavlin
Copy link
Member

vpavlin commented Oct 27, 2023

This is a change request

Problem

Ability to configure peerId in LightPush.send was removed here: 81a52a8#diff-151faa41cf9f93c502df4cb30b090b41d2cadd6eba341246169e28dc7efdc666R100 in favour of peer-exchange (AFAIU)

I believe this should be revisited as the behaviour I am seeing is that js-waku will not really attempt to use different peer when send() fails and I as a user have no ability to change this.

There are multiple scenarios, but one I observed recently:

  1. observe node dropping from libp2p.getConnections() list
  2. call lightpush.send(..)
  3. call fails with some error (for me it is usually Remote peer fault)
  4. retry lightpush.send(..) - it keeps failing for unspecified amount of time/number of retries
  5. the dropped node comes back and connection is reestablished
  6. succeed eventually

I believe that if I would be able to specify a node (e.g. by randomly selecting from libp2p.getConnections() list, I could successfully send the message earlier - i.e. provide better user experience

Proposed Solutions

Bring back the ability to specify optional peerId in lightpush.send()

Notes

@fryorcraken fryorcraken added this to Waku Oct 27, 2023
@fryorcraken
Copy link
Collaborator

fryorcraken commented Oct 30, 2023

To solve this, I expect js-waku to connect to 3 light push peers and send the message to all 3 peers to ensure reliability.

I am not in favor of getting the user to pass a peer id... where would it find the peer id? by collecting "connection" event and basically reimplementing the js-waku connection manager and peer store?

I thought @danisharora099 already implemented this for light push, am I incorrect?

The more advance version would be to also disconnect from peers who are the only one to report an error when using light push.

@weboko weboko moved this to To Do in Waku Oct 31, 2023
@danisharora099
Copy link
Collaborator

danisharora099 commented Nov 1, 2023

I thought @danisharora099 already implemented this for light push, am I incorrect?

While we introduced the ability to send requests to multiple peers in one of the PRs previously, the issue that tracks this exact work is #1463

If this is priority, we can do a PR for just lightpush (which is more straightforward), since other protocols like Filter would need some additional work to account for redundancy.

@fryorcraken
Copy link
Collaborator

If this is priority, we can do a PR for just lightpush (which is more straightforward), since other protocols like Filter would need some additional work to account for redundancy.

I'd prioritize for light push as it's easier and hopefully fix an issue that hacker might have.

@vpavlin do you want that to be scoped under the "presetnation readiness" epic?

@weboko weboko added the enhancement New feature or request label Nov 27, 2023
@chair28980 chair28980 moved this from To Do to Icebox in Waku Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Icebox
Development

No branches or pull requests

4 participants