[mdns] - Support for long mDNS names (Bug #1232) #1287

peat · 2019-10-24T19:34:41Z

Hey folks,

I believe this adds support for long mDNS names, as reported in issue #1232

There are a couple of additional changes that I made in order to test the scenario where long mDNS names are encountered, so I'd love to get feedback about unintended consequences:

I added the std feature for ring in the top-level Cargo.toml. I wasn't able to build or test within the mdns directory without it (eg: cd misc/mdns; cargo test would fail on ring error traits).
I added support for creating PeerId from the Identity multihash. It's in line with the spec, but I'm not familiar with the nuances. This was done so that I could generate long mDNS names for tests.

Cheers!

…tion

…tably `misc/mdns`, so that I could run these tests)

…al QNAME encoding bytes

…reater than 63 characters

tomaka · 2019-10-25T09:40:41Z

I agree that should have this change, but I'm a bit confused by the specs. The fact that you have to modify the code of PeerId in order to be able to generate the long PeerIds that are relevant for this issue kind of shows that this couldn't happen in reality.

peat · 2019-10-25T16:10:56Z

I don't think the Rust implementation would ever create an oversized DNS label because it only responds to queries with the locally generated SHA2256 PeerId -- so I'm happy to yank the code for using the Identity multihashes in the query response use case.

I would like to retain the code in service.rs -- the parsing is a little cleaner, and I think it's good to have the capability to parse the long format mDNS query responses for interop with other implementations.

@tomaka, would you be more comfortable seeing those changes, given your concerns about the spec being a bit loose?

tomaka · 2019-10-25T16:30:55Z

would you be more comfortable seeing those changes, given your concerns about the spec being a bit loose?

The scope of the changes is good (IMO). I'll probably review early next week.

core/Cargo.toml

tomaka · 2019-10-28T11:15:44Z

core/src/peer_id.rs

-const MAX_INLINE_KEY_LENGTH: usize = 42;
+//
+// Note: see `from_public_key` for how this value will be used in the future.
+// const MAX_INLINE_KEY_LENGTH: usize = 42;


Please keep this.

tomaka · 2019-10-28T11:18:16Z

misc/mdns/src/dns.rs

@@ -211,6 +203,37 @@ fn append_u16(out: &mut Vec<u8>, value: u16) {
    out.push((value & 0xff) as u8);
 }

+/// If a peer ID is longer than 63 characters, split it into segments to
+/// be compatible with RFC 1035.
+fn segment_peer_id(peer_id: &String) -> String {


Suggested change

fn segment_peer_id(peer_id: &String) -> String {

fn segment_peer_id(peer_id: String) -> String {

It might be premature optimization, but we could add some pass-through:

if peer_id.as_bytes().len() <= 63 { return peer_id }

Updated 👍

tomaka · 2019-10-28T11:25:03Z

misc/mdns/src/dns.rs

+/// If a peer ID is longer than 63 characters, split it into segments to
+/// be compatible with RFC 1035.
+fn segment_peer_id(peer_id: &String) -> String {
+    // This will only perform one allocation except in extreme circumstances.


I think you can replace the function's body with something like this:

Suggested change

// This will only perform one allocation except in extreme circumstances.

String::from_utf8(peer_id.as_bytes().chunks(63).join(&b'.')).expect("we copy from a UTF8 string; qed")

The problem is that .chunks can't .join (in 1.38.0) ... I went through several iterations to see if I could make that work elegantly, and ended up with the for ... in loop for clarity and minimal number of allocations.

I looked further into this as I was curious, I guess not because it really matters in this case. I am sorry if this is just useless noise to the discussion.

Under the assumption that the string only contains ASCII characters, why not do:

let b = peer_id .as_bytes() .chunks(MAX_LABEL_LENGTH) .collect::<Vec<_>>() .join(&b'.'); String::from_utf8(b).expect("we copy from a UTF8 string; qed")

Using a for-loop and manually preallocating the right amount of memory seems like an early optimization to me. I have benchmarked this here. Using iterators with join allows one to not copy the characters one by one as in the for loop, but instead in chunks (See https://doc.rust-lang.org/src/alloc/slice.rs.html#664).

Running the above linked benchmarks in criterion shows that the above would be twice as fast (on my machine) as the for loop. In addition I find it easier to comprehend.

@peat @tomaka what do you think?

Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

… that don't need to be segmented

tomaka · 2019-10-29T12:53:08Z

misc/mdns/src/dns.rs

+    // This will only perform one allocation except in extreme circumstances.
+    let mut out = String::with_capacity(peer_id.len() + 8);
+
+    for (idx, chr) in peer_id.chars().enumerate() {


This line bothers me a bit. We know the string only contains ASCII characters, so I guess it's fine. I would have preferred using bytes.

* Dead code -- commenting out with a note referencing future implementation * Adding "std" feature so that cargo can build in other directories (notably `misc/mdns`, so that I could run these tests) * Permitting `PeerID` to be built from an `Identity` multihash * The length limit for DNS labels is 63 characters, as per RFC1035 * Allocates the vector with capacity for the service name plus additional QNAME encoding bytes * Added support for encoding/decoding peer IDs with an encoded length greater than 63 characters * Removing "std" from ring features Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com> * Retaining MAX_INLINE_KEY_LENGTH with comment about future usage * `segment_peer_id` consumes `peer_id` ... plus an early return for IDs that don't need to be segmented * Fixing logic

@rschulman

* Implement Debug for (ed25519|secp256k1)::(Keypair|SecretKey) (#1285) * Fix possible arithmetic overflow in libp2p-kad. (#1291) When the number of active queries exceeds the (internal) JOBS_MAX_QUERIES limit, which is only supposed to bound the number of concurrent queries relating to background jobs, an arithmetic overflow occurs. This is fixed by using saturating subtraction. * protocols/plaintext: Add example on how to upgrade with PlainTextConfig1 (#1286) * [mdns] - Support for long mDNS names (Bug #1232) (#1287) * Dead code -- commenting out with a note referencing future implementation * Adding "std" feature so that cargo can build in other directories (notably `misc/mdns`, so that I could run these tests) * Permitting `PeerID` to be built from an `Identity` multihash * The length limit for DNS labels is 63 characters, as per RFC1035 * Allocates the vector with capacity for the service name plus additional QNAME encoding bytes * Added support for encoding/decoding peer IDs with an encoded length greater than 63 characters * Removing "std" from ring features Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com> * Retaining MAX_INLINE_KEY_LENGTH with comment about future usage * `segment_peer_id` consumes `peer_id` ... plus an early return for IDs that don't need to be segmented * Fixing logic * Bump most dependencies (#1268) * Bump most dependencies This actually builds 😊. * Bump all dependencies Includes the excellent work of @rschulman in #1265. * Remove use of ed25519-dalek fork * Monomorphize more dependencies * Add compatibility hack for rand Cargo allows a crate to depend on multiple versions of another, but `cargo-web` panics in that situation. Use a wrapper crate to work around the panic. * Use @tomaka’s idea for using a newer `rand` instead of my own ugly hack. * Switch to Parity master as its dependency-bumping PR has been merged. * Update some depenendencies again * Remove unwraps and `#[allow(deprecated)]`. * Remove spurious changes to dependencies Bumping minor or patch versions is not needed, and increases likelyhood of merge conflicts. * Remove some redundant Cargo.toml changes * Replace a retry loop with an expect `ed25519::SecretKey::from_bytes` will never fail for 32-byte inputs. * Revert changes that don’t belong in this PR * Remove using void to bypass ICE (#1295) * Publish 0.13.0 (#1294)

peat added 6 commits October 24, 2019 12:09

Dead code -- commenting out with a note referencing future implementa…

b68866e

…tion

Adding "std" feature so that cargo can build in other directories (no…

fcef341

…tably `misc/mdns`, so that I could run these tests)

Permitting PeerID to be built from an Identity multihash

68d05c8

The length limit for DNS labels is 63 characters, as per RFC1035

9fde5e2

Allocates the vector with capacity for the service name plus addition…

866dcc1

…al QNAME encoding bytes

Added support for encoding/decoding peer IDs with an encoded length g…

1aa0ae6

…reater than 63 characters

tomaka requested changes Oct 28, 2019

View reviewed changes

peat and others added 7 commits October 28, 2019 10:41

Removing "std" from ring features

da6d6fe

Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com>

Retaining MAX_INLINE_KEY_LENGTH with comment about future usage

ced22fe

segment_peer_id consumes peer_id ... plus an early return for IDs…

a3ed493

… that don't need to be segmented

Merge branch 'master' into long-mdns

8546f56

Merge branch 'master' into long-mdns

ec7e5d5

Merge branch 'long-mdns' of github.com:peat/rust-libp2p into long-mdns

05aee34

Fixing logic

b26b24c

tomaka reviewed Oct 29, 2019

View reviewed changes

tomaka approved these changes Oct 29, 2019

View reviewed changes

tomaka merged commit eb7b7bd into libp2p:master Oct 31, 2019

tomaka mentioned this pull request Jan 7, 2020

mdns peer discovery is not compliant to latest specification #1232

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mdns] - Support for long mDNS names (Bug #1232) #1287

[mdns] - Support for long mDNS names (Bug #1232) #1287

peat commented Oct 24, 2019 •

edited

tomaka commented Oct 25, 2019

peat commented Oct 25, 2019

tomaka commented Oct 25, 2019

tomaka Oct 28, 2019

peat Oct 28, 2019

tomaka Oct 28, 2019

peat Oct 28, 2019

tomaka Oct 28, 2019

peat Oct 28, 2019 •

edited

mxinden Oct 31, 2019

tomaka Oct 29, 2019

	fn segment_peer_id(peer_id: &String) -> String {
	fn segment_peer_id(peer_id: String) -> String {

	// This will only perform one allocation except in extreme circumstances.
	String::from_utf8(peer_id.as_bytes().chunks(63).join(&b'.')).expect("we copy from a UTF8 string; qed")

[mdns] - Support for long mDNS names (Bug #1232) #1287

[mdns] - Support for long mDNS names (Bug #1232) #1287

Conversation

peat commented Oct 24, 2019 • edited

tomaka commented Oct 25, 2019

peat commented Oct 25, 2019

tomaka commented Oct 25, 2019

tomaka Oct 28, 2019

Choose a reason for hiding this comment

peat Oct 28, 2019

Choose a reason for hiding this comment

tomaka Oct 28, 2019

Choose a reason for hiding this comment

peat Oct 28, 2019

Choose a reason for hiding this comment

tomaka Oct 28, 2019

Choose a reason for hiding this comment

peat Oct 28, 2019 • edited

Choose a reason for hiding this comment

mxinden Oct 31, 2019

Choose a reason for hiding this comment

tomaka Oct 29, 2019

Choose a reason for hiding this comment

peat commented Oct 24, 2019 •

edited

peat Oct 28, 2019 •

edited