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

Switch to vanilla libsecp256k1 #54

Merged
merged 1 commit into from
Apr 13, 2022
Merged

Switch to vanilla libsecp256k1 #54

merged 1 commit into from
Apr 13, 2022

Conversation

Sjors
Copy link
Owner

@Sjors Sjors commented Oct 20, 2021

Libwally includes libsecp256k1-zkp as a submodule, because it has support for cryptographic functions used in Elements.

This PR swaps out the submodule with vanilla libsecp256k1 which is used by Bitcoin Core. It also uses the same version.

@Sjors Sjors force-pushed the 2021/10/secp256k-vanilla branch 3 times, most recently from c2907a1 to 65cedf5 Compare October 21, 2021 13:10
@Sjors
Copy link
Owner Author

Sjors commented Oct 29, 2021

Bump commit if bitcoin/bitcoin#23383 lands.

Libwally includes libsecp256k1-zkp as a submodule, because it has support for cryptographic functions used in Elements.

This commit checks out an earlier commit that matches the libsecp256k1 used by Bitcoin Core.
This is possible because libsecp256k1-zkp is regularly rebased on top of libsecp256k1.
@Sjors
Copy link
Owner Author

Sjors commented Apr 13, 2022

Rebased now that libwally-core 0.8.5 is out. Also bumped to the latest commit that Bitcoin Core uses.

One downside of this approach is that it leaves the libwally-core submodule dirty after each build. @jurvis thoughts?

@Sjors Sjors marked this pull request as ready for review April 13, 2022 15:31
@jurvis
Copy link
Contributor

jurvis commented Apr 13, 2022

@Sjors no strong opinion either way 😄

@@ -61,7 +70,7 @@ if [ $simulator == 1 ]; then
arch_target="aarch64-apple-darwin"
fi

./configure --disable-shared --host=$arch_target --enable-static --disable-elements
./configure --disable-shared --host=$arch_target --enable-static --disable-elements --enable-standard-secp
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, can you explain why switching to vanilla libsecp256k1 necessary if we use this new flag? (new to these tools, apologies if this seems obvious)

Copy link
Owner Author

@Sjors Sjors Apr 13, 2022

Choose a reason for hiding this comment

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

It's not necessary for compilation. It only makes it easier for anyone doing a security audit, because it removes the need to review how libsecp256k1-zkp differs from libsecp256k1.

Copy link
Owner Author

@Sjors Sjors Apr 13, 2022

Choose a reason for hiding this comment

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

The big picture goal here is to facilitate deterministic builds for any app that uses this library, see Sjors/nthkey-ios#1 - although that's a bit tangential, since there's no reason to assume zkp introduces something that won't build determinstically.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, appreciate the context! sounds good to me!

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