-
Notifications
You must be signed in to change notification settings - Fork 47
feat: peer exchange discovery improvements #2537
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
Conversation
…covery, move DNS E2E Tests to separate folder, remove not needed E2E tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements significant improvements to the peer exchange discovery functionality to address questionable behavior observed in tests and enhance the cold/warm start behavior of js-waku.
- Increases query window from 10 to 60 peers per query and implements 30-second recurring queries
- Replaces the existing peer exchange implementation with a more efficient one that removes dead code
- Adds comprehensive unit tests and re-enables previously commented E2E tests
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/tests/tests/peer-exchange/query.spec.ts | Completely removed flaky test file that was causing CI issues |
| packages/tests/tests/peer-exchange/peer_exchange.spec.ts | Updated tests to use new API and re-enabled previously skipped tests |
| packages/tests/tests/peer-exchange/pe.optional.spec.ts | Moved DNS discovery test to appropriate location |
| packages/tests/tests/peer-exchange/continuous_discovery.spec.ts | Removed obsolete continuous discovery test file |
| packages/tests/tests/peer-exchange/compliance.spec.ts | Updated to use new peer exchange implementation |
| packages/tests/tests/dns-discovery/dns_discovery.optional.spec.ts | New DNS discovery test file moved from peer exchange |
| packages/tests/tests/dns-discovery/dns-peer-discovery.spec.ts | Fixed import path |
| packages/tests/tests/connection-mananger/discovery_dialer.spec.ts | Re-enabled discovery dialer tests |
| packages/discovery/src/peer-exchange/waku_peer_exchange_discovery.ts | Removed old peer exchange discovery implementation |
| packages/discovery/src/peer-exchange/peer_exchange_discovery.ts | New streamlined peer exchange discovery implementation |
| packages/discovery/src/peer-exchange/peer_exchange_discovery.spec.ts | Comprehensive unit tests for new implementation |
| packages/discovery/src/peer-exchange/peer_exchange.ts | Refactored peer exchange class with improved error handling |
| packages/discovery/src/peer-exchange/peer_exchange.spec.ts | Unit tests for the peer exchange class |
| packages/discovery/src/peer-exchange/index.ts | Updated exports to use new implementation |
| packages/discovery/src/peer-exchange/constants.ts | New constants file with updated peer request limits |
| packages/discovery/src/index.ts | Updated exports structure |
| packages/discovery/src/dns/dns.spec.ts | Removed debug console.log statement |
size-limit report 📦
|
3861807 to
3e80fda
Compare
Problem / Description
We observed some questionable behavior of PX discovery in tests and a lot of them were commented for a while.
For example:
https://github.com/waku-org/js-waku/blob/459fe96fe6beeec61f1403829b49bd07d2d559ef/packages/tests/tests/peer-exchange/query.spec.ts#L144
Additionally while investigating how
js-wakubehaves during cold/warm start - I observed that behavior can be improved.Solution
This PR brings following changes:
Notes
Resolves:
Relevant to: #2276
Checklist