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

Switch to Google Proto #1

Merged
merged 7 commits into from
May 11, 2022
Merged

Switch to Google Proto #1

merged 7 commits into from
May 11, 2022

Conversation

neekolas
Copy link
Collaborator

@neekolas neekolas commented May 10, 2022

Summary

I was tasked by @jazzz to help out with some Protobuf encoding issues. This solution is not my favorite, but I'm struggling to come up with a less bad option.

Here's the background:

  • go-waku uses gogoproto instead of the Google Protobuf library to generate code
  • gogoproto is currently unmaintained and doesn't support newer versions of Protobuf, which our protos implicitly require. When trying to compile our protos using gogoproto we get an error. Even if we could rewrite our protos to avoid the error, it still seems unwise to hitch our wagon to a library that is completely dead.
  • Waku also uses a library called protoio to read and write protobufs to length delimited lib2p2 streams. That library is tied to gogoproto types.

This is a fork of protoio that is compatible with vanilla Google Protobuf

Alternatives Considered

  • Use https://github.com/CrowdStrike/csproto and figure out how to make it work with protoio
    • csproto is not a complete replacement for Google Protobuf and we would still have to rely on the Google Protobuf message interface
    • Main advantages are performance, which aren't our primary concern here. Much less supported and less active Github.
    • Still could drop this in later with minimal impact
    • Still requires the same work to implement with protoio
  • Completely rewrite protoio in xmtp-node-go
    • Fair bit of work and would likely just recreate what LibP2P has already done here
  • Patch gogoproto to fix our immediate problem
    • The issues arise from a major breaking version change in the Google library, which would be a significant task to keep up with. The team literally chose to abandon the project rather than try and keep up with Google
  • Use an old version of gogoproto which apparently works.
    • Feels like we are going to wind up needing an important security update or bug fix at some point and will be trapped.

Notes

  • I had to comment out some code that relies on gogoproto's MarshalTo method. Their code already provides a fallback for types that do not have a MarshalTo method but I have no idea what the implications are for the fallback
  • The original test suite works, but with some modifications to be compatible with Google Protobuf library

@@ -148,3 +141,189 @@ func iotest(writer protoio.WriteCloser, reader protoio.ReadCloser) error {
}
return nil
}

// Everything below this line is borrowed from the generated test code in gogoproto
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

gogoproto has some magic options to generate test helpers. Now that we are not using gogoproto I had to recreate their test fixtures, generate the code with Google Protobuf, and then copy and paste over the test helpers below. It's not pretty.

package test;
option go_package = "github.com/libp2p/go-msgio/test";

message NinOptNative {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This message is borrowed from gogoproto's test suite

@neekolas neekolas marked this pull request as ready for review May 10, 2022 21:58
@neekolas neekolas requested a review from a team May 10, 2022 22:00
protoio/uvarint_writer.go Outdated Show resolved Hide resolved
Copy link

@jazzz jazzz left a comment

Choose a reason for hiding this comment

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

👍

@mkobetic
Copy link

mkobetic commented May 11, 2022

I had to comment out some code that relies on gogoproto's MarshalTo method. Their code already provides a fallback for types that do not have a MarshalTo method but I have no idea what the implications are for the fallback

It's not clear to me when is the uvarint reader/writer used, but assuming it's in all general length-prefixed cases, then the blast radius can be quite wide. It seems that MarshalTo might be intended as a more optimized version of proto.Marshal, but looking at the code it still just seems to write into an internal buffer and then copy the result into the writer, so I'm not sure what would be the optimization here. In general case it could be possible that MarshalTo produces different encoding than proto.Marshal, in that case the impact could be quite bad, as things that implement MarshalTo would start encoding differently. Do you intend to submit this upstream? I suspect there might be pushback.

@neekolas
Copy link
Collaborator Author

@mkobetic no intention to push this upstream. Changing the underlying proto library would be a huge breaking change for all users of the library.

@neekolas
Copy link
Collaborator Author

FWIW, their existing test suite all passes with my changes, so I'm fairly confident that the methods are functionally equivalent for encoding/decoding.

@neekolas neekolas merged commit 27ce145 into master May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants