Aggregate signature shares #9
Conversation
This function will be needed to aggregate signature shares after the round two of FROST is completed.
Participant struct is a base [FROST] participant, no matter the type: signer, coordinator, or both. The logic is used from the Signer and soon will be used from the Coordinator as well (see next commits).
The commitment validation logic that has to be performed by both signer and coordinator has been extracted to validateGroupCommitmentsBase. The signer performs one more validation: if the current signer's commitment is included on the list. This validation does not have to be performed by the coordinator (logic to be implemented) when validating the signature shares because the coordinator will iterate over the received commitments.
Aggregate implements Signature Share Aggregation from [FROST], section 5.3. Signature Share Aggregation.
The tests covering the logic attached to the Participant struct have been moved to participant_test.go and operate on the Participant structure. The commitment validation logic in the validateGroupCommitmentsBase has been covered with unit tests and tests for validateGroupCommitments have been simplified to not repeat already performed tests.
|
With #8 merged, this is ready for review now. Undrafting. |
| @@ -52,6 +52,12 @@ func (bc *Bip340Curve) EcAdd(a *Point, b *Point) *Point { | |||
| return &Point{x, y} | |||
| } | |||
|
|
|||
| // EcSub returns the subtraction of two elliptic curve points. | |||
| func (bc *Bip340Curve) EcSub(a *Point, b *Point) *Point { | |||
| bNeg := &Point{b.X, new(big.Int).Neg(b.Y)} | |||
There was a problem hiding this comment.
I think we may want to add a .Mod here for completeness' sake even though the prototype worked fine without it.
There was a problem hiding this comment.
EcSub calls EcAdd that calls BitCurve.Add which executes modulo over the order of the underlying field (BitCurve.P) on the result. Unless we are talking about doing it for x and y before the EcAdd is called? If so, I think we should consider doing it in DeserializePoint function before checking if the point is on the curve.
There was a problem hiding this comment.
It's not a big deal precisely because of that, but given that EcSub isn't called that often and doing the negation modularly isn't very expensive, I think it might be somewhat preferable to try to stay within the invariants just for clarity's sake so that it doesn't look like a potential bug (like bNeg := &Point{b.X, new(big.Int).Sub(bc.FieldOrder(), b.Y)}), or add a comment explaining why this shortcut works fine.
| // challenge_input = group_comm_enc || group_public_key_enc || msg | ||
| // challenge = H2(challenge_input) | ||
| // return challenge | ||
| return p.ciphersuite.H2(groupCommitmentEncoded, publicKeyEncoded, message) |
There was a problem hiding this comment.
We need to consider the difference between the internal encoding and the external encoding here, and this will need to use the external encoding instead of SerializePoint. For example, in BIP-340 the external encoding is not lossless.
I see two options for this: we can either add something like EncodePoint and DecodePoint to the Curve interface, or we can pass *Point arguments to H2. The latter is a bit more robust in case someone comes up with an esoteric protocol that wants something weird done with the points, but the former may be a bit more convenient for encoding the signatures into []byte from the (*Point, *big.Int) they are produced as, because it's likely that the same functions would be used in both the signature encoding and the challenge computation argument encoding.
There was a problem hiding this comment.
As far as I can tell, this is only a problem for H2 which is used to compute the challenge, correct?
In that case, I'd probably go with the more robust version, which is passing *Point arguments to H2. If H2 is the only function that requires special attention in this matter, I think it is fine if we make it clear in the function signature. Moreover, we already have SerializePoint and DeserializePoint in the public API of the curve and I think having serialize and encode next to each other could be confusing.
Alternatively, and I think I like this version most, we could expose EncodePoint / DecodePoint on the Hashing or Ciphersuite interfaces.
There was a problem hiding this comment.
I think you're right with that; I'm not aware of anywhere apart from H2 which needs to use the external encoding.
I like the idea of exposing the other function on a different interface to make it clear that it's related to how the signature scheme expects things to be formatted and separate the encodings conceptually.
Making H2 take *Point arguments and exposing the external encoding on the Ciphersuite interface would be the most robust just in case someone has a different encoding for challenge computation vs. signatures (code-wise it would be more elegant as part of Hashing but conceptually fits better in Ciphersuite in this case). However, that feels like potential YAGNI.
There was a problem hiding this comment.
Please see the proposition from f6af9a5.
There was a problem hiding this comment.
Thinking more about it, I would say EncodePoint function fits better Hashing interface but it is a cosmetic change I would do in another PR to do not complicate merges (#10 is branched off already).
The Signature type makes the API cleaner and we will be able to use it for the signature verification function (to be implemented).
EncodePoint encodes the given elliptic curve point to a byte slice in a way that is *specific* to the given ciphersuite needs. This is especially important when calculating a signature challenge in [FROST]. This function may yield a different result than SerializePoint function from the Curve interface. While the SerializePoint result should be considered an internal serialization that may be optimized for speed or data consistency, the EncodePoint result should be considered an external serialization, always reflecting the given ciphersuite's specification requirements. In the specific case of BIP-340 ciphersuite implementation, SerializePoint serializes both X and Y coordinates, while EncodePoint serializes just the X coordinate, as it is expected by BIP-340 for the challenge computation. The way SerializePoint works allows to avoid computing Y coordinate manually when exchanging data. The way EncodePoint works is dictated by BIP-340 specification.
Depends on #8The PR implements the
Aggregatefunction as specified in [FROST], section 5.3. Signature Share Aggregation, based on @eth-r's prototype.Moreover, it proposes a new structure of sharing functionality between the Coordinator and Signers. The base logic is placed in the
Participantstructure that is embedded byCoordinatorandSigner. This allows us not to repeat the logic while keeping the flexibility of having a separateCoordinator. TheCoordinatorcould be just another goroutine on a machine running one of theSignersbut also a completely separate instance, depending on a use case.