From 1a582db16021e696effeb6080d1197a23c620cf2 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 10 Jun 2022 12:21:29 +1000 Subject: [PATCH 01/10] Remove redundant import Clippy emits: warning: this import is redundant This is a remnant of edition 2015, now we have edition 2018 we no longer need this import statement. --- src/context.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/context.rs b/src/context.rs index b5b5525..a1f849c 100644 --- a/src/context.rs +++ b/src/context.rs @@ -114,9 +114,6 @@ mod alloc_only { use crate::ffi::{self, types::{c_uint, c_void}}; use crate::{Secp256k1, Signing, Verification, Context, AlignedType}; - #[cfg(feature = "rand-std")] - use rand; - impl private::Sealed for SignOnly {} impl private::Sealed for All {} impl private::Sealed for VerifyOnly {} From 35d59e7cc6954a0109191b2772f0f4a90197ef8c Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 10 Jun 2022 12:25:03 +1000 Subject: [PATCH 02/10] Remove explicit 'static lifetime Clippy emits: warning: statics have by default a `'static` lifetime Static strings no longer need an explicit lifetime, remove it. --- src/ecdh.rs | 4 +--- src/key.rs | 13 ++++--------- src/lib.rs | 2 +- src/schnorr.rs | 6 ++---- 4 files changed, 8 insertions(+), 17 deletions(-) diff --git a/src/ecdh.rs b/src/ecdh.rs index 3ebf948..da50f2a 100644 --- a/src/ecdh.rs +++ b/src/ecdh.rs @@ -272,9 +272,7 @@ mod tests { 0xff, 0xff, 0, 0, 0xff, 0xff, 0, 0, 99, 99, 99, 99, 99, 99, 99, 99 ]; - static STR: &'static str = "\ - 01010101010101010001020304050607ffff0000ffff00006363636363636363\ - "; + static STR: &str = "01010101010101010001020304050607ffff0000ffff00006363636363636363"; let secret = SharedSecret::from_slice(&BYTES).unwrap(); diff --git a/src/key.rs b/src/key.rs index 357831b..adf0e25 100644 --- a/src/key.rs +++ b/src/key.rs @@ -2108,9 +2108,8 @@ mod test { 0xff, 0xff, 0, 0, 0xff, 0xff, 0, 0, 99, 99, 99, 99, 99, 99, 99, 99 ]; - static SK_STR: &'static str = "\ - 01010101010101010001020304050607ffff0000ffff00006363636363636363\ - "; + static SK_STR: &str = "01010101010101010001020304050607ffff0000ffff00006363636363636363"; + #[cfg(fuzzing)] static PK_BYTES: [u8; 33] = [ 0x02, @@ -2119,9 +2118,7 @@ mod test { 0x06, 0x83, 0x7f, 0x30, 0xaa, 0x0c, 0xd0, 0x54, 0x4a, 0xc8, 0x87, 0xfe, 0x91, 0xdd, 0xd1, 0x66, ]; - static PK_STR: &'static str = "\ - 0218845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166\ - "; + static PK_STR: &str = "0218845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166"; #[cfg(not(fuzzing))] let s = Secp256k1::new(); @@ -2238,9 +2235,7 @@ mod test { 0xff, 0xff, 0, 0, 0xff, 0xff, 0, 0, 99, 99, 99, 99, 99, 99, 99, 99 ]; - static SK_STR: &'static str = "\ - 01010101010101010001020304050607ffff0000ffff00006363636363636363\ - "; + static SK_STR: &str = "01010101010101010001020304050607ffff0000ffff00006363636363636363"; let sk = KeyPairWrapper(KeyPair::from_seckey_slice(&crate::SECP256K1, &SK_BYTES).unwrap()); assert_tokens(&sk.compact(), &[ diff --git a/src/lib.rs b/src/lib.rs index aa1c38f..a3df6e8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1001,7 +1001,7 @@ mod tests { 226, 108, 150, 124, 57, 38, 206, 112, 44, 249, 125, 75, 1, 0, 98, 225, 147, 247, 99, 25, 15, 103, 118 ]; - static SIG_STR: &'static str = "\ + static SIG_STR: &str = "\ 30450221009d0bad576719d32ae76bedb34c774866673cbde3f4e12951555c9408e6ce77\ 4b02202876e7102f204f6bfee26c967c3926ce702cf97d4b010062e193f763190f6776\ "; diff --git a/src/schnorr.rs b/src/schnorr.rs index cefaee4..abd95de 100644 --- a/src/schnorr.rs +++ b/src/schnorr.rs @@ -548,7 +548,7 @@ mod tests { 0x9e, 0x4d, 0x4e, 0xf5, 0x67, 0x8a, 0xd0, 0xd9, 0xd5, 0x32, 0xc0, 0xdf, 0xa9, 0x07, 0xb5, 0x68, 0x72, 0x2d, 0x0b, 0x01, 0x19, 0xba, ]; - static SIG_STR: &'static str = "\ + static SIG_STR: &str = "\ 14d0bf1a8953506fb460f58be141af767fd112535fb3922ef217308e2c26706f1eeb432b3dba9a01082f9e4d4ef5678ad0d9d532c0dfa907b568722d0b0119ba\ "; @@ -556,9 +556,7 @@ mod tests { 24, 132, 87, 129, 246, 49, 196, 143, 28, 151, 9, 226, 48, 146, 6, 125, 6, 131, 127, 48, 170, 12, 208, 84, 74, 200, 135, 254, 145, 221, 209, 102 ]; - static PK_STR: &'static str = "\ - 18845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166\ - "; + static PK_STR: &str = "18845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd166"; let pk = XOnlyPublicKey::from_slice(&PK_BYTES).unwrap(); assert_tokens(&sig.compact(), &[Token::BorrowedBytes(&SIG_BYTES[..])]); From c15b9d26997c062c8bb5f26a3ad8ba2f20a91244 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 10 Jun 2022 12:29:16 +1000 Subject: [PATCH 03/10] Remove unneeded explicit reference Clippy emits: warning: this expression creates a reference which is immediately dereferenced by the compiler Remove the explicit reference. --- src/ecdh.rs | 2 +- src/key.rs | 4 ++-- src/lib.rs | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/ecdh.rs b/src/ecdh.rs index da50f2a..70ad705 100644 --- a/src/ecdh.rs +++ b/src/ecdh.rs @@ -176,7 +176,7 @@ impl ::serde::Serialize for SharedSecret { let mut buf = [0u8; SHARED_SECRET_SIZE * 2]; s.serialize_str(crate::to_hex(&self.0, &mut buf).expect("fixed-size hex serialization")) } else { - s.serialize_bytes(&self.as_ref()[..]) + s.serialize_bytes(self.as_ref()) } } } diff --git a/src/key.rs b/src/key.rs index adf0e25..74aeaa3 100644 --- a/src/key.rs +++ b/src/key.rs @@ -431,7 +431,7 @@ impl PublicKey { #[cfg(feature = "global-context")] #[cfg_attr(docsrs, doc(cfg(feature = "global-context")))] pub fn from_secret_key_global(sk: &SecretKey) -> PublicKey { - PublicKey::from_secret_key(&SECP256K1, sk) + PublicKey::from_secret_key(SECP256K1, sk) } /// Creates a public key directly from a slice. @@ -1621,7 +1621,7 @@ pub mod serde_keypair { let secret_key = SecretKey::deserialize(deserializer)?; Ok(KeyPair::from_secret_key( - &crate::SECP256K1, + crate::SECP256K1, &secret_key, )) } diff --git a/src/lib.rs b/src/lib.rs index a3df6e8..dc0b014 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -729,10 +729,10 @@ mod tests { assert_eq!( ecdsa::Signature::from_der(&byte_str).expect("byte str decode"), - ecdsa::Signature::from_str(&hex_str).expect("byte str decode") + ecdsa::Signature::from_str(hex_str).expect("byte str decode") ); - let sig = ecdsa::Signature::from_str(&hex_str).expect("byte str decode"); + let sig = ecdsa::Signature::from_str(hex_str).expect("byte str decode"); assert_eq!(&sig.to_string(), hex_str); assert_eq!(&format!("{:?}", sig), hex_str); @@ -759,7 +759,7 @@ mod tests { // 71 byte signature let hex_str = "30450221009d0bad576719d32ae76bedb34c774866673cbde3f4e12951555c9408e6ce774b02202876e7102f204f6bfee26c967c3926ce702cf97d4b010062e193f763190f6776"; - let sig = ecdsa::Signature::from_str(&hex_str).expect("byte str decode"); + let sig = ecdsa::Signature::from_str(hex_str).expect("byte str decode"); assert_eq!(&format!("{}", sig), hex_str); } @@ -1026,7 +1026,7 @@ mod tests { let msg = Message::from_slice(&msg_data).unwrap(); // Check usage as explicit parameter - let pk = PublicKey::from_secret_key(&SECP256K1, &sk); + let pk = PublicKey::from_secret_key(SECP256K1, &sk); // Check usage as self let sig = SECP256K1.sign_ecdsa(&msg, &sk); From 2cb687fc6999bd38bd3949b0c981e6ffb221b5f9 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 10 Jun 2022 12:31:11 +1000 Subject: [PATCH 04/10] Use to_le_bytes instead of mem::transmute Since we bumped the MSRV we have `to_le_bytes` available now, use it instead of `mem::transmute`. Remove code comment suggesting to do exactly this and clear clippy warning. While we are at it, use `cast` instead of `as`. --- src/ecdsa/mod.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/ecdsa/mod.rs b/src/ecdsa/mod.rs index 8c0f814..d9a8ba4 100644 --- a/src/ecdsa/mod.rs +++ b/src/ecdsa/mod.rs @@ -1,6 +1,6 @@ //! Structs and functionality related to the ECDSA signature algorithm. -use core::{fmt, str, ops, ptr, mem}; +use core::{fmt, str, ops, ptr}; use crate::{Signing, Verification, Message, PublicKey, Secp256k1, SecretKey, from_hex, Error, ffi}; use crate::ffi::CPtr; @@ -398,14 +398,8 @@ impl Secp256k1 { } counter += 1; - // From 1.32 can use `to_le_bytes` instead - let le_counter = counter.to_le(); - let le_counter_bytes : [u8; 4] = mem::transmute(le_counter); - for (i, b) in le_counter_bytes.iter().enumerate() { - extra_entropy[i] = *b; - } - - entropy_p = extra_entropy.as_ptr() as *const ffi::types::c_void; + extra_entropy[..4].copy_from_slice(&counter.to_le_bytes()); + entropy_p = extra_entropy.as_ptr().cast::(); // When fuzzing, these checks will usually spinloop forever, so just short-circuit them. #[cfg(fuzzing)] From d64132cd4b4c03bb08f33cc4f8415669414335b4 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 10 Jun 2022 12:36:50 +1000 Subject: [PATCH 05/10] Allow missing_safety_doc We have a whole bunch of unsafe code that calls down to the FFI layer. It would be nice to have clippy running on CI, these safety docs warnings are prohibiting that. Until we can add the docs add a compiler attribute to allow the lint. --- secp256k1-sys/src/lib.rs | 2 ++ src/lib.rs | 2 ++ 2 files changed, 4 insertions(+) diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index cc9047f..65f7388 100644 --- a/secp256k1-sys/src/lib.rs +++ b/secp256k1-sys/src/lib.rs @@ -19,6 +19,8 @@ // Coding conventions #![deny(non_upper_case_globals, non_camel_case_types, non_snake_case, unused_mut)] +#![allow(clippy::missing_safety_doc)] + #![cfg_attr(all(not(test), not(feature = "std")), no_std)] #![cfg_attr(docsrs, feature(doc_cfg))] diff --git a/src/lib.rs b/src/lib.rs index dc0b014..c2dc65f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -152,6 +152,8 @@ #![deny(non_upper_case_globals, non_camel_case_types, non_snake_case)] #![warn(missing_docs, missing_copy_implementations, missing_debug_implementations)] +#![allow(clippy::missing_safety_doc)] + #![cfg_attr(all(not(test), not(feature = "std")), no_std)] #![cfg_attr(all(test, feature = "unstable"), feature(test))] #![cfg_attr(docsrs, feature(doc_cfg))] From 42de876e01071e0992067b88c620667be5b1391b Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 10 Jun 2022 12:44:04 +1000 Subject: [PATCH 06/10] Allow let_and_return for feature guarded code Clippy emits: warning: returning the result of a `let` binding from a block This is due to feature guarded code, add 'allow' attribute. Use `cfg_attr` to restrict the allow to when it is needed. Add the already present `unused_mut` inside the `cfg_attr` guard also. --- src/context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/context.rs b/src/context.rs index a1f849c..12afb0f 100644 --- a/src/context.rs +++ b/src/context.rs @@ -188,7 +188,7 @@ mod alloc_only { /// ctx.seeded_randomize(&seed); /// # } /// ``` - #[allow(unused_mut)] // Unused when `rand-std` is not enabled. + #[cfg_attr(not(feature = "rand-std"), allow(clippy::let_and_return, unused_mut))] pub fn gen_new() -> Secp256k1 { #[cfg(target_arch = "wasm32")] ffi::types::sanity_checks_for_wasm(); From 685444c34252142a516c3f04c242c73c54c91e8d Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 10 Jun 2022 12:52:17 +1000 Subject: [PATCH 07/10] Use "a".repeats() instead of manual implementation Clippy emits: warning: manual implementation of `str::repeat` using iterators As suggested, use `"a".repeats()`. --- src/key.rs | 2 +- src/schnorr.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/key.rs b/src/key.rs index 74aeaa3..4ffc117 100644 --- a/src/key.rs +++ b/src/key.rs @@ -1875,7 +1875,7 @@ mod test { assert!(PublicKey::from_str("0218845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd1").is_err()); assert!(PublicKey::from_str("xx0218845781f631c48f1c9709e23092067d06837f30aa0cd0544ac887fe91ddd1").is_err()); - let long_str: String = core::iter::repeat('a').take(1024 * 1024).collect(); + let long_str = "a".repeat(1024 * 1024); assert!(SecretKey::from_str(&long_str).is_err()); assert!(PublicKey::from_str(&long_str).is_err()); } diff --git a/src/schnorr.rs b/src/schnorr.rs index abd95de..f5f602d 100644 --- a/src/schnorr.rs +++ b/src/schnorr.rs @@ -505,7 +505,7 @@ mod tests { ) .is_err()); - let long_str: String = core::iter::repeat('a').take(1024 * 1024).collect(); + let long_str: String = "a".repeat(1024 * 1024); assert!(XOnlyPublicKey::from_str(&long_str).is_err()); } From 9f1ebb93cba87f5d43702a138cdce658fbdd511d Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 10 Jun 2022 12:57:30 +1000 Subject: [PATCH 08/10] Allow nonminimal_bool in unit test We are explicitly testing various boolean statements, tell clippy to allow less than minimal statements. --- src/key.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/key.rs b/src/key.rs index 4ffc117..b9154a9 100644 --- a/src/key.rs +++ b/src/key.rs @@ -2077,6 +2077,7 @@ mod test { #[cfg(not(fuzzing))] #[test] + #[allow(clippy::nonminimal_bool)] fn pubkey_equal() { let pk1 = PublicKey::from_slice( &hex!("0241cc121c419921942add6db6482fb36243faf83317c866d2a28d8c6d7089f7ba"), From 6d76bd4a8952f0a03f204b8096eaf6f0dc5d40d3 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 10 Jun 2022 12:49:58 +1000 Subject: [PATCH 09/10] Add clippy to CI In order to keep the codebase linting cleanly add a CI job to run clippy. --- .github/workflows/rust.yml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 88f9467..d21d56e 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -73,3 +73,19 @@ jobs: env: DO_WASM: true run: ./contrib/test.sh + + Clippy: + name: Clippy + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: actions-rs/toolchain@v1 + with: + profile: minimal + toolchain: stable + override: true + - run: rustup component add clippy + - uses: actions-rs/cargo@v1 + with: + command: clippy + args: --features=rand-std,recovery,lowmemory,global-context --all-targets -- -D warnings From 65186e732ac229575afd83f5c11bfb794ebc3ca8 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 10 Jun 2022 12:58:39 +1000 Subject: [PATCH 10/10] Add githooks Add `githooks` directory and: - Copy the default pre-commit hook into new githooks directory - Add a call to clippy to the pre-commit hook - Add a section to the README instructing devs how to use the new githooks --- README.md | 11 ++++++++++ githooks/pre-commit | 50 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100755 githooks/pre-commit diff --git a/README.md b/README.md index a2c85b2..bd19fb6 100644 --- a/README.md +++ b/README.md @@ -22,6 +22,17 @@ Contributions to this library are welcome. A few guidelines: * No crypto should be implemented in Rust, with the possible exception of hash functions. Cryptographic contributions should be directed upstream to libsecp256k1. * This library should always compile with any combination of features on **Rust 1.41.1**. +### Githooks + +To assist devs in catching errors _before_ running CI we provide some githooks. If you do not +already have locally configured githooks you can use the ones in this repository by running, in the +root directory of the repository: +``` +git config --local core.hooksPath githooks/ +``` + +Alternatively add symlinks in your `.git/hooks` directory to any of the githooks we provide. + ## Fuzzing If you want to fuzz this library, or any library which depends on it, you will diff --git a/githooks/pre-commit b/githooks/pre-commit new file mode 100755 index 0000000..cabd06f --- /dev/null +++ b/githooks/pre-commit @@ -0,0 +1,50 @@ +#!/bin/sh +# +# A hook script to verify what is about to be committed. +# Called by "git commit" with no arguments. The hook should +# exit with non-zero status after issuing an appropriate message if +# it wants to stop the commit. + +if git rev-parse --verify HEAD >/dev/null 2>&1 +then + against=HEAD +else + # Initial commit: diff against an empty tree object + against=$(git hash-object -t tree /dev/null) +fi + +# If you want to allow non-ASCII filenames set this variable to true. +allownonascii=$(git config --bool hooks.allownonascii) + +# Redirect output to stderr. +exec 1>&2 + +# Cross platform projects tend to avoid non-ASCII filenames; prevent +# them from being added to the repository. We exploit the fact that the +# printable range starts at the space character and ends with tilde. +if [ "$allownonascii" != "true" ] && + # Note that the use of brackets around a tr range is ok here, (it's + # even required, for portability to Solaris 10's /usr/bin/tr), since + # the square bracket bytes happen to fall in the designated range. + test $(git diff --cached --name-only --diff-filter=A -z $against | + LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0 +then + cat <<\EOF +Error: Attempt to add a non-ASCII file name. + +This can cause problems if you want to work with people on other platforms. + +To be portable it is advisable to rename the file. + +If you know what you are doing you can disable this check using: + + git config hooks.allownonascii true +EOF + exit 1 +fi + +# If there are whitespace errors, print the offending file names and fail. +git diff-index --check --cached $against -- || exit 1 + +# Check that code lints cleanly. +cargo clippy --features=rand-std,recovery,lowmemory,global-context --all-targets -- -D warnings || exit 1