exthereum / exth_crypto

Cryptographic Functions used by Exthereum
MIT License
22 stars 18 forks source link

libsecp256k1 dep broken on Linux #8

Open tsutsu opened 6 years ago

tsutsu commented 6 years ago

This is really an issue with exthereum/libsecp256k1, but issues are disabled there, so I'm filing it here.

It seems that the current Hex package release of libsecp256k1 depended on by this package contains various precompiled object files (.o, .so, .a, .dylib, .la, .lo, etc.).

It seems, as well, that this project is detected by Mix as being a rebar3 project, despite only actually building properly using rebar.

Because of these two factors, when mix compile or mix deps.compile is run in a project that depends on libsecp256k1, libsecp256k1 won't actually be fully recompiled. The actual final NIF build artifact will be built, but it will be linked against the existing library .a file rather than said file being rebuilt.

This goes unnoticed when compiling on macOS, because the build artifacts in the Hex package are of macOS executable format, so the NIF build step will successfully link them.

However, when compiling on Linux, either the linking step will fail, or the linked NIF will fail to load.

Currently, I'm doing this in a Dockerfile I'm using as a workaround:

RUN mix deps.get

# libsecp256k1's Hex package is thoroughly corrupted
RUN ( cd ./deps/libsecp256k1; \
      rm ./priv/libsecp256k1_nif.so ./c_src/libsecp256k1_nif.o && \
      rebar compile )

RUN mix do compile, clean

Obviously, it'd be better to just publish a new release of the libsecp256k1 Hex package that doesn't contain these build artifacts.

As well, in order to properly build the dependent library (rather than just the build artifact), I believe exth_crypto needs to use manager: :rebar in its dep spec for libsecp256k1, as without this libsecp256k1 is detected as a rebar3 project for some reason, and rebar3 seems to ignore pre_hooks and post_hooks. (Alternately, libsecp256k1 could be upgraded to be a proper rebar3 project.)

hayesgm commented 6 years ago

Sorry for the delay. I completely agree and this dependency has been a thorn in my side for a while. Moving the manager to rebar, I believe, prevents us from publishing this on Hex, which I believe is why this is not done previously.

The current workaround I have used is:

export "CFLAGS=-I/usr/local/include -L/usr/local/lib"
cd deps/libsecp256k1 && rebar compile
mix compile

But, none of this is acceptable as a long (or short term) solution.

We currently use the libsecp256k1 library exclusively for ECDSA recover, as that's not available in erlang's crypto module. I would prefer that we build / use a native erlang or Elixir implementation of ECDSA recover and simply drop the dependency on libsecp256k1. This would improve compile times, and it would allow us to change the elliptic curve used (if Ethereum ever goes that route).

Alternatively, we could pick up the libsecp256k1 library and proper bootstrap it into hex.

Happy for your thoughts here, @tsutsu

vans163 commented 6 years ago

@hayesgm

I would prefer that we build / use a native erlang or Elixir implementation of ECDSA recover and simply drop the dependency on libsecp256k1.

Would this need a patch to OTP? But yea also if you target windows you gotta compile libsecp256k1 for windows, the rebar file doesn't seem to indicate win as a target.

hayesgm commented 6 years ago

Well, we would just implement the function in erlang/Elixir instead of relying on a native C library. It wouldn't be hard, but we need to make sure we get the math correct.

This overflow question sums up the math which is straight-forward. There are a few nuances since there are up to 2 possible keys which could generate a message (hence the recovery bit).

tsutsu commented 6 years ago

@hayesgm I think that decision comes down to how often libsecp256k1's ECRecover function is actually called by the code that uses this library. Is it something that is being called in any "hot loops", or is it mostly used once? A pure-Erlang implementation might be a bad idea if this code balloons the overhead of e.g. ex_wire block decoding, or if there are contracts that call the ecrecover Solidity function O(N) times, such that (a better implementation of) evm would be translating those CALL ops to the ecrecover builtin contract, to calls to ECRecover(). Testing would be needed for the former case, and an audit of the corpus of contracts on the public Ethereum chain would be needed in the latter case. If both turn out fine, though, then yes, I think this would make sense.

Alternatively: if everything exth_crypto needs is available in OTP's crypto (i.e. OpenSSL) save for this one function—and this is probably true of other ECDSA "compact" PKI code as well—then maybe this one function could be lifted from libsecp256k (license permitting) and upstreamed all the way into OpenSSL itself, such that OTP's crypto could then expose it.

vans163 commented 6 years ago

@tsutsu its not pure erlang, all the heavy lifting would be done in C via the crypto nif. The performance overhead will literary be exactly the same as it is now. To summarize now we are using the libsecp256k1 nif and this is proposing to start using the otp crypto nif.

tsutsu commented 6 years ago

@vans163 I was responding to @hayesgm’s suggestion:

Well, we would just implement the function in erlang/Elixir instead of relying on a native C library. It wouldn't be hard, but we need to make sure we get the math correct.

hayesgm commented 6 years ago

I was suggesting we build a native erlang version. The question is what the cost of the function is, but I am not sure it's exceedingly computationally expensive.

For reference, ECDSA is called when processing an Ethereum transaction. I do not believe it's used via ex_wire as part of the peer-to-peer protocol, but I would have to go and check. There are maybe 100 transactions per block in Ethereum, so it is called about 100 times per verification of a given block.

I agree it's best to have this upstreamed all the way to a native implementation that is exposed by erlang's :crypto module. Do we want to see if this function is exposed in OpenSSL right now?

vans163 commented 6 years ago

@hayesgm it would make things so much simpler to maintain. Its worth a look.

hayesgm commented 6 years ago

I did some digging: apparently OpenSSL does not expose ECDSA recover in any of its APIs. Additionally, there looks to be very little to no elliptic curve libraries currently implemented in erlang or Elixir. I would be happy to find another solution, but it looks like libsecp256k1 might be our best option, and maybe we should work to make the wrapping project higher quality.

pdobacz commented 6 years ago

Hello,

the fix that is proposed here, and what we've been using by setting in our project:

{:libsecp256k1, "~> 0.1.2", compile: "${HOME}/.mix/rebar compile", override: true}

doesn't work anymore, because libsecp256k1 is now a Makefile project. When I just remove the overrides, and let exth_crypto (or rather blockchain which I'm depending on) handle the libsecp256k1, I get a lot of errors like:

2018-08-27 15:29:02.713 [warn] ⋅The on_load function for module libsecp256k1 returned:
{:error, {:load_failed, 'Failed to load NIF library: \'/home/user/sources/_build/test/lib/libsecp256k1/priv/libsecp256k1_nif.so: undefined symbol: __gmpn_tdiv_qr\''}}

     ** (UndefinedFunctionError) function :libsecp256k1.ec_pubkey_create/2 is undefined (module :libsecp256k1 is not available)
     stacktrace:
       :libsecp256k1.ec_pubkey_create(<<127, 223, 147, 228, 202, 154, 44, 41, 129, 197, 248, 190, 236, 59, 107, 225, 97, 215, 251, 9, 9, 166, 65, 203, 96, 221, 55, 45, 94, 48, 185, 200>>, :uncompressed)
       lib/blockchain/transaction/signature.ex:45: Blockchain.Transaction.Signature.get_public_key/1

What am I doing wrong?

pdobacz commented 6 years ago

a late small test case to reproduce the above issue:

mix.exs:

<snip>
  defp deps do
    [
      {:exth_crypto, "~> 0.1.6"},
      {:blockchain, "~> 0.1.6"}
    ]
  end
<snip>

testtest.exs:

defmodule TestTest do
  use ExUnit.Case
  test "greets the world" do
    :crypto.strong_rand_bytes(32)
    |> Blockchain.Transaction.Signature.get_public_key()
  end
end

To reporoduce mix deps.get and mix test.

On Ubuntu 16.0 and

$ elixir --version
Erlang/OTP 20
Elixir 1.7.2 (compiled with Erlang/OTP 20)

it gives:

17:53:33.139 [warn]  The on_load function for module libsecp256k1 returned:
{:error, {:load_failed, 'Failed to load NIF library: \'/home/user/sources/test/test/_build/test/lib/libsecp256k1/priv/libsecp256k1_nif.so: undefined symbol: __gmpn_tdiv_qr\''}}

  1) test greets the world (TestTest)
     test/test_test.exs:5
     ** (UndefinedFunctionError) function :libsecp256k1.ec_pubkey_create/2 is undefined (module :libsecp256k1 is not available)
     code: |> Blockchain.Transaction.Signature.get_public_key()
     stacktrace:
       (libsecp256k1) :libsecp256k1.ec_pubkey_create(<<44, 199, 137, 214, 114, 184, 143, 102, 153, 244, 236, 170, 94, 3, 244, 249, 187, 19, 221, 145, 112, 13, 70, 243, 207, 81, 182, 125, 28, 139, 128, 111>>, :uncompressed)
       (blockchain) lib/blockchain/transaction/signature.ex:45: Blockchain.Transaction.Signature.get_public_key/1
       test/test_test.exs:7: (test)

whereas if I do

{:libsecp256k1, "0.1.4", compile: "$HOME/.mix/rebar compile", override: true}

in mix.exs, nuke the deps and _build dirs and run again, it works fine

DenisGorbachev commented 6 years ago

@pdobacz I'm getting absolutely same issue. However, $HOME/.mix/rebar compile doesn't compile anything. Instead, it outputs:

==> libsecp256k1 (compile)

... and exits without even cloning the library from github.

Afterwards, if I run the tests, it outputs:

could not find an app file at "_build/test/lib/libsecp256k1/ebin/libsecp256k1.app"
DenisGorbachev commented 6 years ago

@pdobacz OMG, it worked after downgrading:

  libsecp256k1 0.1.9 => 0.1.4

Final line in mix.exs:

{:libsecp256k1, "0.1.4", compile: "$HOME/.mix/rebar compile", hex: "libsecp256k1", repo: "hexpm", optional: false, override: true}

(guess it has the same effect as yours)

pdobacz commented 6 years ago

@DenisGorbachev re

doesn't compile anything. Instead, it outputs and exits without even cloning the library from github.

I think one needs to delete deps/libsecp256k1 before compiling manually, when you change the mix.exs settings (like compile:), unless you're modifying the version.

So yeah 0.1.4 works with the override, 0.1.9 won't work without ("undefined" errors as above) or with (no rebar project to compile) the override.