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

Remove n^2 algorithm from signature/key aggregation #60

Closed
wants to merge 5 commits into from

Conversation

Stebalien
Copy link

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.

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.
Copy link
Member

@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.

If you're trying to optimize as much as possible these bits, I'd say above it could be also a single loop on lines 36-55 in hashPointToR:

https://github.com/drand/kyber/pull/60/files#diff-ab05302c80b55ce58776f14258e11c901a806bbf1005c788d91dd42b2bdac9dcR36-R55

and the XOF could be read 16 bytes at a time in the same loop as well.
Instead of having $3n$ iterations you could have just $n$ in that function.

The only thing I really don't like here is that it seems we're testing the compatibility of both functions with each other but we don't have any explicit KAT in there, so while it does look functionally identical this might potentially break retro-compatibility with past aggregated signatures as far as our tests are concerned.

Any chance you'd have a few such signatures and set of public keys and mask handy to add as KAT vectors in a new test?

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

If you're trying to optimize as much as possible these bits, I'd say above it could be also a single loop on lines 36-55 in hashPointToR:

I'm mostly just trying to avoid the n^2 algorithm, but that's a good point. Fixed.

The only thing I really don't like here is that it seems we're testing the compatibility of both functions with each other but we don't have any explicit KAT in there, so while it does look functionally identical this might potentially break retro-compatibility with past aggregated signatures as far as our tests are concerned.

Hm. Yeah, that's a very fair point. I'll grab and/or generate some test signatures.

Stebalien and others added 2 commits August 13, 2024 07:49
Co-authored-by: AnomalRoil <AnomalRoil@users.noreply.github.com>
@rjan90
Copy link

rjan90 commented Aug 23, 2024

@AnomalRoil Can you give this a second review?

@Stebalien
Copy link
Author

Nah, it's not quite ready. I still need to add the test.

@Stebalien
Copy link
Author

I believe I've addressed the feedback except:

and the XOF could be read 16 bytes at a time in the same loop as well.
Instead of having 3 n iterations you could have just n in that function.

As far as I can tell, I have to finish hashing first and separate loops aren't going to cost much in terms of performance. I guess we could read in 16-byte chunks, but looking at the blake2s code, that actually looks a bit slower (more setup/teardown).

@AnomalRoil
Copy link
Member

@Stebalien you're right I misread the code initially when I said we could do the XOF extraction in the same loop. We need to write all peers to it before starting extraction anyway.

I'll take a look in the coming days, but I'm traveling right now.

@Stebalien
Copy link
Author

Np. When you get a chance, could you also look at #61? That's going to have a much bigger impact.

@Stebalien
Copy link
Author

Closing in favor of dedis#546.

@Stebalien Stebalien closed this Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants