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.

Sign up for GitHub

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

Jump to bottom

feat(filter): enhancing protocol peer management with mutex locks #2137

Merged
merged 35 commits into from
Oct 10, 2024
Merged

feat(filter): enhancing protocol peer management with mutex locks #2137

merged 35 commits into from
Oct 10, 2024

Conversation

danisharora099
Copy link
Collaborator

@danisharora099 danisharora099 commented Sep 17, 2024 •
edited
Loading

Problem

The current implementation showcased a lot of inconsistencies around peer management for protocols.
Multiple weird behaviour were observed:

  • protocol not able to find new peers from available pool, while it truly existed
  • peer renewal for a protocol was unsuccessful about the Peer Manager count didn't reflect that
  • protocol sometimes showed a count of available peers much greater than the total actually connected peers

Solution

  • Implemented mutex locks to ensure thread-safe protocol management
  • Refactored peer management methods in the BaseProtocol class to enhance performance and reliability
  • Improved error handling and logging in the FilterCore class
  • Use async-mutex for all locks-related chores

Notes

Contribution checklist:

  • covered by unit tests;
  • covered by e2e test;
  • add ! in title if breaks public API;

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 17, 2024 •
edited
Loading

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 85.46 KB (+1.47% 🔺) 1.8 s (+1.47% 🔺) 1.4 s (+37.22% 🔺) 3.1 s
Waku Simple Light Node 137.5 KB (+1.86% 🔺) 2.8 s (+1.86% 🔺) 1.6 s (+9.38% 🔺) 4.4 s
ECIES encryption 22.88 KB (-0.3% 🔽) 458 ms (-0.3% 🔽) 595 ms (-15.42% 🔽) 1.1 s
Symmetric encryption 22.37 KB (-0.12% 🔽) 448 ms (-0.12% 🔽) 391 ms (+32.42% 🔺) 838 ms
DNS discovery 72.9 KB (+0.87% 🔺) 1.5 s (+0.87% 🔺) 1.4 s (+52.9% 🔺) 2.8 s
Peer Exchange discovery 74.1 KB (+0.3% 🔺) 1.5 s (+0.3% 🔺) 1.2 s (+40.31% 🔺) 2.7 s
Local Peer Cache Discovery 68.03 KB (+0.6% 🔺) 1.4 s (+0.6% 🔺) 2.3 s (+182.33% 🔺) 3.7 s
Privacy preserving protocols 75.99 KB (+0.51% 🔺) 1.6 s (+0.51% 🔺) 1.7 s (-19.61% 🔽) 3.2 s
Waku Filter 79.75 KB (+1.5% 🔺) 1.6 s (+1.5% 🔺) 1.1 s (-38.92% 🔽) 2.7 s
Waku LightPush 77.77 KB (+1.23% 🔺) 1.6 s (+1.23% 🔺) 1.5 s (+0.72% 🔺) 3 s
History retrieval protocols 77.06 KB (+1.27% 🔺) 1.6 s (+1.27% 🔺) 967 ms (-15.28% 🔽) 2.6 s
Deterministic Message Hashing 7.39 KB (+0.15% 🔺) 148 ms (+0.15% 🔺) 171 ms (-14.07% 🔽) 319 ms

@danisharora099 danisharora099 force-pushed the fix/peer-management branch 2 times, most recently from 4de00a5 to aeb05cd Compare September 24, 2024 05:58
@danisharora099 danisharora099 mentioned this pull request Sep 26, 2024
Merged
@danisharora099 danisharora099 changed the title chore: fix feat: enhancing protocol management with mutex locks Sep 30, 2024
@danisharora099 danisharora099 marked this pull request as ready for review October 1, 2024 13:18
@danisharora099 danisharora099 requested a review from a team as a code owner October 1, 2024 13:18
weboko
weboko reviewed Oct 2, 2024
View reviewed changes
packages/sdk/src/protocols/filter/constants.ts
@@ -1,4 +1,4 @@
export const DEFAULT_KEEP_ALIVE = 30 * 1000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally we should be fine with increasing this number, not decreasing.
How did you reach this number?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No particular reasoning. We can change it. 30S was a guesstimate as well

weboko
weboko reviewed Oct 2, 2024
View reviewed changes
packages/tests/tests/light-push/peer_management.spec.ts
{
payload: utf8ToBytes("Hello_World")
},
{ forceUseAllPeers: true }
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 this added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so that PeerManager is forced to find all peers until numPeersToUse which awaits the process instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, then I think forceUseAllPeers is a bit misleading - I thought it will use all the peers

can be a follow up

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not important for now considering PeerManager is removed from LightPush: #2137 (comment)

weboko
weboko requested changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@weboko weboko left a comment

Choose a reason for hiding this comment

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

added comments

@danisharora099 danisharora099 requested a review from a team October 3, 2024 11:04
@danisharora099 danisharora099 mentioned this pull request Oct 3, 2024
Merged
@danisharora099 danisharora099 changed the title feat: enhancing protocol management with mutex locks feat(filter): enhancing protocol management with mutex locks Oct 9, 2024
@danisharora099 danisharora099 mentioned this pull request Oct 10, 2024
Closed
weboko
weboko reviewed Oct 10, 2024
View reviewed changes
packages/sdk/.mocharc.cjs Outdated Show resolved Hide resolved
weboko
weboko reviewed Oct 10, 2024
View reviewed changes
packages/sdk/package.json Outdated Show resolved Hide resolved
weboko
weboko reviewed Oct 10, 2024
View reviewed changes
packages/sdk/package.json Outdated Show resolved Hide resolved
weboko
weboko reviewed Oct 10, 2024
View reviewed changes
packages/sdk/tsconfig.json Outdated Show resolved Hide resolved
@weboko
Copy link
Collaborator

weboko commented Oct 10, 2024 •
edited
Loading

please, address #2137 (comment) and #2137 (comment) before merging

from code perspective seems fine but after dogfooding it I see following critical issue - filter doesn't work at all as the peers are constantly getting renewed (I see it happens due to checkAndRenewPeers):
image

Let's disable reliabilityManager for Filter in this PR and I will follow up on it in #2158

Used version https://github.com/waku-org/js-waku/actions/runs/11249833553/job/31277485871

weboko
weboko reviewed Oct 10, 2024
View reviewed changes
packages/sdk/src/protocols/base_protocol.ts
}
}

class RenewPeerLocker {
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 this removed?

@weboko weboko mentioned this pull request Oct 10, 2024
Open
@danisharora099 danisharora099 changed the title feat(filter): enhancing protocol management with mutex locks feat(filter): enhancing protocol peer management with mutex locks Oct 10, 2024
@weboko weboko merged commit b2efce5 into master Oct 10, 2024
12 of 13 checks passed
@weboko weboko deleted the fix/peer-management branch October 10, 2024 21:47
@weboko weboko mentioned this pull request Oct 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weboko weboko weboko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: Filter subscriptions are not stable and missing messages bug: inconsistent protocol peer management
2 participants
@danisharora099 @weboko