Protocally Identical: Protocol identifiers and DKG by Shadowfiend · Pull Request #83 · threshold-network/keep-core

Shadowfiend · 2018-04-25T22:37:42Z

This diff is a bit long, but much of it is small adjustments to the DKG process that are a little repetitive. DKG needs to be refactored, but that can happen in a future PR.

The big addition here is the introduction of net.ProtocolIdentifier, and the mapping between a network-side net.ClientIdentifier (name could use improvement) and the protocol-side net.ProtocolIdentifier (via RegisterIdentifier). DKG demonstrates this distinction: the broadcast channels have an internal way that they refer to other nodes, but in DKG each node is associated with a bls.ID, and this is the identifier that matters to the DKG protocol.

The expectation is that recipients in SendTo can be specified either by net.ClientIdentifier or by a registered net.ProtocolIdentifier. Additionally, the protocol is expected to implement the logic that will allow registering the protocol identifier. In DKG, this is done via the JoinMessage, which carries the bls.ID of the sender. Each recipient then registers that id with their local view of the client identifier.

One additional concept is the introduction of net.Message. These are the base network messages that are passed to the recipient callback, and they carry the net.ClientIdentifier, the net.ProtocolIdentifier if an associated one exists, and the payload, which is the unmarshaled wire message. These will map somewhat more directly to our protobuf envelope type (though still with tweaks).

The local package contains a basic implementation of this that doesn't go over the network, and demonstrates some of the moving parts.

Shadowfiend · 2018-04-26T01:01:00Z

+
+func (s *localIdentifier) ProviderName() string {
+	return "local"
+}


Btw @rargulati I don't think you need conversion code to allow a libp2p identifier to be a net.ClientIdentifier. You just need to implement the (extremely small) interface.

So you're saying implement an Identity ie. peerIdentity as a net.ClientIdentifier with peerIdentity.ProviderName() to return a string(peer.ID) if we're in libp2p?

ProviderName() would return something like "libp2p"; it's more meant to be a provider type than the identifier itself. The identifier should be opaque to the consumer, as it's internal to the transport layer.

Ok, peerIdentity.ProviderName() returns libp2p when we're in the libp2p transport. Got it.

Shadowfiend · 2018-04-26T01:09:37Z

A note: I'll probably iterate in this branch, but let's not look to merge it into who-let-the-dkgs-out; instead, let's look to merge it into master after #76 goes in.

rargulati

Quick review, not done

rargulati · 2018-04-26T18:32:39Z


 // ClientIdentifier represents the identity of a recipient for a message.
-type ClientIdentifier string
+type ClientIdentifier interface {


Why net.ClientIdentifier rather than net.Networkidentifier (given that we have a net.Protocolidentifier below.

I think it should just be net.Identifier. I can make that revision.

Eh… Maybe. That would imply ProtocolIdentifier is-an Identifier, which isn't really true. I don't love the stutter in net.NetworkIdentifier. Perhaps net.ChannelIdentifier or net.TransportIdentifier?

Pushed, changed to net.TransportIdentifier.

rargulati · 2018-04-26T18:37:06Z

+
+func (s *localIdentifier) ProviderName() string {
+	return "local"
+}


So you're saying implement an Identity ie. peerIdentity as a net.ClientIdentifier with peerIdentity.ProviderName() to return a string(peer.ID) if we're in libp2p?

rargulati · 2018-04-26T18:37:31Z

+		runes[i] = letterRunes[rand.Intn(len(letterRunes))]
+	}
+
+	return string(runes)


A ton of questions man haha. Why?

Why not?

Kidding aside, just needed some generated identifier. I guess it could just have been a number, but I have an easier time with letters lol.

rargulati · 2018-04-26T18:38:30Z

+		make(map[string]func() proto.Unmarshaler, 0),
+		sync.Mutex{},
+		make(map[net.ClientIdentifier]net.ProtocolIdentifier),
+		make(map[net.ProtocolIdentifier]net.ClientIdentifier)}


comma on the last item, move the } to the \n.

rargulati · 2018-04-26T18:38:52Z

 		sync.Mutex{},
-		make(map[string]func() proto.Unmarshaler, 0)}
+		make(map[string]func() proto.Unmarshaler, 0),
+		sync.Mutex{},


It really helps to have fieldName: initializedMemory

Yah, I've been bad about that in the local packages. Will fix.

rargulati · 2018-04-26T18:39:37Z

+		sync.Mutex{},
+		make(map[net.ClientIdentifier]net.ProtocolIdentifier),
+		make(map[net.ProtocolIdentifier]net.ClientIdentifier)}
+	channels[name] = append(channels[name], channel)


If we're going to use this, we probably need a mutex per map/array and in some cases a RWMutex

What do you mean by “use” :)

The expected use of this right now is exactly what's in main. I think if we find ourselves using it for internal testing that requires more concurrent access we can upgrade our protections here. Current mutexes are there because things break otherwise.

ProtocolIdentifier is meant to allow us to separate the identifier used internally for a network layer, to identify a given node, from identifiers needed for a given protocol, for example BLS IDs. We start by introducing net.ProtocolIdentifier as a type, introducing a Message interface for messages that carry both network and protocol sender ids, and add a RegisterIdentifier method to the BroadcastChannel interface that allows the protocol layer to map a network identifier to a protocol identifier. Consumers are not yet using this abstraction.

Local channels are reworked a bit so that each consumer will have its own channel instance, exactly as if they were across a network, but the instances with the same channel name will communicate with each other. Each channel instance also has a random identifier. We introduce an internal BasicMessage for easy instantiation of the Message interface without external exposure. Also clarify that the local package is not meant for real use.

We were doing BLS ID values, but that's just doing a whole lot of copyin' for nothin'.

The handler function for broadcast channels now accepts a net.Message. We adjust the DKG to deal with this new reality, and tweak a bunch of the pieces of the algorithm to deal with the fact that the Message payload carries sender identification information, as well as to verify that the sender is known. A few bls.ID values are changed to pointers, as well. More abstraction is possible, but this is the first pass at this conversion, and there's much more to come.

We create a channel per member, which is something the new DKG relies on since local channel identity maps to channel instances.

This is more standard Go style.

Also tweak various places where we're referring to a netID or networkID or networkIdentifier to reflect the new term. This term is both clearer (it's really an identifier for whatever transport we're using for our messages) and gives us some consistency across different places where we're using it in terms of naming.

Shadowfiend requested a review from rargulati April 25, 2018 22:37

Shadowfiend mentioned this pull request Apr 25, 2018

Introduce basic network Identity package #82

Closed

2 tasks

Shadowfiend commented Apr 26, 2018

View reviewed changes

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

Shadowfiend force-pushed the protocally-identical branch from cfa9614 to e4f7cf7 Compare April 26, 2018 01:20

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

Shadowfiend force-pushed the protocally-identical branch from e4f7cf7 to 75167c2 Compare April 26, 2018 15:09

rargulati reviewed Apr 26, 2018

View reviewed changes

Shadowfiend added 7 commits April 26, 2018 16:40

Clarify expected type of recipientIdentifier

d16f467

Accusations message carries slice of BLS ID pointers

aacf6d9

We were doing BLS ID values, but that's just doing a whole lot of copyin' for nothin'.

Invoke new DKG correctly from main

ea38e03

We create a channel per member, which is something the new DKG relies on since local channel identity maps to channel instances.

Clean up some comments

e6153e5

Shadowfiend force-pushed the protocally-identical branch from 75167c2 to e6153e5 Compare April 26, 2018 20:43

Shadowfiend changed the base branch from who-let-the-dkgs-out to master April 26, 2018 20:43

Shadowfiend added 2 commits April 26, 2018 16:59

Named keys for localChannel struct instantiation

fc44532

Put closing braces/parens on their own lines in a few places

d81f387

This is more standard Go style.

Shadowfiend force-pushed the protocally-identical branch from 7f14d2e to d81f387 Compare April 26, 2018 21:01

Shadowfiend and others added 3 commits April 27, 2018 11:20

Add docs for net.Message

75a517f

Merge branch 'master' into protocally-identical

24716b5

rargulati approved these changes May 4, 2018

View reviewed changes

rargulati merged commit 6a25f07 into master May 4, 2018

rargulati deleted the protocally-identical branch May 4, 2018 16:04

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

Uh oh!

Conversation

Shadowfiend commented Apr 25, 2018

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants