Use library to_hex function

[tcharding]

We do not need to use the hex module from bitcoin_hashes to encode into hex, we have a function in this library.

Use library hex encoding logic, removes dependency on the hex module of bitcoin_hashes entirely from this crate.

[tcharding]

WTF ASAN issue here too now???

[tcharding]

So it looks like the recent fix in rust-bitcoin worked because it stopped the memory sanitizer being run in a crate that depended on libc (i.e., only hashes not bitcoin). Since rust-secp256k1 depends on libc we are hitting the error again. Wrong, see comment below.

[tcharding]

I hacked up a crate that uses rand as a dependency (because rand depends on libc) and the error is not hit. Adding a dependency on rust-secp256k1 (and a simple SecretKey usage) causes the error to be hit.

[tcharding]

There is something strange going on, if we change (eckey_impl.h)

static int rustsecp256k1_v0_8_0_eckey_pubkey_parse(rustsecp256k1_v0_8_0_ge *elem, const unsigned char *pub, size_t size) {
    printf("\n\nsize: %d\n\n", size);

    if (size != 0) {
        return 0;
    }

The memory sanitizer errors for theif (size != 0) { line but the printf call is ok and size is equal to 33.

Function call stack:
PublicKey::from_slice -> ffi::secp256k1_ec_pubkey_parse -> rustsecp256k1_v0_8_0_ec_pubkey_parse -> rustsecp256k1_v0_8_0_eckey_pubkey_parse

[tcharding]

I'm using this invocation to call ASAN and isolate the call path above.

CARGO_TERM_VERBOSE=true CC='clang -fsanitize=memory -fno-omit-frame-pointer' RUSTFLAGS='-Zsanitizer=memory -Zsanitizer-memory-track-origins -Cforce-frame-pointers=yes' cargo +nightly test  -Zbuild-std --target x86_64-unknown-linux-gnu  public_key_from

[elichai]

That's very weird. valgrind doesn't complain about anything, I'm trying to delete more and more stuff to get a minimal reproducible example

[elichai]

Seems it is an LLVM bug: rust-lang/rust#107149
I also minimized the bug to this small example: https://github.com/elichai/msan_c_rust_bug

Adding -Cllvm-args=-msan-eager-checks=0 seems to solve this for now

[Kixunil]

This one is in tests so no big deal, but I think this library should depend on bitcoin-internals and use hex impl from it, as well as various error handling stuff.

[apoelstra]

@Kixunil libsecp has its own sha256 implementation. The dependency on bitcoin-hashes here is purely to get the ThirtyTwoByteHash trait, not to provide any funcitonality.

[Kixunil]

LOL, I meant to write "hex impl"

[apoelstra]

Oh, lol! That would make much more sense, given that there is no "hash impl" in bitcoin-internals.

Having said this, I don't really like the idea of there being a non-optional dependency on a perma-unstable crate, even if it's one that we maintain. If we got more than just hex decoding from it maybe.

[apoelstra]

ACK bdfa0ff

[Kixunil]

If we got more than just hex decoding from it maybe.

I think some error stuff as well.