Migrate from protoc to rust-protobuf #3050

kckeiks · 2022-10-20T18:03:03Z

Description

Closes #3024.
Fixes #2645.

Links to any relevant issues

gen_mod_rs codegen option stepancheg/rust-protobuf#525

Open Questions

I updated core and a couple of other packages. This is wip atm but I'll update the remaining crates this week.

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

thomaseizinger

Thank you for tackling this! Very appreciated!

I left a few questions.

core/src/identity.rs

core/build.rs

core/src/identity.rs

thomaseizinger · 2022-10-20T20:35:27Z

core/src/peer_record.rs

@@ -129,7 +126,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),


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

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

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!

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!

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.

I starting working on this here: #3058

thomaseizinger

Thanks!

thomaseizinger · 2022-10-21T21:47:03Z

core/build.rs

-    .unwrap();
+            ]
+        )
+        .customize(cust.lite_runtime(true))


All the builder fns take self so I think we can just do this:

Suggested change

.customize(cust.lite_runtime(true))

.customize(protobuf_codegen::Customize::default().lite_runtime(true))

thomaseizinger · 2022-10-21T21:49:11Z

core/src/identity.rs

-            .encode(&mut buf)
-            .expect("Vec<u8> provides capacity as needed");
-        buf
+        public_key.write_to_bytes().expect("Encoding failed.")


Can we say something like "all fields to be initialized" given that's the invariant we are relying on?

protocols/autonat/src/protocol.rs

protocols/dcutr/CHANGELOG.md

thomaseizinger · 2022-10-21T21:56:28Z

transports/plaintext/src/error.rs

@@ -28,7 +28,7 @@ pub enum PlainTextError {
    IoError(IoError),

    /// Failed to parse the handshake protobuf message.
-    InvalidPayload(Option<prost::DecodeError>),
+    InvalidPayload(Option<protobuf::Error>),


This makes it a breaking change too I think.

Yes, it's public.

thomaseizinger · 2022-10-21T22:14:21Z

I kicked CI, seems there are still a couple of compile errors 😊

kckeiks · 2022-10-21T22:19:42Z

I kicked CI, seems there are still a couple of compile errors blush

Yes, I need to clean up a few tests. Will fix.

thomaseizinger · 2022-10-24T02:16:53Z

misc/prost-codec/Cargo.toml

@@ -13,9 +13,6 @@ categories = ["asynchronous"]
 [dependencies]
 asynchronous-codec = { version = "0.6" }
 bytes = { version = "1" }
-prost = "0.11"
+protobuf = "3.2"


This isn't really a prost-codec anymore, now that we are replacing the implementation with protobuf 😅

@mxinden Should we publish a new crate?

kckeiks · 2022-10-25T01:14:32Z

I think there is one crate left to do. I should have time to do it tomorrow and also take care of some of the other pending tasks.

thomaseizinger

Only a few minor comments!

Looking forward to the full solution :)

protocols/relay/src/v2/protocol/inbound_hop.rs

protocols/rendezvous/src/codec.rs

mxinden · 2022-10-25T16:31:58Z

First off, thanks @kckeiks for the work here. This will make using rust-libp2p for newcomers so much easier.

Given that Protobuf is at the core of libp2p, performance is relevant. Are there any comparisons to prost you can point me to @thomaseizinger @kckeiks? Let's make sure to test this, e.g. via a large user, before merging here.

The method write_to_bytes only fails when writting a proto2 message with missing required fields.

kckeiks · 2022-10-25T20:52:35Z

First off, thanks @kckeiks for the work here. This will make using rust-libp2p for newcomers so much easier.

👍

Given that Protobuf is at the core of libp2p, performance is relevant. Are there any comparisons to prost you can point me to @thomaseizinger @kckeiks? Let's make sure to test this, e.g. via a large user, before merging here.

quick-proto has some performance comparisons of prost, protobuf and quick-protobuf. protobuf looks slower than the rest. However, these results are from 4 years ago.

kckeiks · 2022-10-25T21:07:01Z

I kicked CI, seems there are still a couple of compile errors blush

I fixed the tests.

jxs · 2022-10-25T22:24:39Z

quick-proto perfest with protobuf 3.2.0 and prost 0.11 (pushed the changes here)

         labels   rust-protobuf  quick-protobuf           prost      quick/rust      prost/rust
                        ns/iter         ns/iter         ns/iter               %               %

test1
          write             632             471             470            25.5            25.6
           read             966             448             539            53.6            44.2
    read no vec             853             330             440            61.3            48.4
     read reuse             788                              NA              NA

test_repeated_bool
          write             859            1010             890           -17.6            -3.6
           read            1617             924            1136            42.9            29.7
    read no vec            1519             902            1082            40.6            28.8
     read reuse            1027                              NA              NA

test_repeated_packed_int32
          write            1736            1753            1595            -1.0             8.1
           read            2290            1693            1655            26.1            27.7
    read no vec            2261            1622            1575            28.3            30.3
     read reuse            1708                              NA              NA

test_repeated_messages
          write            4542            6754            7764           -48.7           -70.9
           read            9943            6884            7047            30.8            29.1
    read no vec            6616            3939            4032            40.5            39.1
     read reuse            5802                              NA              NA

test_optional_messages
          write            2062            1702            1768            17.5            14.3
           read            4420            1768            2322            60.0            47.5
    read no vec            4390            1949            2626            55.6            40.2
     read reuse            4328                              NA              NA

test_strings
          write            1141             970             951            15.0            16.7
           read            3114             883            2772            71.6            11.0
    read no vec            2972             695            2727            76.6             8.2
     read reuse            2841                              NA              NA

test_small_bytearrays
          write             992             890             900            10.3             9.3
           read            1860             652            2651            64.9           -42.5
    read no vec            1783             551            2445            69.1           -37.1
     read reuse            1638                              NA              NA

test_large_bytearrays
          write           22729           14337           25541            36.9           -12.4
           read           23667            4691           27981            80.2           -18.2
    read no vec            7103             687            8872            90.3           -24.9
     read reuse            5393                              NA              NA
Also (not performance related but fun to know) - Rpc test was successful!

kckeiks · 2022-10-25T22:28:24Z

Thank you for running those tests @jxs !

thomaseizinger · 2022-10-26T00:53:40Z

@kckeiks Please decide if you want to amend commits and force-push or never force-push at all :)

We squash merge in the end so the commits will be gone anyway. But reviews can sometimes be easier by amending commits later but that is also not happening here: For example 8acaa31 would need to be squashed into ea8a303.

Force-pushing but not amending commits is kind of the worst of both worlds: I can't see a clear diff from the last push but the commits also overlap, making it hard to review patch-by-patch :)

thomaseizinger

Thanks!

A few more comments, plus the CI workflow needs adjusting to remove the installation of protoc!

thomaseizinger · 2022-10-26T00:48:33Z

core/src/identity.rs

@@ -174,32 +176,33 @@ impl Keypair {
            }
        };

-        Ok(pk.encode_to_vec())
+        Ok(pk.write_to_bytes().map_err(|e| DecodingError::new("Failed to decode.").source(e))?)


We discovered that this can never fail right? Let's panic here for consistency.

thomaseizinger · 2022-10-26T00:54:41Z

core/src/identity.rs

+        pk.write_to_bytes()
+            .map_err(|e| DecodingError::new("Failed to decode.").source(e))


Please panic here for consistency.

thomaseizinger · 2022-10-26T00:55:18Z

core/src/identity.rs

+        let key_type =
+            keys_proto::KeyType::from_i32(private_key.Type().value()).ok_or_else(|| {
+                DecodingError::new(format!("unknown key type: {:?}", private_key.Type()))
+            })?;


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

thomaseizinger · 2022-10-26T00:56:18Z

core/src/peer_record.rs


        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)?;


Why do we need the map_err now?

thomaseizinger · 2022-10-26T00:59:58Z

protocols/gossipsub/src/behaviour.rs

-                m.msg_sent(&topic_hash, msg_bytes);
+                m.msg_sent(
+                    &topic_hash,
+                    usize::try_from(msg_bytes).expect("Size of sent messages fit in usize."),


If we panic here, we might as well also panic on line 621.

I think as regular cast could do as well.

thomaseizinger · 2022-10-26T01:11:01Z

quick-proto perfest with protobuf 3.2.0 and prost 0.11 (pushed the changes here)

         labels   rust-protobuf  quick-protobuf           prost      quick/rust      prost/rust
                        ns/iter         ns/iter         ns/iter               %               %

test1
          write             632             471             470            25.5            25.6
           read             966             448             539            53.6            44.2
    read no vec             853             330             440            61.3            48.4
     read reuse             788                              NA              NA

test_repeated_bool
          write             859            1010             890           -17.6            -3.6
           read            1617             924            1136            42.9            29.7
    read no vec            1519             902            1082            40.6            28.8
     read reuse            1027                              NA              NA

test_repeated_packed_int32
          write            1736            1753            1595            -1.0             8.1
           read            2290            1693            1655            26.1            27.7
    read no vec            2261            1622            1575            28.3            30.3
     read reuse            1708                              NA              NA

test_repeated_messages
          write            4542            6754            7764           -48.7           -70.9
           read            9943            6884            7047            30.8            29.1
    read no vec            6616            3939            4032            40.5            39.1
     read reuse            5802                              NA              NA

test_optional_messages
          write            2062            1702            1768            17.5            14.3
           read            4420            1768            2322            60.0            47.5
    read no vec            4390            1949            2626            55.6            40.2
     read reuse            4328                              NA              NA

test_strings
          write            1141             970             951            15.0            16.7
           read            3114             883            2772            71.6            11.0
    read no vec            2972             695            2727            76.6             8.2
     read reuse            2841                              NA              NA

test_small_bytearrays
          write             992             890             900            10.3             9.3
           read            1860             652            2651            64.9           -42.5
    read no vec            1783             551            2445            69.1           -37.1
     read reuse            1638                              NA              NA

test_large_bytearrays
          write           22729           14337           25541            36.9           -12.4
           read           23667            4691           27981            80.2           -18.2
    read no vec            7103             687            8872            90.3           -24.9
     read reuse            5393                              NA              NA
Also (not performance related but fun to know) - Rpc test was successful!

Those relative numbers aren't exactly great. I'd be willing to take that hit though for the removal of the protoc dependency.

kckeiks · 2022-10-26T01:52:41Z

@kckeiks Please decide if you want to amend commits and force-push or never force-push at all :)

We squash merge in the end so the commits will be gone anyway. But reviews can sometimes be easier by amending commits later but that is also not happening here: For example 8acaa31 would need to be squashed into ea8a303.

Force-pushing but not amending commits is kind of the worst of both worlds: I can't see a clear diff from the last push but the commits also overlap, making it hard to review patch-by-patch :)

Sorry, I've been relying on my command history too much while amending and force-pushing so I inadvertently force-pushed. I will cut that out 😸 .

kckeiks · 2022-10-26T02:02:33Z

Any thoughts on using quick-protobuf instead? Here is a list of some of its missing features tafia/quick-protobuf#12. I tried it out and the API seems simpler than protobuf. There hasn't been much recent activity/development though.

thomaseizinger · 2022-10-26T08:53:22Z

Any thoughts on using quick-protobuf instead? Here is a list of some of its missing features tafia/quick-protobuf#12. I tried it out and the API seems simpler than protobuf. There hasn't been much recent activity/development though.

Can you share an example of what quick-protobuf looks like if we were to integrate it into our codebase? I don't mind much which implementation we are using, as long as we get rid of the protoc dependency :)

jxs · 2022-10-26T09:04:37Z

Those relative numbers aren't exactly great. I'd be willing to take that hit though for the removal of the protoc dependency.

yeah agree Thomas.

Any thoughts on using quick-protobuf instead? Here is a list of some of its missing features tafia/quick-protobuf#12. I tried it out and the API seems simpler than protobuf. There hasn't been much recent activity/development though.

rust-protobuf also seems to be looking for maintainers

kckeiks · 2022-10-26T22:22:57Z

Any thoughts on using quick-protobuf instead? Here is a list of some of its missing features tafia/quick-protobuf#12. I tried it out and the API seems simpler than protobuf. There hasn't been much recent activity/development though.

Can you share an example of what quick-protobuf looks like if we were to integrate it into our codebase? I don't mind much which implementation we are using, as long as we get rid of the protoc dependency :)

This PR #3066 uses quick-proto in libp2p-core.

thomaseizinger mentioned this pull request Oct 20, 2022

Migrate from protoc to quick-protobuf and remove the buildscripts #3024

Closed

thomaseizinger reviewed Oct 20, 2022

View reviewed changes

kckeiks force-pushed the use-rust-protobuf branch 3 times, most recently from 38b7e97 to f1f6ee4 Compare October 21, 2022 20:48

thomaseizinger reviewed Oct 21, 2022

View reviewed changes

thomaseizinger reviewed Oct 24, 2022

View reviewed changes

thomaseizinger mentioned this pull request Oct 24, 2022

*: Don't leak prost dependency in error types #3058

Merged

4 tasks

kckeiks force-pushed the use-rust-protobuf branch from 7f7e110 to cb1ffbb Compare October 25, 2022 01:12

thomaseizinger reviewed Oct 25, 2022

View reviewed changes

protocols/relay/src/v2/protocol/inbound_hop.rs Outdated Show resolved Hide resolved

protocols/rendezvous/src/codec.rs Outdated Show resolved Hide resolved

protocols/rendezvous/src/codec.rs Outdated Show resolved Hide resolved

protocols/rendezvous/src/codec.rs Outdated Show resolved Hide resolved

kckeiks force-pushed the use-rust-protobuf branch from cb1ffbb to 5e7eb2c Compare October 25, 2022 19:29

kckeiks added 15 commits October 25, 2022 16:03

Replace prost with protobuf in misc/prost-codec

29e6c38

Replace prost with protobuf in core

ea8a303

Replace prost with protobuf in transports/plaintext

1d4f143

Replace prost with protobuf in protocols/autonat

7973feb

Use lite runtime

1d3e247

Remove roundtripping

8acaa31

Replace prost with protobuf in protocols/dcutr

428f86b

Replace prost with protobuf in protocols/floodsub

1f9e009

Add expected invariant

8c7a124

The method write_to_bytes only fails when writting a proto2 message with missing required fields.

Replace prost with protobuf in protocols/gossipsub

1e38a92

Replace prost with protobuf in protocols/identify

102af4c

Replace prost with protobuf in protocols/kad

936d646

Replace prost with protobuf in protocols/relay

543e7fb

Replace prost with protobuf in protocols/rendezvous

d290be2

Replace prost with protobuf in protocols/noise

deb744e

kckeiks added 3 commits October 25, 2022 16:10

Fix keys_proto::KeyType variant spelling

6c27de8

Fix tests in libp2p-core

e7ff68c

Fix tests in libp2p-gossipsub

fb7e6db

kckeiks force-pushed the use-rust-protobuf branch from 5e7eb2c to fb7e6db Compare October 25, 2022 20:10

kckeiks added 3 commits October 25, 2022 18:11

Fix clippy errors

9ae7ac1

Run cargo fmt

c760fff

Remove reference to prost in Codec docs

dfcb5f9

thomaseizinger requested changes Oct 26, 2022

View reviewed changes

kckeiks mentioned this pull request Oct 26, 2022

Migrate from protoc to quick-protobuf #3066

Closed

4 tasks

thomaseizinger closed this Feb 1, 2023

	.customize(cust.lite_runtime(true))
	.customize(protobuf_codegen::Customize::default().lite_runtime(true))

		pk.write_to_bytes()
		.map_err(\|e\| DecodingError::new("Failed to decode.").source(e))

Migrate from protoc to rust-protobuf #3050

Migrate from protoc to rust-protobuf #3050

Conversation

kckeiks commented Oct 20, 2022 • edited by thomaseizinger Loading

Description

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger 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

thomaseizinger 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

thomaseizinger commented Oct 21, 2022

kckeiks commented Oct 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kckeiks commented Oct 25, 2022 • edited Loading

thomaseizinger left a comment

Choose a reason for hiding this comment

mxinden commented Oct 25, 2022

kckeiks commented Oct 25, 2022 • edited Loading

kckeiks commented Oct 25, 2022

jxs commented Oct 25, 2022

kckeiks commented Oct 25, 2022

thomaseizinger commented Oct 26, 2022 • edited Loading

thomaseizinger 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

thomaseizinger commented Oct 26, 2022

kckeiks commented Oct 26, 2022

kckeiks commented Oct 26, 2022

thomaseizinger commented Oct 26, 2022

jxs commented Oct 26, 2022

kckeiks commented Oct 26, 2022

kckeiks commented Oct 20, 2022 •

edited by thomaseizinger

Loading

kckeiks commented Oct 25, 2022 •

edited

Loading

kckeiks commented Oct 25, 2022 •

edited

Loading

thomaseizinger commented Oct 26, 2022 •

edited

Loading