Adds a first simple DKG implementations #41

nkcr · 2020-04-27T14:04:36Z

Adds a first DKG/Pedersen implementation and improves minogrpc.

Defines a DKG interface in dkg/
Implements Pedersen in dkg/pedersen
Updates minogrpc to only use Enveloppe
Updates and fixes the tree routing protocol
Fixes some race conditions with minogrpc
Updates minogrpc/traffic to display a more accurate representation

More extensive testing will follow.

…kg-pedersen

nkcr · 2020-05-12T09:49:39Z

For the record, here is an update of the networking flow diagram for the TestRPC_MultipleRingMesh_Stream test where messages flow as:

CLIENT -> NODE1 -> NODE2 -> ... NODE9 -> CLIENT

tree height is set to 4.

Gilthoniel · 2020-05-13T06:56:21Z

dkg/mod.go

+)
+
+// Factory defines the DKG Starter's factory
+type Factory interface {


I usually defines a factory if I have to decode a Protobuf message into its implementation. In that case, using a factory is very limiting in the way you instantiate the DKG. For instance, if you want a dummy one that won't need keys, you still have to provide something (sure, that can be nil, but it's for the example). It's ok if each implementation provide its constructor like skipchain or byzcoin is doing.

Gilthoniel · 2020-05-13T06:58:30Z

dkg/mod.go

+}
+
+// Starter defines the primitive to start a DKG protocol
+type Starter interface {


For Blockchain and Ledger, I used this abstraction of an actor so that the main interface is called DKG and you have a Listen function that will create a DKGActor. If that makes sense to you, I would prefer keeping the same logic to be consistent.

I'm always for keeping consistency. And yes, it makes sense!
Like so?

// DKG defines the primitive to start a DKG protocol type DKG interface { Listen(players mino.Players, threshold uint32) (DKG, error) } // Actor defines the primitives to use a DKG protocol type Actor interface { Encrypt(message []byte) (K, C kyber.Point, remainder []byte, err error) Decrypt(K, C kyber.Point) ([]byte, error) Reshare() error }

Gilthoniel · 2020-05-13T06:59:40Z

dkg/pedersen/handler.go

+type Handler struct {
+	mino.UnsupportedHandler
+	sync.RWMutex
+	af        mino.AddressFactory


addressFactory for a struct attribute.

Gilthoniel · 2020-05-13T07:03:26Z

dkg/pedersen/handler.go

+		}
+		h.RUnlock()
+
+		errs := out.Send(decryptReply, from)


err = <-out.Send(...) as you're listening to one error anyway, so either it's nil or something went wrong.

Gilthoniel · 2020-05-13T07:05:47Z

dkg/pedersen/handler.go

+		// received our start signal. In this case we will collect the Deals
+		// while waiting for the start signal.
+		deals := []*Deal{msg}
+		for {


You're essentially doing the same thing so you can probably for loop the whole select and remove this one.

You just need to handle the Decrypt if it hasn't started but you have to do it anyway.

Gilthoniel · 2020-05-13T07:30:44Z

mino/minogrpc/mod.go

@@ -143,3 +147,10 @@ func (m Minogrpc) MakeRPC(name string, h mino.Handler) (mino.RPC, error) {

 	return rpc, nil
 }
+
+// AddNeighbours fills the neighbours map of the server
+func (m Minogrpc) AddNeighbours(minoGrpcs ...*Minogrpc) {


You'll have to change this anyway when going to real world, so maybe it's the right time to make an API to pass the certificate?

Gilthoniel · 2020-05-13T07:33:10Z

mino/minogrpc/overlay.go

-		clientStream.Send(&OverlayMsg{Message: overlayMsg.Message})
+		clientStream.Send(&Envelope{
+			From:         o.addr.String(),
+			PhysicalFrom: o.addr.String(),


I would use something more meaningful like LastHop or Relay. Now, is it really necessary ? You're supposed to know the mesh.

IMO it can't be more explicit than that. LastHop is already speaking at a logical level, where here I want to know who sent me this message, regardless its content.
It is hard to know for a node who sent a message because the listen function receive messages from the orchestrator and the children of a node.

Gilthoniel · 2020-05-13T07:34:35Z

mino/minogrpc/overlay.go

+			Message:      envelope.Message,
+		})
+
+		// TODO: think how we can make all the node set the routing then start


It should be impossible to receive a message before the routing message, because the relay will open the connection and then directly send the routing message before relaying anything.

Gilthoniel · 2020-05-13T07:39:15Z

mino/minogrpc/server.go

@@ -400,6 +401,15 @@ func (srv *Server) Serve() error {
 	return nil
 }

+func (srv *Server) addNeighbour(servers ...*Server) {
+	for _, server := range servers {
+		srv.neighbours[server.addr.String()] = Peer{


As mentioned earlier, at some point you will need to be able to extract the server certificate (to a file for instance), transmit it to a peer and then populate the peer server with it. From what I see, it shouldn't hard to adapt but as soon as it's done, you won't get more surprises.

Gilthoniel · 2020-05-13T07:40:48Z

mino/minogrpc/server.go

-		return xerrors.Errorf("couldn't marshal envelope: %v", err)
+	player, ok := playerItf.(overlayStream)
+	if !ok {
+		fabric.Logger.Fatal().Msg("not ok")


I can't believe I'm the one saying an error message is too short ! But it should slightly more detailed. Why would that happen ?

Ugh, you're so right! I forgot to update this one.... : /

Gilthoniel · 2020-05-13T07:44:54Z

When you implement the tests, if you're using minogrpc, it would be nice to make it real in the sense that you don't have access to other servers. I do that with minoch because that's the purpose but a few integration tests with minogrpc are definitely nice.

nkcr · 2020-05-13T15:14:10Z

it would be nice to make it real in the sense that you don't have access to other servers

Yes that's is already the case in the server_test.go except when I can specify a dynamic number of servers (which happens once).

Adds a first simple DKG implementations

nkcr self-assigned this Apr 27, 2020

nkcr marked this pull request as draft April 27, 2020 14:04

nkcr added 3 commits April 30, 2020 09:23

Fixes missing manager when creating a new namespace

27b816c

Adds a first simple DKG implementations

e29d770

Adds missing error check

ac2a0c9

nkcr force-pushed the dkg-pedersen branch from e57db9a to ac2a0c9 Compare April 30, 2020 07:30

nkcr added 2 commits May 1, 2020 17:31

Updates minogrpc

4fa5491

Updates DKG

d95b303

nkcr mentioned this pull request May 4, 2020

Implements a simple tree networking topology #38

Merged

nkcr added 15 commits May 4, 2020 14:36

Fixes missing manager when creating a new namespace

c71e705

Adds a first simple DKG implementations

315fbc5

Adds missing error check

915b3a4

Updates DKG

c40f4de

Merge branch 'dkg-pedersen' of https://github.com/dedis/fabric into d…

83a8692

…kg-pedersen

Removes prints

53e636f

Uses minogrpc version from topology-passing

35f6a3a

Adds back AddNeighbours()

1598160

Refactors the routing so one can select a node to be the root.

8083e6e

Refactors some part, merge OverlayMsg and Enveloppe

32b53da

Merge branch 'master' into dkg-pedersen

3b2919b

Fixes the merge

a4209f3

Refactors traffic and tree topology

8d78abc

Updates the DKG interface

94e6bd7

Polishes the code before review

0ef849f

nkcr marked this pull request as ready for review May 12, 2020 09:35

nkcr requested a review from Gilthoniel May 12, 2020 09:39

nkcr added mod/mino About the Mino module R4M 🚀 The PR is ready to be reviewed and merged labels May 12, 2020

Fixes wrong spell of "enveloppe"

0e1bf33

Gilthoniel reviewed May 13, 2020

View reviewed changes

Addresses Gaylor's comments

e25b248

nkcr added 2 commits May 13, 2020 17:22

Fixes staticcheck

1746558

Merge branch 'master' into dkg-pedersen

5c12442

nkcr requested a review from Gilthoniel May 14, 2020 07:37

Gilthoniel approved these changes May 14, 2020

View reviewed changes

Gilthoniel merged commit 7ea2c0c into master May 14, 2020

Gilthoniel deleted the dkg-pedersen branch May 14, 2020 13:11

bbjubjub2494 pushed a commit to bbjubjub2494/dela that referenced this pull request Aug 3, 2024

Merge pull request dedis#41 from dedis/dkg-pedersen

9f9c922

Adds a first simple DKG implementations

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a first simple DKG implementations #41

Adds a first simple DKG implementations #41

nkcr commented Apr 27, 2020 •

edited

Loading

nkcr commented May 12, 2020

Gilthoniel May 13, 2020

Gilthoniel May 13, 2020

nkcr May 13, 2020

Gilthoniel May 13, 2020

Gilthoniel May 13, 2020

Gilthoniel May 13, 2020

Gilthoniel May 13, 2020

Gilthoniel May 13, 2020

Gilthoniel May 13, 2020

nkcr May 13, 2020

Gilthoniel May 13, 2020

Gilthoniel May 13, 2020

Gilthoniel May 13, 2020

nkcr May 13, 2020

Gilthoniel commented May 13, 2020

nkcr commented May 13, 2020

Adds a first simple DKG implementations #41

Adds a first simple DKG implementations #41

Conversation

nkcr commented Apr 27, 2020 • edited Loading

nkcr commented May 12, 2020

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

Gilthoniel commented May 13, 2020

nkcr commented May 13, 2020

nkcr commented Apr 27, 2020 •

edited

Loading