search

mimblewimble / rust-secp256k1-zkp

ZKP fork for rust-secp256k1, adds wrappers for range proofs, pedersen commitments, etc
Creative Commons Zero v1.0 Universal
57 stars 51 forks source link

Remove rustc-serialize #82

Closed workingjubilee closed 4 months ago

workingjubilee commented 6 months ago

This crate will have difficulties being updated to future compilers if it continues to depend on rustc-serialize. This PR updates it to remove this dependency. Not all tests pass on my machine but this appears to be due to preexisting unsoundness in the library's test suite:

    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.01s
warning: the following packages contain code that will be rejected by a future version of Rust: grin_secp256k1zkp v0.7.12 (/home/jubilee/rust/rust-secp256k1-zkp)
note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 2`
     Running unittests src/lib.rs (target/debug/deps/secp256k1zkp-6adad85efc3e032c)

running 56 tests
test key::test::test_bad_serde_deserialize ... ok
test ecdh::tests::ecdh ... ok
test key::test::invalid_secret_key ... ok
test key::test::pubkey_from_slice ... ok
test key::test::skey_from_slice ... ok
test key::test::test_debug_output ... ok
test aggsig::tests::test_aggsig_fuzz ... ok
test key::test::skey_clear_on_drop ... ok
thread 'key::test::keypair_slice_round_trip' panicked at library/core/src/panicking.rs:152:5:
unsafe precondition(s) violated: slice::get_unchecked_mut requires that the index is within the slice
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused non-unwinding panic. aborting.
error: test failed, to rerun pass `--lib`

Caused by:
  process didn't exit successfully: `/home/jubilee/rust/rust-secp256k1-zkp/target/debug/deps/secp256k1zkp-6adad85efc3e032c` (signal: 6, SIGABRT: process abort signal)
yeastplume commented 6 months ago

Thanks, appreciate you looking at this, and we definitely need to get this library updated soon.

We need to be extremely careful with this though, as we've found binding can have subtle errors that can lead to dangerous issues on the live chain (which we've actually experienced). What's the 'preexisting unsoundness' in the existing test suite? We'd take these kinds of issues in this library very, very seriously.

dagonharett commented 5 months ago

Quite needed indeed. I am already unable to build rust-secp256k1-zkp on rust 1.77.2 due to the dependency on rustc-serialize.

error[E0310]: the parameter type `T` may not live long enough
    --> /home/dagon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-serialize-0.3.24/src/serialize.rs:1155:5
     |
1155 |     fn decode<D: Decoder>(d: &mut D) -> Result<Cow<'static, T>, D::Error> {
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |     |
     |     the parameter type `T` must be valid for the static lifetime...
     |     ...so that the type `T` will meet its required lifetime bounds...
     |
note: ...that is required by this bound
    --> /usr/src/debug/rust/rustc-1.77.2-src/library/alloc/src/borrow.rs:180:30
help: consider adding an explicit lifetime bound
     |
1151 | impl<'a, T: ?Sized + 'static> Decodable for Cow<'a, T>
     |                    +++++++++
For more information about this error, try `rustc --explain E0310`.
error: could not compile `rustc-serialize` (lib) due to 1 previous error

Hope to see this PR in proper shape and merged.

workingjubilee commented 4 months ago

Apologies, I don't get through my entire notification backlog very often. Hm. It seems https://github.com/mimblewimble/rust-secp256k1-zkp/pull/83 duplicates this work, so I can close this.

What's the 'preexisting unsoundness' in the existing test suite?

I posted an error message.

thread 'key::test::keypair_slice_round_trip' panicked at library/core/src/panicking.rs:152:5:
unsafe precondition(s) violated: slice::get_unchecked_mut requires that the index is within the slice
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused non-unwinding panic. aborting.
error: test failed, to rerun pass `--lib`

We'd take these kinds of issues in this library very, very seriously.

Do you take it seriously enough to read the documentation for the functions your code calls? MaybeUninit::assume_init specifically prohibits this usage: