From b872bd9030483847fa650a4c57d6bb3f62e1fd75 Mon Sep 17 00:00:00 2001 From: Roman Borschel Date: Thu, 13 Feb 2020 10:36:14 +0100 Subject: [PATCH] Temporary canonical multihash in peer ID. (#1449) * Temporary canonical multihash in peer ID. * Reduce code duplication. * Remove unnecessary impls. Co-authored-by: Pierre Krieger --- core/src/peer_id.rs | 101 +++++++++++------------ misc/multihash/src/lib.rs | 7 ++ protocols/floodsub/src/layer.rs | 2 +- protocols/gossipsub/src/behaviour.rs | 6 +- protocols/kad/src/behaviour.rs | 2 +- protocols/kad/src/behaviour/test.rs | 8 +- protocols/kad/src/kbucket/key.rs | 5 +- protocols/kad/src/record.rs | 7 ++ protocols/kad/src/record/store/memory.rs | 2 +- 9 files changed, 77 insertions(+), 63 deletions(-) diff --git a/core/src/peer_id.rs b/core/src/peer_id.rs index ed731cf1..09a8d7cd 100644 --- a/core/src/peer_id.rs +++ b/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, } impl fmt::Debug for PeerId { @@ -64,15 +69,19 @@ 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 @@ -80,10 +89,13 @@ impl PeerId { pub fn from_bytes(data: Vec) -> Result> { 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 { - 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 { 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 for PeerId { impl PartialEq 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 } } -// 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 Borrow<[u8]> for PeerId { + fn borrow(&self) -> &[u8] { + self.canonical.as_ref().map_or(self.multihash.as_bytes(), |c| c.as_bytes()) + } +} + +/// **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() diff --git a/misc/multihash/src/lib.rs b/misc/multihash/src/lib.rs index fa9c29f5..fe5d2f52 100644 --- a/misc/multihash/src/lib.rs +++ b/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> for Multihash { fn eq(&self, other: &MultihashRef<'a>) -> bool { &*self.bytes == other.bytes diff --git a/protocols/floodsub/src/layer.rs b/protocols/floodsub/src/layer.rs index abf95efa..323221c9 100644 --- a/protocols/floodsub/src/layer.rs +++ b/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. diff --git a/protocols/gossipsub/src/behaviour.rs b/protocols/gossipsub/src/behaviour.rs index a0f3e3db..fe43a97b 100644 --- a/protocols/gossipsub/src/behaviour.rs +++ b/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 = 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"); diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 51bbe581..b281744a 100644 --- a/protocols/kad/src/behaviour.rs +++ b/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); } } diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index 49915147..3d80db6b 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/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() diff --git a/protocols/kad/src/kbucket/key.rs b/protocols/kad/src/kbucket/key.rs index 10c801a1..95b36803 100644 --- a/protocols/kad/src/kbucket/key.rs +++ b/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 Key { /// [`Key::into_preimage`]. pub fn new(preimage: T) -> Key where - T: AsRef<[u8]> + T: Borrow<[u8]> { - let bytes = KeyBytes::new(&preimage); + let bytes = KeyBytes::new(preimage.borrow()); Key { preimage, bytes } } diff --git a/protocols/kad/src/record.rs b/protocols/kad/src/record.rs index dcd724b5..6d7e50c3 100644 --- a/protocols/kad/src/record.rs +++ b/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[..] diff --git a/protocols/kad/src/record/store/memory.rs b/protocols/kad/src/record/store/memory.rs index 16adb683..cb0503d9 100644 --- a/protocols/kad/src/record/store/memory.rs +++ b/protocols/kad/src/record/store/memory.rs @@ -191,7 +191,7 @@ impl<'a> RecordStore<'a> for MemoryStore { } fn providers(&'a self, key: &Key) -> Vec { - 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 {