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

Better lowmemory feature with precomputed ECMULT #299

Closed
wants to merge 1 commit into from

Conversation

devrandom
Copy link
Contributor

Based on @justinmoon's no_std branch

Reduces the memory cost of a secp context from about 64KB to 704 bytes, while increasing the code section size by 62KB.

Also greatly reduces stack utilization while setting up the context. Previously, about 200KB were being used during context generation, and now we can fit in less than 64KB RAM.

@RCasatta
Copy link
Contributor

Nice improvement
How secp256k1-sys/depend/config/ecmult_static_context.h is built?
Ideally should be created with build.rs ?

@elichai
Copy link
Member

elichai commented May 16, 2021

ping @laanwj who originally added this feature

@elichai
Copy link
Member

elichai commented May 16, 2021

IMHO This sound interesting mostly because of the stack utilization during context creation (related bitcoin-core/secp256k1#188) but I'm not an embedded person so hard for me to judge that use case.
(also semi related: bitcoin-core/secp256k1#893)

@devrandom
Copy link
Contributor Author

Unfortunately, checked_malloc has been patched out of util.h (why?), giving:

$ gcc -I. -I./src -Wall -Wextra -Wno-unused-function -g -O2 -c src/gen_context.c -o gen_context.o
$ gcc -Wall -Wextra -Wno-unused-function -g -O2  gen_context.o -o gen_context
/usr/bin/ld: gen_context.o: in function `main':
/home/dev/proj/rust-secp256k1/secp256k1-sys/depend/secp256k1/src/gen_context.c:59: undefined reference to `checked_malloc'
collect2: error: ld returned 1 exit status

I'll look into patching gen_context.c to match.

@devrandom
Copy link
Contributor Author

devrandom commented May 16, 2021

Actually, compiling and running gen_context seems somewhat difficult, because:

  • the cc crate doesn't produce binaries, only libraries
  • when cross-compiling, the cc crate would be targeting the guest architecture, but we need gen_context for the host architecture

@devrandom
Copy link
Contributor Author

devrandom commented May 17, 2021

How do you feel about doing the same thing (static ECMULT precomputation) for the regular memory case? It would eliminate people worrying about the cost of creating and keeping contexts due to CPU/memory costs. cc @TheBlueMatt

Maybe separately feature gated, but enabled by default.

@real-or-random
Copy link
Collaborator

I understand the need for this but I don't think it's the right approach to tackle it here.

This should be solved upstream and we more or less agree that pregenerated files should be part of the solution, see the discussion here. bitcoin-core/secp256k1#918 (comment)

Once this has been done, integrating this here in the rust bindings should be a matter of turning a flag on (if at all).

@real-or-random
Copy link
Collaborator

Now that bitcoin-core/secp256k1#988 and bitcoin-core/secp256k1#1042 have been merged, we should get this feature for free by just updating the secp256k1 tree.

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.

4 participants