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

Add _ge_set_all_gej and use it in musig for own public nonces #1614

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

real-or-random
Copy link
Contributor

As suggested in #1479 (comment)

Copy link
Contributor

@sipa sipa left a comment

Choose a reason for hiding this comment

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

utACK b4d6028

src/group.h Outdated
@@ -80,7 +80,10 @@ static void secp256k1_ge_set_gej(secp256k1_ge *r, secp256k1_gej *a);
/** Set a group element equal to another which is given in jacobian coordinates. */
static void secp256k1_ge_set_gej_var(secp256k1_ge *r, secp256k1_gej *a);

/** Set a batch of group elements equal to the inputs given in jacobian coordinates */
/** Set a batch of group elements equal to the inputs given in jacobian coordinates (affine). Constant time. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Explicitly say that the inputs are not allowed to be infinity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rephrased the entire docstring

@real-or-random real-or-random force-pushed the 202410-ct-batch-inv branch 2 times, most recently from 218ebfb to 1ba332a Compare October 8, 2024 14:44
src/group.h Outdated
static void secp256k1_ge_set_all_gej(secp256k1_ge *r, const secp256k1_gej *a, size_t len);

/** Set group elements r[0:len] (affine) equal to group elements a[0:len] (jacobian).
* None of the group elements in a[0:len] may be infinity. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this description differ from the constant-time variant, as infinity group elements are allowed here? (e.g. "can be infinity")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, copy and paste... Should be fixed, I simply dropped the sentence.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK f24e3e6

secp256k1_scalar_clear(&k[i]);
}
secp256k1_ge_set_all_gej(nonce_pts, nonce_ptj, 2);
for (i = 0; i < 2; i++) {
secp256k1_declassify(ctx, &nonce_pts[i], sizeof(nonce_pts));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not directly related to this PR, as the _declassify call is only moved but still called with the same values, but the size passed seems too large, if I'm not missing anything (should be sizeof(nonce_pts[i]), rather than the full array size, otherwise we mark beyond the array on the second iteration, and the second half of the array twice).

real-or-random added a commit that referenced this pull request Oct 22, 2024
…once points

57eda3b musig: ctimetests: fix _declassify range for generated nonce points (Sebastian Falbesoner)

Pull request description:

  As noticed in #1614 (comment), the area marked as non-secret exceeds the nonce_pts array in the second iteration of the for loop. Fix that by passing the correct size to the _declassify call.

ACKs for top commit:
  sipa:
    utACK 57eda3b
  real-or-random:
    utACK 57eda3b

Tree-SHA512: ff8074e3d1078d66a52d08c661997856ff586b3b4564a865a75212b32fafd7906d58885371bd63005007fde554ebcad121ab66125abe4331cf0aac63fc018ed0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants