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

configure: Add unsupported --with-system-libsecp256k1 configure flag #5416

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@luke-jr
Copy link
Member

luke-jr commented Dec 3, 2014

No description provided.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Dec 4, 2014

I don't think makes sense without guarantee that even the API remains stable.

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Dec 4, 2014

@sipa Lots of libraries don't have stable APIs... The only reason this is risky/questionable in our case is the consensus-critical nature of it, but since we maintain libsecp256k1 anyway (unlike LevelDB), that risk is IMO much smaller (the unstable API actually helps for this).

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Dec 4, 2014

IMO: this critical library should not be linked against a system install lib. Security: control your stack.
Even if sipa/core-devs maintains it, it throws over the whole deterministic build system in case of critical crypto functions.

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Dec 4, 2014

@jonasschnelli Obviously we won't be using this in the deterministic binaries. It's also disabled by default and has a big fat warning label on it.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Dec 4, 2014

@luke-jr right. My fault. This won't affect the gitian part, right. Oversaw the warning as well. Looks good.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 4, 2014

NACK. Same reasoning as for leveldb but even stronger. The consensus code
must stay self-contained.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Dec 4, 2014

@laanwj while I agree about not doing this, currently libsecp256k1 isn't used in consensus code.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 4, 2014

I know. But that was the plan right?

I don't see how that's an argument for this. You want to merge this and than remove it again once it is used for verification/consensus?

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Dec 4, 2014

No, I don't want to merge this. I'm just saying that currently the consensus argument is not (yet) valid :)

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented Dec 4, 2014

-1 I don't see why this additional option should be supported.

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Dec 4, 2014

I'd also prefer only one: --with-system-libs. This would allow distro packagers to make clean packages. But one such option is enough ;-)

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented Dec 4, 2014

@paveljanik Based on practical experience, that would be dangerous. Packagers would ignorantly enable that by default, without understanding the consequences.

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Dec 4, 2014

I think it is better to teach packagers to do it properly than package glibc/openssl/boost inside Bitcoin Core. And of course, they will surely modify our code to use system libs anyway... Thus it is much cleaner to do that even in upstream where they all can share their code/changes.

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Dec 4, 2014

... and we can sanity check the libs at the startup as we already do.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 4, 2014

Forget teaching packagers anything. There are so many distros that we'd have to start a packager training school. Just making it hard to do is a good signal IMO.

@laanwj laanwj closed this Dec 4, 2014

@rebroad

This comment has been minimized.

Copy link
Contributor

rebroad commented Mar 1, 2016

@luke-jr, when would/should this option be used please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment