Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

Service names #68

Merged
merged 6 commits into from
Feb 1, 2018
Merged

Service names #68

merged 6 commits into from
Feb 1, 2018

Conversation

richardschneider
Copy link
Contributor

@richardschneider richardschneider commented Jan 30, 2018

@ghost ghost added the status/in-progress In progress label Jan 30, 2018
@richardschneider
Copy link
Contributor Author

richardschneider commented Jan 31, 2018

@diasdavid had to make changes to the tests, please review them.

Also, how to add AppVeyor CI?

* This test is WRONG. Peers are async. How can you assume
* that the mdnsA.stop happens before mdnsC gets a response
* from mdnsA?
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richardschneider wanna fix the test then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it and could not come up with a test. How does one prove that something will not happen?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can stop and wait "two intervals" to make sure that indeed no more broadcasts happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@richardschneider
Copy link
Contributor Author

@diasdavid RTM

@daviddias daviddias merged commit fa8fe22 into master Feb 1, 2018
@daviddias daviddias deleted the names branch February 1, 2018 21:12
@ghost ghost removed the status/in-progress In progress label Feb 1, 2018
@lidel
Copy link
Member

lidel commented Jul 13, 2018

@richardschneider Hi! There is ongoing work on mDNS API for browser extensions and ipfs.local came up.

Is there a technical reason why this change was done, or was it just pure aesthetics? :)

Change the service name from _ipfs-discovery._udp to ipfs

Problem I: ipfs.local does not seem to follow DNS-SD spec (https://tools.ietf.org/html/rfc6763#section-7) which expects _ipfs._tcp.local for TCP and _ipfs._udp.local [sic!] for everything else.
This breaks things, as we can't reuse Firefox code compatible with mentioned spec.

Problem II: go version seems to be using _ipfs-discovery._udp, which breaks mDNS interop between go and js versions:
https://github.com/libp2p/go-libp2p/blob/f5e43430a1d0f4c1af97338537f3c9b0c7442354/p2p/discovery/mdns.go#L24

See mozilla/libdweb#7 (comment) for more details.

cc @jacobheun

@richardschneider
Copy link
Contributor Author

@lidel MDNS does not require DNS-SD nor does DNS-SD require MDNS. However, they can work togther.

I was wrong to use _ipfs.local it should be ipfs._tcp.local, thus making it comply with DNS-SD.

DNS-SD also requires a SRV and TXT resource record and expects A and/or AAAA records.

@lidel
Copy link
Member

lidel commented Jul 14, 2018

@richardschneider If we can comply with DNS-SD then we should, as it makes it more interoperable with how things work in Firefox and other tools.

I was going to PR to fix this, but not sure what value should we switch to.
go-ipfs is using _ipfs-discovery._udp.local, shouldn't we use the same?

@richardschneider
Copy link
Contributor Author

Let's move this discussion over to libp2p/libp2p#28.

The current consensus is _ipfs._upd.local.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants