Introduce protobuf types under new pkg directory #56

rargulati · 2018-03-22T18:16:48Z

Barbershop: check it, we got a new 'do. 2 because sequels are sometimes
better.

This PR introduces very preliminary work regarding protobuf types
Further work is needed to make this useful.

Protobufs allow us to solidify our types which allows us
to communicate across and within service boundaries.

TODO:

generate proto
add dependencies under dep
add small bit of docs for things that can't be added to dep
address PR comments

Sub-TODO's to satisfy PR comments:

Add newline to message proto
Remove generated code
Improve comments for GossipMessage - channel name
Introduce encrypted envelope type
change dep to be a commit id, not version
wait for comments on bls_id
kill the makefile
Make sure the dockerfile reflects the new requirements
add a note in the README regarding what needs to be installed

rargulati · 2018-03-22T18:18:06Z

pkg/types/gossip/message.proto

+// Envelope contains a marshalled Message as the payload
+// and a signature over the payload as signature.
+message Envelope {
+  bytes payload = 1;


Need to introduce message that will be stuffed into payload.

Barbershop: check it, we got a new 'do. 2 because sequels are sometimes better. This PR introduces very preliminary work regarding protobuf types. Protobufs allow us to solidify our types. This allows us to communicate across and within service boundaries.

We leverage go generate, the protobuf compiler, and shell to take the proto files (in each directory), and then spit out *.pb.go files which is our generated code.

Shadowfiend · 2018-03-23T20:24:28Z

pkg/types/gen.go

@@ -0,0 +1,3 @@
+package types
+
+//go:generate sh -c "protoc --proto_path=$GOPATH/src:. --gogoslick_out=. */*.proto"


gogoslick_out: that can't possibly be real you made that up :p

Identities are how we communicate attestation and ownership across network and service boundaries. Groups at the network level care about PeerIDs (libp2p) and public keys (PKI). Within a group, we utilize BLS IDs, which are containers around peerIDs/public keys (tbd).

Test that gen.go appropriately spits out generated code. Check in our inital stab at spec'ing out the basic relay types (Envelopes, Messages, and Identities).

This was initalized by running `dep init` in the `pkg` directory. Of note, this duplicates our vendored dependencies between `go/` and `pkg/`, but for all intents and purposes, they are separate.

The Makefile contains an install command to install the entire protobuf toolchain for the relay. As an aside, if the user already has the compiler, we expose the `proto-gogo` installation command to just grab those dependencies. Much work is needed here.

rargulati · 2018-03-26T15:02:01Z

Here's a quick attempt at this commit. I'm not tied to the README, Makefile, or generated code. Included them here for completeness, but understand if we decide to go a different direction.

Leaving the protobuf types simple as to add them as we add code.

rargulati · 2018-03-26T15:03:56Z

pkg/types/Makefile

@@ -0,0 +1,12 @@
+.PHONY: install proto-gogo


If we want a central Makefile at root with sub Makefiles, we have two options:

A pattern used by cockroachdb: https://github.com/cockroachdb/cockroach/blob/master/pkg/Makefile

A tool Shantanu and I cooked up: https://github.com/joshi4/gmake

I'm actually a fan of submake files as used in the cockroadb codebase. You only ever need to run make at any directory, and it simplifies the things for new users of the application.

Shadowfiend

Lookin' nice. Left a few comments, and…

Some quick thoughts on makefiles and such:

I'm down with sub-Makefiles where it makes sense.
I'd rather avoid depending on additional external tools (referring to gmake, not the protobuf stuff which I assume we just need). However, we can absolutely have a “handy tools” section at the end of the project README that points to stuff like that if we want.
As usual, let's avoid committing generated code if possible.
I'm leaning towards not having the Makefile initialize things. I'd rather have a separate script that does setup for a developer's machine, and focus the Makefile on builds.

Shadowfiend · 2018-03-26T15:41:29Z

pkg/Gopkg.toml

+
+[[constraint]]
+  name = "github.com/gogo/protobuf"
+  version = "1.0.0"


From a glance at gogo/protobuf, most of its changes are in golang/protobuf. Was there something specific that you wanted from them? I would rather depend on the upstream if possible.

Notably, we'll need to fork this to Keep as well… But we'll hold off for now.

Also, remember to depend on a commit id, not a tag. Tags are mutable.

Can you point where most of gogo/protobuf's features are in golang/protobuf? I know it's definitely the other way around, but not seeing evidence that golang/protobuf has gogo's fast marshalling + unmarshalling (this issue does a great job of detailing why folks use gogo: golang/protobuf#280), other serialization formats (grpc, json if we need to switch / test things out), and more canonical go structures (figures a fork has better go support than the original). That being said, as things change, we should definitely evaluate them. Folks in the industry focused on performance have gravitated around gogo's solution. As we'll be having a lot of chatter on the wire, we'll want the one that's most supported and optimized for our use case.

Will change to commit id - thanks for catching that (why oh why dep do you default to tags)

Here is the tradeoff that you end up making: https://groups.google.com/forum/#!msg/golang-nuts/F5xFHTfwRnY/sPv5nTVXBQAJ (this is the go teams opinion of the tradeoff).

From past use, the un/marshallers in gogo are super helpful. Writing that code can be annoying, and it's one of the few times that I'm a fan of code generation.

Do you think the tradeoffs that we will need to make in relay/Keep land are more around speed/performance or size?

Tbh I'm pretty concerned with the forward-compatibility changes the gogo representative says they haven't dealt with… It sounds like it's a young project used by young projects, and that usually means tradeoffs that haven't been fully understood. I trust the Google folks to understand problems like that a bit better.

My initial understanding was wrong here though: gogo/protobuf#191 was what I based my comment on, and it tracks the opposite of what I thought, namely whether the mainline has been merged into this fork.

I suspect our performance will be absolutely dominated by almost everything except serialization/deserialization, since our data structures right now aren't terribly complicated or large, and though we'll have chatter it won't be super-frequent. Given the emphasis on compatibility, I'm tempted to say we stick with golang's version and switch to gogo if and when we find ourselves with problems it tackles in a way we think is good.

Lastly, re: marshaller/unmarshaller, presumably the golang version has that as well, it's just not repeated for each type? So it's not like we have no insight into how it's doing things, right?

I see where you're coming from, but I need to respectfully disagree on a few points here:

Given the number of users (including dropbox, docker, and redhat), and even our friends in decentra-land are in the game! https://github.com/ipfs/go-ipfs/search?utf8=%E2%9C%93&q=gogo&type=

I'd say there are good reasons for using it! That being said, you bring up a great point about us being dominated by other issues before serialization becomes a concern. That being said, if we're shipping BLS ids and group signature shares around, I think we'll be contending with these issues as well...

Re confusion over gogo becoming compatible with golang/protobuf: they're working on merging mainline back in - I think that's a good thing in terms of giving us options.

More importantly it looks like libp2p uses it, so I think we'll be pulling the dependency in transitively anyway, and therefore I guess we'll need to pin it regardless. I don't love it, and I continue to be more convinced by upstream's arguments more than downstream's, but so it goes. Let's do whatever minimizes the cross-section of packages we need to pin, in this case that looks like it's sticking to gogo.

Ok, for personal curiosity, off-thread I'd love to hear your explanation of upstream's arguments vs downstream's args. I might be missing something here.

Shadowfiend · 2018-03-26T15:42:27Z

pkg/types/Makefile

@@ -0,0 +1,12 @@
+.PHONY: install proto-gogo
+
+install: proto-darwin proto-gogo


While I'm noting above that I'm not a huge fan of having setup in the makefile, it's also worth calling out that make install generally means “install this package”, rather than “install dependencies”, so even if we did keep this we would want to avoid install as the target.

Shadowfiend · 2018-03-26T15:44:19Z

pkg/types/gossip/message.proto

+
+  // The PublicKey and BLS ID of the sender and receiver
+  Identity sender = 2;
+  Identity receiver = 3;


Not sure if this is implied in the envelope, but receiver is optional (e.g. for broadcast messages).

It may make sense to have two different Envelopes, one for broadcast and one for point-to-point, since point-to-point messages will also be encrypted. The broadcast-related interfaces we have should be fairly easy to adapt to different envelopes for those message types, I believe.

Yep, Receivers are optional when you leave that type out (in go-land, after it successfully unmarshalls the type, the pointer of Identity will be nil / the value will lack the necessary values).

Sure, I'll add a SecretEnvelope or EncryptedEnvelope type as well (I figure this is better than having a field on the Envelope type).

Shadowfiend · 2018-03-26T15:46:22Z

pkg/types/gossip/message.proto

+
+message Identity {
+  bytes public_key = 1;
+  bytes bls_id = 2;


I think we're still aiming at deriving the public key from the BLS id or vice versa, so we may just want to include one of these? @mhluongo did you have any specific thoughts on relationship between BLS id, public key for participation in libp2p, and staking key?

I guess one question is whether the BLS id will be stable across groups... It could be, but it doesn't have to be, and if we're looking at anonymity and such for the future, it might be ideal to design with an independent BLS id. On the flip side… BLS ids are pretty big, so passing them around with all messages could be inefficient.

Punted the BLS ID discussion to #61.

Shadowfiend · 2018-03-26T15:57:58Z

pkg/types/gossip/message.proto

+  bytes channel_MAC = 2;
+
+  // TODO: define various messages this GossipMessage can be. Use oneof.
+}


We should probably end our files with newlines. Editor should have a setting (I always turn it on), but I think git can also do this with the core.autocrlf config flag? (Notably, go fmt respects whatever you want to do, so we can't rely on it for this particular thing.)

This setting never seems to make it across to my new computers, though may be specific to proto files :(. Is this best as a git config or something in the editor settings (ie. in emacs)

Either way. GitHub highlights it in the diffs, so that's helpful. It's definitely an ongoing annoyance that this isn't the default everywhere 🙄

Shadowfiend · 2018-03-26T15:59:11Z

pkg/types/gossip/message.proto

+
+message GossipMessage {
+  // Channel is isomorphic to the group name as well as the pubsub channel
+  // Channel names are HASH(PubKey1 || ... || PubKeyN) of all valid group members.


PubKey should probably say StakingPubKey, and HASH should probably be clear that it's keccak, since we'll be doing this on-chain.

rargulati · 2018-03-26T17:30:03Z

TODOs:

Add newline to message proto
Remove generated code
Improve comments for GossipMessage - channel name
Introduce encrypted envelope type
change dep to be a commit id, not version
wait for comments on bls_id
kill the makefile
Make sure the dockerfile reflects the new requirements
add a note in the README regarding what needs to be installed

rargulati · 2018-03-26T17:30:55Z

@Shadowfiend Should we keep the Makefile or axe it? If we keep it, you're saying change install to get-protobuf-deps or something?

Shadowfiend · 2018-03-26T17:52:58Z

Leeet's axe it for now. Do make sure the dockerfile reflects the new requirements and add a note in the README regarding what needs to be installed.

Side note: do we need to go get the generator tool if we depend on it?

Shadowfiend · 2018-03-26T17:54:00Z

Re: comments on BLS id btw, I think let's set a ceiling on those and if we don't resolve it in the next couple of days let's not block the PR on it.

rargulati · 2018-03-26T19:36:10Z

Nope, go generate's built in :)

Re: makefile, sounds good to me, added it to my todo list.

Re: BLS id, sounds good.

Shadowfiend · 2018-03-26T19:37:49Z

Sorry, no, I know go generate is built in; I see the install steps install the C++ protobuf tool, and then we also pull in the gogo generator tool. I'm asking can we pull in the generator tool via dep instead of requiring separate installation via go get.

rargulati · 2018-03-26T19:39:50Z

Ohhhh...yeah I meant to ask you about that. I'm not seeing how that works. Alternatively, I had the idea that we can write a hacky cli tool that calls in those dependencies (which then allows us to have gogo in our deps). But I'm not sure if we should do that right this minute (though some projects end up doing this...)

Shadowfiend · 2018-03-26T19:44:34Z

Doesn't work because we're in vendor/, and GOBIN doesn't include the vendored binaries unless you do that separately. Makes sense. Let's stick with this for now, but let's make sure the Dockerfile does that go get alongside the dependency step.

Previously pointed to a tag. This is not great as tags are mutable.

As our policy is to not check in generated code, we no longer need the dependency.

An EncryptedEnvelope is a special envelope needed for certain phases of DKG. Update document for the channel field in GossipMessage; note that we use keccak as the has function, and that pub keys are specifically StakingPubKeys.

rargulati · 2018-03-26T22:23:05Z

@Shadowfiend please take a look at recent commits.

Hilariously realized that if we aren't auto-generating code, we don't need the dependencies / can't ensure it without having dep in a strange state.

Is it fine to punt the Dockerfile update for a subsequent PR?

Shadowfiend · 2018-03-26T22:29:06Z

Let's have the dockerfile update in this PR. CI runs dockerfile, so including code that is invisible without build changes without updating the dockerfile means CI doesn't see it. Seems annoying.

Shouldn't be hard though, just adding the go get in the dep ensure RUN statement and go generate as its own step. I would say optimizations can wait if you want (e.g., only copying files that need generation into go generate so that the build step won't rerun unless those files have changed, etc).

Shadowfiend · 2018-03-26T22:31:00Z

pkg/types/gossip/message.proto

+  // Signature of the message
+  bytes signature = 4;
+
+  EncryptedEnvelope encrypted_envelope = 5;


If the encrypted envelope is at the end, should the payload also be at the end? Seems like it will make it easier to see that these are related.

Reiterating this because I think it got collapsed incorrectly:

If the encrypted envelope is at the end, should the payload also be at the end? Seems like it will make it easier to see that these are related.

Move closer to EncryptedMessage to show that there is a relation between the fields.

We introduce the protobuf compiler, gogo/proto toolchain, and introduce the pkg/ directory. This will be the directory from which future changes are made.

Shadowfiend

Left a couple of tiny notes but I'm not blocking the merge on it. I think we're ready to move on this guy, and we can address the remaining stuff in the next PR.

Shadowfiend · 2018-03-27T14:24:41Z

Dockerfile

+RUN dep init
+RUN dep ensure --vendor-only
+
+WORKDIR $APP_DIR/go/


We'll want to rework this a bit, but we need to wait until after the go/ stuff has been moved to the top level. I'll do that in a PR today so you can focus on the other stuff, and we won't block this merge on it.

Shadowfiend · 2018-03-27T14:25:02Z

pkg/types/gossip/message.proto

@@ -5,25 +5,26 @@ package gossip;

 // Envelope contains a marshalled Message as the payload
 // and a signature over the payload as signature.
-// It also contains the Identities of the sender and receiver.
+// It also contains the (optional) Identities of the sender and receiver.


Both aren't really optional, only the receiver, I think. Is there a way to express this directly in protobuf or nah?

This was an important detail I misunderstood. Addressed in e81b9dd. I think we'll have to address this in the application layer (strict requirement for sender, optional requirement for receiver). I think as of proto2/3, all fields are optional. I'll need to double check that as we dive deeper into proto land.

Shadowfiend · 2018-03-27T14:25:30Z

pkg/types/gossip/message.proto

+  // Signature of the message
+  bytes signature = 4;
+
+  EncryptedEnvelope encrypted_envelope = 5;


Reiterating this because I think it got collapsed incorrectly:

If the encrypted envelope is at the end, should the payload also be at the end? Seems like it will make it easier to see that these are related.

Shadowfiend

Reiterating at the top level: punted the BLS ID discussion to #61.

Changes look good, let's let 'er merge!

New Merkle Distribution for Jan 1st

rargulati commented Mar 22, 2018

View reviewed changes

rargulati force-pushed the barbershop-2 branch 2 times, most recently from 14dc55a to 566bb48 Compare March 22, 2018 18:27

rargulati changed the title ~~Introduce protobuf types and new repo layout.~~ Introduce protobuf types under new pkg directory. Mar 22, 2018

Raghav Gulati added 2 commits March 22, 2018 14:33

Introduce a mechanism to generate protobuf

1bcb6f7

We leverage go generate, the protobuf compiler, and shell to take the proto files (in each directory), and then spit out *.pb.go files which is our generated code.

rargulati force-pushed the barbershop-2 branch from 566bb48 to 1bcb6f7 Compare March 23, 2018 16:07

rargulati changed the title ~~Introduce protobuf types under new pkg directory.~~ Introduce protobuf types under new pkg directory Mar 23, 2018

Shadowfiend reviewed Mar 23, 2018

View reviewed changes

Raghav Gulati added 5 commits March 26, 2018 09:39

Generate protobuf code with "go generate".

30b7ae1

Test that gen.go appropriately spits out generated code. Check in our inital stab at spec'ing out the basic relay types (Envelopes, Messages, and Identities).

Add protobuf dependency with dep.

62b8038

This was initalized by running `dep init` in the `pkg` directory. Of note, this duplicates our vendored dependencies between `go/` and `pkg/`, but for all intents and purposes, they are separate.

Remove weird spacing in Makefile

57df3f2

rargulati commented Mar 26, 2018

View reviewed changes

Shadowfiend requested changes Mar 26, 2018

View reviewed changes

Raghav Gulati added 5 commits March 26, 2018 16:34

Point our protobuf dependency to a commit.

ca1913c

Previously pointed to a tag. This is not great as tags are mutable.

Remove generated code and dependency.

c7228d1

As our policy is to not check in generated code, we no longer need the dependency.

Remove Makefile, note dependencies in Readme.

bd88baf

Add EncryptedEnvelope, update GossipMsg doc.

8cc74d6

An EncryptedEnvelope is a special envelope needed for certain phases of DKG. Update document for the channel field in GossipMessage; note that we use keccak as the has function, and that pub keys are specifically StakingPubKeys.

Remove unnecessary readme.

19d4a44

Shadowfiend reviewed Mar 26, 2018

View reviewed changes

Raghav Gulati added 2 commits March 26, 2018 18:33

Move payload and signature fiedls in GossipMessage

c8b128d

Move closer to EncryptedMessage to show that there is a relation between the fields.

Update Dockerfile with proto dependencies

94adfc7

We introduce the protobuf compiler, gogo/proto toolchain, and introduce the pkg/ directory. This will be the directory from which future changes are made.

Shadowfiend previously approved these changes Mar 27, 2018

View reviewed changes

Note that only the receiver is optional, not sender

e81b9dd

rargulati dismissed Shadowfiend’s stale review via e81b9dd March 27, 2018 14:29

Shadowfiend mentioned this pull request Mar 27, 2018

Relationship between keys and bls.ID #61

Open

Shadowfiend approved these changes Mar 27, 2018

View reviewed changes

Shadowfiend merged commit f22c305 into master Mar 27, 2018

Shadowfiend deleted the barbershop-2 branch March 27, 2018 14:39

dimpar pushed a commit that referenced this pull request Feb 10, 2023

Merge pull request #56 from manumonti/merkle-dist

54639b0

New Merkle Distribution for Jan 1st

		@@ -0,0 +1,3 @@
		package types

		//go:generate sh -c "protoc --proto_path=$GOPATH/src:. --gogoslick_out=. /.proto"

		@@ -0,0 +1,12 @@
		.PHONY: install proto-gogo

		install: proto-darwin proto-gogo

Introduce protobuf types under new pkg directory #56

Introduce protobuf types under new pkg directory #56

Conversation

rargulati commented Mar 22, 2018 • edited Loading

rargulati Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rargulati commented Mar 26, 2018

Choose a reason for hiding this comment

Shadowfiend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rargulati Mar 26, 2018 • edited Loading

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

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

rargulati commented Mar 26, 2018 • edited Loading

rargulati commented Mar 26, 2018

Shadowfiend commented Mar 26, 2018

Shadowfiend commented Mar 26, 2018

rargulati commented Mar 26, 2018

Shadowfiend commented Mar 26, 2018

rargulati commented Mar 26, 2018 • edited Loading

Shadowfiend commented Mar 26, 2018

rargulati commented Mar 26, 2018

Shadowfiend commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend 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

Shadowfiend left a comment

Choose a reason for hiding this comment

rargulati commented Mar 22, 2018 •

edited

Loading

rargulati Mar 22, 2018 •

edited

Loading

rargulati Mar 26, 2018 •

edited

Loading

rargulati commented Mar 26, 2018 •

edited

Loading

rargulati commented Mar 26, 2018 •

edited

Loading