fix(swarm): add dnsaddr support in swarm connect #5535

overbool · 2018-09-28T01:44:31Z

License: MIT
Signed-off-by: Overbool overbool.xu@gmail.com

michaelavila · 2018-09-28T18:44:40Z

core/commands/swarm.go

+		if err != nil {
+			return nil, err
+		}
+		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)


I've looked through this whole PR and everything makes sense to me 👍. I do have a question about how these timeouts are specified in go-ipfs (both here and more generally). I notice lines like the following (and there are a handful):

https://github.com/ipfs/go-ipfs/blob/master/core/bootstrap.go#L60

And also lines like yours (e.g.):

https://github.com/ipfs/go-ipfs/blob/master/core/commands/pubsub.go#L178

Is there a rule of thumb for deciding when to specify these timeouts with a name vs inline?

Well, we should probably name this. We just get lazy sometimes.

Stebalien

So, this subtly changes some important semantics. Now, if I run ipfs swarm connect /dnsaddr/a.b.c/ipfs/QmPeer, we'll insert the resolved addrs into the peerstore instead of the dnsaddr. That means future connects won't try resolving the dnsaddr again. They'll instead use the resolved addresses, never learning about any changes to this DNS record.

I wonder if we should instead:

For each address that doesn't end in /ipfs/Qm..., resolve it.
After resolving it, parse it but ignore everything but the peer ID.
Create a IPFSAddr structs out of the original (unresolved) dnsaddrs and these peer IDs.

Thoughts @hsanjuan @overbool?

Stebalien · 2018-09-28T21:23:55Z

core/commands/swarm.go

+		if err != nil {
+			return nil, err
+		}
+		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)


Well, we should probably name this. We just get lazy sometimes.

Stebalien · 2018-09-28T21:26:15Z

core/commands/swarm.go

+			return nil, err
+		}
+		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+		raddrs, err := madns.Resolve(ctx, maddr)


Can we parallelize this?

Stebalien · 2018-09-28T21:27:05Z

core/commands/swarm.go

+		if len(raddrs) == 0 {
+			return nil, fmt.Errorf("non-resolvable multiaddr about %v", maddr)
+		}
+		maddrs = append(maddrs, raddrs...)


I believe we may need to filter out addresses that still don't end in /ipfs/Qm....

hsanjuan · 2018-09-29T00:00:28Z

we'll insert the resolved addrs into the peerstore instead of the dnsaddr.

Maybe this has changed, but do unresolved addresses in the peerstore get resolved when building a connection? My impression when I was testing this a while ago is they didn't. I remember a discussion or PR in which @lanzafame was involved (??) suggesting to do this at a lower level in libp2p.

A few months back, addresses were only resolved in basichost.Connect (https://github.com/libp2p/go-libp2p/blob/master/p2p/host/basic/basic_host.go#L383) but normally things will use NewStream directly and I think (i might be wrong) that that codepath does not resolve if it has to open a new connection.

So might be better off storing resolved addresses for the moment?

Stebalien · 2018-09-29T03:15:29Z

Ah... No, they don't. That's broken. Yeah, you're right. But we should really fix this. We don't even do routing on that path.

overbool · 2018-09-29T07:49:11Z

For each address that doesn't end in /ipfs/Qm..., resolve it.

@Stebalien Did you mean that if address ends in /ipfs/Qm..., we don't need to resolve it, because it will resolve address inner the basichost.Connect?

However, I don't think it will solve the issue as mentioned:

They'll instead use the resolved addresses, never learning about any changes to this DNS record.

overbool · 2018-10-05T05:22:33Z

@Stebalien I had made some changes according to your suggestion.

Stebalien

Sorry for the delay, I've been trying to push through a change that will make it easier to do this correctly (although, in retrospect, it was always possible).

Stebalien · 2018-10-25T12:25:25Z

core/commands/swarm.go

+			return nil, err
+		}
+		// check whether address ends in `ipfs/Qm...`
+		if _, err := maddr.ValueForProtocol(ma.P_IPFS); err != ma.ErrProtocolNotFound {


Sorry for the delay, I've been trying to push through a change to make this check more robust. You can now use:

if _, last := ma.SplitLast(maddr); last.Protocol().Code == ma.P_IPFS { maddrs = append(maddrs, maddr) continue }

The current check will trip over relay addrs. That is, /p2p/QmRelayId/p2p-circuit/dnsaddr/foo.com will pass the check but doesn't end in /p2p/QmTargetId.

Stebalien · 2018-10-25T12:26:04Z

core/commands/swarm.go

+			// filter out addresses that still doesn't end in `ipfs/Qm...`
+			for _, raddr := range raddrs {
+				if _, err := raddr.ValueForProtocol(ma.P_IPFS); err != ma.ErrProtocolNotFound {
+					maddrs = append(maddrs, raddr)


This is going to be racy. We should probably be using a channel and collecting these in a central location.

Stebalien · 2018-10-25T12:26:39Z

core/commands/swarm.go

+			}
+			// filter out addresses that still doesn't end in `ipfs/Qm...`
+			for _, raddr := range raddrs {
+				if _, err := raddr.ValueForProtocol(ma.P_IPFS); err != ma.ErrProtocolNotFound {


(will need the same "is an IPFS address as the one I suggested above")

overbool · 2018-10-25T14:06:16Z

@Stebalien Thx your advice, i had fixed it.

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

overbool requested a review from Kubuxu as a code owner September 28, 2018 01:44

overbool force-pushed the fix/support-for-dnsaddr branch 2 times, most recently from a282768 to b794220 Compare September 28, 2018 01:58

overbool mentioned this pull request Sep 28, 2018

Better support for /dnsaddr/ (i.e. in swarm connect) #5522

Closed

michaelavila reviewed Sep 28, 2018

View reviewed changes

Stebalien reviewed Sep 28, 2018

View reviewed changes

hsanjuan mentioned this pull request Sep 29, 2018

ipfs p2p forward should accept full multiaddresses as target (not only /ipfs/<peerID>) #5524

Closed

overbool force-pushed the fix/support-for-dnsaddr branch 3 times, most recently from 3b0b8f8 to a448431 Compare October 5, 2018 05:14

Stebalien requested changes Oct 25, 2018

View reviewed changes

Stebalien force-pushed the fix/support-for-dnsaddr branch from a448431 to c25ad4f Compare October 25, 2018 12:28

ghost assigned Stebalien Oct 25, 2018

ghost added the status/in-progress In progress label Oct 25, 2018

overbool force-pushed the fix/support-for-dnsaddr branch 3 times, most recently from 04a410b to 66be4f4 Compare October 25, 2018 13:33

overbool force-pushed the fix/support-for-dnsaddr branch from 66be4f4 to f6d8c07 Compare October 26, 2018 02:40

overbool force-pushed the fix/support-for-dnsaddr branch from f6d8c07 to 38aeedf Compare November 12, 2018 02:10

travisperson mentioned this pull request Feb 7, 2019

Maintainers policy filecoin-project/venus#1832

Merged

overbool added 2 commits March 18, 2019 18:51

fix(swarm): add dnsaddr support in swarm connect

309cf70

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

fix(swarm): collect maddr by channel

597bf2a

License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>

Stebalien force-pushed the fix/support-for-dnsaddr branch from 38aeedf to 4826173 Compare March 19, 2019 02:08

Stebalien approved these changes Mar 19, 2019

View reviewed changes

Stebalien force-pushed the fix/support-for-dnsaddr branch from 4826173 to 393a67b Compare March 19, 2019 02:13

Stebalien added 4 commits March 18, 2019 19:13

fix(swarm): feed through context

2916148

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

fix(swarm): use the go fork/join idiom

f1b6888

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

fix(swarm): catch cases where we fail to resolve an address to a peer

1cc8c3a

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

fix(swarm): improve swarm connect no peers found error

5fc7b02

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

Stebalien force-pushed the fix/support-for-dnsaddr branch from 393a67b to 5fc7b02 Compare March 19, 2019 02:14

Stebalien merged commit 37db5be into ipfs:master Mar 19, 2019

ghost removed the status/in-progress In progress label Mar 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(swarm): add dnsaddr support in swarm connect #5535

fix(swarm): add dnsaddr support in swarm connect #5535

overbool commented Sep 28, 2018

michaelavila Sep 28, 2018 •

edited

Loading

Stebalien Sep 28, 2018

Stebalien left a comment

Stebalien Sep 28, 2018

Stebalien Sep 28, 2018

Stebalien Sep 28, 2018

hsanjuan commented Sep 29, 2018

Stebalien commented Sep 29, 2018

overbool commented Sep 29, 2018 •

edited

Loading

overbool commented Oct 5, 2018

Stebalien left a comment

Stebalien Oct 25, 2018

Stebalien Oct 25, 2018

Stebalien Oct 25, 2018

overbool commented Oct 25, 2018

fix(swarm): add dnsaddr support in swarm connect #5535

fix(swarm): add dnsaddr support in swarm connect #5535

Conversation

overbool commented Sep 28, 2018

michaelavila Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Stebalien Sep 28, 2018

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Sep 28, 2018

Choose a reason for hiding this comment

Stebalien Sep 28, 2018

Choose a reason for hiding this comment

Stebalien Sep 28, 2018

Choose a reason for hiding this comment

hsanjuan commented Sep 29, 2018

Stebalien commented Sep 29, 2018

overbool commented Sep 29, 2018 • edited Loading

overbool commented Oct 5, 2018

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Oct 25, 2018

Choose a reason for hiding this comment

Stebalien Oct 25, 2018

Choose a reason for hiding this comment

Stebalien Oct 25, 2018

Choose a reason for hiding this comment

overbool commented Oct 25, 2018

michaelavila Sep 28, 2018 •

edited

Loading

overbool commented Sep 29, 2018 •

edited

Loading