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

Migrate from protoc to rust-protobuf #3050

Closed
wants to merge 21 commits into from
Closed

Migrate from protoc to rust-protobuf #3050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
29e6c38
Replace prost with protobuf in misc/prost-codec
kckeiks Oct 20, 2022
ea8a303
Replace prost with protobuf in core
kckeiks Oct 20, 2022
1d4f143
Replace prost with protobuf in transports/plaintext
kckeiks Oct 20, 2022
7973feb
Replace prost with protobuf in protocols/autonat
kckeiks Oct 20, 2022
1d3e247
Use lite runtime
kckeiks Oct 21, 2022
8acaa31
Remove roundtripping
kckeiks Oct 21, 2022
428f86b
Replace prost with protobuf in protocols/dcutr
kckeiks Oct 21, 2022
1f9e009
Replace prost with protobuf in protocols/floodsub
kckeiks Oct 21, 2022
8c7a124
Add expected invariant
kckeiks Oct 23, 2022
1e38a92
Replace prost with protobuf in protocols/gossipsub
kckeiks Oct 23, 2022
102af4c
Replace prost with protobuf in protocols/identify
kckeiks Oct 24, 2022
936d646
Replace prost with protobuf in protocols/kad
kckeiks Oct 24, 2022
543e7fb
Replace prost with protobuf in protocols/relay
kckeiks Oct 24, 2022
d290be2
Replace prost with protobuf in protocols/rendezvous
kckeiks Oct 25, 2022
deb744e
Replace prost with protobuf in protocols/noise
kckeiks Oct 25, 2022
6c27de8
Fix keys_proto::KeyType variant spelling
kckeiks Oct 25, 2022
e7ff68c
Fix tests in libp2p-core
kckeiks Oct 25, 2022
fb7e6db
Fix tests in libp2p-gossipsub
kckeiks Oct 25, 2022
9ae7ac1
Fix clippy errors
kckeiks Oct 25, 2022
c760fff
Run cargo fmt
kckeiks Oct 25, 2022
dfcb5f9
Remove reference to prost in Codec docs
kckeiks Oct 25, 2022
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
6 changes: 6 additions & 0 deletions core/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
# 0.38.0 [unreleased]

- Remove `prost` and add `protobuf`. See [PR 3050].

[PR 3050]: https://github.com/libp2p/rust-libp2p/pull/3050

# 0.37.0

- Implement `Hash` and `Ord` for `PublicKey`. See [PR 2915].
Expand Down
4 changes: 2 additions & 2 deletions core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ multistream-select = { version = "0.12", path = "../misc/multistream-select" }
p256 = { version = "0.11.1", default-features = false, features = ["ecdsa"], optional = true }
parking_lot = "0.12.0"
pin-project = "1.0.0"
prost = "0.11"
protobuf = "3.2"
rand = "0.8"
rw-stream-sink = { version = "0.3.0", path = "../misc/rw-stream-sink" }
sha2 = "0.10.0"
Expand All @@ -53,7 +53,7 @@ rmp-serde = "1.0"
serde_json = "1.0"

[build-dependencies]
prost-build = "0.11"
protobuf-codegen = "3.2"

[features]
secp256k1 = [ "libsecp256k1" ]
Expand Down
15 changes: 9 additions & 6 deletions core/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,16 @@
// DEALINGS IN THE SOFTWARE.

fn main() {
prost_build::compile_protos(
&[
protobuf_codegen::Codegen::new()
.pure()
.includes(&["src"])
.inputs(&[
"src/keys.proto",
"src/envelope.proto",
"src/peer_record.proto",
],
&["src"],
)
.unwrap();
])
.customize(protobuf_codegen::Customize::default().lite_runtime(true))
.cargo_out_dir("protos")
.run()
.unwrap()
}
110 changes: 59 additions & 51 deletions core/src/identity.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,15 @@ impl Keypair {

/// Encode a private key as protobuf structure.
pub fn to_protobuf_encoding(&self) -> Result<Vec<u8>, DecodingError> {
use prost::Message;
use protobuf::Message;

let pk = match self {
Self::Ed25519(data) => keys_proto::PrivateKey {
r#type: keys_proto::KeyType::Ed25519.into(),
data: data.encode().into(),
},
Self::Ed25519(data) => {
let mut key = keys_proto::PrivateKey::new();
key.set_Type(keys_proto::KeyType::Ed25519);
key.set_Data(data.encode().into());
key
}
#[cfg(all(feature = "rsa", not(target_arch = "wasm32")))]
Self::Rsa(_) => {
return Err(DecodingError::new(
Expand All @@ -174,32 +176,35 @@ impl Keypair {
}
};

Ok(pk.encode_to_vec())
pk.write_to_bytes()
.map_err(|e| DecodingError::new("Failed to decode.").source(e))
Comment on lines +179 to +180
Copy link
Contributor

Choose a reason for hiding this comment

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

Please panic here for consistency.

}

/// Decode a private key from a protobuf structure and parse it as a [`Keypair`].
pub fn from_protobuf_encoding(bytes: &[u8]) -> Result<Keypair, DecodingError> {
use prost::Message;
use protobuf::Enum;
use protobuf::Message;

let mut private_key = keys_proto::PrivateKey::decode(bytes)
let mut private_key = keys_proto::PrivateKey::parse_from_bytes(bytes)
.map_err(|e| DecodingError::new("Protobuf").source(e))
.map(zeroize::Zeroizing::new)?;

let key_type = keys_proto::KeyType::from_i32(private_key.r#type).ok_or_else(|| {
DecodingError::new(format!("unknown key type: {}", private_key.r#type))
})?;
let key_type =
keys_proto::KeyType::from_i32(private_key.Type().value()).ok_or_else(|| {
DecodingError::new(format!("unknown key type: {:?}", private_key.Type()))
})?;
Comment on lines +192 to +195
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like another round-trip to me? Wouldn't Type() already give you the enum value?


match key_type {
keys_proto::KeyType::Ed25519 => {
ed25519::Keypair::decode(&mut private_key.data).map(Keypair::Ed25519)
ed25519::Keypair::decode(private_key.mut_Data()).map(Keypair::Ed25519)
}
keys_proto::KeyType::Rsa => Err(DecodingError::new(
keys_proto::KeyType::RSA => Err(DecodingError::new(
"Decoding RSA key from Protobuf is unsupported.",
)),
keys_proto::KeyType::Secp256k1 => Err(DecodingError::new(
"Decoding Secp256k1 key from Protobuf is unsupported.",
)),
keys_proto::KeyType::Ecdsa => Err(DecodingError::new(
keys_proto::KeyType::ECDSA => Err(DecodingError::new(
"Decoding ECDSA key from Protobuf is unsupported.",
)),
}
Expand All @@ -208,8 +213,8 @@ impl Keypair {

impl zeroize::Zeroize for keys_proto::PrivateKey {
fn zeroize(&mut self) {
self.r#type.zeroize();
self.data.zeroize();
use protobuf::Message;
self.clear()
}
}

Expand Down Expand Up @@ -251,23 +256,21 @@ impl PublicKey {
/// Encode the public key into a protobuf structure for storage or
/// exchange with other nodes.
pub fn to_protobuf_encoding(&self) -> Vec<u8> {
use prost::Message;
use protobuf::Message;

let public_key = keys_proto::PublicKey::from(self);

let mut buf = Vec::with_capacity(public_key.encoded_len());
public_key
.encode(&mut buf)
.expect("Vec<u8> provides capacity as needed");
buf
.write_to_bytes()
.expect("All fields to be initialized.")
}

/// Decode a public key from a protobuf structure, e.g. read from storage
/// or received from another node.
pub fn from_protobuf_encoding(bytes: &[u8]) -> Result<PublicKey, DecodingError> {
use prost::Message;
use protobuf::Message;

let pubkey = keys_proto::PublicKey::decode(bytes)
let pubkey = keys_proto::PublicKey::parse_from_bytes(bytes)
.map_err(|e| DecodingError::new("Protobuf").source(e))?;

pubkey.try_into()
Expand All @@ -282,25 +285,33 @@ impl PublicKey {
impl From<&PublicKey> for keys_proto::PublicKey {
fn from(key: &PublicKey) -> Self {
match key {
PublicKey::Ed25519(key) => keys_proto::PublicKey {
r#type: keys_proto::KeyType::Ed25519 as i32,
data: key.encode().to_vec(),
},
PublicKey::Ed25519(key) => {
let mut pubkey = keys_proto::PublicKey::new();
pubkey.set_Type(keys_proto::KeyType::Ed25519);
pubkey.set_Data(key.encode().to_vec());
pubkey
}
#[cfg(all(feature = "rsa", not(target_arch = "wasm32")))]
PublicKey::Rsa(key) => keys_proto::PublicKey {
r#type: keys_proto::KeyType::Rsa as i32,
data: key.encode_x509(),
},
PublicKey::Rsa(key) => {
let mut pubkey = keys_proto::PublicKey::new();
pubkey.set_Type(keys_proto::KeyType::RSA);
pubkey.set_Data(key.encode_x509());
pubkey
}
#[cfg(feature = "secp256k1")]
PublicKey::Secp256k1(key) => keys_proto::PublicKey {
r#type: keys_proto::KeyType::Secp256k1 as i32,
data: key.encode().to_vec(),
},
PublicKey::Secp256k1(key) => {
let mut pubkey = keys_proto::PublicKey::new();
pubkey.set_Type(keys_proto::KeyType::Secp256k1);
pubkey.set_Data(key.encode().to_vec());
pubkey
}
#[cfg(feature = "ecdsa")]
PublicKey::Ecdsa(key) => keys_proto::PublicKey {
r#type: keys_proto::KeyType::Ecdsa as i32,
data: key.encode_der(),
},
PublicKey::Ecdsa(key) => {
let mut pubkey = keys_proto::PublicKey::new();
pubkey.set_Type(keys_proto::KeyType::ECDSA);
pubkey.set_Data(key.encode_der());
pubkey
}
}
}
}
Expand All @@ -309,37 +320,34 @@ impl TryFrom<keys_proto::PublicKey> for PublicKey {
type Error = DecodingError;

fn try_from(pubkey: keys_proto::PublicKey) -> Result<Self, Self::Error> {
let key_type = keys_proto::KeyType::from_i32(pubkey.r#type)
.ok_or_else(|| DecodingError::new(format!("unknown key type: {}", pubkey.r#type)))?;

match key_type {
match pubkey.Type() {
keys_proto::KeyType::Ed25519 => {
ed25519::PublicKey::decode(&pubkey.data).map(PublicKey::Ed25519)
ed25519::PublicKey::decode(pubkey.Data()).map(PublicKey::Ed25519)
}
#[cfg(all(feature = "rsa", not(target_arch = "wasm32")))]
keys_proto::KeyType::Rsa => {
rsa::PublicKey::decode_x509(&pubkey.data).map(PublicKey::Rsa)
keys_proto::KeyType::RSA => {
rsa::PublicKey::decode_x509(pubkey.Data()).map(PublicKey::Rsa)
}
#[cfg(any(not(feature = "rsa"), target_arch = "wasm32"))]
keys_proto::KeyType::Rsa => {
keys_proto::KeyType::RSA => {
log::debug!("support for RSA was disabled at compile-time");
Err(DecodingError::new("Unsupported"))
}
#[cfg(feature = "secp256k1")]
keys_proto::KeyType::Secp256k1 => {
secp256k1::PublicKey::decode(&pubkey.data).map(PublicKey::Secp256k1)
secp256k1::PublicKey::decode(pubkey.Data()).map(PublicKey::Secp256k1)
}
#[cfg(not(feature = "secp256k1"))]
keys_proto::KeyType::Secp256k1 => {
log::debug!("support for secp256k1 was disabled at compile-time");
Err(DecodingError::new("Unsupported"))
}
#[cfg(feature = "ecdsa")]
keys_proto::KeyType::Ecdsa => {
ecdsa::PublicKey::decode_der(&pubkey.data).map(PublicKey::Ecdsa)
keys_proto::KeyType::ECDSA => {
ecdsa::PublicKey::decode_der(pubkey.Data()).map(PublicKey::Ecdsa)
}
#[cfg(not(feature = "ecdsa"))]
keys_proto::KeyType::Ecdsa => {
keys_proto::KeyType::ECDSA => {
log::debug!("support for ECDSA was disabled at compile-time");
Err(DecodingError::new("Unsupported"))
}
Expand Down
15 changes: 5 additions & 10 deletions core/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,13 @@
#![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))]

#[allow(clippy::derive_partial_eq_without_eq)]
mod keys_proto {
include!(concat!(env!("OUT_DIR"), "/keys_proto.rs"));
mod protos {
include!(concat!(env!("OUT_DIR"), "/protos/mod.rs"));
}

mod envelope_proto {
include!(concat!(env!("OUT_DIR"), "/envelope_proto.rs"));
}

#[allow(clippy::derive_partial_eq_without_eq)]
mod peer_record_proto {
include!(concat!(env!("OUT_DIR"), "/peer_record_proto.rs"));
}
use protos::envelope as envelope_proto;
use protos::keys as keys_proto;
use protos::peer_record as peer_record_proto;

/// Multi-address re-export.
pub use multiaddr;
Expand Down
50 changes: 24 additions & 26 deletions core/src/peer_record.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,12 @@ impl PeerRecord {
///
/// If this function succeeds, the [`SignedEnvelope`] contained a peer record with a valid signature and can hence be considered authenticated.
pub fn from_signed_envelope(envelope: SignedEnvelope) -> Result<Self, FromEnvelopeError> {
use prost::Message;
use protobuf::Message;

let (payload, signing_key) =
envelope.payload_and_signing_key(String::from(DOMAIN_SEP), PAYLOAD_TYPE.as_bytes())?;
let record = peer_record_proto::PeerRecord::decode(payload)?;
let record = peer_record_proto::PeerRecord::parse_from_bytes(payload)
.map_err(FromEnvelopeError::from)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the map_err now?


let peer_id = PeerId::from_bytes(&record.peer_id)?;

Expand All @@ -61,7 +62,7 @@ impl PeerRecord {
///
/// This is the same key that is used for authenticating every libp2p connection of your application, i.e. what you use when setting up your [`crate::transport::Transport`].
pub fn new(key: &Keypair, addresses: Vec<Multiaddr>) -> Result<Self, SigningError> {
use prost::Message;
use protobuf::Message;

let seq = SystemTime::now()
.duration_since(SystemTime::UNIX_EPOCH)
Expand All @@ -70,22 +71,21 @@ impl PeerRecord {
let peer_id = key.public().to_peer_id();

let payload = {
let record = peer_record_proto::PeerRecord {
peer_id: peer_id.to_bytes(),
seq,
addresses: addresses
.iter()
.map(|m| peer_record_proto::peer_record::AddressInfo {
multiaddr: m.to_vec(),
})
.collect(),
};
let mut record = peer_record_proto::PeerRecord::new();
record.peer_id = peer_id.to_bytes();
record.seq = seq;
record.addresses = addresses
.iter()
.map(|m| {
let mut addr_info = peer_record_proto::peer_record::AddressInfo::new();
addr_info.multiaddr = m.to_vec();
addr_info
})
.collect();

let mut buf = Vec::with_capacity(record.encoded_len());
record
.encode(&mut buf)
.expect("Vec<u8> provides capacity as needed");
buf
.write_to_bytes()
.expect("All fields to be initialized.")
};

let envelope = SignedEnvelope::new(
Expand Down Expand Up @@ -129,7 +129,7 @@ pub enum FromEnvelopeError {
/// Failed to extract the payload from the envelope.
BadPayload(signed_envelope::ReadPayloadError),
/// Failed to decode the provided bytes as a [`PeerRecord`].
InvalidPeerRecord(prost::DecodeError),
InvalidPeerRecord(protobuf::Error),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this part of the public API? If yes, then this is actually a breaking change which is unfortunate :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I missed that. Yes, it is part of the public API 😞.

Copy link
Contributor

Choose a reason for hiding this comment

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

Damn okay. Well don't worry about it here for now, I'll send a PR that wraps the error here. That is still a breaking change but we can ship that separately!

Copy link
Contributor

Choose a reason for hiding this comment

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

So my idea here is to make a dedicated error like this:

#[derive(thiserror::Error, Debug)]
#[error(transparent)]
pub struct DecodeError(protobuf::Error)

This preserves the source chain but hides the concrete error type from the public API.

It'd be nice to do this in a separate PR and then put this feature on top of that. Let me know if you'd be happy to do that otherwise I'll try to get to that next week!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So my idea here is to make a dedicated error like this:

#[derive(thiserror::Error, Debug)]
#[error(transparent)]
pub struct DecodeError(protobuf::Error)

This preserves the source chain but hides the concrete error type from the public API.

That sounds good!

It'd be nice to do this in a separate PR and then put this feature on top of that. Let me know if you'd be happy to do that otherwise I'll try to get to that next week!

If you could please do that that would be great! I'll rebase once that is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

I starting working on this here: #3058

/// Failed to decode the peer ID.
InvalidPeerId(multihash::Error),
/// The signer of the envelope is different than the peer id in the record.
Expand All @@ -144,8 +144,8 @@ impl From<signed_envelope::ReadPayloadError> for FromEnvelopeError {
}
}

impl From<prost::DecodeError> for FromEnvelopeError {
fn from(e: prost::DecodeError) -> Self {
impl From<protobuf::Error> for FromEnvelopeError {
fn from(e: protobuf::Error) -> Self {
Self::InvalidPeerRecord(e)
}
}
Expand Down Expand Up @@ -213,7 +213,7 @@ mod tests {

#[test]
fn mismatched_signature() {
use prost::Message;
use protobuf::Message;

let addr: Multiaddr = HOME.parse().unwrap();

Expand All @@ -227,14 +227,12 @@ mod tests {
seq: 0,
addresses: vec![peer_record_proto::peer_record::AddressInfo {
multiaddr: addr.to_vec(),
..peer_record_proto::peer_record::AddressInfo::default()
}],
..peer_record_proto::PeerRecord::default()
};

let mut buf = Vec::with_capacity(record.encoded_len());
record
.encode(&mut buf)
.expect("Vec<u8> provides capacity as needed");
buf
record.write_to_bytes().unwrap()
};

SignedEnvelope::new(
Expand Down
Loading