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

secp256k1 jets #18

Closed
wants to merge 2 commits into from
Closed

secp256k1 jets #18

wants to merge 2 commits into from

Conversation

roconnor-blockstream
Copy link
Collaborator

Review C/secp256k1/README.md for information about our copy of libsecp256k1.

Also included is a commit that eliminates unused parameter warnings.

In GCC you can "use" a parameter 'foo' with the statement '(void) foo;'.
Using this trick we can eliminate the unused parameter warnings in the particular cases where we necessarily have unused parameters.
Specifically we add
* sqrt for field elemnts
* offsetPoint for group elements (adding a point in affine coordinates to a point in jacobian coordinates)
* ecMult: Add a scalar multiple of a group element (in jacobian coordinates) to a scalar multiple of the secp256k1 generator point
* schnorrVerify: Decide if a schnorr signature is valid for a given public key-message pair.

These jets "exercise" the various domains of secp256k1.
Later we will add a whole host of jets for secp256k1 functionality, but for now ecMult and schnorrVerify are the most useful functions.
@apoelstra
Copy link
Contributor

What commit did you get these libsecp files from? and I guess you renamed secp256k1.c to secp256k1_impl.h, and moved the schnorrsig files out from the modules/ tree to the root?

@apoelstra
Copy link
Contributor

oh, I see, you actually modified all the files

} ecmult_static;

/* This function will initialize the 'ecmult_static' global variable if it hasn't already been initialized. */
static void ensure_ecmult_static(void) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The thread-unsafety of this really makes me uncomfortable, though I can't think of any other way to do this. Maybe putting a secp context into txEnv?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a more general problem with the entire library. I've created a separate issue to track it .

static int secp256k1_eckey_pubkey_parse(secp256k1_ge *elem, const unsigned char *pub, size_t size);
static int secp256k1_eckey_pubkey_serialize(secp256k1_ge *elem, unsigned char *pub, size_t *size);

#if 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this #if 0 code meant to be committed?

* It only operates on tables sized for WINDOW_A wnaf multiples.
* - secp256k1_ecmult_odd_multiples_table_storage_var, which converts its
* resulting point set to actually affine points, and stores those in pre.
* It operates on tables of any size, but uses heap-allocated temporaries.
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW this comment was updated in bitcoin-core/secp256k1#677 -- apparently at your advice.


/* Converts the point encoded by a secp256k1_pubkey into its absolute value,
* i.e. keeps it as is if it is positive and otherwise negates it. Sign is set
* to 1 if the input point was negative and set to 0 otherwise. */
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment (and the function implementation) are also different upstream. See https://github.com/jonasnick/secp256k1/blob/schnorrsig/src/secp256k1.c#L736

Copy link
Contributor

Choose a reason for hiding this comment

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

And in particular, I find the existing text which talks about a point being "positive" or "negative" to be confusing

@roconnor-blockstream
Copy link
Collaborator Author

Moved to #34.

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.

2 participants