Skip to content

Make secp256k1 optional#527

Closed
Kixunil wants to merge 3 commits intorust-bitcoin:masterfrom
Kixunil:optional_secp256k1
Closed

Make secp256k1 optional#527
Kixunil wants to merge 3 commits intorust-bitcoin:masterfrom
Kixunil:optional_secp256k1

Conversation

@Kixunil
Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil commented Nov 27, 2020

This change makes secp256k1 an optional dependency so that
applications requiring chain parsing but not doing any validation nor
signing do not have to compile secp256k1. This is a breaking change
because dependencies of this crate must fix their feature settings but
apart from that they don't need to do anything.

There are two things which may be surprising: moving of some modules and
removing secp256k1/serde from use-serde. The former one is explained
in the doc of src/util/psbt_no_key/mod.rs. The latter is due to
inability to avoid compiling secp256k1 when not needed. (electrs hit
this)

Closes #526

This change makes `secp256k1` an optional dependency so that
applications requiring chain parsing but not doing any validation nor
signing do not have to compile `secp256k1`. This is a breaking change
because dependencies of this crate must fix their feature settings but
apart from that they don't need to do anything.

There are two things which may be surprising: moving of some modules and
removing `secp256k1/serde` from `use-serde`. The former one is explained
in the doc of `src/util/psbt_no_key/mod.rs`. The latter is due to
inability to avoid compiling `secp256k1` when not needed. (`electrs` hit
this)
@Kixunil
Copy link
Copy Markdown
Collaborator Author

Kixunil commented Nov 27, 2020

Compile times on my machine after the change:

$ time cargo build --no-default-features
...

real	0m18.578s
user	0m25.393s
sys	0m2.141s
$ time cargo build --features=secp256k1
...

real	0m29.741s
user	1m7.488s
sys	0m5.765s
$ time cargo build --release --no-default-features
...

real	0m21.474s
user	0m50.332s
sys	0m2.200s
$ time cargo build --release --features=secp256k1
...

real	0m42.318s
user	2m19.932s
sys	0m5.932s

This feature makes it possible to enable `serde` in `secp256k1` without
having to import it and without forcing use of `secp256k1` when `serde`
is needed.
@elichai
Copy link
Copy Markdown
Member

elichai commented Nov 30, 2020

IMO secp should be on by default

@Kixunil
Copy link
Copy Markdown
Collaborator Author

Kixunil commented Nov 30, 2020

@elichai I'm personally not very happy about default features as users sometimes forget to turn them off when not needed. Forgetting to turn it on is impossible, since compiler will let you know very quickly. :)

But if other agree with you, I will add it to deafults.

@apoelstra apoelstra added this to the 0.27.0 milestone Jan 11, 2021
@apoelstra
Copy link
Copy Markdown
Member

Targeting 0.27. It may be that we want to split the crates up more to achieve this same goal.

@Kixunil
Copy link
Copy Markdown
Collaborator Author

Kixunil commented Jan 12, 2021

Is the intended design described somewhere?

@apoelstra
Copy link
Copy Markdown
Member

No, I'll try to open a tracking/discussion issue today.

@apoelstra
Copy link
Copy Markdown
Member

@Kixunil here is my proposal #550

@TheBlueMatt
Copy link
Copy Markdown
Member

Note that with rust-bitcoin/rust-secp256k1#289, while compile times won't change, release binaries should entirely drop any secp functions that are unused.

@dr-orlovsky dr-orlovsky modified the milestones: 0.28.0, 0.29.0 Sep 17, 2021
@Kixunil
Copy link
Copy Markdown
Collaborator Author

Kixunil commented Apr 20, 2022

Making the C library optional seems to be cleaner approach, closing.

@Kixunil Kixunil closed this Apr 20, 2022
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.

Make secp256k1 optional

6 participants