Who let the DKGs out: Move DKG to new net interfaces by Shadowfiend · Pull Request #76 · threshold-network/keep-core

Shadowfiend · 2018-04-21T00:16:26Z

Depends on #75, implements the DKG code on top of the new net
interfaces. Some tweaks throughout to make everything fit together
right.

rargulati · 2018-04-21T00:41:14Z

+
+func (channel *localChannel) RegisterUnmarshaler(
+	tpe string,
+	unmarshaler func() proto.Unmarshaler) (err error) {


This style of breaks between params is tough to read for me (personally). Not something that I've seen before. Maybe if the last param was on a new line?

rargulati · 2018-04-21T00:47:38Z

+		channel.unmarshalersByType[tpe] = unmarshaler
+	}
+	channel.unmarshalersMutex.Unlock()
+	return


Consider rewriting this as:

+ channel.unmarshalersMutex.Lock() + if _, exists := channel.unmarshalersByType[tpe]; !exists { + channel.unmarshalersByType[tpe] = unmarshaler return nil + } + channel.unmarshalersMutex.Unlock() + return fmt.Errorf("type %s already has an associated unmarshaler", tpe)

or change it to error first and then return nil

The above is an example. Of note, it's preferable to return something concrete (ie. nil) rather than a naked return (can be seen as a code smell under some linters).

rargulati · 2018-04-21T00:51:22Z

-// Message corresponds with our proto Envelope type.
-type Message struct{}
+// ClientIdentifier represents the identity of a recipient for a message.
+type ClientIdentifier string


Given what we talked about today is this the right abstraction level / type?

Shadowfiend · 2018-04-23T14:16:53Z

Going to ignore comments on this PR as I believe they're all supposed to be on the #75 dependency.

Shadowfiend · 2018-04-26T01:11:21Z

This guy is good for review now that #75 is in.

Shadowfiend · 2018-04-26T14:40:10Z

Just realized there are actually no chain-related changes here <_<

This will let us use them directly for sending through the net.BroadcastChannel interface.

The new DKG code has been adapted to work with net.BroadcastChannel and the new protobuf-based messages in the dkg package. We also adapt main to invoke the new DKG code instead of the old one.

Since the old DKG code is gone, we no longer need this; local.Channel should be used instead.

Even though it isn't live, we bring it back since we've resolved circular package dependency issues.

This fixes our docker build, which was failing because the Go compile wasn't finding the Go files we failed to generate for the DKG messages.

rargulati

Good stuff!

rargulati · 2018-04-26T15:47:03Z

 	localMember := thresholdgroup.NewMember(memberID, threshold)

-	recvChan := channel.RecvChan()
+	recvChan := make(chan interface{})


This is in the critical path: will we rework this or is it worth putting a note to buffering this channel?

We will be reworking this yeah. Huge refactoring needed here... later. :D

rargulati · 2018-04-26T15:53:41Z

-		channel.Send(broadcast.NewPrivateMessage(member.BlsID, receiverID, MemberShareMessage{share}))
+		channel.SendTo(
+			net.ClientIdentifier(receiverID.GetHexString()),
+			&MemberShareMessage{&member.BlsID, &receiverID, &share})


Go allows for trailing commas as the last items delimiter. Could you add one after &share}, and then move the ) to the next line. Do that wherever possible. Thanks.

I’m aware, and do use that in a few places, particularly with multi line function declarations where avoiding it makes things harder to read. Here it seems to add little in legibility and additional vertical space, so I’ve avoided it.

Is your concern consistency with the rest of the code, consistency with common Go style, or something else entirely?

Both of the first two + the way it looks.

rargulati · 2018-04-26T15:56:53Z

+				justifyingMember.RecordJustificationFromID(
+					*justificationsMsg.id,
+					accuserID,
+					justification)


last item with a comma to pop the ) to the \n

rargulati · 2018-04-26T15:59:45Z

+// key generation interactions.
+func Init(channel net.BroadcastChannel) {
+	channel.RegisterUnmarshaler(
+		"dkg/join",


Wouldn't this be a great opportunity to use Type()? Though I like the way this code flows and see why you made the decision you did. Would be nice to have to only remember to update the Type() in a protocol message rather than in the init as well!

Excellent point!

The main goal of this is to make it so BroadcastChannel's RegisterIdentifier function only needs to take one parameter, and can derive the type from that parameter.

rargulati

LGTM

Shadowfiend requested a review from rargulati April 21, 2018 00:16

rargulati reviewed Apr 21, 2018

View reviewed changes

This was referenced Apr 23, 2018

Proto NIC: Initial tweaks to net interfaces #75

Merged

Protocally Identical: Protocol identifiers and DKG #83

Merged

Shadowfiend force-pushed the who-let-the-dkgs-out branch from 69ef6c2 to 66a450c Compare April 26, 2018 01:11

Shadowfiend changed the title ~~Who let the DKGs out: Move DKG to new net/chain interfaces~~ Who let the DKGs out: Move DKG to new net interfaces Apr 26, 2018

Shadowfiend added 5 commits April 26, 2018 11:06

Give dkg messages Type() to conform to TaggedMarshaler

d3b8630

This will let us use them directly for sending through the net.BroadcastChannel interface.

Move pkg/beacon/relay DKG into pkg/beacon/relay/dkg

cb083b2

The new DKG code has been adapted to work with net.BroadcastChannel and the new protobuf-based messages in the dkg package. We also adapt main to invoke the new DKG code instead of the old one.

Rip out broadcast.LocalChannel

2d614c0

Since the old DKG code is gone, we no longer need this; local.Channel should be used instead.

Restore old beacon startup code

12678aa

Even though it isn't live, we bring it back since we've resolved circular package dependency issues.

Copy DKG protobuf files into docker build

09e5928

This fixes our docker build, which was failing because the Go compile wasn't finding the Go files we failed to generate for the DKG messages.

Shadowfiend force-pushed the who-let-the-dkgs-out branch from 66a450c to 09e5928 Compare April 26, 2018 15:07

rargulati reviewed Apr 26, 2018

View reviewed changes

Introduce net.TaggedUnmarshaler, dual to net.TaggedMarshaler

e2551de

The main goal of this is to make it so BroadcastChannel's RegisterIdentifier function only needs to take one parameter, and can derive the type from that parameter.

rargulati approved these changes Apr 26, 2018

View reviewed changes

rargulati merged commit 35072d7 into master Apr 26, 2018

Shadowfiend deleted the who-let-the-dkgs-out branch May 17, 2018 14:57

Shadowfiend added this to the Relay Milestone 1 milestone May 17, 2018

Uh oh!

Conversation

Shadowfiend commented Apr 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shadowfiend commented Apr 23, 2018

Uh oh!

Shadowfiend commented Apr 26, 2018

Uh oh!

Shadowfiend commented Apr 26, 2018

Uh oh!

rargulati left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rargulati left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Shadowfiend commented Apr 21, 2018 •

edited

Loading