Trust-Machines / p256k1

Wrappers around secp256k1 to expose the underlying math, specifically unwrapped points and scalars with multiexponentiation
Apache License 2.0
12 stars 6 forks source link

Add prefix to all exported C symbols in wrapped secp256k1 #33

Closed xoloki closed 1 year ago

xoloki commented 1 year ago

We keep running into problems trying to mix p256k1 with rust-secp/rust-bitcoin, who also wrap C secp256k1; the exported symbols clash leading to linker errors.

The most straightforward way to deal with this is to change the prefix on all of the exported C symbols. It will require a great deal of mucking about with the C code, then changing the wrappers to point to the new symbols.

The repo where the forked secp256k1 lives is

https://github.com/Trust-Machines/secp256k1

It's fallen rather far behind upstream, so I suggest the following plan of attack:

  1. Squash all of the current delta from the last upstream rebase into a single commit. This will make the following steps less painful.
  2. Rebase on current upstream, before we make things worse by changing symbol names
  3. Change the existing secp256k1_ prefix to p256k1_ on all exported structs and functions, making sure all tests still pass
  4. Point the git submodule in p256k1 to the new tm/secp256k1 rev
  5. Fix the p256k1 wrappers in scalar.rs and point.rs to use the new prefix
xoloki commented 1 year ago

Fixes #31

sergey-shandar commented 1 year ago

We should consider to use secp256k1-sys (note, this is a kind of private library so, they may be Ok to expose additional symbols). May be, we should contribute to the library, for example

One of the benefits to use the ..-sys library is that they support other platforms, like Windows and WASM. I think, WASM is important, even if it doesn't perform well.

xoloki commented 1 year ago

We should consider to use secp256k1-sys (note, this is a kind of private library so, they may be Ok to expose additional symbols). May be, we should contribute to the library, for example

  • add bindgen

    • serialization
  • export additional functions (remove static)
  • anything else?

One of the benefits to use the ..-sys library is that they support other platforms, like Windows and WASM. I think, WASM is important, even if it doesn't perform well.

Serialization is definitely something they might want, but exporting additional functions comes at a cost, increasing the size of the libraries and generally going against the spirit of the original code.

And using bindgen also comes at a cost, you need llvm which is not necessary with the way secp256k1-sys compiles and links their code.

So I think it's not worth the effort to try to switch to -sys. I do think we should harvest everything good from them, specifically the prefix mangling shell script, and if WASM is useful let's try to grab that too.

xoloki commented 1 year ago

One thing which might be worth considering is to run bindgen, checkin the built wrappers, then comment bindgen out from build.rs. This would hopefully get rid of the llvm requirement, and we don't really need to run bindgen on every build, only when we update the libsecp256k1 dep.

sergey-shandar commented 1 year ago

The PR #39 is PoC that we can add prefixes using Rust and C preprocessor instead of flaky regexps.

Process: