chore: choose peers from the connected pool, instead of dialing unconnected peers #2096
Conversation
danisharora099
force-pushed
the
fix/base-protocol
branch
from
fc45fd4
to
88a29c3
Compare
size-limit report 📦
|
|
Good initiative, we discussed this 2 weeks ago and reached some solution.
The problem was that when a protocol was trying to Also, there were some other problems that #2065 has solved, e.g it happened that As for solution we agreed that following logic makes sense:
|
danisharora099
force-pushed
the
fix/base-protocol
branch
from
15400a5
to
2b1b3ae
Compare
The errors received from nodes are RLN limits being hit. I suggest to iteratively dogfood and improve once we have infra sorted + bug tickets filed. |
danisharora099
force-pushed
the
fix/base-protocol
branch
from
2b1b3ae
to
9a5e3f3
Compare
| this.log.warn("No new peers found."); | ||
| this.log.warn("No new peers found, trying with bootstrap peers"); | ||
| newPeers = await this.core.getPeers({ | ||
| maxBootstrapPeers: numPeers, |
There was a problem hiding this comment.
shouldn't previous line return all bootstrap peers already?
I mean 0 is for returning all peers, right?
| @@ -198,11 +198,6 @@ export class BaseProtocolSDK implements IBaseProtocolSDK { | |||
| this.log.info(`Finding and adding ${numPeers} new peers`); | |||
| try { | |||
| const additionalPeers = await this.findAdditionalPeers(numPeers); | |||
| const dials = additionalPeers.map((peer) => | |||
| @@ -227,10 +222,17 @@ export class BaseProtocolSDK implements IBaseProtocolSDK { | |||
| private async findAdditionalPeers(numPeers: number): Promise<Peer[]> { | |||
| this.log.info(`Finding ${numPeers} additional peers`); | |||
| try { | |||
| let newPeers = await this.core.allPeers(); | |||
| let newPeers = await this.core.getPeers({ | |||
There was a problem hiding this comment.
did you try this in practice, e.g dogfooding app?
I used allPeers because getPeers were not returning everything, and was limited for some reason
|
@danisharora099 should this PR be marked as draft as per waku-org/pm#186 (comment) ? |
|
pretty much continued by newer efforts related to protocol peer management #2137 |
Problem
With #2065, we changed the logic to fetch peers for a protocol from choosing from connected peers to finding all available peers (unconnected), and attempting to dial them instead.
This does not seem correct considering if the peer was dialable, it should have been connected already. If the peer is actually healthy, we can assume it has a connection open.
Solution
We should find connected peers that support the given protocol, and do not have a stream opened for that protocol yet, and then connect over that protocol.
Notes
I understand this change in #2065 was introduced because it was seen that in practicality peers weren't found when using this strategy, which seems like there is actually something else preventing it.
Will do further investigations.
This is also similar to #1758.