RandHound 341 #519

Daeinar · 2016-08-16T13:34:24Z

Addresses #341 and #321.

…_round

ineiti

Very shallow 1st review.

Lots of for-loops where you can use range
You can use log.ErrFatal or require.Nil in tests

ineiti · 2016-10-04T10:26:18Z

crypto/hash.go

@@ -92,10 +92,10 @@ func HashFileSuite(suite abstract.Suite, file string) ([]byte, error) {
 }

 // HashArgs takes all args and returns the hash. Every arg has to implement
-// BinaryMarshaler.
+// BinaryMarshaler or will be added using fmt.Sprint


Sorry, that was my comment - it is incorrect that missing things will be added using fmt.Sprint.

ineiti · 2016-10-04T10:26:59Z

protocols/randhound/proof.go

+	"github.com/dedis/crypto/random"
+)
+
+// Package (XXX: name) provides functionality to show equality of discrete


s/(XXX: name)/randhound/ ?

No, since the entire proof.go file implements PVSS and should be extracted at some point and be put into dedis/crypto, see #624.

ineiti · 2016-10-04T10:28:07Z

protocols/randhound/proof.go

+
+	n := len(g)
+	base := make([]ProofBase, n)
+	for i := 0; i < n; i++ {


for i := range base

ineiti · 2016-10-04T13:11:12Z

protocols/randhound/proof.go

+
+// Setup initializes the proof by randomly selecting a commitment v,
+// determining the challenge c = H(xG,xH,vG,vH), and the response r = v - cx.
+func (p *Proof) Setup(scalar ...abstract.Scalar) ([]abstract.Point, []abstract.Point, []ProofCore, error) {


Would it make sense to put the []abstract.Point together in a structure?

I am not sure. I tried to keep it as simple as possible and I am not sure if you get any benefit of having just []abstract.Point in a structure. You would probably have to put all the return values into a structure. I guess that's what you mean? Iirc I tried that a while ago and it complicated quite a few things in the other code that uses PVSS. I'll think about it again.

You would probably have to put all the return values into a structure.

Yes, that'd be nice if possible. If you remember which part posed problems, don't hesitate to drop it to us to see if something's possible.

Would it make sense to put the []abstract.Point together in a structure?

If this is only about readability of the code, here is yet another suggestion: You could also consider named return values. The signature of the function could then look like this:
func (p *Proof) Setup(scalar ...abstract.Scalar) (xG, xH []abstract.Point, proofCore []ProofCore, err error) {
or (together with my above comment below) even simpler
func (p *Proof) Setup(scalar ...abstract.Scalar) (xG, xH []abstract.Point, err error) {

ineiti · 2016-10-04T13:11:40Z

protocols/randhound/proof.go

+		return nil, nil, errors.New("Received unexpected number of points")
+	}
+
+	var good, bad []int


The good, the bad, and the ugly.

ineiti · 2016-10-04T13:32:13Z

protocols/randhound/randhound.go

+			_ = goodEnc
+			_ = badEnc
+
+			//log.Lvlf1("Enc: %v %v %v %v %v %v", goodEnc, badEnc, len(X), len(encShare), len(decShare), len(decProof))


Still used?

ineiti · 2016-10-04T13:32:34Z

protocols/randhound/randhound.go

+				decShare = append(decShare[:k], decShare[k+1:]...)
+			}
+
+			//log.Lvlf1("Dec: %v %v", goodDec, badDec)


Is this still used?

ineiti · 2016-10-04T13:32:40Z

protocols/randhound/randhound.go

+				return err
+			}
+			rnd = rh.Suite().Point().Add(rnd, ps)
+			//log.Lvlf1("Transcript: %v %v", src, ps)


Is this still used?

ineiti · 2016-10-04T13:33:31Z

protocols/randhound/randhound.go

-		return nil, nil, err
+
+	// Record valid encrypted shares per secret/server
+	for i := 0; i < len(good); i++ {


for i, g := range good
and replace good[i] with g.

ineiti · 2016-10-04T13:33:59Z

protocols/randhound/randhound.go

+			rh.chosenSecret[i] = secret
+		}
+
+		//log.Lvlf1("Grouping: %v", rh.group)


Still used? Possible to use log.Lvlf3?

Daeinar · 2016-10-04T16:02:51Z

@ineiti: Addressed most of your comments. I did not change the format of the return values in Setup(...) though. I guess we can do that when we resolve #624.

nikkolasg · 2016-10-05T13:15:10Z

protocols/randhound/randhound.go

+	for i := range m {
+		j := int(random.Uint64(prng) % uint64(i+1))
+		m[i] = m[j]
+		m[j] = i + 1


I don't know where this comes from but it deviates a bit from the "Fisher Yates " shuffle then derived by Durstenfeld for CS ( see link ).
The part that seems missing is the if i != j { m[i] = m[j] }. Of course I haven't checked out the recursion proof of the wikipedia algo so I would have to spend more time to check if this impl. is correct.
How did you came up with this implementation ?

It's basically an adaptation of Golang's rand.Perm(...) function, see here: https://golang.org/src/math/rand/rand.go?s=5071:5103#L164

Sorry, just saw that The condition that checks if j ≠ i may be omitted in languages that have no problems accessing uninitialized array values, and for which assigning is cheaper than comparing.

Ah, yes that makes sense!

nikkolasg · 2016-10-06T09:11:42Z

protocols/randhound/randhound.go

+	hs := rh.Suite().Hash().Size()
+	rand := make([]byte, hs)
+	random.Stream.XORKeyStream(rand, rand)
+	rh.cliRand = rand


This is already done in dedis/crypto . Even if it's not wrong here, let's try to use the crypto library when it's possible to avoid duplication :)

Ah thanks I wasn't aware of this function in dedis/crypto, I will of course use that instead.

nikkolasg · 2016-10-06T10:44:14Z

protocols/randhound/randhound.go

+				// Check availability of corresponding R2 messages, skip if not there
+				target := r1.EncShare[j].Target
+				if _, ok := t.R2s[target]; !ok {
+					continue OUTER


I don't get it. If you go to OUTER you start again with the same data so it will go again to this line and go again to OUTER ?

This is meant to skip all the code in the inner loop in case the R2 message is not there. Meaning when we reach continue OUTER in the inner loop then we go to the next jin the outer loop without executing anything that's coming after line 372 for the skipped loop iteration. Here's an analogous example:

for i := 0; i < 3; i++ { OUTER: for j := 0; j < 3; j++ { if i == j { continue OUTER } fmt.Printf("(i,j) = (%v,%v)\n", i, j) } }

See also http://relistan.com/continue-statement-with-labels-in-go/ for more information.

@nikkolasg: Okay leaving out the OUTER also works. ;)

nikkolasg · 2016-10-06T11:49:29Z

Ok this is a huge PR and I'm not sure to be familiar enough with all the crypto inside to be a really good reviewer for this one. I'd need to read the paper on JVSS and the one you're writing to get a full grasp. I'm planning on this but can't now so on the surface, to me the code looks good :)

On a more structural level, I see you coded PVSS yourself in the same package. Would it make sense now / later to decouple that from the randhound code and to integrate that in the crypto library ? I think it would be a real good target for the crypto library, the same way we have jvss & cosi integrated in the crypto library. Of course these were introduced after we coded the protocol so it does not have to be now. What do you think ?

Daeinar · 2016-10-06T12:24:33Z

@nikkolasg: Fully agree on the PVSS part. I've already added an issue on that topic a couple of days ago, see #624. I am not sure how urgent that is though and we maybe first want to focus on other things?

…simulation

liamsi · 2016-10-10T13:39:19Z

crypto/hash.go

-// a slice of BinaryMarshalers.
-func convertToBinaryMarshaler(args ...interface{}) ([]encoding.BinaryMarshaler, error) {
+// a slice of BinaryMarshalers
+func ConvertToBinaryMarshaler(args ...interface{}) ([]encoding.BinaryMarshaler, error) {


Will this be used externally (would make sense). If not, I think this should remain un-exported.

We will move all of the stuff from crypto/hash.go to the crypto library at some point. So I guess this can be addressed later.

liamsi · 2016-10-10T13:45:10Z

protocols/randhound/proof.go

+		p.core[i] = ProofCore{c, r, vG, vH}
+	}
+
+	return xG, xH, p.core, nil


I wonder if it wouldn't make sense to not return p.core but make p.core public (or accessible through a "getter" if you prefer that)? The caller has the corresponding Proof struct anyways...

I am somewhat undecided on this one. After Setup() you usually want to do something with the p.core part of the proof right away, like sending it to some other peer for verification. Not sure what the advantage would be to have a "getter" that you need to call separately every time you do a Setup(). Would it be better coding style to have a one?

Would it be better coding style to have a "getter"?

I think this depends on your preferences. As p.core is an array, the returned object (in any case) can be manipulated. So you could as well just make core a public/exported field:
See here: https://play.golang.org/p/pkZHSVzSgk

And the reason I suggested that isn't about the getter, it's just to make the code more readable by reducing the number of returned objects (as the caller could have access to core by naming it Core without returning it and the provided "safety" provided by golang would be the same ). Mainly because the others complained about the number of args returned. But if you don't like it, just keep it like this. It's just a matter of style (and I'm sure many people could have vim vs. emacs like discussion about it ;-)

Okay convinced, I make it public and adapt the rest. :)

liamsi · 2016-10-10T14:02:14Z

protocols/randhound/proof.go

+
+// Verify validates the proof against the given input by checking that
+// vG == rG + c(xG) and vH == rH + c(xH).
+func (p *Proof) Verify(xG []abstract.Point, xH []abstract.Point) ([]int, []int, error) {


This Verify function looks like something an external dev/PhD student would use. It would be great if was clearer for the caller when this Proof is valid or not. From looking into the implementation and the naming of the local vars (good, bad) it looks like the proof is valid iff the second returned array is empty? (OK, looking into the tests clarifies this). But you could either use named vars or if you prefer comments, you could add a short comment, when this Proof is a valid one or not.

Yes good point, I'll fix the comments! Since we are doing threshold crypto here, it is allowed that some proofs fail, which is also the reason for the good and bad return values.

liamsi · 2016-10-10T14:04:41Z

protocols/randhound/proof.go

+}
+
+// Verify checks that log_H(sH) == log_X(sX) using the given proof.
+func (pv *PVSS) Verify(H abstract.Point, X []abstract.Point, sH []abstract.Point, sX []abstract.Point, core []ProofCore) ([]int, []int, error) {


Same comment as https://github.com/dedis/cothority/pull/519/files#r82609495

liamsi · 2016-10-10T14:11:37Z

protocols/randhound/struct.go

+	// Group information
+	server              [][]*sda.TreeNode  // Grouped servers
+	group               [][]int            // Grouped server indices
+	threshold           []int              // Group thresholds


Nitpicking (as this is only unexported): thresholds would make more sense. Otherwise one would expect this to be a single int and not an array.

I am usually sticking to the singular, so that when you iterate over the variable you have threshold[i] instead of thresholds[i] which is more intuitive to me. In the above example the same holds for server[i][j] etc. I should probably check if there are some Golang coding conventions on that.

I think the most idiomatic golang way would be to iterate using range instead. Like here: https://play.golang.org/p/HtZlb8Ctav
But as I said, this was just nitpicking ;-)

Hmm right. I should get used to that. ;)

+1 for range ;)

liamsi · 2016-10-10T14:32:52Z

Besides a few comments this PR looks good to me. Feel free to decide which are relevant to you (and which not).

Daeinar and others added 6 commits August 15, 2016 15:59

first version of NIZK dlog-equality proofs

4d8f413

implemented 517

73f0b31

ZK-proofs

0dfa6cb

Merge remote-tracking branch 'origin/hashargs_517' into randhound_two…

a9b4a7e

…_round

tests for dlog proofs

56682e8

tweak

2bcbfdb

Daeinar self-assigned this Aug 16, 2016

Daeinar added the refactoring label Aug 16, 2016

Daeinar added 12 commits August 16, 2016 15:45

tweaks

1e909d9

renaming stuff

c321542

renaming stuff

e633b17

randhound protocol skeleton, client-side initialisation

d1007f4

started PVSS implementation

1550e89

Restructured the NIZK dlog proof

8771306

PVSS verification

f39433a

refactoring PVSS

7bfd34f

...

115d088

PVSS reveal shares

36d7a52

...

dd54f10

PVSS: first fully working version

6543333

Daeinar changed the title ~~Randhound 341~~ RandHound 341 Aug 19, 2016

Daeinar added 9 commits August 19, 2016 16:14

PVSS: fix bug in decryption proofs

f606d8e

PVSS: ...

847a536

RH: first (somewhat) working version

507446f

RH: renaming stuff

0cd5bb7

RH: ensure that last sharding group has at least two elements

2670f5c

RH: restructuring and cleanup

77af27a

RH: more cleanup

bf19791

RH: ...

d66f75c

RH: ...

95c2f2e

Daeinar added 3 commits October 3, 2016 09:29

fix import cycle

61d81e5

rh: some cleanup

91c9289

rh: remove code to simulate Byzantine servers

d727f6d

ineiti requested changes Oct 4, 2016

View reviewed changes

Daeinar added 2 commits October 4, 2016 16:28

rh: address review comments

7e75f64

rh: resolve conflicts with master

6f7257e

nikkolasg reviewed Oct 5, 2016

View reviewed changes

nikkolasg reviewed Oct 6, 2016

View reviewed changes

Daeinar added 5 commits October 6, 2016 14:38

rh: use randomness generation from dedis/crypto && add BW monitor to …

e0027fb

…simulation

rh: removing OUTER label

fbfbb44

Merge remote-tracking branch 'origin/master' into randhound_341

cad16e8

rh: update install.sh

17e04de

rh: ServerIdentityIdx -> RosterIndex

77a36a3

liamsi reviewed Oct 10, 2016

View reviewed changes

Daeinar added 3 commits October 13, 2016 15:40

rh: address review comments

79a3ed4

resolve conflict

024bd84

rh: cleanup tests

99380da

ineiti approved these changes Oct 13, 2016

View reviewed changes

liamsi merged commit 47375c4 into master Oct 13, 2016

liamsi deleted the randhound_341 branch October 13, 2016 14:52

This was referenced Oct 13, 2016

RandHound Refactoring #341

Closed

RandHound Transcript Generation/Verification #321

Closed

RandHound 341 #519

RandHound 341 #519

Conversation

Daeinar commented Aug 16, 2016 • edited by ineiti

ineiti 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Oct 10, 2016 • edited

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

Daeinar commented Oct 4, 2016 • edited

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

Daeinar Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daeinar Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikkolasg commented Oct 6, 2016 • edited

Daeinar commented Oct 6, 2016

Choose a reason for hiding this comment

Daeinar Oct 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daeinar Oct 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Oct 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Oct 10, 2016 • edited

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

Daeinar Oct 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi commented Oct 10, 2016

Daeinar commented Aug 16, 2016 •

edited by ineiti

liamsi Oct 10, 2016 •

edited

Daeinar commented Oct 4, 2016 •

edited

Daeinar Oct 6, 2016 •

edited

Daeinar Oct 6, 2016 •

edited

nikkolasg commented Oct 6, 2016 •

edited

Daeinar Oct 13, 2016 •

edited

Daeinar Oct 13, 2016 •

edited

liamsi Oct 13, 2016 •

edited

liamsi Oct 10, 2016 •

edited

Daeinar Oct 13, 2016 •

edited