Open stepansnigirev opened 4 years ago
Nice! Yeah, I think it makes sense to integrate these changes with lowmemory
.
I think it would make sense to have a separate option for USE_ECMULT_STATIC_PRECOMPUTATION
. People have different needs for embedded system, and some may prefer to use 70KB of RAM instead of 70KB binary size.
Moreover, I'm unsure about pic
. I think it's very unexpected that an option like lowmemory
disables pic. But I guess we also don't want a separate option for every compile option. Could this maybe fixed in cc
depending on the target? What's your "rust target"?
I actually don't understand what pic
is - sometimes when I do embedded stuff, the linker complains about this flag existing or not existing, and I try to placate it, and if I can't I just go back to my day job.
Regarding static precomputation, I agree that we ought to add another feature for this because there's a nonobvious tradeoff.
@real-or-random, @apoelstra
According to https://github.com/rust-embedded/cortex-m-rt/blob/master/link.x.in#L144 pic
is not supported by cortex-m-rt
.
My target is thumbv7em-none-eabihf
(Cortex-M4F) but I think the same applies to any cortex-m target.
cc
docs say about pic
:
"This option defaults to false
for windows-gnu
targets and to true
for all other targets."
Probably I should open an issue in cc
then to disable pic
for cortex-m...
Adding static precomputation as a feature looks good to me.
rs-cc
just merged the pull request (https://github.com/alexcrichton/cc-rs/pull/470) that disables pic
for bare metal targets. I think with the next release build.rs
will work out of the box.
Regarding USE_ECMULT_STATIC_PRECOMPUTATION
feature how about to call it precomputed
?
I'm starting to think that the real solution to USE_ECMULT_STATIC_PRECOMPUTATION
is that cc
will let the end user override define
s, ie RUSTFLAGS=-DUSE_ECMULT_STATIC_PRECOMPUTATION=0
or even the good old CFLAGS=-DUSE_ECMULT_STATIC_PRECOMPUTATION=0
That way the end user could override our default for special cases.
Regarding
USE_ECMULT_STATIC_PRECOMPUTATION
feature how about to call itprecomputed
?
Maybe signing-static-precomputation
? "Precomputed" is very generic. (And something along the lines of ECMULT_STATIC_PRECOMPUTATION
is not a good idea. It does not help the user. And this define should have been called ECMULT_GEN_STATIC_PRECOMPUTATION
in the first place.)
I'm starting to think that the real solution to
USE_ECMULT_STATIC_PRECOMPUTATION
is thatcc
will let the end user overridedefine
s, ieRUSTFLAGS=-DUSE_ECMULT_STATIC_PRECOMPUTATION=0
or even the good oldCFLAGS=-DUSE_ECMULT_STATIC_PRECOMPUTATION=0
Something like this will certainly help, in particular for exotic targets and options. But we should still have features for common stuff, I think.
Spoke with @stepansnigirev in person and I'd like to move forward on this ... plan is to add both a lowmemory
feature and a signing_static_precomputation
feature?
Sounds good to me.
I'm fine with it too
I am using rust-secp256k1 library on a cortex-m4 microcontroller with 2MB flash and 256k RAM. Unfortunately, it doesn't work out of the box if I just add it to my
Cargo.toml
with justdefault-features = false, features = ['lowmemory']
I had to make a few changes to the
build.rs
file in the library to use it with my board and also to reduce RAM usage:base_config.pic(false)
) to get it linkedUSE_ECMULT_STATIC_PRECOMPUTATION
to the config to reduce required preallocated buffer sizeecmult_static_context.h
file to the build that was generated when I was compiling original secp256k1 library from source (I think generation of this file can be also included inbuild.rs
)It's great that we have a
lowmemory
feature but withoutUSE_ECMULT_STATIC_PRECOMPUTATION
it still uses ~70k of RAM for the context. With a precomputed table RAM usage is reduced to <1k.Would it make sense to integrate these changes to
lowmemory
or to create a new feature for the library?Here is the only commit to my fork of the library that makes it working on cortex-m4: https://github.com/stepansnigirev/rust-secp256k1/commit/9cbbb931a93ba96d40874cb55e6bae5604f95872