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

RoutedHost should search for addrs when none of the addrs we tried worked #351

Closed
Stebalien opened this issue Jun 9, 2018 · 4 comments
Closed

RoutedHost should search for addrs when none of the addrs we tried worked #351

Stebalien opened this issue Jun 9, 2018 · 4 comments
Labels
effort/days Estimated to take multiple days, but less than a week kind/enhancement A net-new feature or improvement to an existing feature P2 Medium: Good to have, but can wait until someone steps up

Comments

@Stebalien
Copy link
Member

Currently, if we have at least one address, we try that and never search the DHT. We should search the DHT. Ideally, we'd search in parallel after some timeout but that's tricky. The simple solution is to:

  1. Try dialing with the addresses we have.
  2. If that fails, get some more addresses.
  3. If we now have new addresses, try dialing with those.
@Stebalien Stebalien mentioned this issue Jun 9, 2018
Closed
@jmintb
Copy link

jmintb commented Jun 20, 2018

I would like to have a crack at this task. I think it makes sense to also add tests that cover this case, there is no test file in the go-libp2p/p2p/host/routed folder. Should I put one there or is there somewhere else that the routed host is tested?

jmintb added a commit to jmintb/go-libp2p that referenced this issue Jun 24, 2018
@jmintb
Add test case for routed host
1198616 
This test case covers issue libp2p#351, which covers the routed
host searching for new addresses if the current addresses
for a peer do not work when connecting.

The test uses a mock testrouter which implements the routing
interface. This makes it easy to manipulate which addresses
are available through the router and also makes the test
self contained, meaning it isn't dependent on an external
router.
jmintb added a commit to jmintb/go-libp2p that referenced this issue Jun 24, 2018
@jmintb
Routed host searches for new addrs issue libp2p#351
3f77c5d 
If there were addresses available for a given peer,
and they did not work, an err would be returned.
This doesn't make sense as there should be an attempt
to find new addresses through the router before determining
that a connection cannot be made.

Now the router is searched if a connection with the current
addresses cannot be made. If the router had already been
searched(This happens if no addresses were present in the peerinfo)
then we do not search repeat the search as it doesn't to
search the router twice within such a short timespan.
@Stebalien
Copy link
Member Author

Should I put one there or is there somewhere else that the routed host is tested?

That's the correct place to test this (for now).

Note: The real fix will probably require some refactoring. I'm steadily coming to the conclusion that we should be passing a pluggable router down into the Network (swarm). Really, we should probably be wrapping the peerstore with a router.

However, that's not a simple fix...

@Stebalien Stebalien mentioned this issue Oct 5, 2018
Closed
@raulk
Copy link
Member

raulk commented Oct 24, 2018

With the new dial pipeline being worked on here libp2p/go-libp2p-swarm#88, a RoutedHost could use a Dial Planner that falls back to a DHT lookup when all addresses failed. It would be trivial to implement, I think.

@Stebalien Stebalien mentioned this issue Jul 22, 2021
Merged
@Stebalien Stebalien added effort/days Estimated to take multiple days, but less than a week kind/enhancement A net-new feature or improvement to an existing feature P2 Medium: Good to have, but can wait until someone steps up labels Jul 22, 2021
@marten-seemann
Copy link
Contributor

This was fixed by #1835.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
effort/days Estimated to take multiple days, but less than a week kind/enhancement A net-new feature or improvement to an existing feature P2 Medium: Good to have, but can wait until someone steps up
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@Stebalien @raulk @marten-seemann @jmintb