refactor: address code review feedback

- Rename MAX_SEQUENCE_VALUE to MAX_RELATIVE_HEIGHT for clarity
- Use Script::is_p2tr() for more robust taproot input detection
- Replace fallible LockTime::from_height with expect for valid Height
- Remove unnecessary clippy allow attributes in lib.rs
- Remove Box usage from error and enums
- Rename anti_fee_snipping example to anti_fee_sniping

Author: aagbotemi

Cargo.toml

@@ -35,4 +35,4 @@ name = "common"
 crate-type = ["lib"]
 
 [[example]]
-name = "anti_fee_snipping"
+name = "anti_fee_sniping"

examples/anti_fee_snipping.rs renamed to examples/anti_fee_sniping.rs

@@ -1,4 +1,3 @@
-use alloc::boxed::Box;
 use alloc::sync::Arc;
 use alloc::vec::Vec;
 use core::fmt;
@@ -35,9 +34,9 @@ impl TxStatus {
 
 #[derive(Debug, Clone)]
 enum PlanOrPsbtInput {
-    Plan(Box<Plan>),
+    Plan(Plan),
     PsbtInput {
-        psbt_input: Box<psbt::Input>,
+        psbt_input: psbt::Input,
         sequence: Sequence,
         absolute_timelock: absolute::LockTime,
         satisfaction_weight: usize,
@@ -58,7 +57,7 @@ impl PlanOrPsbtInput {
             return Err(FromPsbtInputError::UtxoCheck);
         }
         Ok(Self::PsbtInput {
-            psbt_input: Box::new(psbt_input),
+            psbt_input,
             sequence,
             absolute_timelock: absolute::LockTime::ZERO,
             satisfaction_weight,
@@ -217,7 +216,7 @@ impl Input {
             prev_outpoint: OutPoint::new(tx.compute_txid(), output_index as _),
             prev_txout: tx.tx_out(output_index).cloned()?,
             prev_tx: Some(tx),
-            plan: PlanOrPsbtInput::Plan(Box::new(plan)),
+            plan: PlanOrPsbtInput::Plan(plan),
             status,
             is_coinbase,
         })
@@ -235,7 +234,7 @@ impl Input {
             prev_outpoint,
             prev_txout,
             prev_tx: None,
-            plan: PlanOrPsbtInput::Plan(Box::new(plan)),
+            plan: PlanOrPsbtInput::Plan(plan),
             status,
             is_coinbase,
         }

src/input.rs

@@ -1,8 +1,4 @@
 //! `bdk_tx`
-
-// FIXME: try to remove clippy "allows"
-#![allow(clippy::large_enum_variant)]
-#![allow(clippy::result_large_err)]
 #![warn(missing_docs)]
 #![no_std]
 

src/lib.rs

@@ -1,5 +1,3 @@
-use alloc::boxed::Box;
-
 use bitcoin::{Amount, ScriptBuf, TxOut};
 use miniscript::bitcoin;
 
@@ -11,7 +9,7 @@ pub enum ScriptSource {
     /// bitcoin script
     Script(ScriptBuf),
     /// definite descriptor
-    Descriptor(Box<DefiniteDescriptor>),
+    Descriptor(DefiniteDescriptor),
 }
 
 impl From<ScriptBuf> for ScriptSource {
@@ -34,7 +32,7 @@ impl ScriptSource {
 
     /// From descriptor
     pub fn from_descriptor(descriptor: DefiniteDescriptor) -> Self {
-        Self::Descriptor(Box::new(descriptor))
+        Self::Descriptor(descriptor)
     }
 
     /// To ScriptBuf

src/output.rs

@@ -1,12 +1,13 @@
-use alloc::{boxed::Box, vec::Vec};
+use alloc::vec::Vec;
 use core::fmt::{Debug, Display};
 
 use bdk_coin_select::FeeRate;
-use bitcoin::{
+use miniscript::bitcoin;
+use miniscript::bitcoin::{
     absolute::{self, LockTime},
-    transaction, Psbt, Sequence,
+    transaction, OutPoint, Psbt, Sequence,
 };
-use miniscript::bitcoin;
+
 use miniscript::psbt::PsbtExt;
 
 use crate::{apply_anti_fee_sniping, Finalizer, Input, Output};
@@ -70,17 +71,15 @@ pub enum CreatePsbtError {
     /// Attempted to mix locktime types.
     LockTypeMismatch,
     /// Missing tx for legacy input.
-    MissingFullTxForLegacyInput(Box<Input>),
+    MissingFullTxForLegacyInput(OutPoint),
     /// Missing tx for segwit v0 input.
-    MissingFullTxForSegwitV0Input(Box<Input>),
+    MissingFullTxForSegwitV0Input(OutPoint),
     /// Psbt error.
     Psbt(bitcoin::psbt::Error),
     /// Update psbt output with descriptor error.
     OutputUpdate(miniscript::psbt::OutputUpdateError),
     /// Invalid locktime
     InvalidLockTime(absolute::LockTime),
-    /// Invalid height
-    InvalidHeight(u32),
     /// Unsupported version for anti fee snipping
     UnsupportedVersion(transaction::Version),
 }
@@ -89,15 +88,15 @@ impl core::fmt::Display for CreatePsbtError {
     fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
         match self {
             CreatePsbtError::LockTypeMismatch => write!(f, "cannot mix locktime units"),
-            CreatePsbtError::MissingFullTxForLegacyInput(input) => write!(
+            CreatePsbtError::MissingFullTxForLegacyInput(outpoint) => write!(
                 f,
                 "legacy input that spends {} requires PSBT_IN_NON_WITNESS_UTXO",
-                input.prev_outpoint()
+                outpoint
             ),
-            CreatePsbtError::MissingFullTxForSegwitV0Input(input) => write!(
+            CreatePsbtError::MissingFullTxForSegwitV0Input(outpoint) => write!(
                 f,
                 "segwit v0 input that spends {} requires PSBT_IN_NON_WITNESS_UTXO",
-                input.prev_outpoint()
+                outpoint
             ),
             CreatePsbtError::Psbt(error) => Display::fmt(&error, f),
             CreatePsbtError::OutputUpdate(output_update_error) => {
@@ -106,9 +105,6 @@ impl core::fmt::Display for CreatePsbtError {
             CreatePsbtError::InvalidLockTime(locktime) => {
                 write!(f, "The locktime - {}, is invalid", locktime)
             }
-            CreatePsbtError::InvalidHeight(height) => {
-                write!(f, "The height - {}, is invalid", height)
-            }
             CreatePsbtError::UnsupportedVersion(version) => {
                 write!(f, "Unsupported version {}", version)
             }
@@ -202,16 +198,16 @@ impl Selection {
                 psbt_input.non_witness_utxo = plan_input.prev_tx().cloned();
                 if psbt_input.non_witness_utxo.is_none() {
                     if witness_version.is_none() {
-                        return Err(CreatePsbtError::MissingFullTxForLegacyInput(Box::new(
-                            plan_input.clone(),
-                        )));
+                        return Err(CreatePsbtError::MissingFullTxForLegacyInput(
+                            plan_input.prev_outpoint(),
+                        ));
                     }
                     if params.mandate_full_tx_for_segwit_v0
                         && witness_version == Some(bitcoin::WitnessVersion::V0)
                     {
-                        return Err(CreatePsbtError::MissingFullTxForSegwitV0Input(Box::new(
-                            plan_input.clone(),
-                        )));
+                        return Err(CreatePsbtError::MissingFullTxForSegwitV0Input(
+                            plan_input.prev_outpoint(),
+                        ));
                     }
                 }
                 continue;

src/selection.rs

@@ -3,7 +3,7 @@ use alloc::vec::Vec;
 use miniscript::bitcoin::{
     absolute::{self, LockTime},
     transaction::Version,
-    Sequence, Transaction, WitnessVersion,
+    Sequence, Transaction,
 };
 
 use rand_core::{OsRng, RngCore};
@@ -15,39 +15,41 @@ pub fn apply_anti_fee_sniping(
     current_height: absolute::Height,
     rbf_enabled: bool,
 ) -> Result<(), CreatePsbtError> {
-    const MAX_SEQUENCE_VALUE: u32 = 65_535;
+    const MAX_RELATIVE_HEIGHT: u32 = 65_535;
     const USE_NLOCKTIME_PROBABILITY: u32 = 2;
     const MIN_SEQUENCE_VALUE: u32 = 1;
     const FURTHER_BACK_PROBABILITY: u32 = 10;
-    const MAX_RANDOM_OFFSET: u32 = 99;
+    const MAX_RANDOM_OFFSET: u32 = 100;
 
     let mut rng = OsRng;
 
     if tx.version < Version::TWO {
         return Err(CreatePsbtError::UnsupportedVersion(tx.version));
     }
 
-    let taproot_inputs: Vec<usize> = (0..tx.input.len())
-        .filter(|&idx| {
-            // Check if this input is taproot using the corresponding Input data
-            inputs
-                .get(idx)
-                .and_then(|input| input.plan())
-                .and_then(|plan| plan.witness_version())
-                .map(|version| version == WitnessVersion::V1)
-                .unwrap_or(false)
+    // vector of input_index and associated Input ref.
+    let taproot_inputs: Vec<(usize, &Input)> = tx
+        .input
+        .iter()
+        .enumerate()
+        .filter_map(|(vin, txin)| {
+            let input = inputs
+                .iter()
+                .find(|input| input.prev_outpoint() == txin.previous_output)?;
+            if input.prev_txout().script_pubkey.is_p2tr() {
+                Some((vin, input))
+            } else {
+                None
+            }
         })
         .collect();
 
     // Check always‐locktime conditions
     let must_use_locktime = inputs.iter().any(|input| {
         let confirmation = input.confirmations(current_height);
         confirmation == 0
-            || confirmation > MAX_SEQUENCE_VALUE
-            || !matches!(
-                input.plan().and_then(|plan| plan.witness_version()),
-                Some(WitnessVersion::V1)
-            )
+            || confirmation > MAX_RELATIVE_HEIGHT
+            || !input.prev_txout().script_pubkey.is_p2tr()
     });
 
     let use_locktime = !rbf_enabled
@@ -64,15 +66,15 @@ pub fn apply_anti_fee_sniping(
             locktime = locktime.saturating_sub(random_offset);
         }
 
-        let new_locktime = LockTime::from_height(locktime)
-            .map_err(|_| CreatePsbtError::InvalidHeight(locktime))?;
+        let new_locktime = LockTime::from_height(locktime).expect("must be valid Height");
 
         tx.lock_time = new_locktime;
     } else {
         // Use Sequence
         tx.lock_time = LockTime::ZERO;
-        let input_index = random_range(&mut rng, taproot_inputs.len() as u32) as usize;
-        let confirmation = inputs[input_index].confirmations(current_height);
+        let random_index = random_range(&mut rng, taproot_inputs.len() as u32);
+        let (input_index, input) = taproot_inputs[random_index as usize];
+        let confirmation = input.confirmations(current_height);
 
         let mut sequence_value = confirmation;
         if random_probability(&mut rng, FURTHER_BACK_PROBABILITY) {