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.

Sign up for GitHub

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

Jump to bottom

Add optional bitcoin_hashes feature to implement ThirtyTwoByteHash #206

Merged
merged 2 commits into from May 24, 2020
+91 −6
Merged

Add optional bitcoin_hashes feature to implement ThirtyTwoByteHash #206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d85753
Add optional bitcoin_hashes feature to implement ThirtyTwoByteHash
sgeisler Apr 7, 2020
8979a93
Document proper usage of `from_slice`
sgeisler Apr 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .travis.yml
View file
Open in desktop
Expand Up @@ -30,13 +30,15 @@ script:
cargo generate-lockfile --verbose && cargo update -p cc --precise "1.0.41" --verbose;
fi
- cargo build --verbose --no-default-features
- cargo build --verbose --no-default-features --features="bitcoin_hashes"
- cargo build --verbose --no-default-features --features="serde"
- cargo build --verbose --no-default-features --features="lowmemory"
- cargo build --verbose --no-default-features --features="rand"
- cargo build --verbose --no-default-features --features="rand serde recovery endomorphism"
- cargo build --verbose --no-default-features --features="fuzztarget recovery"
- cargo build --verbose --features=rand
- cargo test --no-run --features=fuzztarget
- cargo test --verbose --features="bitcoin_hashes"
- cargo test --verbose --features=rand
- cargo test --verbose --features="rand rand-std"
- cargo test --verbose --features="rand serde"
Expand Down
4 changes: 4 additions & 0 deletions Cargo.toml
View file
Open in desktop
Expand Up @@ -50,6 +50,10 @@ bitcoin_hashes = "0.7"
wasm-bindgen-test = "0.3"
rand = { version = "0.6", features = ["wasm-bindgen"] }

[dependencies.bitcoin_hashes]
version = "0.7"
optional = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any arguments here against making that default? Annoyingly someone that is depending on bitcoin and depends on secp and hashes using bitcoin::secp256k1 and bitcoin::hashes can't add features to the secp dependency. Well, that's a broader Cargo problem, but well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this a lot 😅 (also because of features I might want to activate in secp256k1/bitcoin_hashes to not cause incompatibilities), turns out that cargo builds dependencies with the union of all the features activated if a dependency exists multiple times in the dependency graph (just tested it with a minimal example). So all they had to do was to specify secp256k1 as a dependency themselves and activate the feature.

On the other hand we could just activate that feature in bitcoin and use it there too. After taking a closer look we don't seem to have any transaction signing code? At least we don't use secp256k1::Message anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure? It might be that we use an into() and that Message is never mentioned verbatim. I add a usage of this in the Bitcoin Signed Message PR.


[dependencies.rand]
version = "0.6"
optional = true
Expand Down
91 changes: 85 additions & 6 deletions src/lib.rs
View file
Open in desktop
Expand Up @@ -38,28 +38,31 @@
//!
//! ```rust
//! extern crate secp256k1;
//! # #[cfg(feature="bitcoin_hashes")]
//! extern crate bitcoin_hashes;
//! # #[cfg(feature="rand")]
//! extern crate rand;
//!
//! #
//! # fn main() {
//! # #[cfg(feature="rand")] {
//! use rand::OsRng;
//! # #[cfg(all(feature="rand", feature="bitcoin_hashes"))] {
//! use rand::rngs::OsRng;
//! use secp256k1::{Secp256k1, Message};
//! use bitcoin_hashes::sha256;
//!
//! let secp = Secp256k1::new();
//! let mut rng = OsRng::new().expect("OsRng");
//! let (secret_key, public_key) = secp.generate_keypair(&mut rng);
//! let message = Message::from_slice(&[0xab; 32]).expect("32 bytes");
//! let message = Message::from_hashed_data::<sha256::Hash>("Hello World!".as_bytes());
//!
//! let sig = secp.sign(&message, &secret_key);
//! assert!(secp.verify(&message, &sig, &public_key).is_ok());
//! # } }
//! ```
//!
//! The above code requires `rust-secp256k1` to be compiled with the `rand`
//! The above code requires `rust-secp256k1` to be compiled with the `rand` and `bitcoin_hashes`
//! feature enabled, to get access to [`generate_keypair`](struct.Secp256k1.html#method.generate_keypair)
//! Alternately, keys can be parsed from slices, like
//! Alternately, keys and messages can be parsed from slices, like
//!
//! ```rust
//! # fn main() {
Expand All @@ -68,6 +71,8 @@
//! let secp = Secp256k1::new();
//! let secret_key = SecretKey::from_slice(&[0xcd; 32]).expect("32 bytes, within curve order");
//! let public_key = PublicKey::from_secret_key(&secp, &secret_key);
//! // This is unsafe unless the supplied byte slice is the output of a cryptographic hash function.
//! // See the above example for how to use this library together with bitcoin_hashes.
//! let message = Message::from_slice(&[0xab; 32]).expect("32 bytes");
//!
//! let sig = secp.sign(&message, &secret_key);
Expand Down Expand Up @@ -147,6 +152,7 @@
pub extern crate secp256k1_sys;
pub use secp256k1_sys as ffi;

#[cfg(feature = "bitcoin_hashes")] extern crate bitcoin_hashes;
#[cfg(all(test, feature = "unstable"))] extern crate test;
#[cfg(any(test, feature = "rand"))] pub extern crate rand;
#[cfg(any(test))] extern crate rand_core;
Expand All @@ -173,6 +179,9 @@ use core::marker::PhantomData;
use core::ops::Deref;
use ffi::CPtr;

#[cfg(feature = "bitcoin_hashes")]
use bitcoin_hashes::Hash;

/// An ECDSA signature
#[derive(Copy, Clone, PartialEq, Eq)]
pub struct Signature(ffi::Signature);
Expand Down Expand Up @@ -219,6 +228,27 @@ pub trait ThirtyTwoByteHash {
fn into_32(self) -> [u8; 32];
}

#[cfg(feature = "bitcoin_hashes")]
impl ThirtyTwoByteHash for bitcoin_hashes::sha256::Hash {
fn into_32(self) -> [u8; 32] {
self.into_inner()
}
}

#[cfg(feature = "bitcoin_hashes")]
impl ThirtyTwoByteHash for bitcoin_hashes::sha256d::Hash {
fn into_32(self) -> [u8; 32] {
self.into_inner()
}
}

#[cfg(feature = "bitcoin_hashes")]
impl<T: bitcoin_hashes::sha256t::Tag> ThirtyTwoByteHash for bitcoin_hashes::sha256t::Hash<T> {
fn into_32(self) -> [u8; 32] {
self.into_inner()
}
}

impl SerializedSignature {
/// Get a pointer to the underlying data with the specified capacity.
pub(crate) fn get_data_mut_ptr(&mut self) -> *mut u8 {
Expand Down Expand Up @@ -451,7 +481,12 @@ impl_array_newtype!(Message, u8, constants::MESSAGE_SIZE);
impl_pretty_debug!(Message);

impl Message {
/// Converts a `MESSAGE_SIZE`-byte slice to a message object
/// **If you just want to sign an arbitrary message use `Message::from_hashed_data` instead.**
///
/// Converts a `MESSAGE_SIZE`-byte slice to a message object. **WARNING:** the slice has to be a
/// cryptographically secure hash of the actual message that's going to be signed. Otherwise
/// the result of signing isn't a
/// [secure signature](https://twitter.com/pwuille/status/1063582706288586752).
#[inline]
pub fn from_slice(data: &[u8]) -> Result<Message, Error> {
if data == [0; constants::MESSAGE_SIZE] {
Expand All @@ -467,6 +502,25 @@ impl Message {
_ => Err(Error::InvalidMessage)
}
}

/// Constructs a `Message` by hashing `data` with hash algorithm `H`. This requires the feature
/// `bitcoin_hashes` to be enabled.
/// ```rust
/// extern crate bitcoin_hashes;
/// use secp256k1::Message;
/// use bitcoin_hashes::sha256;
/// use bitcoin_hashes::Hash;
///
/// let m1 = Message::from_hashed_data::<sha256::Hash>("Hello world!".as_bytes());
/// // is equivalent to
/// let m2 = Message::from(sha256::Hash::hash("Hello world!".as_bytes()));
///
/// assert_eq!(m1, m2);
/// ```
#[cfg(feature = "bitcoin_hashes")]
pub fn from_hashed_data<H: ThirtyTwoByteHash + bitcoin_hashes::Hash>(data: &[u8]) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm how is a method like this used? By explicitly providing the type param? That might be a bit awkward UX?

let msg = Message::from_hashes_data<sha256::Hash>(&my_data[..]);

Or is there another way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's a bit awkward, but imo still better than having a separate method for each hash function. Given the confusion I should definitely document it better since using a type parameter in such a way is not too common. Alternatively I could just remove it, Message::from(sha256::Hash::hash(&my_data)) will do the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added an example to the docs. I personally find that, although a bit longer, the first version reads better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

<H as bitcoin_hashes::Hash>::hash(data).into()
}
}

impl<T: ThirtyTwoByteHash> From<T> for Message {
Expand Down Expand Up @@ -1110,6 +1164,31 @@ mod tests {
test_bad_slice();
test_low_s();
}

#[cfg(feature = "bitcoin_hashes")]
#[test]
fn test_from_hash() {
use bitcoin_hashes;
use bitcoin_hashes::Hash;

let test_bytes = "Hello world!".as_bytes();

let hash = bitcoin_hashes::sha256::Hash::hash(test_bytes);
let msg = Message::from(hash);
assert_eq!(msg.0, hash.into_inner());
assert_eq!(
msg,
Message::from_hashed_data::<bitcoin_hashes::sha256::Hash>(test_bytes)
);

let hash = bitcoin_hashes::sha256d::Hash::hash(test_bytes);
let msg = Message::from(hash);
assert_eq!(msg.0, hash.into_inner());
assert_eq!(
msg,
Message::from_hashed_data::<bitcoin_hashes::sha256d::Hash>(test_bytes)
);
}
}

#[cfg(all(test, feature = "unstable"))]
Expand Down