Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove temporary peer ID compatibility mode. #1608

Merged
merged 7 commits into from
Aug 26, 2020
Merged

Remove temporary peer ID compatibility mode. #1608

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3147a41
Remove temporary peer ID compatibility.
Jun 15, 2020
745ba13
Merge branch 'master' into finish-peer-id-migration
romanb Aug 11, 2020
c308938
Merge branch 'master' into finish-peer-id-migration
romanb Aug 24, 2020
c251d8f
Check digest length in PeerId::from_multihash for Identity hash.
Aug 25, 2020
ef70db9
Update core/src/peer_id.rs
romanb Aug 25, 2020
7504764
Update core changelog.
Aug 26, 2020
10df04b
Update core changelog.
Aug 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 22 additions & 109 deletions core/src/peer_id.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
use crate::PublicKey;
use bs58;
use thiserror::Error;
use multihash::{self, Code, Sha2_256};
use multihash::{self, Code, Multihash};
use rand::Rng;
use std::{convert::TryFrom, borrow::Borrow, fmt, hash, str::FromStr, cmp};

Expand All @@ -35,12 +35,7 @@ const MAX_INLINE_KEY_LENGTH: usize = 42;
// TODO: maybe keep things in decoded version?
#[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>,
multihash: Multihash,
}

impl fmt::Debug for PeerId {
Expand Down Expand Up @@ -77,57 +72,35 @@ impl PeerId {
pub fn from_public_key(key: PublicKey) -> PeerId {
let key_enc = key.into_protobuf_encoding();

// 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 compare
// `PeerId`s of different hashes equal, which makes it possible to connect through
// secio or noise to nodes with an identity hash. Starting from version 0.17, rust-libp2p
// 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.
Comment on lines -80 to -87
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could leave that comment, but I don't have a strong opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I incorporated it into the changelog (see 7504764).

let (hash_algorithm, canonical_algorithm) = if key_enc.len() <= MAX_INLINE_KEY_LENGTH {
(Code::Identity, Some(Code::Sha2_256))
let hash_algorithm = if key_enc.len() <= MAX_INLINE_KEY_LENGTH {
Code::Identity
} else {
(Code::Sha2_256, None)
Code::Sha2_256
};

let canonical = canonical_algorithm.map(|alg|
alg.digest(&key_enc));

let multihash = hash_algorithm.digest(&key_enc);

PeerId { multihash, canonical }
PeerId { multihash }
}

/// 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::Code::Sha2_256 {
Ok(PeerId { multihash, canonical: None })
}
else if multihash.algorithm() == multihash::Code::Identity {
let canonical = Sha2_256::digest(&multihash.digest());
Ok(PeerId { multihash, canonical: Some(canonical) })
} else {
Err(multihash.into_bytes())
}
}
match Multihash::from_bytes(data) {
Ok(multihash) => match multihash.algorithm() {
Code::Sha2_256 | Code::Identity => Ok(PeerId { multihash }),
_ => Err(multihash.into_bytes())
},
romanb marked this conversation as resolved.
Show resolved Hide resolved
Err(err) => Err(err.data),
}
}

/// 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::Code::Sha2_256 {
Ok(PeerId { multihash: data, canonical: None })
} else if data.algorithm() == multihash::Code::Identity {
let canonical = Sha2_256::digest(data.digest());
Ok(PeerId { multihash: data, canonical: Some(canonical) })
} else {
Err(data)
pub fn from_multihash(data: Multihash) -> Result<PeerId, Multihash> {
match data.algorithm() {
Code::Sha2_256 | Code::Identity => Ok(PeerId { multihash: data }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the case right now, but maybe we should check that data.len() <= MAX_INLINE_KEY_LENGTH when the algorithm is identity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See c251d8f.

_ => Err(data)
}
}

Expand All @@ -137,8 +110,7 @@ impl PeerId {
pub fn random() -> PeerId {
let peer_id = rand::thread_rng().gen::<[u8; 32]>();
PeerId {
multihash: multihash::wrap(multihash::Code::Sha2_256, &peer_id),
canonical: None,
multihash: multihash::wrap(Code::Identity, &peer_id),
}
}

Expand All @@ -162,7 +134,7 @@ impl PeerId {

/// Returns a base-58 encoded string of this `PeerId`.
pub fn to_base58(&self) -> String {
bs58::encode(self.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`.
Expand All @@ -187,7 +159,6 @@ impl hash::Hash for PeerId {
}

impl From<PublicKey> for PeerId {
#[inline]
fn from(key: PublicKey) -> PeerId {
PeerId::from_public_key(key)
}
Expand All @@ -201,10 +172,10 @@ impl TryFrom<Vec<u8>> for PeerId {
}
}

impl TryFrom<multihash::Multihash> for PeerId {
type Error = multihash::Multihash;
impl TryFrom<Multihash> for PeerId {
type Error = Multihash;

fn try_from(value: multihash::Multihash) -> Result<Self, Self::Error> {
fn try_from(value: Multihash) -> Result<Self, Self::Error> {
PeerId::from_multihash(value)
}
}
Expand All @@ -219,7 +190,7 @@ impl PartialEq<PeerId> for PeerId {

impl Borrow<[u8]> for PeerId {
fn borrow(&self) -> &[u8] {
self.canonical.as_ref().map_or(self.multihash.as_bytes(), |c| c.as_bytes())
self.multihash.as_bytes()
}
}

Expand All @@ -232,7 +203,7 @@ impl AsRef<[u8]> for PeerId {
}
}

impl From<PeerId> for multihash::Multihash {
impl From<PeerId> for Multihash {
fn from(peer_id: PeerId) -> Self {
peer_id.multihash
}
Expand Down Expand Up @@ -289,62 +260,4 @@ 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::Sha2_256::digest(&random_bytes);
let mh2 = multihash::Identity::digest(&random_bytes);
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::Sha2_256::digest(&random_bytes);
let mh2 = multihash::Identity::digest(&random_bytes);
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::Code;
use quickcheck::{Arbitrary, Gen};

#[derive(Debug, Clone, PartialEq, Eq)]
struct HashAlgo(Code);

impl Arbitrary for HashAlgo {
fn arbitrary<G: Gen>(g: &mut G) -> Self {
match g.next_u32() % 4 { // make Hash::Identity more likely
0 => HashAlgo(Code::Sha2_256),
_ => HashAlgo(Code::Identity)
}
}
}

fn property(data: Vec<u8>, algo1: HashAlgo, algo2: HashAlgo) -> bool {
let a = PeerId::try_from(algo1.0.digest(&data)).unwrap();
let b = PeerId::try_from(algo2.0.digest(&data)).unwrap();

if algo1 == algo2 || algo1.0 == Code::Identity || algo2.0 == Code::Identity {
a == b
} else {
a != b
}
}

quickcheck::quickcheck(property as fn(Vec<u8>, HashAlgo, HashAlgo) -> bool)
}
}