From bf08ee44996dd898f741c755e844cec03d18b95b Mon Sep 17 00:00:00 2001 From: yancy Date: Thu, 19 Oct 2023 09:57:07 +0200 Subject: [PATCH 1/8] Replace helper macro with helper function Remove the macro hex_psbt and replace with a function. Using track_caller to accuretly report the test name and line number during a panic is used in place of a macro. --- bitcoin/tests/psbt.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/bitcoin/tests/psbt.rs b/bitcoin/tests/psbt.rs index 94d58c9d65..f9d5abf8a2 100644 --- a/bitcoin/tests/psbt.rs +++ b/bitcoin/tests/psbt.rs @@ -26,10 +26,10 @@ macro_rules! hex_script { }; } -macro_rules! hex_psbt { - ($s:expr) => { - Psbt::deserialize(& as FromHex>::from_hex($s).unwrap()) - }; +#[track_caller] +fn hex_psbt(s: &str) -> Psbt { + let v: Vec = Vec::from_hex(s).expect("valid hex digits"); + Psbt::deserialize(&v).expect("valid magic and valid separators") } #[test] @@ -206,8 +206,7 @@ fn create_transaction() -> Transaction { fn create_psbt(tx: Transaction) -> Psbt { // String from BIP 174 test vector. let expected_psbt_hex = include_str!("data/create_psbt_hex"); - let expected_psbt = hex_psbt!(expected_psbt_hex).unwrap(); - + let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); let psbt = Psbt::from_unsigned_tx(tx).unwrap(); assert_eq!(psbt, expected_psbt); @@ -236,7 +235,7 @@ fn update_psbt(mut psbt: Psbt, fingerprint: Fingerprint) -> Psbt { ]; let expected_psbt_hex = include_str!("data/update_1_psbt_hex"); - let expected_psbt = hex_psbt!(expected_psbt_hex).unwrap(); + let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); let mut input_0 = psbt.inputs[0].clone(); @@ -293,7 +292,7 @@ fn bip32_derivation( /// Does the second update according to the BIP, returns the newly updated PSBT. Verifies against BIP 174 test vector. fn update_psbt_with_sighash_all(mut psbt: Psbt) -> Psbt { let expected_psbt_hex = include_str!("data/update_2_psbt_hex"); - let expected_psbt = hex_psbt!(expected_psbt_hex).unwrap(); + let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); let ty = PsbtSighashType::from_str("SIGHASH_ALL").unwrap(); @@ -333,7 +332,7 @@ fn parse_and_verify_keys( /// Does the first signing according to the BIP, returns the signed PSBT. Verifies against BIP 174 test vector. fn signer_one_sign(psbt: Psbt, key_map: BTreeMap) -> Psbt { let expected_psbt_hex = include_str!("data/sign_1_psbt_hex"); - let expected_psbt = hex_psbt!(expected_psbt_hex).unwrap(); + let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); let psbt = sign(psbt, key_map); @@ -344,7 +343,7 @@ fn signer_one_sign(psbt: Psbt, key_map: BTreeMap /// Does the second signing according to the BIP, returns the signed PSBT. Verifies against BIP 174 test vector. fn signer_two_sign(psbt: Psbt, key_map: BTreeMap) -> Psbt { let expected_psbt_hex = include_str!("data/sign_2_psbt_hex"); - let expected_psbt = hex_psbt!(expected_psbt_hex).unwrap(); + let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); let psbt = sign(psbt, key_map); @@ -355,7 +354,7 @@ fn signer_two_sign(psbt: Psbt, key_map: BTreeMap /// Does the combine according to the BIP, returns the combined PSBT. Verifies against BIP 174 test vector. fn combine(mut this: Psbt, that: Psbt) -> Psbt { let expected_psbt_hex = include_str!("data/combine_psbt_hex"); - let expected_psbt = hex_psbt!(expected_psbt_hex).unwrap(); + let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); this.combine(that).expect("failed to combine PSBTs"); @@ -367,7 +366,7 @@ fn combine(mut this: Psbt, that: Psbt) -> Psbt { /// test vector. fn finalize(psbt: Psbt) -> Psbt { let expected_psbt_hex = include_str!("data/finalize_psbt_hex"); - let expected_psbt = hex_psbt!(expected_psbt_hex).unwrap(); + let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); let psbt = finalize_psbt(psbt); @@ -394,7 +393,7 @@ fn combine_lexicographically() { let psbt_2_hex = include_str!("data/lex_psbt_2_hex"); let expected_psbt_hex = include_str!("data/lex_combine_psbt_hex"); - let expected_psbt = hex_psbt!(expected_psbt_hex).unwrap(); + let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); let v = Vec::from_hex(psbt_1_hex).unwrap(); let mut psbt_1 = Psbt::deserialize(&v).expect("failed to deserialize psbt 1"); From 7f26439e2036a9914c23b9f61a0f809534ef7566 Mon Sep 17 00:00:00 2001 From: yancy Date: Fri, 20 Oct 2023 09:41:01 +0200 Subject: [PATCH 2/8] Add track_caller to test helper functions The test helper files can panic when calling hex_psbt(). hex_psbt() will report the calling function, instead of the offending test. Adding track_caller to functions that call hex_psbt will report the line number of the failing test. --- bitcoin/tests/psbt.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/bitcoin/tests/psbt.rs b/bitcoin/tests/psbt.rs index f9d5abf8a2..f25f46e222 100644 --- a/bitcoin/tests/psbt.rs +++ b/bitcoin/tests/psbt.rs @@ -203,6 +203,7 @@ fn create_transaction() -> Transaction { } /// Creates the initial PSBT, called by the Creator. Verifies against BIP 174 test vector. +#[track_caller] fn create_psbt(tx: Transaction) -> Psbt { // String from BIP 174 test vector. let expected_psbt_hex = include_str!("data/create_psbt_hex"); @@ -214,6 +215,7 @@ fn create_psbt(tx: Transaction) -> Psbt { } /// Updates `psbt` according to the BIP, returns the newly updated PSBT. Verifies against BIP 174 test vector. +#[track_caller] fn update_psbt(mut psbt: Psbt, fingerprint: Fingerprint) -> Psbt { // Strings from BIP 174 test vector. let previous_tx_0 = include_str!("data/previous_tx_0_hex"); @@ -290,6 +292,7 @@ fn bip32_derivation( } /// Does the second update according to the BIP, returns the newly updated PSBT. Verifies against BIP 174 test vector. +#[track_caller] fn update_psbt_with_sighash_all(mut psbt: Psbt) -> Psbt { let expected_psbt_hex = include_str!("data/update_2_psbt_hex"); let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); @@ -330,6 +333,7 @@ fn parse_and_verify_keys( } /// Does the first signing according to the BIP, returns the signed PSBT. Verifies against BIP 174 test vector. +#[track_caller] fn signer_one_sign(psbt: Psbt, key_map: BTreeMap) -> Psbt { let expected_psbt_hex = include_str!("data/sign_1_psbt_hex"); let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); @@ -341,6 +345,7 @@ fn signer_one_sign(psbt: Psbt, key_map: BTreeMap } /// Does the second signing according to the BIP, returns the signed PSBT. Verifies against BIP 174 test vector. +#[track_caller] fn signer_two_sign(psbt: Psbt, key_map: BTreeMap) -> Psbt { let expected_psbt_hex = include_str!("data/sign_2_psbt_hex"); let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); @@ -352,6 +357,7 @@ fn signer_two_sign(psbt: Psbt, key_map: BTreeMap } /// Does the combine according to the BIP, returns the combined PSBT. Verifies against BIP 174 test vector. +#[track_caller] fn combine(mut this: Psbt, that: Psbt) -> Psbt { let expected_psbt_hex = include_str!("data/combine_psbt_hex"); let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); @@ -364,6 +370,7 @@ fn combine(mut this: Psbt, that: Psbt) -> Psbt { /// Does the finalize step according to the BIP, returns the combined PSBT. Verifies against BIP 174 /// test vector. +#[track_caller] fn finalize(psbt: Psbt) -> Psbt { let expected_psbt_hex = include_str!("data/finalize_psbt_hex"); let expected_psbt: Psbt = hex_psbt(expected_psbt_hex); @@ -388,6 +395,7 @@ fn extract_transaction(psbt: Psbt) -> Transaction { } /// Combines two PSBTs lexicographically according to the BIP. Verifies against BIP 174 test vector. +#[track_caller] fn combine_lexicographically() { let psbt_1_hex = include_str!("data/lex_psbt_1_hex"); let psbt_2_hex = include_str!("data/lex_psbt_2_hex"); From b163d9b59aecb4506705d04457cd2c2111986251 Mon Sep 17 00:00:00 2001 From: yancy Date: Fri, 20 Oct 2023 09:52:59 +0200 Subject: [PATCH 3/8] Replace hex_script macro with a helper function Remove the macro hex_script and replace with a function. Using track_caller to accuretly report the test name and line number during a panic is used in place of a macro. --- bitcoin/tests/psbt.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/bitcoin/tests/psbt.rs b/bitcoin/tests/psbt.rs index f25f46e222..360be93d42 100644 --- a/bitcoin/tests/psbt.rs +++ b/bitcoin/tests/psbt.rs @@ -20,18 +20,15 @@ use bitcoin::{ const NETWORK: Network = Network::Testnet; -macro_rules! hex_script { - ($s:expr) => { - ::from_hex($s).unwrap() - }; -} - #[track_caller] fn hex_psbt(s: &str) -> Psbt { let v: Vec = Vec::from_hex(s).expect("valid hex digits"); Psbt::deserialize(&v).expect("valid magic and valid separators") } +#[track_caller] +fn hex_script(s: &str) -> ScriptBuf { ScriptBuf::from_hex(s).expect("valid hex digits") } + #[test] fn bip174_psbt_workflow() { let secp = Secp256k1::new(); @@ -244,7 +241,7 @@ fn update_psbt(mut psbt: Psbt, fingerprint: Fingerprint) -> Psbt { let v = Vec::from_hex(previous_tx_1).unwrap(); let tx: Transaction = deserialize(&v).unwrap(); input_0.non_witness_utxo = Some(tx); - input_0.redeem_script = Some(hex_script!(redeem_script_0)); + input_0.redeem_script = Some(hex_script(redeem_script_0)); input_0.bip32_derivation = bip32_derivation(fingerprint, &pk_path, vec![0, 1]); let mut input_1 = psbt.inputs[1].clone(); @@ -253,8 +250,8 @@ fn update_psbt(mut psbt: Psbt, fingerprint: Fingerprint) -> Psbt { let tx: Transaction = deserialize(&v).unwrap(); input_1.witness_utxo = Some(tx.output[1].clone()); - input_1.redeem_script = Some(hex_script!(redeem_script_1)); - input_1.witness_script = Some(hex_script!(witness_script)); + input_1.redeem_script = Some(hex_script(redeem_script_1)); + input_1.witness_script = Some(hex_script(witness_script)); input_1.bip32_derivation = bip32_derivation(fingerprint, &pk_path, vec![2, 3]); psbt.inputs = vec![input_0, input_1]; From d391ada5b82a2e744a30d7ada79a1e2894ad544c Mon Sep 17 00:00:00 2001 From: Einherjar Date: Sat, 21 Oct 2023 10:37:24 -0300 Subject: [PATCH 4/8] ci: nightly rustfmt PR scheduled/manual --- .github/workflows/rust.yml | 2 +- .github/workflows/rustfmt.yml | 26 ++++++++++++++++++++++++++ CONTRIBUTING.md | 6 ------ 3 files changed, 27 insertions(+), 7 deletions(-) create mode 100644 .github/workflows/rustfmt.yml diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 2e7bcadeac..a59589ef50 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -58,7 +58,7 @@ jobs: uses: dtolnay/rust-toolchain@nightly - name: Running test script env: - DO_FMT: true + DO_FMT: false DO_BENCH: true AS_DEPENDENCY: false DO_NO_STD: true diff --git a/.github/workflows/rustfmt.yml b/.github/workflows/rustfmt.yml new file mode 100644 index 0000000000..4fb721e07e --- /dev/null +++ b/.github/workflows/rustfmt.yml @@ -0,0 +1,26 @@ +name: Nightly rustfmt +on: + schedule: + - cron: "0 0 * * 0" # runs weekly on Sunday at 00:00 + workflow_dispatch: # allows manual triggering +jobs: + format: + name: Nightly rustfmt + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@nightly + with: + components: rustfmt + - name: Run Nightly rustfmt + run: cargo +nightly fmt + - name: Get the current date + run: echo "date=$(date +'%Y-%m-%d')" >> $GITHUB_ENV + - name: Create Pull Request + uses: peter-evans/create-pull-request@v5 + with: + title: Automated nightly rustfmt (${{ env.date }}) + body: | + Automated nightly `rustfmt` changes by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action + commit-message: ${{ env.date }} automated rustfmt nightly + labels: rustfmt diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ee3087ed73..813c57e74c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -195,12 +195,6 @@ any of the following conditions: Library reflects Bitcoin Core approach whenever possible. -### Formatting - -The repository currently uses `rustfmt` (WIP, some directories are excluded). We use nightly -features so to run the formatter use `cargo +nightly fmt`. (Remember that your editor may be -configured to fmt with a stable toolchain, this will result in many unwanted changes.) - ### Naming conventions Naming of data structures/enums and their fields/variants must follow names used From b7f11d4493f8894ec4c91ea753e900c57e7718d9 Mon Sep 17 00:00:00 2001 From: Steven Roose Date: Mon, 23 Oct 2023 01:36:19 +0100 Subject: [PATCH 5/8] Remove unnecessary clippy attribute on absolute::LockTime I ran the clippy locally without it and it doesn't seem to be necessary anymore. --- bitcoin/src/blockdata/locktime/absolute.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/bitcoin/src/blockdata/locktime/absolute.rs b/bitcoin/src/blockdata/locktime/absolute.rs index 5f5a27bd3b..c6a9e11293 100644 --- a/bitcoin/src/blockdata/locktime/absolute.rs +++ b/bitcoin/src/blockdata/locktime/absolute.rs @@ -66,7 +66,6 @@ pub const LOCK_TIME_THRESHOLD: u32 = 500_000_000; /// _ => panic!("handle invalid comparison error"), /// }; /// ``` -#[allow(clippy::derive_ord_xor_partial_ord)] #[derive(Clone, Copy, PartialEq, Eq, Hash)] pub enum LockTime { /// A block height lock time value. From f522a0290c92663b27d0f62c74517914653e593a Mon Sep 17 00:00:00 2001 From: Steven Roose Date: Mon, 23 Oct 2023 01:37:50 +0100 Subject: [PATCH 6/8] Remove unnecessary clippy attribute on relative::LockTime --- bitcoin/src/blockdata/locktime/relative.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/bitcoin/src/blockdata/locktime/relative.rs b/bitcoin/src/blockdata/locktime/relative.rs index fca20938eb..198091deb3 100644 --- a/bitcoin/src/blockdata/locktime/relative.rs +++ b/bitcoin/src/blockdata/locktime/relative.rs @@ -26,7 +26,6 @@ use crate::relative; /// /// * [BIP 68 Relative lock-time using consensus-enforced sequence numbers](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki) /// * [BIP 112 CHECKSEQUENCEVERIFY](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) -#[allow(clippy::derive_ord_xor_partial_ord)] #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))] From 750ee2ba565e4ea79164b93b3547f7ad85d3e2c1 Mon Sep 17 00:00:00 2001 From: Steven Roose Date: Mon, 23 Oct 2023 01:41:44 +0100 Subject: [PATCH 7/8] Remove unnecessary clippy attribute on is_sighash_single_bug --- bitcoin/src/crypto/sighash.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/bitcoin/src/crypto/sighash.rs b/bitcoin/src/crypto/sighash.rs index 912f87fe72..6dcb6e620c 100644 --- a/bitcoin/src/crypto/sighash.rs +++ b/bitcoin/src/crypto/sighash.rs @@ -1237,7 +1237,6 @@ impl EncodeSigningDataResult { /// // use a hash value of "1", instead of computing the actual hash due to SIGHASH_SINGLE bug /// } /// ``` - #[allow(clippy::wrong_self_convention)] // E is not Copy so we consume self. pub fn is_sighash_single_bug(self) -> Result { match self { EncodeSigningDataResult::SighashSingleBug => Ok(true), From 875545517dd6514b67cd42bd08f00f1256f19e7d Mon Sep 17 00:00:00 2001 From: Steven Roose Date: Fri, 20 Oct 2023 15:58:42 +0100 Subject: [PATCH 8/8] Add clippy exceptions for needless_question_mark lint --- bitcoin/src/lib.rs | 2 ++ hashes/src/lib.rs | 2 ++ internals/src/lib.rs | 2 ++ 3 files changed, 6 insertions(+) diff --git a/bitcoin/src/lib.rs b/bitcoin/src/lib.rs index dfdf4ad0ca..68b7d24633 100644 --- a/bitcoin/src/lib.rs +++ b/bitcoin/src/lib.rs @@ -38,6 +38,8 @@ #![warn(missing_docs)] // Instead of littering the codebase for non-fuzzing code just globally allow. #![cfg_attr(fuzzing, allow(dead_code, unused_imports))] +// Exclude clippy lints we don't think are valuable +#![allow(clippy::needless_question_mark)] // https://github.com/rust-bitcoin/rust-bitcoin/pull/2134 #[cfg(not(any(feature = "std", feature = "no-std")))] compile_error!("at least one of the `std` or `no-std` features must be enabled"); diff --git a/hashes/src/lib.rs b/hashes/src/lib.rs index 667d2df912..aa1c6ffc42 100644 --- a/hashes/src/lib.rs +++ b/hashes/src/lib.rs @@ -78,6 +78,8 @@ #![cfg_attr(all(not(test), not(feature = "std")), no_std)] // Instead of littering the codebase for non-fuzzing code just globally allow. #![cfg_attr(hashes_fuzz, allow(dead_code, unused_imports))] +// Exclude clippy lints we don't think are valuable +#![allow(clippy::needless_question_mark)] // https://github.com/rust-bitcoin/rust-bitcoin/pull/2134 #[cfg(all(not(test), not(feature = "std"), feature = "core2"))] extern crate actual_core2 as core2; diff --git a/internals/src/lib.rs b/internals/src/lib.rs index a024e273a4..b81da6b84e 100644 --- a/internals/src/lib.rs +++ b/internals/src/lib.rs @@ -11,6 +11,8 @@ #![cfg_attr(docsrs, feature(doc_auto_cfg))] // Coding conventions #![warn(missing_docs)] +// Exclude clippy lints we don't think are valuable +#![allow(clippy::needless_question_mark)] // https://github.com/rust-bitcoin/rust-bitcoin/pull/2134 #[cfg(feature = "alloc")] extern crate alloc;