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.

Sign up for GitHub

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

Jump to bottom

add cmake gears to enable ECDSA sign-to-contract - rangeproof - surjectionproof - whitelist - generator modules #275

Closed
wants to merge 1 commit into from
Closed

add cmake gears to enable ECDSA sign-to-contract - rangeproof - surjectionproof - whitelist - generator modules #275

wants to merge 1 commit into from

Conversation

lightyear15
Copy link
Contributor

No description provided.

@lightyear15 lightyear15 changed the title add cmake gears to enable ECDSA sign-to-contract module add cmake gears to enable ECDSA sign-to-contract - rangeproof - surjectionproof - whitelist - generator modules Nov 3, 2023
@real-or-random
Copy link
Collaborator

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 CMakeLists.txt for an example where schnorrsig depends on extrakeys, and see https://github.com/BlockstreamResearch/secp256k1-zkp/blob/master/configure.ac#L447-L449 for all dependencies.

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.)

@lightyear15
Copy link
Contributor Author

lightyear15 commented Nov 16, 2023 •
edited
Loading

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.
Sorry if I reshuffled the existing ones but I thought it would be easier to review if cmake modules declarations mirrored the autoconf ones.

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 😄

@lightyear15
Copy link
Contributor Author

lightyear15 commented Dec 11, 2023 •
edited
Loading

@real-or-random ping 😃

@real-or-random real-or-random mentioned this pull request Jan 16, 2024
Merged
@real-or-random
Copy link
Collaborator

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.

real-or-random added a commit to bitcoin-core/secp256k1 that referenced this pull request Jan 17, 2024
@real-or-random
Merge #1482: build: Clean up handling of module dependencies
3bf4d68 
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
@jonasnick
Copy link
Contributor

The upstream commit is has been merged.

@lightyear15 lightyear15 force-pushed the cmake_ecdsa_s2c_module branch 2 times, most recently from 7330b7f to 88ac69f Compare January 25, 2024 10:02
@lightyear15
add cmake gears to deal with optional modules
3eeac88 
- Bulletproofs++
- MuSig
- Generator
- Rangeproof
- Whitelist
- ECDSA s2c
- ECDSA adaptor
- Surjectionproof

also reshuffle the existing modules declarations to mirror GNU Make
@lightyear15
Copy link
Contributor Author

lightyear15 commented Jan 25, 2024 •
edited
Loading

I rebased my branch
it seems there is a bit still missing from the bitcoin-core/secp256k1 PR: exporting header files based on the enabled modules

@real-or-random real-or-random mentioned this pull request Jan 25, 2024
Merged
@real-or-random
Copy link
Collaborator

See #288.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lightyear15 @real-or-random @jonasnick