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 static context object which has no capabilities #553

Merged
merged 2 commits into from
Nov 6, 2018

Conversation

apoelstra
Copy link
Contributor

No description provided.

@gmaxwell
Copy link
Contributor

gmaxwell commented Sep 4, 2018

Should we document that you can't call set_illegal_callback on this and perhaps add some checks on some of the disallowed calls to fail if you try?

@sipa
Copy link
Contributor

sipa commented Sep 4, 2018

@gmaxwell I believe the set of permitted functions on this object is exactly the functions which take a const context pointer.

@apoelstra
Copy link
Contributor Author

Sure, we can do a VERIFY_CHECK on those functions that the ctx is not the static one, much like how we compare to NULL now.

@apoelstra
Copy link
Contributor Author

apoelstra commented Sep 20, 2018

Added a commit which VERIFY_CHECKs on all functions that modify the context object.

I considered using ARG_CHECK but I didn't see the point, since you can't change the callback function from its default CHECK-like behaviour. Also, since it is actually UB to call these functions on static data (without synchronization primitives anyway), I wanted to prevent it more emphatically.

Edit Changed from VERIFY_CHECK to CHECK because Greg pointed out that VERIFY is only defined during unit testing.

* API consistency, but currently do not require expensive precomputations or dynamic
* allocations.
*/
SECP256K1_API extern secp256k1_context *secp256k1_context_no_precomp;
Copy link
Contributor

@sipa sipa Oct 4, 2018

Choose a reason for hiding this comment

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

Can you mark this object const? If that works you don't need any of the CHECK invocations I think as they all require a non-const argument. It would also alleviate any concerns about synchronization etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -91,6 +99,7 @@ secp256k1_context* secp256k1_context_clone(const secp256k1_context* ctx) {
}

void secp256k1_context_destroy(secp256k1_context* ctx) {
CHECK(ctx != secp256k1_context_no_precomp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these necessary? A non-const ctx pointer is used, which can't be secp256k1_context_no_precomp (as that one is const).

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, good point. I guess they're not necessary. I can remove them if you want, though I don't mind having them there.

@sipa
Copy link
Contributor

sipa commented Oct 17, 2018

utACK 40fde61

@real-or-random
Copy link
Contributor

ACK 40fde61

@sipa sipa merged commit 40fde61 into bitcoin-core:master Nov 6, 2018
sipa added a commit that referenced this pull request Nov 6, 2018
40fde61 prevent attempts to modify `secp256k1_context_no_precomp` (Andrew Poelstra)
ed7c084 add static context object which has no capabilities (Andrew Poelstra)

Pull request description:

Tree-SHA512: a843ed7ba00a00a46eec3146ce428d4b49eb440af766f44d731b1f51553d08de8cc9a0af5ed114d0dfdca6f4bf4a2ede4dbd6a37d6bd818b81630089424a0ba5
sipa added a commit that referenced this pull request Nov 26, 2018
b3bf5f9 ecmult_impl: expand comment to explain how effective affine interacts with everything (Andrew Poelstra)
efa783f Store z-ratios in the 'x' coord they'll recover (Peter Dettman)
ffd3b34 add `secp256k1_ge_set_all_gej_var` test which deals with many infinite points (Andrew Poelstra)
84740ac ecmult_impl: save one fe_inv_var (Andrew Poelstra)
4704527 ecmult_impl: eliminate scratch memory used when generating context (Andrew Poelstra)
7f7a2ed ecmult_gen_impl: eliminate scratch memory used when generating context (Andrew Poelstra)

Pull request description:

  Builds on #553

Tree-SHA512: 6031a601a4a476c1d21fc8db219383e7930434d2f199543c61aca0118412322dd814a0109c385ff1f83d16897170dd0c25051697b0f88f15234b0059b661af41
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.

4 participants