Use a mask for BLS aggregation and improve caching #604
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Stebalien
commented
Aug 29, 2024
Comment on lines
+103
to
143
| type Aggregate interface { | ||
| // Aggregates signatures from a participants. | ||
| // | ||
| // Implementations must be safe for concurrent use. | ||
| Aggregate(signerMask []int, sigs [][]byte) ([]byte, error) | ||
| // VerifyAggregate verifies an aggregate signature. | ||
| // | ||
| // Implementations must be safe for concurrent use. | ||
| VerifyAggregate(signerMask []int, payload, aggSig []byte) error | ||
| } | ||
|
|
||
| type Verifier interface { | ||
| // Verifies a signature for the given public key. | ||
| // | ||
| // Implementations must be safe for concurrent use. | ||
| Verify(pubKey PubKey, msg, sig []byte) error | ||
| // Aggregates signatures from a participants. | ||
| Aggregate(pubKeys []PubKey, sigs [][]byte) ([]byte, error) | ||
| // VerifyAggregate verifies an aggregate signature. | ||
| // Return an Aggregate that can aggregate and verify aggregate signatures made by the given | ||
| // public keys. | ||
| // | ||
| // Implementations must be safe for concurrent use. | ||
| VerifyAggregate(payload, aggSig []byte, signers []PubKey) error | ||
| Aggregate(pubKeys []PubKey) (Aggregate, error) | ||
| } |
There was a problem hiding this comment.
The interface changes. Basically:
- You call
Verifier.Aggregate(allPublicKeys). - Then you call either
Aggregate(name clash) orVerifyAggregateon the resultingAggregate(naming?).
Stebalien
commented
Aug 29, 2024
gpbft/participant.go
Outdated
Comment on lines
490
to
499
| // TODO: this is slow and under a lock, but we only want to do it once per | ||
| // instance... ideally we'd have a per-instance lock/once, but that probably isn't | ||
| // worth it. |
There was a problem hiding this comment.
IMO, punt till later (probably not much worse than fetching the committee under the lock).
Stebalien
force-pushed
the
steb/bls-mask
branch
3 times, most recently
from
fa0a974 to
0860f44
Compare
|
This is ready for review although still WIP until the upstream changes (drand/kyber#61) land. Reviews there would also be helpful (only look at the last two commits, everything else is from a previous PR). |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #604 +/- ##
==========================================
- Coverage 79.47% 76.90% -2.58%
==========================================
Files 55 64 +9
Lines 4907 5451 +544
==========================================
+ Hits 3900 4192 +292
- Misses 631 857 +226
- Partials 376 402 +26
|
masih
changed the title
masih
changed the title
Woops; sorry for PR title change noise. |
Stebalien
force-pushed
the
steb/bls-mask
branch
from
e9683d1 to
0ebf539
Compare
|
Ok, waiting on dedis/kyber#546 now. |
Stebalien
force-pushed
the
steb/bls-mask
branch
2 times, most recently
from
1c1c3ad to
fdee71b
Compare
Stebalien
force-pushed
the
steb/bls-mask
branch
from
70702e5 to
02278ad
Compare
Stebalien
changed the title
Stebalien
force-pushed
the
steb/bls-mask
branch
3 times, most recently
from
be0652f to
1c17db1
Compare
Kubuxu
reviewed
Sep 23, 2024
| byteIndex := i / 8 | ||
| mask := byte(1) << uint(i&7) | ||
| if (m.mask[byteIndex] & mask) != 0 { | ||
| count++ |
There was a problem hiding this comment.
We could just popcount but doens't matter.
There was a problem hiding this comment.
I'll make a PR upstream.
There was a problem hiding this comment.
Hm. Later. I have too many stacked.
Kubuxu
approved these changes
Sep 23, 2024
It's probably going to take a while for upstream to merge the changes so we're importing just the changed package (BDN) and the new package (Gnark) into this repo. That way we avoid forking the entire repo but can still import our changes. Any changes to these pacakges should be submitted as PRs to upstream _first_, then backported to this repo. Includes: - dedis/kyber#546 - dedis/kyber#551 - dedis/kyber#553
Stebalien
force-pushed
the
steb/bls-mask
branch
from
1c17db1 to
f5af3ff
Compare
|
Merged by rebase to preserve the three separate commits. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Design:
There are three commits here and this PR should be merged by rebase, not squash.
The code imported in commit 2 should be (nearly) identical to the code in the
bdnandgnarkpackages in https://github.com/Stebalien/kyber/tree/f3.fixes #592