Temporary canonical multihash in peer ID. (#1449)

* Temporary canonical multihash in peer ID.

* Reduce code duplication.

* Remove unnecessary impls.

Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

core/src/peer_id.rs

@@ -22,7 +22,7 @@ use crate::PublicKey;
 use bs58;
 use thiserror::Error;
 use multihash;
-use std::{convert::TryFrom, fmt, hash, str::FromStr};
+use std::{convert::TryFrom, borrow::Borrow, 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.
@@ -35,6 +35,11 @@ const _MAX_INLINE_KEY_LENGTH: usize = 42;
 #[derive(Clone, Eq)]
 pub struct PeerId {
     multihash: multihash::Multihash,
+    /// A (temporary) "canonical" multihash if `multihash` is of type
+    /// multihash::Hash::Identity, so that `Borrow<[u8]>` semantics
+    /// can be given, i.e. a view of a byte representation whose
+    /// equality is consistent with `PartialEq`.
+    canonical: Option<multihash::Multihash>,
 }
 
 impl fmt::Debug for PeerId {
@@ -64,26 +69,33 @@ impl PeerId {
         // will switch to not hashing the key (i.e. the correct behaviour).
         // In other words, rust-libp2p 0.16 is compatible with all versions of rust-libp2p.
         // Rust-libp2p 0.12 and below is **NOT** compatible with rust-libp2p 0.17 and above.
-        let hash_algorithm = /*if key_enc.len() <= MAX_INLINE_KEY_LENGTH {
-            multihash::Hash::Identity
+        let (hash_algorithm, canonical_algorithm) = /*if key_enc.len() <= MAX_INLINE_KEY_LENGTH {
+            (multihash::Hash::Identity, Some(multihash::Hash::SHA2256))
         } else {*/
-            multihash::Hash::SHA2256;
+            (multihash::Hash::SHA2256, None);
         //};
 
+        let canonical = canonical_algorithm.map(|alg|
+            multihash::encode(alg, &key_enc).expect("SHA2256 is always supported"));
+
         let multihash = multihash::encode(hash_algorithm, &key_enc)
             .expect("identity and sha2-256 are always supported by known public key types");
-        PeerId { multihash }
+
+        PeerId { multihash, canonical }
     }
 
     /// Checks whether `data` is a valid `PeerId`. If so, returns the `PeerId`. If not, returns
     /// back the data as an error.
     pub fn from_bytes(data: Vec<u8>) -> Result<PeerId, Vec<u8>> {
         match multihash::Multihash::from_bytes(data) {
             Ok(multihash) => {
-                if multihash.algorithm() == multihash::Hash::SHA2256
-                    || multihash.algorithm() == multihash::Hash::Identity
-                {
-                    Ok(PeerId { multihash })
+                if multihash.algorithm() == multihash::Hash::SHA2256 {
+                    Ok(PeerId { multihash, canonical: None })
+                }
+                else if multihash.algorithm() == multihash::Hash::Identity {
+                    let canonical = multihash::encode(multihash::Hash::SHA2256, multihash.digest())
+                        .expect("SHA2256 is always supported");
+                    Ok(PeerId { multihash, canonical: Some(canonical) })
                 } else {
                     Err(multihash.into_bytes())
                 }
@@ -95,8 +107,12 @@ impl PeerId {
     /// Turns a `Multihash` into a `PeerId`. If the multihash doesn't use the correct algorithm,
     /// returns back the data as an error.
     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 })
+        if data.algorithm() == multihash::Hash::SHA2256 {
+            Ok(PeerId { multihash: data, canonical: None })
+        } else if data.algorithm() == multihash::Hash::Identity {
+            let canonical = multihash::encode(multihash::Hash::SHA2256, data.digest())
+                .expect("SHA2256 is always supported");
+            Ok(PeerId { multihash: data, canonical: Some(canonical) })
         } else {
             Err(data)
         }
@@ -107,27 +123,32 @@ impl PeerId {
     /// This is useful for randomly walking on a DHT, or for testing purposes.
     pub fn random() -> PeerId {
         PeerId {
-            multihash: multihash::Multihash::random(multihash::Hash::SHA2256)
+            multihash: multihash::Multihash::random(multihash::Hash::SHA2256),
+            canonical: None,
         }
     }
 
     /// Returns a raw bytes representation of this `PeerId`.
     ///
-    /// Note that this is not the same as the public key of the peer.
+    /// **NOTE:** This byte representation is not necessarily consistent with
+    /// equality of peer IDs. That is, two peer IDs may be considered equal
+    /// while having a different byte representation as per `into_bytes`.
     pub fn into_bytes(self) -> Vec<u8> {
         self.multihash.into_bytes()
     }
 
     /// Returns a raw bytes representation of this `PeerId`.
     ///
-    /// Note that this is not the same as the public key of the peer.
+    /// **NOTE:** This byte representation is not necessarily consistent with
+    /// equality of peer IDs. That is, two peer IDs may be considered equal
+    /// while having a different byte representation as per `as_bytes`.
     pub fn as_bytes(&self) -> &[u8] {
         self.multihash.as_bytes()
     }
 
     /// Returns a base-58 encoded string of this `PeerId`.
     pub fn to_base58(&self) -> String {
-        bs58::encode(self.multihash.as_bytes()).into_string()
+        bs58::encode(self.borrow() as &[u8]).into_string()
     }
 
     /// Checks whether the public key passed as parameter matches the public key of this `PeerId`.
@@ -150,17 +171,8 @@ impl hash::Hash for PeerId {
     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")
-        }
+        let digest = self.borrow() as &[u8];
+        hash::Hash::hash(digest, state)
     }
 }
 
@@ -189,34 +201,21 @@ impl TryFrom<multihash::Multihash> for PeerId {
 
 impl PartialEq<PeerId> for PeerId {
     fn eq(&self, other: &PeerId) -> bool {
-        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
-        }
+        let self_digest = self.borrow() as &[u8];
+        let other_digest = other.borrow() as &[u8];
+        self_digest == other_digest
+    }
+}
+
+impl Borrow<[u8]> for PeerId {
+    fn borrow(&self) -> &[u8] {
+        self.canonical.as_ref().map_or(self.multihash.as_bytes(), |c| c.as_bytes())
     }
 }
 
-// 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.
+/// **NOTE:** This byte representation is not necessarily consistent with
+/// equality of peer IDs. That is, two peer IDs may be considered equal
+/// while having a different byte representation as per `AsRef<[u8]>`.
 impl AsRef<[u8]> for PeerId {
     fn as_ref(&self) -> &[u8] {
         self.as_bytes()

misc/multihash/src/lib.rs

@@ -13,6 +13,7 @@ use std::{convert::TryFrom, fmt::Write};
 use bytes::{BufMut, Bytes, BytesMut};
 use rand::RngCore;
 use sha2::digest::{self, VariableOutput};
+use std::borrow::Borrow;
 use unsigned_varint::{decode, encode};
 
 pub use self::errors::{DecodeError, DecodeOwnedError, EncodeError};
@@ -245,6 +246,12 @@ impl AsRef<[u8]> for Multihash {
     }
 }
 
+impl Borrow<[u8]> for Multihash {
+    fn borrow(&self) -> &[u8] {
+        self.as_bytes()
+    }
+}
+
 impl<'a> PartialEq<MultihashRef<'a>> for Multihash {
     fn eq(&self, other: &MultihashRef<'a>) -> bool {
         &*self.bytes == other.bytes

protocols/floodsub/src/layer.rs

@@ -101,7 +101,7 @@ impl Floodsub {
     /// Remove a node from the list of nodes to propagate messages to.
     #[inline]
     pub fn remove_node_from_partial_view(&mut self, peer_id: &PeerId) {
-        self.target_peers.remove(&peer_id);
+        self.target_peers.remove(peer_id);
     }
 
     /// Subscribes to a topic.

protocols/gossipsub/src/behaviour.rs

@@ -578,7 +578,7 @@ impl Gossipsub {
             "Handling subscriptions: {:?}, from source: {:?}",
             subscriptions, propagation_source
         );
-        let subscribed_topics = match self.peer_topics.get_mut(&propagation_source) {
+        let subscribed_topics = match self.peer_topics.get_mut(propagation_source) {
             Some(topics) => topics,
             None => {
                 error!("Subscription by unknown peer: {:?}", &propagation_source);
@@ -841,7 +841,7 @@ impl Gossipsub {
                 })
                 .collect();
             let mut prunes: Vec<GossipsubControlAction> = to_prune
-                .remove(&peer)
+                .remove(peer)
                 .unwrap_or_else(|| vec![])
                 .iter()
                 .map(|topic_hash| GossipsubControlAction::Prune {
@@ -1028,7 +1028,7 @@ impl NetworkBehaviour for Gossipsub {
         // remove from mesh, topic_peers, peer_topic and fanout
         debug!("Peer disconnected: {:?}", id);
         {
-            let topics = match self.peer_topics.get(&id) {
+            let topics = match self.peer_topics.get(id) {
                 Some(topics) => (topics),
                 None => {
                     warn!("Disconnected node, not in connected nodes");

protocols/kad/src/behaviour.rs

@@ -1100,7 +1100,7 @@ where
             }
 
             for query in self.queries.iter_mut() {
-                if let Some(addrs) = query.inner.addresses.get_mut(&peer_id) {
+                if let Some(addrs) = query.inner.addresses.get_mut(peer_id) {
                     addrs.retain(|a| a != addr);
                 }
             }

protocols/kad/src/behaviour/test.rs

@@ -409,8 +409,8 @@ fn put_record() {
                     let key = kbucket::Key::new(r.key.clone());
                     let mut expected = swarm_ids.clone().split_off(1);
                     expected.sort_by(|id1, id2|
-                        kbucket::Key::new(id1).distance(&key).cmp(
-                            &kbucket::Key::new(id2).distance(&key)));
+                        kbucket::Key::new(id1.clone()).distance(&key).cmp(
+                            &kbucket::Key::new(id2.clone()).distance(&key)));
 
                     let expected = expected
                         .into_iter()
@@ -608,8 +608,8 @@ fn add_provider() {
                     let mut expected = swarm_ids.clone().split_off(1);
                     let kbucket_key = kbucket::Key::new(key);
                     expected.sort_by(|id1, id2|
-                        kbucket::Key::new(id1).distance(&kbucket_key).cmp(
-                            &kbucket::Key::new(id2).distance(&kbucket_key)));
+                        kbucket::Key::new(id1.clone()).distance(&kbucket_key).cmp(
+                            &kbucket::Key::new(id2.clone()).distance(&kbucket_key)));
 
                     let expected = expected
                         .into_iter()

protocols/kad/src/kbucket/key.rs

@@ -23,6 +23,7 @@ use libp2p_core::PeerId;
 use multihash::Multihash;
 use sha2::{Digest, Sha256};
 use sha2::digest::generic_array::{GenericArray, typenum::U32};
+use std::borrow::Borrow;
 use std::hash::{Hash, Hasher};
 
 construct_uint! {
@@ -51,9 +52,9 @@ impl<T> Key<T> {
     /// [`Key::into_preimage`].
     pub fn new(preimage: T) -> Key<T>
     where
-        T: AsRef<[u8]>
+        T: Borrow<[u8]>
     {
-        let bytes = KeyBytes::new(&preimage);
+        let bytes = KeyBytes::new(preimage.borrow());
         Key { preimage, bytes }
     }
 

protocols/kad/src/record.rs

@@ -25,6 +25,7 @@ pub mod store;
 use bytes::Bytes;
 use libp2p_core::PeerId;
 use multihash::Multihash;
+use std::borrow::Borrow;
 use std::hash::{Hash, Hasher};
 use wasm_timer::Instant;
 
@@ -44,6 +45,12 @@ impl Key {
     }
 }
 
+impl Borrow<[u8]> for Key {
+    fn borrow(&self) -> &[u8] {
+        &self.0[..]
+    }
+}
+
 impl AsRef<[u8]> for Key {
     fn as_ref(&self) -> &[u8] {
         &self.0[..]

protocols/kad/src/record/store/memory.rs

@@ -191,7 +191,7 @@ impl<'a> RecordStore<'a> for MemoryStore {
     }
 
     fn providers(&'a self, key: &Key) -> Vec<ProviderRecord> {
-        self.providers.get(&key).map_or_else(Vec::new, |ps| ps.clone().into_vec())
+        self.providers.get(key).map_or_else(Vec::new, |ps| ps.clone().into_vec())
     }
 
     fn provided(&'a self) -> Self::ProvidedIter {