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 <vyzo@hackzen.org>