Circuit Relay integration #4091

vyzo · 2017-07-26T17:19:42Z

#4090 Integrates circuit relay as a transport, enabled by default.

Notes:

Integration is direct, modulo Circuit Relay integration with basic_host libp2p/go-libp2p#210
~~Rebased on top of gx: update deps #4094. requires up-to-date go-libp2p. [BLOCKED]~~

Adds the following options to swarm configuration options:

DisableRelay (default: false) -- set to disable circuit relay transport
EnableRelayHop (default: false) -- set to enable the node to act as a Hop relay.

vyzo · 2017-07-26T17:24:11Z

summning @whyrusleeping @Stebalien

vyzo · 2017-07-26T17:46:15Z

iptb testing scenario:

$ iptb start --args --routing=none
Started daemon ..., pid = 21741
Started daemon ..., pid = 21755
Started daemon ..., pid = 21768

$ iptb shell 1
$ ipfs config Swarm.EnableRelayHop
true

$ iptb shell 2
$ ipfs id
{
	"ID": "QmbCdQcF3f2oPGE2Ch7wdmjfGjD227TBWCZNUMxjXqTZJ7",
	"PublicKey": "CAASogEwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAMs+WIfPPtXG66PPIasxnzNtas9yz3Eo4ahMB9lvOJv9QR197z68RnM4aNRQsiLv0NufxNVdkJi+7dD8Kj2HrX8hklZgrnAQISmHD/nNhamUyYRR+qYFG5nZgshV0yOMreFL9WwQgVau2W0KO/80PmOVe7evsbx/wy4A9XzPUDnVAgMBAAE=",
	"Addresses": [
		"/ip4/127.0.0.1/tcp/50527/ipfs/QmbCdQcF3f2oPGE2Ch7wdmjfGjD227TBWCZNUMxjXqTZJ7",
		"/ip4/10.0.1.30/tcp/50527/ipfs/QmbCdQcF3f2oPGE2Ch7wdmjfGjD227TBWCZNUMxjXqTZJ7"
	],
	"AgentVersion": "go-ipfs/0.4.11-dev/159fa52c",
	"ProtocolVersion": "ipfs/0.1.0"
}

$ iptb connect 0 1
connecting QmcsnBupwKXW8XsTxLMtvgKUVsocyunv6FydY2GJNsg5Qo -> QmUzUq7Bf6vBkSdvatRAokZLjPh1kjNNpQLiYZmduXpxSi
Addresses:  [/ip4/127.0.0.1/tcp/38266/ipfs/QmUzUq7Bf6vBkSdvatRAokZLjPh1kjNNpQLiYZmduXpxSi /ip4/10.0.1.30/tcp/38266/ipfs/QmUzUq7Bf6vBkSdvatRAokZLjPh1kjNNpQLiYZmduXpxSi]
trying ipfs swarm connect /ip4/127.0.0.1/tcp/38266/ipfs/QmUzUq7Bf6vBkSdvatRAokZLjPh1kjNNpQLiYZmduXpxSi
$ iptb connect 1 2
connecting QmUzUq7Bf6vBkSdvatRAokZLjPh1kjNNpQLiYZmduXpxSi -> QmbCdQcF3f2oPGE2Ch7wdmjfGjD227TBWCZNUMxjXqTZJ7
Addresses:  [/ip4/127.0.0.1/tcp/50527/ipfs/QmbCdQcF3f2oPGE2Ch7wdmjfGjD227TBWCZNUMxjXqTZJ7 /ip4/10.0.1.30/tcp/50527/ipfs/QmbCdQcF3f2oPGE2Ch7wdmjfGjD227TBWCZNUMxjXqTZJ7]
trying ipfs swarm connect /ip4/127.0.0.1/tcp/50527/ipfs/QmbCdQcF3f2oPGE2Ch7wdmjfGjD227TBWCZNUMxjXqTZJ7
connection success!

$ iptb shell 0
$ ipfs swarm peers
/ip4/127.0.0.1/tcp/38266/ipfs/QmUzUq7Bf6vBkSdvatRAokZLjPh1kjNNpQLiYZmduXpxSi
$ ipfs swarm connect /p2p-circuit/ipfs/QmbCdQcF3f2oPGE2Ch7wdmjfGjD227TBWCZNUMxjXqTZJ7
connect QmbCdQcF3f2oPGE2Ch7wdmjfGjD227TBWCZNUMxjXqTZJ7 success
$ ipfs swarm peers
/ip4/127.0.0.1/tcp/38266/ipfs/QmUzUq7Bf6vBkSdvatRAokZLjPh1kjNNpQLiYZmduXpxSi
/ipfs/QmUzUq7Bf6vBkSdvatRAokZLjPh1kjNNpQLiYZmduXpxSi/p2p-circuit/ipfs/QmbCdQcF3f2oPGE2Ch7wdmjfGjD227TBWCZNUMxjXqTZJ7

$ iptb shell 1
$ ipfs swarm peers
/ip4/127.0.0.1/tcp/44524/ipfs/QmcsnBupwKXW8XsTxLMtvgKUVsocyunv6FydY2GJNsg5Qo
/ip4/127.0.0.1/tcp/50527/ipfs/QmbCdQcF3f2oPGE2Ch7wdmjfGjD227TBWCZNUMxjXqTZJ7

$ iptb shell 2
$ ipfs swarm peers
/ip4/127.0.0.1/tcp/40106/ipfs/QmUzUq7Bf6vBkSdvatRAokZLjPh1kjNNpQLiYZmduXpxSi
/ipfs/QmUzUq7Bf6vBkSdvatRAokZLjPh1kjNNpQLiYZmduXpxSi/p2p-circuit/ipfs/QmcsnBupwKXW8XsTxLMtvgKUVsocyunv6FydY2GJNsg5Qo

vyzo · 2017-07-27T10:16:14Z

Rebased on #4094.

vyzo · 2017-07-28T09:16:35Z

Added a test/sharness script that implements the iptb scenario described above.

vyzo · 2017-07-28T14:02:07Z

More iptb interaction: verify the connection works:

$ iptb shell 0
$ echo "hello world" | ipfs add
added QmT78zSuBmuS4z925WZfrqQ1qHaJ56DQaTfyMUF7F8ff5o QmT78zSuBmuS4z925WZfrqQ1qHaJ56DQaTfyMUF7F8ff5o

$ iptb shell 2
$ ipfs cat QmT78zSuBmuS4z925WZfrqQ1qHaJ56DQaTfyMUF7F8ff5o
hello world

I will extend the sharness test to verify the connection.

vyzo · 2017-07-28T14:21:41Z

I think it's ready for review.
pinging @Stebalien @whyrusleeping @lgierth

whyrusleeping · 2017-07-28T18:56:47Z

core/commands/swarm.go

+				if strings.Contains(info.Addr, "/p2p-circuit/") {
+					fmt.Fprintf(buf, "%s", info.Addr)
+				} else {
+					fmt.Fprintf(buf, "%s/ipfs/%s", info.Addr, info.Peer)


Whats the purpose of this?

The address already contains the peer id in this case.
It's of the form .../p2p-circuit/ipfs/QmId, so pasting the peer id again would result in the address formatted as .../p2p-circuit/ipfs/QmId/ipfs/QmId

do you want me to add a comment to that extent?

Hrm... that feels a bit weird. @Kubuxu @lgierth @Stebalien thoughts?

I'd expect RemoteMultiaddr to return just the /ipfs/QmProxy/p2p-circuit part, not the final /ipfs/QmId. As-is, this is inconsistent with TCP/IP addresses. Is there a technical reason not to do it this way?

So we actually need it: the address reported by Conn.RemoteMultiaddr should be dialable, and the partial address won't be.

I will add a comment here explaining why we need this.

Basically /ipfs/QmId serves two purposes:

Routing information: An alias for an internet address (stored in the DHT): /ip4/xxx.xxx.xxx.xxx/tcp/1234

Identity information: The key of the target machine.

In the RemoteMultiaddr case, the second ipfs address is routing information.

We'd actually run into the same problem something returned /ipfs/QmId as the entire RemoteMultiaddr (which would actually make sense if you had a lower-level VPN that understood IPFS addresses).

Actually, that brings up a good point. Maybe we should just be checking to see if the last address is an /ipfs/ address (i.e., the multiaddr ends in /ipfs/QmId) and, if it is, assert that the QmIds match instead of appending something. Thoughts @vyzo?

Maybe we should just be checking to see if the last address is an /ipfs/ address (i.e., the multiaddr ends in /ipfs/QmId) and, if it is, assert that the QmIds match instead of appending something.

That's perhaps the generic case to handle routing addresses similar to how circuit reports address.
So instead of checking if it is a p2p-circuit address, check if it ends with the /ipfs/peerId and if so only append if it doesn't match.

Sure, we can do that.

I have changed the check to look for the /ipfs/QmPeer suffix in the remote multiaddr.
If the suffix is present, then it doesn't duplicate it, otherwise it appends /ipfs/QmPeer.

Not sure we need a comment any more, it's not p2p-circuit specific and the code is self-explaining.

vyzo · 2017-07-29T07:19:18Z

Rebased on master.

vyzo · 2017-07-29T08:06:45Z

~~Blocked on libp2p/go-libp2p-circuit#6~~

Reverted: libp2p/go-libp2p-circuit#7

vyzo · 2017-07-29T08:48:31Z

Upon discussion with @Stebalien on irc we concluded that it is important to keep the invariant that multiaddrs returned by Conn.RemoteMultiaddr are dialiable.
A partial p2p-circuit address will not be, so we need to keep the ipfs part in the address as was the original design.
I have reverted ~~and will add a comment to the code to explain why we need to filter~~.

whyrusleeping · 2017-07-29T20:22:39Z

core/core.go

+			}
+			return raddrs
+		}
+		hostOpts = append(hostOpts, p2pbhost.AddrsFactory(filterRelayAddr))


What happens if we have multiple address factories in the hostOpts?

We would need to compose them.

Do you want me to add scaffolding for the composition?

Yeah, Since we're going to be using the address factory pretty soon anyways for limiting which addresses we announce: #3948

I will need to add the AddrsFactory option to ConstructPeerHostOpts in order to scaffold the composition properly.

Then we have two options for the composition: filter first or announce factory first. It makes some sense to use the filter first, the announce factory might actually want to announce some relay addresses in the future. On the other hand it also makes sense for the announce factory to see the p2p-circuit relay address, so that it knows that relay is activated.

Nonetheless, the announce factory can also tell whether relay is in use from the swarm options already, so the latter argument is not all that compelling.

So the filter should be applied before the announce address factory.

Added the scaffolding for address factory composition in 231f8e4:

added an AddrsFactory option to ConstructPeerHostOpts

composed the specified address factory with the relay filter when present

whyrusleeping · 2017-07-29T20:25:35Z

thirdparty/ipfsaddr/ipfsaddr.go

@@ -106,6 +107,13 @@ func ParseMultiaddr(m ma.Multiaddr) (a IPFSAddr, err error) {

 func Transport(iaddr IPFSAddr) (maddr ma.Multiaddr) {
 	maddr = iaddr.Multiaddr()
+
+	// /ipfs/QmId is part of the transport address for p2p-circuit
+	_, err := maddr.ValueForProtocol(circuit.P_CIRCUIT)


Would prefer if we at least put a TODO here to clean this up. We need to rethink (later, not now) how addresses get composed and consumed so that we don't have to have this special case here.

Sure, will add a TODO.

Added TODO comment in a11f632

I wonder if we could just stop distinguishing between the transport part and the application part. Multiaddrs don't really have a clear transport/application layer separation (they can have transport -> application -> transport -> transport -> application, etc...).

Instead, we could either:

a. Have dialers recursively dial (dial the parts they understand and then recursively call the "main" dialer to dial the rest).
b. Dial like we resolve through IPLD objects and return an intermediate transport + the rest of the undialed path.

whyrusleeping

This LGTM for the most part, some questions around host construction, and the special casing of relay multiaddrs still weirds me out. But thats probably okay to figure out later.

Stebalien

The code looks good to me. Just a question on configuration/documentation.

Stebalien · 2017-08-16T00:35:11Z

docs/config.md

@@ -221,5 +221,12 @@ improvement, as well as a reduction in memory usage.
 - `DisableNatPortMap`
 Disable NAT discovery.

+- `DisableRelay`
+Disables the p2p-circuit relay transport.


Will this disable relays entirely or just disable listening on relayed addresses? There are three cases:

Listen using relays.

Act as a relay.

Connect to nodes through relays.

It would be nice to be able to enable/disable these independently. Can we currently do that?

Setting to true will disable the relay transport entirely.

The DislableRelay controls option 1 and 3, so by default we listen for relay connections and we can connect to peers through relay.
In order to act as a hop relay (2), you need to EnableRelayHop, in addition to having the relay transport enabled.

I don't think it makes sense to have more fine-grained independent configuration, as we can't really separate listening from dial (and do we really want to do that?) and we already have an option to control hop relaying.

As discussed on IRC, I misunderstood what these toggles did. DisableRelay disables relaying entirely and we don't yet support advertising relayed addresses; when implemented, that will probably have an additional toggle.

Yes, that's the plan.

- enabled by default, so that we can dial/receive dials - /p2p-circuit/QmId address is not announced; filtered at host with AddrsFactory - use case is manual swarm connect with relay address License: MIT Signed-off-by: vyzo <[email protected]>

License: MIT Signed-off-by: vyzo <[email protected]>

Handles p2p-circuit addresses and any other address that uses a similar routing scheme. License: MIT Signed-off-by: vyzo <[email protected]>

License: MIT Signed-off-by: vyzo <[email protected]>

- Adds AddrsFactory to ConstructPeerHostOpts - Composes the AddrsFactory option with the relay filter License: MIT Signed-off-by: vyzo <[email protected]>

License: MIT Signed-off-by: vyzo <[email protected]>

integrate #3948 License: MIT Signed-off-by: vyzo <[email protected]>

Necessary for meaningful semantics in the presence of address filtering. License: MIT Signed-off-by: vyzo <[email protected]>

License: MIT Signed-off-by: vyzo <[email protected]>

vyzo · 2017-08-16T06:39:30Z

Rebased on master and fixed the conflict.

License: MIT Signed-off-by: vyzo <[email protected]>

vyzo · 2017-08-16T18:07:31Z

@whyrusleeping this is ready to merge.

whyrusleeping · 2017-08-16T19:46:48Z

Merged! ipfs has relay support!! Thank you @vyzo!

whyrusleeping · 2017-08-16T20:35:29Z

@vyzo do you think you could write up a small doc on how to use the relay feature? That would be immensely helpful to have

vyzo · 2017-08-17T05:58:28Z

Sure! Where should it live though?

Kubuxu · 2017-08-17T06:18:21Z

docs/experimental-features.md

vyzo · 2017-08-17T08:53:08Z

#4148 adds documentation in docs/experimental-features.md

daviddias added the status/in-progress In progress label Jul 26, 2017

vyzo changed the title ~~Circuit Relay integration~~ [WIP] Circuit Relay integration Jul 26, 2017

vyzo mentioned this pull request Jul 27, 2017

gx: update deps #4094

Merged

vyzo added the status/blocked Unable to be worked further until needs are met label Jul 27, 2017

vyzo force-pushed the feat/circuit-relay branch from 722a44a to 5333bfc Compare July 27, 2017 10:09

vyzo force-pushed the feat/circuit-relay branch from 5333bfc to 943639d Compare July 28, 2017 07:31

vyzo changed the title ~~[WIP] Circuit Relay integration~~ Circuit Relay integration Jul 28, 2017

whyrusleeping reviewed Jul 28, 2017

View reviewed changes

vyzo force-pushed the feat/circuit-relay branch from cd92b47 to 63afb70 Compare July 29, 2017 07:18

vyzo removed the status/blocked Unable to be worked further until needs are met label Jul 29, 2017

vyzo force-pushed the feat/circuit-relay branch 2 times, most recently from 1d60f08 to e9fa416 Compare July 29, 2017 07:37

vyzo added status/blocked Unable to be worked further until needs are met and removed status/blocked Unable to be worked further until needs are met labels Jul 29, 2017

vyzo force-pushed the feat/circuit-relay branch from d2dea51 to b5bba3d Compare July 29, 2017 08:16

vyzo force-pushed the feat/circuit-relay branch from b5bba3d to e9fa416 Compare July 29, 2017 08:49

whyrusleeping reviewed Jul 29, 2017

View reviewed changes

whyrusleeping requested changes Jul 29, 2017

View reviewed changes

whyrusleeping requested review from a user and Kubuxu July 29, 2017 20:27

Stebalien approved these changes Aug 16, 2017

View reviewed changes

Stebalien mentioned this pull request Aug 16, 2017

bitswap_test: make racy test less racy #4109

Merged

vyzo added 15 commits August 16, 2017 09:26

integrate circuit-relay transport

ba5940e

- enabled by default, so that we can dial/receive dials - /p2p-circuit/QmId address is not announced; filtered at host with AddrsFactory - use case is manual swarm connect with relay address License: MIT Signed-off-by: vyzo <[email protected]>

account for p2p-circuit address intricacies

4900935

License: MIT Signed-off-by: vyzo <[email protected]>

test/sharness: circuit relay test

1f40c52

License: MIT Signed-off-by: vyzo <[email protected]>

test/sharness: extended circuit relay test

98da085

License: MIT Signed-off-by: vyzo <[email protected]>

swarm: more robust logic in remote address printing

6c63cec

Handles p2p-circuit addresses and any other address that uses a similar routing scheme. License: MIT Signed-off-by: vyzo <[email protected]>

ipfsaddr: add TODO comment to clean up p2p-circuit special case

3e7dce2

License: MIT Signed-off-by: vyzo <[email protected]>

core: address factory composition for constructPeerHost

9d15e63

- Adds AddrsFactory to ConstructPeerHostOpts - Composes the AddrsFactory option with the relay filter License: MIT Signed-off-by: vyzo <[email protected]>

docs/config: document relay options

16247cb

License: MIT Signed-off-by: vyzo <[email protected]>

fix competing address factories in host options

4f39962

integrate #3948 License: MIT Signed-off-by: vyzo <[email protected]>

ipfs swarm addrs listen command, fix t0060-daemon

6f54181

Necessary for meaningful semantics in the presence of address filtering. License: MIT Signed-off-by: vyzo <[email protected]>

more descriptive help message for ipfs swarm addrs local

d7922e7

License: MIT Signed-off-by: vyzo <[email protected]>

gx: update go-libp2p-circuit

1141705

License: MIT Signed-off-by: vyzo <[email protected]>

don't hardcode ipfs protocol name in multiaddr display

a130659

License: MIT Signed-off-by: vyzo <[email protected]>

simplify code for ipfs protocol scheme in multiaddr display

31e3b8f

License: MIT Signed-off-by: vyzo <[email protected]>

don't leak host in case of error adding relay

322bb98

License: MIT Signed-off-by: vyzo <[email protected]>

make filterRelayAddrs a named function, not a closure

b6150b2

License: MIT Signed-off-by: vyzo <[email protected]>

vyzo force-pushed the feat/circuit-relay branch from 9c22ded to b6150b2 Compare August 16, 2017 06:48

whyrusleeping merged commit 22ee73d into master Aug 16, 2017

whyrusleeping deleted the feat/circuit-relay branch August 16, 2017 19:46

magik6k added this to the Ipfs 0.4.11 milestone Aug 17, 2017

Circuit Relay integration #4091

Circuit Relay integration #4091

Conversation

vyzo commented Jul 26, 2017 • edited Loading

vyzo commented Jul 26, 2017

vyzo commented Jul 26, 2017 • edited Loading

vyzo commented Jul 27, 2017

vyzo commented Jul 28, 2017

vyzo commented Jul 28, 2017

vyzo commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo Jul 29, 2017 • edited Loading

Choose a reason for hiding this comment

Stebalien Jul 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Jul 29, 2017

vyzo commented Jul 29, 2017 • edited Loading

vyzo commented Jul 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo Jul 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping left a comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Aug 16, 2017 • edited Loading

vyzo commented Aug 16, 2017

whyrusleeping commented Aug 16, 2017

whyrusleeping commented Aug 16, 2017

vyzo commented Aug 17, 2017

Kubuxu commented Aug 17, 2017

vyzo commented Aug 17, 2017

vyzo commented Jul 26, 2017 •

edited

Loading

vyzo commented Jul 26, 2017 •

edited

Loading

vyzo Jul 29, 2017 •

edited

Loading

Stebalien Jul 29, 2017 •

edited

Loading

vyzo commented Jul 29, 2017 •

edited

Loading

vyzo commented Jul 29, 2017 •

edited

Loading

vyzo Jul 30, 2017 •

edited

Loading

vyzo Aug 16, 2017 •

edited

Loading

vyzo commented Aug 16, 2017 •

edited

Loading