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.

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

Replace Multiaddr & related types with substrate-specific types #4198

Merged
merged 26 commits into from
May 21, 2024

Conversation

dmitry-markin
Copy link
Contributor

@dmitry-markin dmitry-markin commented Apr 18, 2024

This PR introduces custom types / substrate wrappers for Multiaddr, multiaddr::Protocol, Multihash, ed25519::* and supplementary types like errors and iterators.

This is needed to unblock libp2p upgrade PR #1631 after #2944 was merged. libp2p and litep2p currently depend on different versions of multiaddr crate, and introduction of this "common ground" types is needed to support independent version upgrades of multiaddr and dependent crates in libp2p & litep2p.

While being just convenient to not tie versions of libp2p & litep2p dependencies together, it's currently not even possible to keep libp2p & litep2p dependencies updated to the same versions as multiaddr in libp2p depends on libp2p-identity that we can't include as a dependency of litep2p, which has it's own PeerId type. In the future, to keep things updated on litep2p side, we will likely need to fork multiaddr and make it use litep2p PeerId as a payload of /p2p/... protocol.

With these changes, common code in substrate uses these custom types, and litep2p & libp2p backends use corresponding libraries types.

@dmitry-markin dmitry-markin added the T0-node This PR/Issue is related to the topic “node”. label Apr 18, 2024
@dmitry-markin dmitry-markin self-assigned this Apr 18, 2024
@dmitry-markin dmitry-markin changed the title Introduce network backend agnostic Multiaddr & Multihash types Replace libp2p-identity types with substrate-specific types Apr 23, 2024
dmitry-markin added a commit to paritytech/litep2p that referenced this pull request Apr 23, 2024
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6238603

@dmitry-markin dmitry-markin changed the title Replace libp2p-identity types with substrate-specific types Replace libp2p-identity related types with substrate-specific types May 17, 2024
@dmitry-markin dmitry-markin changed the title Replace libp2p-identity related types with substrate-specific types Replace Multiaddr & related types with substrate-specific types May 17, 2024
@dmitry-markin dmitry-markin requested a review from bkchr May 17, 2024 14:13
@dmitry-markin dmitry-markin marked this pull request as ready for review May 17, 2024 14:13
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Didn't look that closely, but these are mainly wrappers any way?

substrate/client/network/types/src/multiaddr.rs Outdated Show resolved Hide resolved

/// Convert public key to `PeerId`.
pub fn to_peer_id(&self) -> PeerId {
litep2p::PeerId::from(litep2p::crypto::PublicKey::Ed25519(self.clone().into())).into()
Copy link
Member

Choose a reason for hiding this comment

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

Can we not directly construct the PeerId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would require hashing PublicKey type serialized as protobuf, and I tried to avoid including the protobuf schema into the already bloated wrapper sources.

}
}

// TODO: uncomment this after upgrading `multihash` crate to v0.19.1.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything blocking us from updating to multihash 0.19.1? Maybe we could do that in a followup and raise an issue to not forget about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should be done at the same time as upgrading libp2p. I will mention it in the libp2p PR.

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Nice job here!

There are a few CI steps that are failing, but other than that we are good to go 👍

@dmitry-markin dmitry-markin requested a review from lexnv May 20, 2024 16:02
@dmitry-markin dmitry-markin added this pull request to the merge queue May 21, 2024
Merged via the queue into master with commit d05786f May 21, 2024
151 of 152 checks passed
@dmitry-markin dmitry-markin deleted the dm-network-types-multiaddr branch May 21, 2024 16:45
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request Jun 5, 2024
…ritytech#4198)

This PR introduces custom types / substrate wrappers for `Multiaddr`,
`multiaddr::Protocol`, `Multihash`, `ed25519::*` and supplementary types
like errors and iterators.

This is needed to unblock `libp2p` upgrade PR
paritytech#1631 after
paritytech#2944 was merged.
`libp2p` and `litep2p` currently depend on different versions of
`multiaddr` crate, and introduction of this "common ground" types is
needed to support independent version upgrades of `multiaddr` and
dependent crates in `libp2p` & `litep2p`.

While being just convenient to not tie versions of `libp2p` & `litep2p`
dependencies together, it's currently not even possible to keep `libp2p`
& `litep2p` dependencies updated to the same versions as `multiaddr` in
`libp2p` depends on `libp2p-identity` that we can't include as a
dependency of `litep2p`, which has it's own `PeerId` type. In the
future, to keep things updated on `litep2p` side, we will likely need to
fork `multiaddr` and make it use `litep2p` `PeerId` as a payload of
`/p2p/...` protocol.

With these changes, common code in substrate uses these custom types,
and `litep2p` & `libp2p` backends use corresponding libraries types.
/// or litep2p's `Multiaddr` when passed to the corresponding network backend.

#[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Hash)]
pub struct Multiaddr {
Copy link
Contributor

Choose a reason for hiding this comment

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

This version is missing impl From<IpAddr> for Multiaddr from libp2p's implementation, can you add it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in #4855.

github-merge-queue bot pushed a commit that referenced this pull request Jun 21, 2024
Add `From` implementation used by downstream project.

Ref.
#4198 (comment)

CC @nazar-pc
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
…ritytech#4198)

This PR introduces custom types / substrate wrappers for `Multiaddr`,
`multiaddr::Protocol`, `Multihash`, `ed25519::*` and supplementary types
like errors and iterators.

This is needed to unblock `libp2p` upgrade PR
paritytech#1631 after
paritytech#2944 was merged.
`libp2p` and `litep2p` currently depend on different versions of
`multiaddr` crate, and introduction of this "common ground" types is
needed to support independent version upgrades of `multiaddr` and
dependent crates in `libp2p` & `litep2p`.

While being just convenient to not tie versions of `libp2p` & `litep2p`
dependencies together, it's currently not even possible to keep `libp2p`
& `litep2p` dependencies updated to the same versions as `multiaddr` in
`libp2p` depends on `libp2p-identity` that we can't include as a
dependency of `litep2p`, which has it's own `PeerId` type. In the
future, to keep things updated on `litep2p` side, we will likely need to
fork `multiaddr` and make it use `litep2p` `PeerId` as a payload of
`/p2p/...` protocol.

With these changes, common code in substrate uses these custom types,
and `litep2p` & `libp2p` backends use corresponding libraries types.
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants