Conversation
AnomalRoil
left a comment
AnomalRoil
left a comment
There was a problem hiding this comment.
This should not work since you're always hashing the empty string as far as I understand the code atm. Please make sure to add some "negative tests" to catch that failure
|
|
||
| on: | ||
| push: | ||
| branches: [ commoncoin ] # TODO should be done differently |
There was a problem hiding this comment.
yes this should be done properly, not just on your branch.
commoncoin/commoncoin.go
Outdated
| @@ -0,0 +1,195 @@ | |||
| package commoncoin | |||
|
|
|||
| // The value of a coin C is obtained by first | |||
There was a problem hiding this comment.
Always refer in the docs to the paper, including page and section/chapter that you're implementing when it's based on a paper.
commoncoin/commoncoin.go
Outdated
| proof, xG, xH, err := dleq.NewDLEQProof(c.s, c.gVK, c.gTilde, c.si.V) | ||
| gTildeShare := c.s.Point().Mul(c.si.V, c.gTilde) | ||
| if !xH.Equal(gTildeShare) { | ||
| panic("NOOOO") | ||
| } | ||
| if !xG.Equal(c.s.Point().Mul(c.si.V, c.gVK)) { |
There was a problem hiding this comment.
When implementing cryptographic papers, or papers/algorithm in general, it's a good practice to refer to the spec / documentation / paper using comments, e.g.:
| proof, xG, xH, err := dleq.NewDLEQProof(c.s, c.gVK, c.gTilde, c.si.V) | |
| gTildeShare := c.s.Point().Mul(c.si.V, c.gTilde) | |
| if !xH.Equal(gTildeShare) { | |
| panic("NOOOO") | |
| } | |
| if !xG.Equal(c.s.Point().Mul(c.si.V, c.gVK)) { | |
| proof, xG, xH, err := dleq.NewDLEQProof(c.s, c.gVK, c.gTilde, c.si.V) | |
| // ~g_i = x_i*~g | |
| gTildeShare := c.s.Point().Mul(c.si.V, c.gTilde) | |
| // sanity check that xH == ~g_i | |
| if !xH.Equal(gTildeShare) { | |
| panic("NOOOO") | |
| } | |
| // sanity check that xG == x_i*VK_i | |
| if !xG.Equal(c.s.Point().Mul(c.si.V, c.gVK)) { |
Because then we can see that these last two checks are actually redundant: the goal of NewDLEQProof is to compute these values, so while I see why you might have wanted to check that it was correct, I think these two checks are not useful.
Instead add maybe a comment above NewDLEQProof saying what xG and xH are, because they aren't called xG and xH in the paper, right? Maybe even rename xH into gTildeShare since that's what you want.
commoncoin/commoncoin.go
Outdated
| tossResult := InvalidTossResult | ||
| for _, coinShare := range c.shares { | ||
| if c.VerifyShare(coinShare) != nil { | ||
| // log debug |
There was a problem hiding this comment.
Yeah, you probably want to log that, maybe at the Warn level even, not even Debug level.
| n int // number of parties | ||
| } | ||
|
|
||
| func NewECDHCommonCoin( |
There was a problem hiding this comment.
This function definitively needs some docs about what it does and what it expects as parameters since the name of the parameters aren't self-explanatory at all
| hash := c.s.Hash() | ||
| if _, err := pubPoly.Commit().MarshalTo(c.s.Hash()); err != nil { | ||
| return tossResult, errors.New("err marshalling gTilde^secret to the hash function") | ||
| } | ||
|
|
||
| hashGTildeZero := hash.Sum(nil) |
There was a problem hiding this comment.
this seems very wrong. The tests should catch that it's not working.
You're Marshalling to a new Hash instance, not the one you're computing the Sum of next. So you're always getting the same value for hashGTildeZero.
Please add a test that catches the problem and fix it.
commoncoin/commoncoin.go
Outdated
| // could copy lagrangeBasis(...) from share instead | ||
| pubPoly, err := share.RecoverPubPoly(c.s, validPubShares, c.k, c.n) | ||
| if err != nil { | ||
| return CoinOne, err |
There was a problem hiding this comment.
shouldn't it be rather
| return CoinOne, err | |
| return tossResult, err |
if not, it deserves a comment.
… to add some comments referring to the paper for clarification
AnomalRoil
left a comment
AnomalRoil
left a comment
There was a problem hiding this comment.
LGTM just need to make sure your tests and linter are passing now.
| on: | ||
| push: | ||
| branches: [ commoncoin ] # TODO should be done differently | ||
| branches: [ main ] |
| validPubShares = append(validPubShares, &coinShare.GTildeShare) | ||
| } | ||
| if len(validPubShares) < c.k { | ||
| return tossResult, fmt.Errorf("not enough calid coin shares to reveil common coin value") |
There was a problem hiding this comment.
| return tossResult, fmt.Errorf("not enough calid coin shares to reveil common coin value") | |
| return tossResult, fmt.Errorf("not enough valid coin shares to reveal common coin value") |
| type CommonCoin interface { | ||
| LocalShare() (CoinShare, error) | ||
| Toss() (CoinToss, error) // combines shares internally | ||
| VerifyShare() error | ||
| AddShare(CoinShare) error | ||
| NumShares() int | ||
| ResetCoinID(coinID string) error // to start another round | ||
| } |
There was a problem hiding this comment.
You can enforce at compile time that the ECDHCommonCoin struct satisfies the CommonCoin interface by adding an empty global variable like that:
var _ CommonCoin = &ECDHCommonCoin{}
2 participants
Common coin package with ECDH common coin implementation.
My main question is if the overall logic and use of kyber libraries (dleq, share, bn254) is correct.
And a follow-up one: do
RecoverPubPoly and Commit(lines 142-150) do the same as the combination of shares as in the combination of shares? If not then I misunderstood something profoundly.