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

Compiler error when altering ecmult generation #533

Open
cryptoquick opened this issue Nov 24, 2022 · 15 comments
Open

Compiler error when altering ecmult generation #533

cryptoquick opened this issue Nov 24, 2022 · 15 comments

Comments

@cryptoquick
Copy link

I'm trying to compile a performant version of this library for testing on my local machine. I experimented with the following changes:

https://github.com/rust-bitcoin/rust-secp256k1/compare/master...cryptoquick:rust-secp256k1:perf?expand=1

And got this error:


warning: depend/secp256k1/src/precomputed_ecmult.c:14:5: error: #error configuration mismatch, invalid ECMULT_WINDOW_SIZE. Try deleting precomputed_ecmult.c before the build.
warning:    14 |    #error configuration mismatch, invalid ECMULT_WINDOW_SIZE. Try deleting precomputed_ecmult.c before the build.
warning:       |     ^~~~~
warning: depend/secp256k1/src/precomputed_ecmult.c:14:5: error: #error configuration mismatch, invalid ECMULT_WINDOW_SIZE. Try deleting precomputed_ecmult.c before the build.
warning:    14 |    #error configuration mismatch, invalid ECMULT_WINDOW_SIZE. Try deleting precomputed_ecmult.c before the build.
warning:       |     ^~~~~

error: failed to run custom build command for `secp256k1-sys v0.6.1 (/home/.../rust-secp256k1/secp256k1-sys)`

Caused by:
  process didn't exit successfully: `/home/.../rana/target/release/build/secp256k1-sys-68e31a69b35e42d8/build-script-build` (exit status: 1)
  --- stdout
  TARGET = Some("x86_64-unknown-linux-gnu")
  OPT_LEVEL = Some("3")
  HOST = Some("x86_64-unknown-linux-gnu")
  cargo:rerun-if-env-changed=CC_x86_64-unknown-linux-gnu
  CC_x86_64-unknown-linux-gnu = None
  cargo:rerun-if-env-changed=CC_x86_64_unknown_linux_gnu
  CC_x86_64_unknown_linux_gnu = None
  cargo:rerun-if-env-changed=HOST_CC
  HOST_CC = None
  cargo:rerun-if-env-changed=CC
  CC = None
  cargo:rerun-if-env-changed=CFLAGS_x86_64-unknown-linux-gnu
  CFLAGS_x86_64-unknown-linux-gnu = None
  cargo:rerun-if-env-changed=CFLAGS_x86_64_unknown_linux_gnu
  CFLAGS_x86_64_unknown_linux_gnu = None
  cargo:rerun-if-env-changed=HOST_CFLAGS
  HOST_CFLAGS = None
  cargo:rerun-if-env-changed=CFLAGS
  CFLAGS = None
  cargo:rerun-if-env-changed=CRATE_CC_NO_DEFAULTS
  CRATE_CC_NO_DEFAULTS = None
  DEBUG = Some("false")
  CARGO_CFG_TARGET_FEATURE = Some("adx,aes,avx,avx2,bmi1,bmi2,fma,fxsr,lzcnt,pclmulqdq,popcnt,rdrand,rdseed,sha,sse,sse2,sse3,sse4.1,sse4.2,ssse3,xsave,xsavec,xsaveopt,xsaves")
  cargo:rerun-if-env-changed=CC_x86_64-unknown-linux-gnu
  CC_x86_64-unknown-linux-gnu = None
  cargo:rerun-if-env-changed=CC_x86_64_unknown_linux_gnu
  CC_x86_64_unknown_linux_gnu = None
  cargo:rerun-if-env-changed=HOST_CC
  HOST_CC = None
  cargo:rerun-if-env-changed=CC
  CC = None
  cargo:rerun-if-env-changed=CFLAGS_x86_64-unknown-linux-gnu
  CFLAGS_x86_64-unknown-linux-gnu = None
  cargo:rerun-if-env-changed=CFLAGS_x86_64_unknown_linux_gnu
  CFLAGS_x86_64_unknown_linux_gnu = None
  cargo:rerun-if-env-changed=HOST_CFLAGS
  HOST_CFLAGS = None
  cargo:rerun-if-env-changed=CFLAGS
  CFLAGS = None
  cargo:rerun-if-env-changed=CRATE_CC_NO_DEFAULTS
  CRATE_CC_NO_DEFAULTS = None
  CARGO_CFG_TARGET_FEATURE = Some("adx,aes,avx,avx2,bmi1,bmi2,fma,fxsr,lzcnt,pclmulqdq,popcnt,rdrand,rdseed,sha,sse,sse2,sse3,sse4.1,sse4.2,ssse3,xsave,xsavec,xsaveopt,xsaves")
  running: "cc" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "-I" "depend/secp256k1/" "-I" "depend/secp256k1/include" "-I" "depend/secp256k1/src" "-Wall" "-Wextra" "-Wno-unused-function" "-DSECP256K1_API=" "-DENABLE_MODULE_ECDH=1" "-DENABLE_MODULE_SCHNORRSIG=1" "-DENABLE_MODULE_EXTRAKEYS=1" "-DUSE_NUM_NONE=1" "-DUSE_FIELD_INV_BUILTIN=1" "-DUSE_SCALAR_INV_BUILTIN=1" "-DCFLAGS=O3" "-DECMULT_GEN_PREC_BITS=4" "-DECMULT_WINDOW_SIZE=24" "-DUSE_EXTERNAL_DEFAULT_CALLBACKS=1" "-o" "/home/.../rana/target/release/build/secp256k1-sys-a187a4710acc4568/out/depend/secp256k1/contrib/lax_der_parsing.o" "-c" "depend/secp256k1/contrib/lax_der_parsing.c"
  exit status: 0
  running: "cc" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "-I" "depend/secp256k1/" "-I" "depend/secp256k1/include" "-I" "depend/secp256k1/src" "-Wall" "-Wextra" "-Wno-unused-function" "-DSECP256K1_API=" "-DENABLE_MODULE_ECDH=1" "-DENABLE_MODULE_SCHNORRSIG=1" "-DENABLE_MODULE_EXTRAKEYS=1" "-DUSE_NUM_NONE=1" "-DUSE_FIELD_INV_BUILTIN=1" "-DUSE_SCALAR_INV_BUILTIN=1" "-DCFLAGS=O3" "-DECMULT_GEN_PREC_BITS=4" "-DECMULT_WINDOW_SIZE=24" "-DUSE_EXTERNAL_DEFAULT_CALLBACKS=1" "-o" "/home/.../rana/target/release/build/secp256k1-sys-a187a4710acc4568/out/depend/secp256k1/src/precomputed_ecmult_gen.o" "-c" "depend/secp256k1/src/precomputed_ecmult_gen.c"
  exit status: 0
  running: "cc" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "-I" "depend/secp256k1/" "-I" "depend/secp256k1/include" "-I" "depend/secp256k1/src" "-Wall" "-Wextra" "-Wno-unused-function" "-DSECP256K1_API=" "-DENABLE_MODULE_ECDH=1" "-DENABLE_MODULE_SCHNORRSIG=1" "-DENABLE_MODULE_EXTRAKEYS=1" "-DUSE_NUM_NONE=1" "-DUSE_FIELD_INV_BUILTIN=1" "-DUSE_SCALAR_INV_BUILTIN=1" "-DCFLAGS=O3" "-DECMULT_GEN_PREC_BITS=4" "-DECMULT_WINDOW_SIZE=24" "-DUSE_EXTERNAL_DEFAULT_CALLBACKS=1" "-o" "/home/.../rana/target/release/build/secp256k1-sys-a187a4710acc4568/out/depend/secp256k1/src/precomputed_ecmult.o" "-c" "depend/secp256k1/src/precomputed_ecmult.c"
  cargo:warning=depend/secp256k1/src/precomputed_ecmult.c:14:5: error: #error configuration mismatch, invalid ECMULT_WINDOW_SIZE. Try deleting precomputed_ecmult.c before the build.
  cargo:warning=   14 |    #error configuration mismatch, invalid ECMULT_WINDOW_SIZE. Try deleting precomputed_ecmult.c before the build.
  cargo:warning=      |     ^~~~~
  exit status: 1
  running: "cc" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "-I" "depend/secp256k1/" "-I" "depend/secp256k1/include" "-I" "depend/secp256k1/src" "-I" "wasm/wasm-sysroot" "-Wall" "-Wextra" "-Wno-unused-function" "-DSECP256K1_API=" "-DENABLE_MODULE_ECDH=1" "-DENABLE_MODULE_SCHNORRSIG=1" "-DENABLE_MODULE_EXTRAKEYS=1" "-DUSE_NUM_NONE=1" "-DUSE_FIELD_INV_BUILTIN=1" "-DUSE_SCALAR_INV_BUILTIN=1" "-DCFLAGS=O3" "-DECMULT_GEN_PREC_BITS=4" "-DECMULT_WINDOW_SIZE=24" "-DUSE_EXTERNAL_DEFAULT_CALLBACKS=1" "-o" "/home/.../rana/target/release/build/secp256k1-sys-a187a4710acc4568/out/depend/secp256k1/contrib/lax_der_parsing.o" "-c" "depend/secp256k1/contrib/lax_der_parsing.c"
  exit status: 0
  running: "cc" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "-I" "depend/secp256k1/" "-I" "depend/secp256k1/include" "-I" "depend/secp256k1/src" "-I" "wasm/wasm-sysroot" "-Wall" "-Wextra" "-Wno-unused-function" "-DSECP256K1_API=" "-DENABLE_MODULE_ECDH=1" "-DENABLE_MODULE_SCHNORRSIG=1" "-DENABLE_MODULE_EXTRAKEYS=1" "-DUSE_NUM_NONE=1" "-DUSE_FIELD_INV_BUILTIN=1" "-DUSE_SCALAR_INV_BUILTIN=1" "-DCFLAGS=O3" "-DECMULT_GEN_PREC_BITS=4" "-DECMULT_WINDOW_SIZE=24" "-DUSE_EXTERNAL_DEFAULT_CALLBACKS=1" "-o" "/home/.../rana/target/release/build/secp256k1-sys-a187a4710acc4568/out/depend/secp256k1/src/precomputed_ecmult_gen.o" "-c" "depend/secp256k1/src/precomputed_ecmult_gen.c"
  exit status: 0
  running: "cc" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "-I" "depend/secp256k1/" "-I" "depend/secp256k1/include" "-I" "depend/secp256k1/src" "-I" "wasm/wasm-sysroot" "-Wall" "-Wextra" "-Wno-unused-function" "-DSECP256K1_API=" "-DENABLE_MODULE_ECDH=1" "-DENABLE_MODULE_SCHNORRSIG=1" "-DENABLE_MODULE_EXTRAKEYS=1" "-DUSE_NUM_NONE=1" "-DUSE_FIELD_INV_BUILTIN=1" "-DUSE_SCALAR_INV_BUILTIN=1" "-DCFLAGS=O3" "-DECMULT_GEN_PREC_BITS=4" "-DECMULT_WINDOW_SIZE=24" "-DUSE_EXTERNAL_DEFAULT_CALLBACKS=1" "-o" "/home/.../rana/target/release/build/secp256k1-sys-a187a4710acc4568/out/depend/secp256k1/src/precomputed_ecmult.o" "-c" "depend/secp256k1/src/precomputed_ecmult.c"
  cargo:warning=depend/secp256k1/src/precomputed_ecmult.c:14:5: error: #error configuration mismatch, invalid ECMULT_WINDOW_SIZE. Try deleting precomputed_ecmult.c before the build.
  cargo:warning=   14 |    #error configuration mismatch, invalid ECMULT_WINDOW_SIZE. Try deleting precomputed_ecmult.c before the build.
  cargo:warning=      |     ^~~~~
  exit status: 1

  --- stderr


  error occurred: Command "cc" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "-I" "depend/secp256k1/" "-I" "depend/secp256k1/include" "-I" "depend/secp256k1/src" "-I" "wasm/wasm-sysroot" "-Wall" "-Wextra" "-Wno-unused-function" "-DSECP256K1_API=" "-DENABLE_MODULE_ECDH=1" "-DENABLE_MODULE_SCHNORRSIG=1" "-DENABLE_MODULE_EXTRAKEYS=1" "-DUSE_NUM_NONE=1" "-DUSE_FIELD_INV_BUILTIN=1" "-DUSE_SCALAR_INV_BUILTIN=1" "-DCFLAGS=O3" "-DECMULT_GEN_PREC_BITS=4" "-DECMULT_WINDOW_SIZE=24" "-DUSE_EXTERNAL_DEFAULT_CALLBACKS=1" "-o" "/home/.../rana/target/release/build/secp256k1-sys-a187a4710acc4568/out/depend/secp256k1/src/precomputed_ecmult.o" "-c" "depend/secp256k1/src/precomputed_ecmult.c" with args "cc" did not execute successfully (status code exit status: 1).

I tried doing as it says, via rm secp256k1-sys/depend/secp256k1/src/precomputed_ecmult.c, but that only results in this error:

   Compiling secp256k1-sys v0.6.1 (/home/.../rust-secp256k1/secp256k1-sys)
The following warnings were emitted during compilation:

warning: cc1: fatal error: depend/secp256k1/src/precomputed_ecmult.c: No such file or directory
warning: compilation terminated.
warning: cc1: fatal error: depend/secp256k1/src/precomputed_ecmult.c: No such file or directory
warning: compilation terminated.

error: failed to run custom build command for `secp256k1-sys v0.6.1 (/home/.../rust-secp256k1/secp256k1-sys)`

How can I resolve this error, and are there any other performance optimizations I can make use of?

Not sure if it's helpful to mention or relevant, but for what it's worth, I'm building on a Zen 2 AMD system running Arch Linux with 128GB of DDR4.

@elichai
Copy link
Member

elichai commented Nov 24, 2022

Ok, the reason is that we are using the default precomputed_ecmult.c which uses ECMULT_WINDOW_SIZE=15
you should delete that file and then use secp256k1 build system (inside ./secp256k1-sys/depend/secp256k1/) to generate a new one with the params you want

@apoelstra
Copy link
Member

apoelstra commented Nov 24, 2022

@cryptoquick in general I recommend you edit build.rs to try to mess with performance settings. We have hacked up our copy of libsecp in a couple of ways to integrate it with Rust's build system, in particular

  • Renaming all the exported symbols to avoid link conflicts with multiple versions of this lib
  • Removing the context_create and context_destroy functions which call malloc (we reimplemented the memory management in Rust) for WASM support
  • Removing the default abort callbacks (which call abort and printf, also unavailable in WASM)
  • Assuming the existence of the precomp tables, which we've committed but upstream's autotools build system generates

I think that's a complete list but I can't promise :)

@apoelstra
Copy link
Member

Alternately, I'd suggest using the upstream library directly and running its benchmarks, and then trying to port things back to this lib if you find improvements.

@cryptoquick
Copy link
Author

Ok, the reason is that we are using the default precomputed_ecmult.c which uses ECMULT_WINDOW_SIZE=15

you should delete that file and then use secp256k1 build system (inside ./secp256k1-sys/depend/secp256k1/) to generate a new one with the params you want

I tried that, but it unfortunately threw more errors. I think it's related to the changes @apoelstra mentioned. In the code I linked to, I was indeed making the changes to build.rs, and while other changes took effect, the window size change does not, but results in the error I originally posted.

I wonder if a crate feature could be added to support changing the window size, similar to lowmemory?

@apoelstra
Copy link
Member

I wonder if a crate feature could be added to support changing the window size, similar to lowmemory?

If you have a use case I'd be happy to make this configurable at compile time. Maybe not with a feature flag (it's unclear how that'd interact with lowmemory) but maybe by setting a cfg flag in your RUSTFLAGS.

@cryptoquick
Copy link
Author

I was just curious if changing it would result in a performance improvement, since I believe that was indicated in the comments.

@apoelstra
Copy link
Member

Heh, I wouldn't be surprised if I wrote something like "higher == better" in the comments, but I've never tested this, and sipa says that it's untrue (in the issue you filed on libsecp).

If you're just experimenting I'd recommend you just edit build.rs locally and then use path = "../rust-secp256k1/" or whatever in your deps to see what happens.

@cryptoquick
Copy link
Author

Heh, I wouldn't be surprised if I wrote something like "higher == better" in the comments, but I've never tested this, and sipa says that it's untrue (in the issue you filed on libsecp).

sipa is correct (of course), but he meant precision bits, not window size. I can test prec bits easily enough and setting it to 8 does reduce performance by about 50%. I'd be curious if that were still true at higher window sizes, however, I can't get those changes to compile.

@apoelstra
Copy link
Member

Maybe you need to make the equivalent changes upstream, build there, then copy the resulting precomp files into this library?

@cryptoquick
Copy link
Author

I tried that also, like this:

cd secp256k1-sys/depend/secp256k1
make clean
rm src/precomputed_ecmult.c
CFLAGS="-O3" ./configure --with-ecmult-window=24 --with-ecmult-gen-precision=4
make -j 1

I get this error:

make  all-am
make[1]: Entering directory '/home/.../rust-secp256k1/secp256k1-sys/depend/secp256k1'
  CC       src/bench.o
src/bench.c: In function 'main':
src/bench.c:191:16: warning: implicit declaration of function 'rustsecp256k1_v0_6_1_context_create'; did you mean 'rustsecp256k1_v0_6_1_context_randomize'? [-Wimplicit-function-declaration]
  191 |     data.ctx = rustsecp256k1_v0_6_1_context_create(SECP256K1_CONTEXT_SIGN | SECP256K1_CONTEXT_VERIFY);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                rustsecp256k1_v0_6_1_context_randomize
src/bench.c:191:16: warning: nested extern declaration of 'rustsecp256k1_v0_6_1_context_create' [-Wnested-externs]
src/bench.c:191:14: warning: assignment to 'rustsecp256k1_v0_6_1_context *' {aka 'struct rustsecp256k1_v0_6_1_context_struct *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
  191 |     data.ctx = rustsecp256k1_v0_6_1_context_create(SECP256K1_CONTEXT_SIGN | SECP256K1_CONTEXT_VERIFY);
      |              ^
src/bench.c:209:5: warning: implicit declaration of function 'rustsecp256k1_v0_6_1_context_destroy'; did you mean 'rustsecp256k1_v0_6_1_context_randomize'? [-Wimplicit-function-declaration]
  209 |     rustsecp256k1_v0_6_1_context_destroy(data.ctx);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |     rustsecp256k1_v0_6_1_context_randomize
src/bench.c:209:5: warning: nested extern declaration of 'rustsecp256k1_v0_6_1_context_destroy' [-Wnested-externs]
src/bench.c:212:14: warning: assignment to 'rustsecp256k1_v0_6_1_context *' {aka 'struct rustsecp256k1_v0_6_1_context_struct *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
  212 |     data.ctx = rustsecp256k1_v0_6_1_context_create(SECP256K1_CONTEXT_SIGN);
      |              ^
make[1]: *** No rule to make target 'libsecp256k1.c', needed by 'libsecp256k1.lo'.  Stop.
make[1]: Leaving directory '/home/.../rust-secp256k1/secp256k1-sys/depend/secp256k1'
make: *** [Makefile:825: all] Error 2

@apoelstra
Copy link
Member

Right, you can't directly build the libsecp that's bundled with this library because it doesn't have context_create. You need to do this upstream.

@cryptoquick
Copy link
Author

I'll admit, I'm not quite sure how to do that...

@apoelstra
Copy link
Member

In libsecp256k1, build with a larger window size. Copy precomputed_ecmult.c from there into this library. Then edit build.rs, then build this library.

@Kixunil
Copy link
Collaborator

Kixunil commented Nov 30, 2022

Assuming the existence of the precomp tables, which we've committed but upstream's autotools build system generates

Could we re-generate them based on rerun-if-changed/rerun-if-env-changed and comparing known values/hashes?

@apoelstra
Copy link
Member

Could we re-generate them based on rerun-if-changed/rerun-if-env-changed and comparing known values/hashes?

It'd be fine to unconditionally regenerate them, they're not super slow, the problem is that I don't know how to make cc-rs do it :)

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

No branches or pull requests

4 participants