-
Notifications
You must be signed in to change notification settings - Fork 209
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 cmake gears to enable ECDSA sign-to-contract - rangeproof - surjectionproof - whitelist - generator modules #275
add cmake gears to enable ECDSA sign-to-contract - rangeproof - surjectionproof - whitelist - generator modules #275
Conversation
Thanks! I think this should also cover module dependencies, i.e., some modules should be enabled automatically if others are enabled. See a few lines above your changes in the root And if you're at it, would you mind adding all modules to CMake? The remaining ones shouldn't be harder to add. (If not, no worries, we can do it in a separate PR.) |
thanks @real-or-random for the feedback I think I added all the modules. I am not sure I got the default values right though. Also, I think that splitting option declarations and option dependencies into two separate sections makes code cleaner If you have a dual-monitor set-up reviewing this PR is going to be a piece of cake: CMakeLists.txt vs configure.ac 😄 |
@real-or-random ping 😃 |
This looks good, but since we're a fork of libsecp256k1, we tend to stay close to their code to avoid conflicts and additional work when we rebase on libsecp256k1s master. (So it's for example not a good idea to add a trailing "." to the existing options.) When I reviewed this PR last week, my first thought was "let me just fixup your PR a bit, that's quicker done than explained", but then it turned out to be more complex than I expected. But you're totally right, it's cleaner to declare all the module options first and only then process them. bitcoin-core/secp256k1#1482 does this upstream. Let's see, if we can get it quickly it, we can cherry-pick it here and then rebase this PR on top of the result. I can take care of this then, I have some local changes already. |
e682267 build: Error if required module explicitly off (Tim Ruffing) 89ec583 build: Clean up handling of module dependencies (Tim Ruffing) Pull request description: This is a cleanup which makes it easier to add further modules with dependencies, e.g., in #1452. The diff looks larger than it is because I also reordered the modules and made the order consistent between CMake and autotools. (We noticed that the current logic could be improved in BlockstreamResearch/secp256k1-zkp#275.) ACKs for top commit: jonasnick: ACK e682267 hebasto: ACK e682267. Tree-SHA512: 040e791e5b5b9b8845a39632633a45ca759391455910bdefba2b7b77c6340e65df6eda18199ae2ad65c30ee2fc6630471437aec143c26fe09ae4c11409a37622
The upstream commit is has been merged. |
- Bulletproofs++ - MuSig - Generator - Rangeproof - Whitelist - ECDSA s2c - ECDSA adaptor - Surjectionproof also reshuffle the existing modules declarations to mirror GNU Make
I rebased my branch |
See #288. |
No description provided.