Collective Signing using stream and with threshold support #36
Conversation
blockchain/skipchain/conode.go
Outdated
| @@ -136,6 +135,18 @@ func (cc Conodes) Len() int { | |||
| return len(cc) | |||
| } | |||
|
|
|||
| // GetPublicKey implements crypto.CollectiveAuthority. It returns the public key | |||
| // associated with the address and its index. | |||
There was a problem hiding this comment.
I would add a precision in the comment: its index from what?
cosi/threshold/actor.go
Outdated
| innerCtx, cancel := context.WithCancel(context.Background()) | ||
|
|
||
| defer cancel() | ||
| sender, rcvr := a.rpc.Stream(innerCtx, ca) |
There was a problem hiding this comment.
| innerCtx, cancel := context.WithCancel(context.Background()) | |
| defer cancel() | |
| sender, rcvr := a.rpc.Stream(innerCtx, ca) | |
| innerCtx, cancel := context.WithCancel(context.Background()) | |
| defer cancel() | |
| sender, rcvr := a.rpc.Stream(innerCtx, ca) |
cosi/threshold/actor.go
Outdated
| } | ||
|
|
||
| // Each signature is individually verified so we can assume the aggregated | ||
| // is correct. |
There was a problem hiding this comment.
| // is correct. | |
| // signature is correct. |
cosi/threshold/actor.go
Outdated
| err, ok := <-errs | ||
| if !ok { |
There was a problem hiding this comment.
In this case I find it less misleading if we use more instead of ok, as it better describe the role of this boolean.
| err, ok := <-errs | |
| if !ok { | |
| err, more := <-errs | |
| if !more { |
cosi/threshold/actor.go
Outdated
|
|
||
| func (a thresholdActor) waitResp(errs <-chan error, maxErrs int, cancel func()) { | ||
| errCount := 0 | ||
| for { |
There was a problem hiding this comment.
Maybe a
for err := range errs {
}
is what you want to do?
There was a problem hiding this comment.
+ it should be really clear in the specification (mino/mod.go:Sender.Send) that the returned error chan MUST be closed, as this could just wait indefinitely. Actually we didn't mention it.
There was a problem hiding this comment.
Good point. I've updated the interface to comment that point.
cosi/threshold/signature.go
Outdated
| mask = 0x7 | ||
| ) | ||
|
|
||
| // Signature is a threshold signature which includes a aggregated signature and |
There was a problem hiding this comment.
| // Signature is a threshold signature which includes a aggregated signature and | |
| // Signature is a threshold signature which includes an aggregated signature and |
| ) | ||
|
|
||
| // Signature is a threshold signature which includes a aggregated signature and | ||
| // the mask of signers from the associated collective authority. |
There was a problem hiding this comment.
Not sure if I am right: Is there N masks for N signers?
| // the mask of signers from the associated collective authority. | |
| // the masks of signers from the associated collective authority. |
There was a problem hiding this comment.
No, there is one single mask composed of bits (it's a bitmask basically) but a slice of bytes is used as the length is dynamic depending on the number of participants.
cosi/threshold/signature.go
Outdated
| func (s *Signature) GetIndices() []int { | ||
| indices := []int{} | ||
| for i, word := range s.mask { | ||
| for j := 0; j < 8; j++ { |
There was a problem hiding this comment.
I would comment why 8 or use a constant
| return false | ||
| } | ||
|
|
||
| i := index >> shift |
There was a problem hiding this comment.
I would find it much clearer to do i := index / 8, like this one can easily understand we are converting an absolute bit index to a corresponding byte slice index.
There was a problem hiding this comment.
Here is a proposal that I find a bit clearer (and since you are already using 8 in GetIndices:
i := index / 8 // byte index from the slice
j := index % 8 // bit index from the byte
return s.mak[i]&(1<<j) != 0
There was a problem hiding this comment.
It's actually quite a common pattern to use shift and a mask when dealing with divisions of power of 2 as this is much more efficient (quite a lot!).
| @@ -0,0 +1,408 @@ | |||
| package fake | |||
There was a problem hiding this comment.
I would explain a bit what one can do with this package
|
I'm curious: did you already try to change minoch to minogrpc in the test just to see if it works? |
|
No, I didn't. I left that to you ;) |
Collective Signing using stream and with threshold support
This implements collective signing through the stream feature of Mino to mvoe from a flat approach to a distributed mesh of nodes. It also adds the threshold feature to require less than the number of participants.