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

Connected notification handler races with identify protocol #358

Closed
whyrusleeping opened this issue Jun 24, 2019 · 3 comments
Closed

Connected notification handler races with identify protocol #358

whyrusleeping opened this issue Jun 24, 2019 · 3 comments

Comments

@whyrusleeping
Copy link
Contributor

As far as I can tell, identify and the dht 'check if this peer supports the dht protocol' happen concurrently. This doesnt bite us too bad, except for the fact that our 'happy path' for DHT peer detection relies on identify discovering protocols of the peers we talk to.

I noticed this when I was looking into using a peers reported listen addrs to determine if we should add them to our routing table.

We should probably use the event bus to signal 'identify complete' for peers, and have the DHT use that to find new peers to add to their table, instead of the connection notifications (i think this is the plan anyways, but just pointing it out)

@raulk
Copy link
Member

raulk commented Jun 26, 2019

Proposal:

  1. Add an event EvtPeerIdentifyComplete, emitted by identify once identify succeeds. EvtPeerIdentifyFailed if it times out.
  2. Have the DHT subscribe to those events.
  3. Park routing table candidates in a temporary map.
  4. When we receive EvtPeerIdentifyComplete, we evaluate inclusion in routing table as per don't add peers with only private addresses to your routing table #360.
  5. When we receive EvtPeerIdentifyFailed, we remove the candidate from the parking area.

@magik6k fancy implementing the emitter side?

@whyrusleeping
Copy link
Contributor Author

This means that we will be dropping our 'connected' notification handler

@raulk
Copy link
Member

raulk commented Jun 27, 2019

Fixed in #365. Will eventually be merged to master.

@raulk raulk closed this as completed Jun 27, 2019
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

No branches or pull requests

2 participants