-
Notifications
You must be signed in to change notification settings - Fork 208
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
Add Schnorrsig half aggregation #130
Conversation
This doesn't allow aggregating aggregate sigs. Would be nice if it could. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rough review since I was reading it anyway. Obv as PR states, needs more work!
ARG_CHECK(msg32 != NULL); | ||
ARG_CHECK(pubkey != NULL); | ||
|
||
if (*aggsig_size < 32*(1 + n_sigs)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overflow protection needed here, AFAICT. You probably want to divide aggsig_size, check it's > 0, then sub 1 and compare with n_sigs.
size_t* aggsig_size, | ||
unsigned char **sig64, | ||
unsigned char **msg32, | ||
secp256k1_xonly_pubkey *pubkey, | ||
uint32_t n_sigs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know C is sloppy with this, but const unsigned char **sig64 would be nice. And msg32.
Also, n_sigs here and n_msgs below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And sig64 adn msg32 are arrays of ptrys, pubkey is a direct array of objects. That's a bit weird? Also, please pubkeys
to give the poor user a hint?
const secp256k1_context* ctx, | ||
unsigned char **msg32, | ||
uint32_t n_msgs, | ||
secp256k1_xonly_pubkey *pubkey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const here too?
/* TODO: fix endianness issue */ | ||
secp256k1_sha256_write(&hash, (unsigned char *)&i, sizeof(i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Please do :)
ARG_CHECK(aggsig != NULL); | ||
|
||
secp256k1_gej_set_infinity(&rhs); | ||
if (aggsig_size != 32*(1 + n_msgs)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overflow
secp256k1_sha256_initialize(&hash); | ||
secp256k1_sha256_write(&hash, midhash, sizeof(midhash)); | ||
/* TODO: fix endianness issue */ | ||
secp256k1_sha256_write(&hash, (unsigned char *)&i, sizeof(i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted.
We should make this PR compliant with the draft spec (which was written after this PR was opened). |
Closing in favor of #261 |
3a9b1d4 New Experimental Module: Incremental Half-Aggregation for Schnorr Signatures (Benedikt) Pull request description: Revisited PR #130 by jonasnick. I am happy to hear your thoughts. **Summary of changes compared to #130:** - Address comments from rustyrussell - Use tagged hash - Compute hashes with common prefix by copying midstate - Allow Incremental Aggregation and make code consistent with the [draft spec](https://github.com/BlockstreamResearch/cross-input-aggregation/blob/master/half-aggregation.mediawiki) ACKs for top commit: real-or-random: ACK 3a9b1d4 Tree-SHA512: 27239033f8b28ecf87ea310b3dd5a19dbbe6fd07495db71ef7017f8f444ec25a12897087d1bea0a2e9c3df77d7f17c38b183d7fe768858da2180f26624add4aa
Result of a recorded pair-programming session with @real-or-random to demonstrate how to write libsecp(-zkp) code. Therefore the code is still in a very early stage.