protocols/mdns: Use a random string as peer-name

[mxinden]

With libp2p/specs#368 the definition of the peer name changed in the mDNS specification.

peer-name is the case-insensitive unique identifier of the peer, and is less than 64 characters.

As the this field doesn't carry any meaning, it is sufficient to ensure the uniqueness of this identifier. Peers SHOULD generate a random, lower-case alphanumeric string of least 32 characters in length when booting up their node. Peers SHOULD NOT use their Peer ID here because a future Peer ID could exceed the DNS label limit of 63 characters.

https://github.com/libp2p/specs/blob/master/discovery/mdns.md

libp2p-mdns should be adjusted accordingly.

Also see libp2p/go-libp2p#1222 for the corresponding change on the Golang side.

[jochasinga]

Is varying size needed? libp2p/go-libp2p#1222 (review)

[mxinden]

Is varying size needed? libp2p/go-libp2p#1222 (review)

I am fine either way. Given that you already implemented varying size strings, would you mind adding a comment to your patch pointing to the discussion linked? I didn't find it very intuitive, thus might stumble across this in the future.

[mxinden]

As promised, here is a more detailed follow up to #2311 (comment):

The latest release of libp2p-mdns does not include the peer ID in each address it sends to other peers. Instead when receiving an mDNS response, rust-libp2p actually does parse the peer name as a PeerId and discards an address in case it contains a /p2p/QmXX PeerId which doesn't match the one retrieved from the peer name field (e.g. send by go-libp2p).

rust-libp2p/protocols/mdns/src/query.rs

Lines 162 to 176 in 012287a

	let mut peer_name = match record_value.rsplitn(4, |c| c == '.').last() {
	Some(n) => n.to_owned(),
	None => return None,
	};
	// if we have a segmented name, remove the '.'
	peer_name.retain(|c| c != '.');
	let peer_id = match data_encoding::BASE32_DNSCURVE.decode(peer_name.as_bytes()) {
	Ok(bytes) => match PeerId::from_bytes(&bytes) {
	Ok(id) => id,
	Err(_) => return None,
	},
	Err(_) => return None,
	};

The above implies that peers not running #2311 will discard any responses send by peers that are running #2311, as they will try to to parse the peer name which will fail in most cases as it is not a valid peer ID.

I see two ways moving forward:

1. Breaking change

   Only do a single release. Include the peer id of a peer in each multiaddr and embrace the fact that old peers will not be able to discover new peers and vice versa.

2. Staged roll out

   1. Release a version of libp2p-mdns that includes the peer id of a peer in each multiaddr.
   2. Release a version of libp2p-mdns that ignores the peer name, treating it as a random blob.
   3. Release a version of libp2p-mdns that sets the peer name to a random blob, i.e. what protocols/mdns: Use a random alphanumeric string for peer name #2311 does today.

I am leaning towards (1) as I don't know any libp2p users that depend on libp2p-mdns as their only discovery mechanism. That said, I am happy to help do (2).

Any comments? //CC @jochasinga

[jochasinga]

I am leaning towards (1) as I don't know any libp2p users that depend on libp2p-mdns as their only discovery mechanism. That said, I am happy to help do (2).

Since mDNS use case is local updating to a breaking change (1) shouldn't be a big deal.

[jochasinga]

1. Breaking change

   Only do a single release. Include the peer id of a peer in each multiaddr and embrace the fact that old peers will not be able to discover new peers and vice versa.

@mxinden did you mean "Include the peer name in each multiaddr"? (the random string in #2311 vs the current base58 peer id)

[MarcoPolo]

I think he means the peer id. e.g. the multiaddr that is in the txt field should be of the form:

dnsaddr=/ip4/192.168.178.21/tcp/4001/p2p/QmPeerId

---

I'm kind of curious what the staged rollout actually looks like in practice. Do we use feature flags here? What does our process look like for these kinds changes? are they documented? (release.md doesn't have anything)

This could be a useful low-risk exercise to create this process.

[mxinden]

I'm kind of curious what the staged rollout actually looks like in practice. Do we use feature flags here? What does our process look like for these kinds changes? are they documented? (release.md doesn't have anything)

We use consecutive versions, e.g. see libp2p-yamux v0.22.0 to v0.25.0. Binaries then only need to upgrade in order, one at a time. (Unfortunately) this is not documented anywhere.

This could be a useful low-risk exercise to create this process.

Would be great to have this documented for sure. I am happy to help, though I don't have the capacity to drive this myself at the moment.

[mxinden]

Release a version of libp2p-mdns that includes the peer id of a peer in each multiaddr.

For the record, looking at a wireshark dump of the Golang implementation, this is already the case in go-libp2p.

[MarcoPolo]

Ah so this is already leading to disconnects in rust <-> go interactions then?

I'm happy to document the process (although looking at the example, it's as simple as I expected – some flags that get incrementally changed over versions), but it also seems fine to make the breaking change.

I have a slight preference to just making the breaking change which would bring this part level with the Go implementation. But I'll defer to @mxinden to pick how they want this resolved.

[mxinden]

Ah so this is already leading to disconnects in rust <-> go interactions then?

Yes, given that Go already uses a random string as the peer name. Given that the peer name and the peer id in each multiaddr do not match, Rust will discard any Go packet.

The above discussion is about whether to break compatibility within Rust and not with Rust and Go. The latter ship has already sailed. My bad.

I have a slight preference to just making the breaking change which would bring this part level with the Go implementation.

👍 same preference for me. We just need to call it out very prominently in our changelog. In case anyone needs a non-breaking roll out, they can still contribute a patch version that they can then ship beforehand.

@jochasinga #2311 would only need to update the parsing logic and add a changelog description.

[jochasinga]

The above discussion is about whether to break compatibility within Rust and not with Rust and Go. The latter ship has already sailed. My bad.

You meant Go already uses the peer name in libp2p/go-libp2p#1222, and by introducing #2311 here newer Rust will become compatible with Go.

[jochasinga]

@mxinden @MarcoPolo seems like MdnsPeer::new(packet: &Packet<'_>, record_value: String, my_peer_id: PeerId, ttl: u32) -> MdnsPeer only takes PeerId to check if it matches one it parses from the TXT field from the response's record. Since #2311 will no longer parse a PeerId chunk from the record, how will this play out?

rust-libp2p/protocols/mdns/src/query.rs

Lines 256 to 267 in 220f84a

	match addr.pop() {
	Some(Protocol::P2p(peer_id)) => {
	if let Ok(peer_id) = PeerId::try_from(peer_id) {
	if peer_id != my_peer_id {
	return None;
	}
	} else {
	return None;
	}
	}
	_ => return None,
	};

)

[MarcoPolo]

I think we should:

1. remove the my_peer_id param from that function
2. grab the peer_id from the parsed addrs (and add some validation that they are all the same peer ids),
3. Remove the parsing logic that reads the peer_name.
4. Make sure there is a test that 1. checks that a p2p/QmPeerID component is added to the TXT field, and 2. given a well formed Packet we can properly create an MdnsResponse.

What do you think?

[jochasinga]

1. remove the my_peer_id param from that function

I agree.

2. grab the peer_id from the parsed addrs (and add some validation that they are all the same peer ids)

I like the idea of parsing a peer ID out of the first multiaddr, then filter out those with different peer IDs. Curve ball: what if a peer ID is malformed? Do we keep iterating until we find a plausible peer ID and use it as the base or do we ignore it entirely?

3. Remove the parsing logic that reads the peer_name.

How about instead of removing the whole parsing logic, we parse the peer name and just assert that its length is between 32 - 64 characters, and return None otherwise?

4. Make sure there is a test that 1. checks that a p2p/QmPeerID component is added to the TXT field, and 2. given a well formed Packet we can properly create an MdnsResponse.

sounds good to me.

[mxinden]

I like the idea of parsing a peer ID out of the first multiaddr, then filter out those with different peer IDs. Curve ball: what if a peer ID is malformed? Do we keep iterating until we find a plausible peer ID and use it as the base or do we ignore it entirely?

I would suggest the following:

• Ignore all addresses with malformed peer IDs.
• Use the peer ID of the first address that has a peer ID as a reference.
• Ignore all addresses that contain a peer ID if that peer ID does not match the peer ID of the first address containing a peer ID.

[dvc94ch]

FYI I don't think it's ok to just go breaking things because PL has a brainfart. There is no reason that the rust implementation had to break backwards compatibility here.

[dvc94ch]

cc @rkuhn as this probably affects you

[rkuhn]

Hmm, I missed this ping somehow and saw the change now in the 0.42 change log.

@dvc94ch With the reasoning given at the top, it does seem that compatibility will have to be broken at some point due to the mDNS protocol constraints. At that point Actyx would be broken for other reasons, though, so it would make things easier for more static deployments if the breaking change only happened at that distant future point in time.

@mxinden The three-step rollout strategy for breaking changes sounds nice in theory, but in practice it depends on everyone following the step by step instructions without skipping some. We’re using libp2p in places where changes occur once every few years, so it is very likely that incompatible versions will be in the same swarm. Not sure how to reconcile this with the current scheme of protocol evolution, other than saying “only forward and backward compatible changes, ever” and adding new protocols instead of changing old ones. Perhaps this might be a good topic for a community call — I’ll try to attend the next one.

In this particular case, I think I’ve seen the ramifications of the Go vs. Rust incompatibility, which was fixed by adding bootstrap nodes (we call them initial peers) in the settings. Since we recommend this approach in any case (as assumed by Max regarding mDNS not being the only discovery mechanism), I think in practice the one-step breaking change is the right choice here.