Implemented deriveInterpolatingValue function #7
Conversation
deriveInterpolatingValue implements def derive_interpolating_value(L, x_i) function from [FROST], as defined in section 4.2 Polynomials. deriveInterpolatingValue is a simplified version of the algorithm for computing Lagrange coefficient given we only care about the value f(0) instead of reconstructing the entire polynomial. L is the list of the indices of the members of the particular group. xi is the index of the participant i.
| // for x_j in L: | ||
| // if count(x_j, L) > 1: | ||
| // raise "invalid parameters" | ||
| if found { |
There was a problem hiding this comment.
This doesn't actually implement the specified functionality. We need to expect that the indices L are sorted in ascending order, with no repeated instances, and keep track of the previous x_j'. Comparing x_j to x_j' and requiring that x_j > x_j' gives the specified functionality. This was a genuine oversight in the prototype.
There was a problem hiding this comment.
That's true though looking at Round2 implementation in #8 I do wonder if we need to worry about it in deriveInterpolatingValue at all. validateGroupCommitments makes sure that all commitments are sorted in ascending order and there is no duplicates.
We could improve unit tests for validateGroupCommitments to cover duplicates and make it clear in deriveInterpolatingValue that the validation must happen before this function is called.
This would allow us to eliminate this error handling:
Lines 120 to 125 in 5795702
and validateGroupCommitments would be the only function returning errors in Round2. This wouldn't be 1:1 with the paper but this solution feels quite elegant to me.
What do you think about it?
There was a problem hiding this comment.
Regardless of the decision here, I added a test case to validateGroupCommitments unit tests covering the duplicate element case: 1398d05.
There was a problem hiding this comment.
I think if we note in a comment that validateGroupCommitments is a necessary part for checking the correctness, it should be fine.
| @@ -46,6 +46,9 @@ type Curve interface { | |||
| // Identity returns elliptic curve identity element. | |||
| Identity() *Point | |||
|
|
|||
| // Order returns the order of the elliptic curve. | |||
| Order() *big.Int | |||
There was a problem hiding this comment.
For some potential future implementations, this comment will need to be amended to "returns the order of the group produced by the generator". On some curves the generator produces a prime-order subgroup, whose order is not the same as that of the entire curve.
There was a problem hiding this comment.
That's a really good point. Improved in e1c912c.
On some curves the generator produces a prime-order subgroup, whose order is not the same as that of the entire curve.
Improved the test coverage to include the duplicate commitment case.
| // for x_j in L: | ||
| // if count(x_j, L) > 1: | ||
| // raise "invalid parameters" | ||
| if found { |
There was a problem hiding this comment.
I think if we note in a comment that validateGroupCommitments is a necessary part for checking the correctness, it should be fine.
deriveInterpolatingValue implements
def derive_interpolating_value(L, x_i)function from [FROST], as defined in section 4.2 Polynomials.deriveInterpolatingValue is a simplified version of the algorithm for computing Lagrange coefficient given we only care about the value f(0) instead of reconstructing the entire polynomial.
Lis the list of the indices of the members of the particular group.xiis the index of the participanti.The function is identical as in @eth-r's prototype except that we return errors instead of panicking. Unit tests covering the function and explaining how interpolating value is calculated were added.