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

[API BREAK] Introduce explicit contexts #208

Merged
merged 1 commit into from
Apr 11, 2015
Merged

Conversation

sipa
Copy link
Contributor

@sipa sipa commented Feb 4, 2015

No description provided.

@DavidEGrayson
Copy link

I looked at the changes to the main header file and they look good. I would be tempted to remove the flags argument from secp256k1_context_create and just let people use the other functions, since it makes the API surface a bit smaller.

@gmaxwell
Copy link
Contributor

gmaxwell commented Feb 4, 2015

Need to worry about some combinatorial blowup, e.g. with each mixture of starting options having a new entry point. (We may have more options in the future). Otherwise I'd be agreeing with that.

@sipa
Copy link
Contributor Author

sipa commented Feb 4, 2015

@gmaxwell I'm not sure whether you're in favor or against the flags argument...

@gmaxwell
Copy link
Contributor

gmaxwell commented Feb 4, 2015

For flags. Because if you have six flags or wahtever you might need 2^6 (or more) entriy points otherwise)

@DavidEGrayson
Copy link

You just need one secp256k1_context_initialize_* entry point for each high-level feature, because multiple initialization functions can be called on the same context after it is created.

@gmaxwell
Copy link
Contributor

gmaxwell commented Feb 4, 2015

Hm. Doesn't address the case where it's cheaper to initilize a&&b than a, b or where b means you throw out what A does, without having a weird interface where some interact and some don't and some are slower unless you combine them.

Now I don't know what I prefer. I know I don't want separate contexts. Since state can potentially be shared (esp in low-mem builds).

@DavidEGrayson
Copy link

Yeah, so that would be a good reason to keep the flags argument around.

@DavidEGrayson
Copy link

I think the API for creation/initialization could be simplified to just two functions:

secp256k1_context* secp256k1_context_create();
void secp256k1_context_initialize(secp256k1_context* ctx, int flags); // can call multiple times

This allows special optimizations of the initialization process, regardless of when the initialization is performed, without combinatorial blowup of entry points. It makes the API simpler because there would be fewer functions and only one way to enable a feature. (Sorry if I'm missing something and this is just another lame idea for some reason!)

@sipa
Copy link
Contributor Author

sipa commented Feb 4, 2015

So there's a follow-up plan which is probably not obvious here, namely that some initializations may gain parameters (specifically: the size of the tables), which you probably can't encode as just flags. The explicit initialization functions can then serve as 'advanced' versions, while the flagged create_context does defaulty things.

@sipa
Copy link
Contributor Author

sipa commented Mar 2, 2015

Rebased.

@sipa
Copy link
Contributor Author

sipa commented Mar 2, 2015

And rebased again.

@sipa
Copy link
Contributor Author

sipa commented Mar 16, 2015

And again.

@gmaxwell I think we'll see significant API changes still soon (especially if we'd incorporate #179, #190, and similar things). The bulk of this PR is introducing the contexts, and is a bit of a pain to maintain. How to actually build the contexts can be changed later, I think.

@sipa
Copy link
Contributor Author

sipa commented Mar 16, 2015

Removed the per-feature initialization functions for now.

@sipa
Copy link
Contributor Author

sipa commented Mar 29, 2015

Rebased.

@sipa
Copy link
Contributor Author

sipa commented Apr 3, 2015

@gmaxwell Review plz? puppy eyes

@gmaxwell
Copy link
Contributor

gmaxwell commented Apr 3, 2015

Okay, I think I've run out of nits.

@sipa
Copy link
Contributor Author

sipa commented Apr 10, 2015

Nits addressed.

@gmaxwell
Copy link
Contributor

Would be nice to brace that IF; either way: ACK.

@sipa sipa merged commit a9b6595 into bitcoin-core:master Apr 11, 2015
sipa added a commit that referenced this pull request Apr 11, 2015
a9b6595 [API BREAK] Introduce explicit contexts (Pieter Wuille)
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.

None yet

3 participants