Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize BDN Signature/Key Aggregation #546

Merged
merged 9 commits into from
Sep 24, 2024

Conversation

Stebalien
Copy link
Contributor

Originally filed against drand's fork as drand#60 and drand#61.

This set of patches:

  1. Removes an N^2 bitfield operation from aggregation. Not a huge deal, but it can become an issue when the set of participants grow.
  2. Introduces a "cached" mask for BDN where coefficients and public key aggregation terms are precomputed. This significantly improves the performance of public key aggregation (88x on my machine) when the different subsets of the same set of public keys are aggregated over and over.

I've also added some test fixtures for BDN at @AnomalRoil's request and added a benchmark for the CachedMask.

@CLAassistant
Copy link

CLAassistant commented Sep 5, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

github-actions bot commented Sep 5, 2024

🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer.

@Stebalien Stebalien marked this pull request as draft September 5, 2024 22:29
@Stebalien Stebalien force-pushed the steb/optimize-aggregation branch from 0f25cd6 to 6b09955 Compare September 5, 2024 22:48
Copy link

github-actions bot commented Sep 5, 2024

🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer.

CountEnabled and IndexOfNthEnabled are both O(n) in the size of the
mask, making this loop n^2. The BLS operations still tend to be the slow
part, but the n^2 factor will start to show up with thousands of keys.
This new mask will pre-compute reusable values, speeding up repeated
verification and aggregation of aggregate signatures (mostly the former).
@Stebalien Stebalien force-pushed the steb/optimize-aggregation branch from 6b09955 to 9acfb6f Compare September 5, 2024 22:53
Copy link

github-actions bot commented Sep 5, 2024

🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer.

@Stebalien Stebalien marked this pull request as ready for review September 5, 2024 22:53
@Stebalien Stebalien closed this Sep 5, 2024
@Stebalien Stebalien reopened this Sep 5, 2024
Copy link

github-actions bot commented Sep 5, 2024

🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer.

sign/bdn/bdn.go Outdated
}

coefs, err := hashPointToR(mask.Publics())
func (scheme *Scheme) AggregateSignatures(sigs [][]byte, mask Mask) (kyber.Point, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is technically a breaking change if someone is abstracting over Scheme via an interface. I'm happy to explore alternatives (e.g., add a new function, etc.) if that's an issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are in the process of preparing a V4 release anyway 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case.... I can drop the interface and switch to a BDN specific mask if that works better for you (not sure how much breaking you want).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would be totally fine by me. Actually Mask seems to only be used in bdn anyway, I'd be fine with it also being in internal or so, and have BDN expose the methods required to extract the list of participants from an aggregate signature.

Summoning @pierluca and @K1li4nL in case they have other opinions

Copy link

sonarqubecloud bot commented Sep 6, 2024

sign/bdn/mask.go Outdated Show resolved Hide resolved
sign/bdn/mask.go Outdated Show resolved Hide resolved
Copy link
Contributor

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Stebalien Now that we have the notion of Scheme for bdn, I wonder if the masks shouldn't just be embedded in these and just have new exported methods on the Scheme to manage participants and public keys, as well as a method taking an aggregate signature and returning the list of participants or so?

sign/bdn/mask.go Outdated Show resolved Hide resolved
@Stebalien
Copy link
Contributor Author

Now that we have the notion of Scheme for bdn, I wonder if the masks shouldn't just be embedded in these and just have new exported methods on the Scheme to manage participants and public keys, as well as a method taking an aggregate signature and returning the list of participants or so?

IMO, it's best to keep scheme independent of the participants. But I can make Mask BDN specific and probably simplify it a bit.

Copy link

sonarqubecloud bot commented Sep 9, 2024

@Stebalien Stebalien force-pushed the steb/optimize-aggregation branch 2 times, most recently from d3d39e2 to b664250 Compare September 9, 2024 18:08
@Stebalien Stebalien force-pushed the steb/optimize-aggregation branch from b664250 to 39ccfaa Compare September 9, 2024 18:08
@Stebalien
Copy link
Contributor Author

@AnomalRoil I've moved Mask into BDN and removed the abstraction. This is significantly cleaner.

AnomalRoil
AnomalRoil previously approved these changes Sep 11, 2024
sign/bdn/mask.go Outdated Show resolved Hide resolved
sign/bdn/mask.go Show resolved Hide resolved
Co-authored-by: AnomalRoil <[email protected]>
@rjan90
Copy link

rjan90 commented Sep 20, 2024

@AnomalRoil could you give this a final review? I think Steb has addressed your last outstanding comments

Stebalien added a commit to filecoin-project/go-f3 that referenced this pull request Sep 22, 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 added a commit to filecoin-project/go-f3 that referenced this pull request Sep 22, 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
Copy link
Contributor Author

Honestly, no rush; our deadlines aren't your deadlines. Given the interface-based decoupling in kyber, we were able to pretty cleanly vendor just the BDN package so we're not blocked on any of our PRs.

Copy link

Stebalien added a commit to filecoin-project/go-f3 that referenced this pull request 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
@AnomalRoil AnomalRoil merged commit 0ba2750 into dedis:master Sep 24, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants