Finish PeerId inlining change (#1413)

* Add peer id inlining for small public keys

* Apply @twittner suggestions

* Make PeerId compare equal accross hashes

* Fix mDNS

* Remove useless functions

* Add property test

Co-authored-by: Age Manning <Age@AgeManning.com>

core/src/peer_id.rs

@@ -22,19 +22,17 @@ use crate::PublicKey;
 use bs58;
 use thiserror::Error;
 use multihash;
-use std::{convert::TryFrom, fmt, str::FromStr};
+use std::{convert::TryFrom, fmt, hash, str::FromStr};
 
 /// Public keys with byte-lengths smaller than `MAX_INLINE_KEY_LENGTH` will be
 /// automatically used as the peer id using an identity multihash.
-//
-// Note: see `from_public_key` for how this value will be used in the future.
-const _MAX_INLINE_KEY_LENGTH: usize = 42;
+const MAX_INLINE_KEY_LENGTH: usize = 42;
 
 /// Identifier of a peer of the network.
 ///
 /// The data is a multihash of the public key of the peer.
 // TODO: maybe keep things in decoded version?
-#[derive(Clone, PartialEq, Eq, Hash)]
+#[derive(Clone, Eq)]
 pub struct PeerId {
     multihash: multihash::Multihash,
 }
@@ -55,24 +53,21 @@ impl fmt::Display for PeerId {
 
 impl PeerId {
     /// Builds a `PeerId` from a public key.
-    #[inline]
     pub fn from_public_key(key: PublicKey) -> PeerId {
         let key_enc = key.into_protobuf_encoding();
 
-        // Note: the correct behaviour, according to the libp2p specifications, is the
-        // commented-out code, which consists it transmitting small keys un-hashed. However, this
-        // version and all previous versions of rust-libp2p always hash the key. Starting from
-        // version 0.13, rust-libp2p accepts both hashed and non-hashed keys as input
-        // (see `from_bytes`). Starting from version 0.14, rust-libp2p will switch to not hashing
-        // the key (a.k.a. the correct behaviour).
+        // Note: before 0.12, this was incorrectly implemented and `SHA2256` was always used.
+        // Starting from version 0.13, rust-libp2p accepts both hashed and non-hashed keys as
+        // input (see `from_bytes`). Starting from version 0.16, rust-libp2p will switch to
+        // not hashing the key (a.k.a. the correct behaviour).
         // In other words, rust-libp2p 0.13 is compatible with all versions of rust-libp2p.
-        // Rust-libp2p 0.12 and below is **NOT** compatible with rust-libp2p 0.14 and above.
-        /*let hash_algorithm = if key_enc.len() <= MAX_INLINE_KEY_LENGTH {
+        // Rust-libp2p 0.12 and below is **NOT** compatible with rust-libp2p 0.16 and above.
+        let hash_algorithm = if key_enc.len() <= MAX_INLINE_KEY_LENGTH {
             multihash::Hash::Identity
         } else {
             multihash::Hash::SHA2256
-        };*/
-        let hash_algorithm = multihash::Hash::SHA2256;
+        };
+
         let multihash = multihash::encode(hash_algorithm, &key_enc)
             .expect("identity and sha2-256 are always supported by known public key types");
         PeerId { multihash }
@@ -80,7 +75,6 @@ impl PeerId {
 
     /// Checks whether `data` is a valid `PeerId`. If so, returns the `PeerId`. If not, returns
     /// back the data as an error.
-    #[inline]
     pub fn from_bytes(data: Vec<u8>) -> Result<PeerId, Vec<u8>> {
         match multihash::Multihash::from_bytes(data) {
             Ok(multihash) => {
@@ -98,7 +92,6 @@ impl PeerId {
 
     /// Turns a `Multihash` into a `PeerId`. If the multihash doesn't use the correct algorithm,
     /// returns back the data as an error.
-    #[inline]
     pub fn from_multihash(data: multihash::Multihash) -> Result<PeerId, multihash::Multihash> {
         if data.algorithm() == multihash::Hash::SHA2256 || data.algorithm() == multihash::Hash::Identity {
             Ok(PeerId { multihash: data })
@@ -110,7 +103,6 @@ impl PeerId {
     /// Generates a random peer ID from a cryptographically secure PRNG.
     ///
     /// This is useful for randomly walking on a DHT, or for testing purposes.
-    #[inline]
     pub fn random() -> PeerId {
         PeerId {
             multihash: multihash::Multihash::random(multihash::Hash::SHA2256)
@@ -120,7 +112,6 @@ impl PeerId {
     /// Returns a raw bytes representation of this `PeerId`.
     ///
     /// Note that this is not the same as the public key of the peer.
-    #[inline]
     pub fn into_bytes(self) -> Vec<u8> {
         self.multihash.into_bytes()
     }
@@ -128,23 +119,15 @@ impl PeerId {
     /// Returns a raw bytes representation of this `PeerId`.
     ///
     /// Note that this is not the same as the public key of the peer.
-    #[inline]
     pub fn as_bytes(&self) -> &[u8] {
         self.multihash.as_bytes()
     }
 
     /// Returns a base-58 encoded string of this `PeerId`.
-    #[inline]
     pub fn to_base58(&self) -> String {
         bs58::encode(self.multihash.as_bytes()).into_string()
     }
 
-    /// Returns the raw bytes of the hash of this `PeerId`.
-    #[inline]
-    pub fn digest(&self) -> &[u8] {
-        self.multihash.digest()
-    }
-
     /// Checks whether the public key passed as parameter matches the public key of this `PeerId`.
     ///
     /// Returns `None` if this `PeerId`s hash algorithm is not supported when encoding the
@@ -160,6 +143,25 @@ impl PeerId {
     }
 }
 
+impl hash::Hash for PeerId {
+    fn hash<H>(&self, state: &mut H)
+    where
+        H: hash::Hasher
+    {
+        match self.multihash.algorithm() {
+            multihash::Hash::Identity => {
+                let sha256 = multihash::encode(multihash::Hash::SHA2256, self.multihash.digest())
+                    .expect("encoding a SHA2256 multihash never fails; qed");
+                hash::Hash::hash(sha256.digest(), state)
+            },
+            multihash::Hash::SHA2256 => {
+                hash::Hash::hash(self.multihash.digest(), state)
+            },
+            _ => unreachable!("PeerId can only be built from Identity or SHA2256; qed")
+        }
+    }
+}
+
 impl From<PublicKey> for PeerId {
     #[inline]
     fn from(key: PublicKey) -> PeerId {
@@ -183,38 +185,45 @@ impl TryFrom<multihash::Multihash> for PeerId {
     }
 }
 
-impl PartialEq<multihash::Multihash> for PeerId {
-    #[inline]
-    fn eq(&self, other: &multihash::Multihash) -> bool {
-        &self.multihash == other
-    }
-}
-
-impl PartialEq<PeerId> for multihash::Multihash {
-    #[inline]
+impl PartialEq<PeerId> for PeerId {
     fn eq(&self, other: &PeerId) -> bool {
-        self == &other.multihash
-    }
-}
-
-impl AsRef<multihash::Multihash> for PeerId {
-    #[inline]
-    fn as_ref(&self) -> &multihash::Multihash {
-        &self.multihash
+        match (self.multihash.algorithm(), other.multihash.algorithm()) {
+            (multihash::Hash::SHA2256, multihash::Hash::SHA2256) => {
+                self.multihash.digest() == other.multihash.digest()
+            },
+            (multihash::Hash::Identity, multihash::Hash::Identity) => {
+                self.multihash.digest() == other.multihash.digest()
+            },
+            (multihash::Hash::SHA2256, multihash::Hash::Identity) => {
+                multihash::encode(multihash::Hash::SHA2256, other.multihash.digest())
+                    .map(|mh| mh == self.multihash)
+                    .unwrap_or(false)
+            },
+            (multihash::Hash::Identity, multihash::Hash::SHA2256) => {
+                multihash::encode(multihash::Hash::SHA2256, self.multihash.digest())
+                    .map(|mh| mh == other.multihash)
+                    .unwrap_or(false)
+            },
+            _ => false
+        }
     }
 }
 
+// TODO: The semantics of that function aren't very precise. It is possible for two `PeerId`s to
+//       compare equal while their bytes representation are not. Right now, this `AsRef`
+//       implementation is only used to define precedence over two `PeerId`s in case of a
+//       simultaneous connection between two nodes. Since the simultaneous connection system
+//       is planned to be removed (https://github.com/libp2p/rust-libp2p/issues/912), we went for
+//       we keeping that function with the intent of removing it as soon as possible.
 impl AsRef<[u8]> for PeerId {
-    #[inline]
     fn as_ref(&self) -> &[u8] {
         self.as_bytes()
     }
 }
 
-impl Into<multihash::Multihash> for PeerId {
-    #[inline]
-    fn into(self) -> multihash::Multihash {
-        self.multihash
+impl From<PeerId> for multihash::Multihash {
+    fn from(peer_id: PeerId) -> Self {
+        peer_id.multihash
     }
 }
 
@@ -239,6 +248,7 @@ impl FromStr for PeerId {
 #[cfg(test)]
 mod tests {
     use crate::{PeerId, identity};
+    use std::{convert::TryFrom as _, hash::{self, Hasher as _}};
 
     #[test]
     fn peer_id_is_public_key() {
@@ -268,4 +278,62 @@ mod tests {
             assert_eq!(peer_id, PeerId::from_bytes(peer_id.clone().into_bytes()).unwrap());
         }
     }
+
+    #[test]
+    fn peer_id_identity_equal_to_sha2256() {
+        let random_bytes = (0..64).map(|_| rand::random::<u8>()).collect::<Vec<u8>>();
+        let mh1 = multihash::encode(multihash::Hash::SHA2256, &random_bytes).unwrap();
+        let mh2 = multihash::encode(multihash::Hash::Identity, &random_bytes).unwrap();
+        let peer_id1 = PeerId::try_from(mh1).unwrap();
+        let peer_id2 = PeerId::try_from(mh2).unwrap();
+        assert_eq!(peer_id1, peer_id2);
+        assert_eq!(peer_id2, peer_id1);
+    }
+
+    #[test]
+    fn peer_id_identity_hashes_equal_to_sha2256() {
+        let random_bytes = (0..64).map(|_| rand::random::<u8>()).collect::<Vec<u8>>();
+        let mh1 = multihash::encode(multihash::Hash::SHA2256, &random_bytes).unwrap();
+        let mh2 = multihash::encode(multihash::Hash::Identity, &random_bytes).unwrap();
+        let peer_id1 = PeerId::try_from(mh1).unwrap();
+        let peer_id2 = PeerId::try_from(mh2).unwrap();
+
+        let mut hasher1 = fnv::FnvHasher::with_key(0);
+        hash::Hash::hash(&peer_id1, &mut hasher1);
+        let mut hasher2 = fnv::FnvHasher::with_key(0);
+        hash::Hash::hash(&peer_id2, &mut hasher2);
+
+        assert_eq!(hasher1.finish(), hasher2.finish());
+    }
+
+    #[test]
+    fn peer_id_equal_across_algorithms() {
+        use multihash::Hash;
+        use quickcheck::{Arbitrary, Gen};
+
+        #[derive(Debug, Clone, PartialEq, Eq)]
+        struct HashAlgo(Hash);
+
+        impl Arbitrary for HashAlgo {
+            fn arbitrary<G: Gen>(g: &mut G) -> Self {
+                match g.next_u32() % 4 { // make Hash::Identity more likely
+                    0 => HashAlgo(Hash::SHA2256),
+                    _ => HashAlgo(Hash::Identity)
+                }
+            }
+        }
+
+        fn property(data: Vec<u8>, algo1: HashAlgo, algo2: HashAlgo) -> bool {
+            let a = PeerId::try_from(multihash::encode(algo1.0, &data).unwrap()).unwrap();
+            let b = PeerId::try_from(multihash::encode(algo2.0, &data).unwrap()).unwrap();
+
+            if algo1 == algo2 || algo1.0 == Hash::Identity || algo2.0 == Hash::Identity {
+                a == b
+            } else {
+                a != b
+            }
+        }
+
+        quickcheck::quickcheck(property as fn(Vec<u8>, HashAlgo, HashAlgo) -> bool)
+    }
 }

misc/mdns/src/service.rs

@@ -24,7 +24,7 @@ use dns_parser::{Packet, RData};
 use either::Either::{Left, Right};
 use futures::{future, prelude::*};
 use libp2p_core::{multiaddr::{Multiaddr, Protocol}, PeerId};
-use std::{fmt, io, net::Ipv4Addr, net::SocketAddr, str, time::{Duration, Instant}};
+use std::{convert::TryFrom as _, fmt, io, net::Ipv4Addr, net::SocketAddr, str, time::{Duration, Instant}};
 use wasm_timer::Interval;
 use lazy_static::lazy_static;
 
@@ -505,7 +505,15 @@ impl MdnsPeer {
                     Err(_) => return None,
                 };
                 match addr.pop() {
-                    Some(Protocol::P2p(ref peer_id)) if peer_id == &my_peer_id => (),
+                    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,
                 };
                 Some(addr)