Change representation of PeerId to CIDv1 #2322

JerryHue · 2021-11-01T07:12:25Z

Addresses #2259.

To encapsulate both errors from Multihash and CID, we change the interface of from_bytes to return a PeerIdError in the case of Err.

To encapsulate both errors from Multihash and CID, we change the interface of `from_bytes` to return a `PeerIdError` in the case of `Err`.

mxinden

First off, very solid work! Thanks @JerryHue.

I have a couple of comments below:

mxinden · 2021-11-01T17:26:29Z

core/src/peer_id.rs

+    ///
+    /// If the CID's multihash does not fit the specifications:
+    ///
+    /// * use a valid hashing algorithm for peer IDs, or


Suggested change

/// * use a valid hashing algorithm for peer IDs, or

/// * use an invalid hashing algorithm for peer IDs, or

Correct?

mxinden · 2021-11-01T17:28:33Z

core/src/peer_id.rs

+    /// * is a CIDv0 with a multihash that fits the specifications
+    /// for multihash, or
+    /// * is a CIDv1 with the proper codec (LIBP2P_KEY_CODEC),


I am confused by this. Shouldn't the opposite be written, along the lines of: "If XXX does not fit the specification, ..."?

mxinden · 2021-11-01T18:19:37Z

core/src/peer_id.rs

+    /// Returns a raw bytes representation of the `PeerId`.
+    pub fn to_cidv1_bytes(&self) -> Vec<u8> {
+        self.cid.to_bytes()
+    }


Is this needed anywhere? If not, I would prefer removing it to ease future breaking changes. If I am not mistaken test logic should be able to call self.cid.to_bytes directly.

core/src/peer_id.rs

mxinden · 2021-11-01T18:23:58Z

core/src/peer_id.rs

+        self.cid.to_bytes()
+    }
+
+    /// Returns a base-58 raw encoded string of this `PeerId`.


Suggested change

/// Returns a base-58 raw encoded string of this `PeerId`.

/// Returns a base-58 raw (non-multibase) encoded string of this `PeerId`.

core/src/peer_id.rs

mxinden · 2021-11-01T18:28:46Z

core/src/peer_id.rs

    pub fn to_base58(&self) -> String {
        bs58::encode(self.to_bytes()).into_string()
    }

+    /// Returns a `base32` encoded string of this `PeerId`.


Suggested change

/// Returns a `base32` encoded string of this `PeerId`.

/// Returns a `base32` encoded string of this `PeerId`.

///

/// This corresponds to the new peer ID [representation].

///

/// [representation]: https://github.com/libp2p/specs/blob/master/RFC/0001-text-peerid-cid.md

mxinden · 2021-11-01T18:37:33Z

core/src/peer_id.rs

+        self.cid.to_bytes()
+    }
+
+    /// Returns a base-58 raw encoded string of this `PeerId`.
    pub fn to_base58(&self) -> String {
        bs58::encode(self.to_bytes()).into_string()


The following would fail given that <PeerId as FromStr> expects the string to start with Qm or 1, right?

assert!(cidv1_peer_id == PeerId::from_str(cidv1_peer_id.to_base58()));

I am not quite sure how to proceed in this case. Maybe convert the CIDv1 peer ID to the old format first?

Maybe convert the CIDv1 peer ID to the old format first?

What do you think @JerryHue?

mxinden · 2021-11-01T18:39:24Z

core/src/peer_id.rs

+    CidError(CidError),
+}
+
+impl fmt::Display for PeerIdError {


libp2p-core already imports thiserror. What do you think of using it instead of this manual implementation?

core/src/peer_id.rs

Co-authored-by: Max Inden <[email protected]>

We check whether the old representation of a PeerId and the new representation of a PeerId produce the same PeerId.

mxinden · 2022-03-02T14:25:42Z

Friendly ping @JerryHue. Are you still interested in taking this fix to the finish line? :)

JerryHue · 2022-03-08T02:30:34Z

Hello again, @mxinden! I would like to try to bring this to the finish line throughout this week, sorry for the excruciatingly long wait. I wonder if I should improve this PR by taking into account @tomaka's comments on the original issue. What do you think?

thomaseizinger · 2022-11-02T03:45:22Z

@mxinden I believe this is waiting for your input.

tomaka · 2022-11-02T07:25:53Z

This PR would break compatibility with Substrate/Polkadot and is a no-no for us.

thomaseizinger · 2022-11-02T12:48:08Z

This PR would break compatibility with Substrate/Polkadot and is a no-no for us.

Thanks for mentioning this. This definitely needs an upgrade path where we first support reading in a CIDv1 PeerId as described in the RFC.

tomaka · 2022-11-02T12:51:27Z

For what it's worth, I've taken the decision for Polkadot a while ago to always use CIDv0. The spec precisely mentions how to encode it: https://spec.polkadot.network/#defn-peer-id. Being able to easily encode/decode PeerIds, and ensuring uniqueness of PeerIds, are more important than conforming to some libp2p spec outside of our control and that brings zero benefits for us.

It's not a deal breaker for adding support for CIDv1 in rust-libp2p, as long as there's always an option to force CIDv0.

mxinden · 2022-11-14T10:19:35Z

@mxinden I believe this is waiting for your input.

As far as I can tell blocked on addressing comments above.

It's not a deal breaker for adding support for CIDv1 in rust-libp2p, as long as there's always an option to force CIDv0.

I suggest we support reading CIDv1 as a first iteration, though always writing and handling CIDv0s.

mergify · 2022-11-15T00:16:46Z

This pull request has merge conflicts. Could you please resolve them @JerryHue? 🙏

mergify · 2023-01-14T00:22:53Z

This pull request has merge conflicts. Could you please resolve them @JerryHue? 🙏

mergify · 2023-03-16T00:26:09Z

This pull request has merge conflicts. Could you please resolve them @JerryHue? 🙏

Change representation of PeerId to CIDv1

9d20f59

To encapsulate both errors from Multihash and CID, we change the interface of `from_bytes` to return a `PeerIdError` in the case of `Err`.

mxinden reviewed Nov 1, 2021

View reviewed changes

mxinden reviewed Nov 3, 2021

View reviewed changes

core/src/peer_id.rs Show resolved Hide resolved

JerryHue and others added 4 commits November 3, 2021 17:49

Fix omission of multibase on docs

389eb89

Co-authored-by: Max Inden <[email protected]>

Add detailed description for to_base58

c3bfdea

Co-authored-by: Max Inden <[email protected]>

Improve doc for from_cid

8cc9674

Add test to check old PeerId equals new PeerId

b0b5e20

We check whether the old representation of a PeerId and the new representation of a PeerId produce the same PeerId.

thomaseizinger added the need/author-input Needs input from the original author label Nov 15, 2022

github-actions bot added the Stale label Jan 14, 2023

github-actions bot removed the Stale label Jan 15, 2023

github-actions bot added the Stale label Mar 16, 2023

github-actions bot removed the Stale label Mar 17, 2023

github-actions bot added the Stale label May 17, 2023

github-actions bot closed this May 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change representation of PeerId to CIDv1 #2322

Change representation of PeerId to CIDv1 #2322

JerryHue commented Nov 1, 2021

mxinden left a comment

mxinden Nov 1, 2021

mxinden Nov 1, 2021

mxinden Nov 1, 2021

mxinden Nov 1, 2021

mxinden Nov 1, 2021

mxinden Nov 1, 2021

mxinden Nov 15, 2021

mxinden Nov 1, 2021

mxinden commented Mar 2, 2022

JerryHue commented Mar 8, 2022

thomaseizinger commented Nov 2, 2022

tomaka commented Nov 2, 2022

thomaseizinger commented Nov 2, 2022

tomaka commented Nov 2, 2022 •

edited

Loading

mxinden commented Nov 14, 2022

mergify bot commented Nov 15, 2022

mergify bot commented Jan 14, 2023

mergify bot commented Mar 16, 2023

	/// * use a valid hashing algorithm for peer IDs, or
	/// * use an invalid hashing algorithm for peer IDs, or

	/// Returns a base-58 raw encoded string of this `PeerId`.
	/// Returns a base-58 raw (non-multibase) encoded string of this `PeerId`.

Change representation of PeerId to CIDv1 #2322

Change representation of PeerId to CIDv1 #2322

Conversation

JerryHue commented Nov 1, 2021

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Mar 2, 2022

JerryHue commented Mar 8, 2022

thomaseizinger commented Nov 2, 2022

tomaka commented Nov 2, 2022

thomaseizinger commented Nov 2, 2022

tomaka commented Nov 2, 2022 • edited Loading

mxinden commented Nov 14, 2022

mergify bot commented Nov 15, 2022

mergify bot commented Jan 14, 2023

mergify bot commented Mar 16, 2023

tomaka commented Nov 2, 2022 •

edited

Loading