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: lightpush & filter send requests to multiple peers #1779

Merged
merged 30 commits into from
Jan 24, 2024

Conversation

danisharora099
Copy link
Collaborator

@danisharora099 danisharora099 commented Jan 10, 2024

Problem

Currently for LightPush & Filter, we send requests only to 1 peer. This is not ideal as we want to increase decentralisation & reliability for our SDK. Reference issue: #1463

Solution

This PR, addressing #1463 partly, sends LightPush & Filter requests to multiple peers, defined as a constant in BaseProtocol.

Notes

  • Introduces the ServiceNodes wrapper class for ServiceNode and MessageCollector and provides helpers to account for redundancy in the latter
  • Adds a test to confirm the functionality for redundant usage of these two protocols.
  • Related to feat: SDK for redundant usage of filter/lightpush #1463
  • TODO:
    • Write moar tests for Filter & LightPush

): Subscription | undefined {
return this.activeSubscriptions.get(`${pubsubTopic}_${peerIdStr}`);
return this.activeSubscriptions.get(pubsubTopic);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

peerIdStr isn't required to be maintained as each Subscription object already corresponds to only one pubsub topic

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

information about the peers used for that protocol exist within the Subscription object, which, if necessary, can be accessed as a readonly property.

Copy link

github-actions bot commented Jan 11, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 32.59 KB (-0.01% 🔽) 652 ms (-0.01% 🔽) 120 ms (-51.47% 🔽) 772 ms
Waku Simple Light Node 298.88 KB (+0.17% 🔺) 6 s (+0.17% 🔺) 621 ms (+41.34% 🔺) 6.6 s
ECIES encryption 31.97 KB (0%) 640 ms (0%) 295 ms (+98.42% 🔺) 935 ms
Symmetric encryption 31.96 KB (0%) 640 ms (0%) 218 ms (-11.15% 🔽) 858 ms
DNS discovery 106.72 KB (0%) 2.2 s (0%) 398 ms (+35.59% 🔺) 2.6 s
Privacy preserving protocols 129.44 KB (0%) 2.6 s (0%) 655 ms (+234.93% 🔺) 3.3 s
Light protocols 30.62 KB (+1.56% 🔺) 613 ms (+1.56% 🔺) 190 ms (-30.04% 🔽) 802 ms
History retrieval protocols 29.08 KB (+0.16% 🔺) 582 ms (+0.16% 🔺) 179 ms (+59.81% 🔺) 760 ms
Deterministic Message Hashing 5.92 KB (0%) 119 ms (0%) 39 ms (-27.4% 🔽) 157 ms

@danisharora099 danisharora099 marked this pull request as ready for review January 19, 2024 12:21
@danisharora099 danisharora099 requested a review from a team as a code owner January 19, 2024 12:21
@@ -24,6 +24,7 @@ import { StreamManager } from "./stream_manager.js";
export class BaseProtocol implements IBaseProtocol {
public readonly addLibp2pEventListener: Libp2p["addEventListener"];
public readonly removeLibp2pEventListener: Libp2p["removeEventListener"];
readonly NUM_PEERS_TO_USE = 3;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it not configurable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. made it configurable!

@@ -27,3 +27,12 @@ export function messageHash(
}
return sha256(bytes);
}

export function messageHashStr(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember why this package was created but as I see it contains only one function.
Considering how it is used it can be safely moved to core and re-exported from sdk.

@waku-org/js-waku-developers wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe because it depends on @noble/hashes. But @waku/core already relies on the same as well. So perhaps a good idea to move it into @waku/core.
PR here to move this: #1810 where we can also continue this discussion if required.

packages/tests/package.json Outdated Show resolved Hide resolved
@danisharora099 danisharora099 merged commit 7affbe2 into master Jan 24, 2024
9 of 11 checks passed
@danisharora099 danisharora099 deleted the feat/redundant-sdk branch January 24, 2024 12:54
@weboko weboko mentioned this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants