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(kad-dht): dht-clients can ignore peers that don't have supported transports #1956

Closed
SgtPooki opened this issue Aug 10, 2023 · 1 comment
Labels
need/triage Needs initial labeling and prioritization

Comments

@SgtPooki
Copy link
Member

SgtPooki commented Aug 10, 2023

Porting over libp2p/js-libp2p-kad-dht#481

This issue is specifically for improving the in-browser experience when using js-libp2p

Due to browsers having a limited capacity for dialing, we should enable the restricting the potential invalid dials as much as possible when running in the browser. I believe this improvement could significantly help with ipfs/helia#182

Original problem:

it seems like the distance calculation has no awareness of peers that may not have valid (for my node) transports, which could easily result in filling up closestPeers to ‘this.capacity’ that I can’t talk to because they don’t talk the same transport as me.

When running as a DHT-client, we should add a config option that can prevent adding peers to peerDistanceList unless that peer has a multiaddr of a supported transport. i.e. this.transportManager.transportForMultiaddr(addr.multiaddr)) see

async * getClosestPeers (key: Uint8Array, options: QueryOptions = {}): AsyncGenerator<DialPeerEvent | QueryEvent> {
this.log('getClosestPeers to %b', key)
const id = await utils.convertBuffer(key)
const tablePeers = this.routingTable.closestPeers(id)
const self = this // eslint-disable-line @typescript-eslint/no-this-alias
const peers = new PeerDistanceList(id, this.routingTable.kBucketSize)
await Promise.all(tablePeers.map(async peer => { await peers.add(peer) }))

Exceptions or potential problems

note from @achingbrain regarding an exception/issue here when running as DHT server:

it's a nice optimisation to only add peers that we can actually dial, but if you are a DHT server and another peer asks you for peers closer to a given KAD ID, you might have previously excluded peers that you can't dial, but that the querying peer can.

@SgtPooki SgtPooki added the need/triage Needs initial labeling and prioritization label Aug 10, 2023
@SgtPooki SgtPooki changed the title feat(kad-dht): dht-clients _can_ ignore peers that don't have supported transports feat(kad-dht): dht-clients can ignore peers that don't have supported transports Aug 10, 2023
@achingbrain
Copy link
Member

Another thing to consider here is that when getting the closest peers, you're typically doing this to ask one to store a provider record or to retrieve it.

This query has to be replicable by another node on the network - the intermediate states of the peer distance list don't matter but the final list of peers does.

If the other peer has a different set of supported transports and they finish their query with a different set of peers as the "closest" they won't be able to resolve the provider record since they'll be asking the wrong peers.

@SgtPooki SgtPooki closed this as not planned Won't fix, can't repro, duplicate, stale Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/triage Needs initial labeling and prioritization
Projects
None yet
Development

No branches or pull requests

2 participants