Merge pull request #14 from tcharding/01-09-cleanup

Do cleanups and fixes

.github/workflows/rust.yml

@@ -19,6 +19,8 @@ jobs:
       - name: Checkout Toolchain
         # https://github.com/dtolnay/rust-toolchain
         uses: dtolnay/rust-toolchain@stable
+      - name: Install clippy
+        run: rustup component add clippy
       - name: Running test script
         env:
           DO_LINT: true

Cargo.toml

@@ -55,3 +55,4 @@ required-features = ["std"]
 
 [[example]]
 name = "v2-separate-creator-constructor"
+required-features = ["std"]

contrib/test.sh

@@ -41,7 +41,7 @@ done
 
 cargo run --example v0
 cargo run --example v2
-cargo clippy --example v2-separate-creator-constructor
+cargo run --example v2-separate-creator-constructor
 
 if [ "$DO_NO_STD" = true ]
 then

src/lib.rs

@@ -10,14 +10,9 @@
 
 #![cfg_attr(all(not(feature = "std"), not(test)), no_std)]
 // Experimental features we need.
-#![cfg_attr(bench, feature(test))]
 #![cfg_attr(docsrs, feature(doc_auto_cfg))]
 // Coding conventions
 #![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");

src/macros.rs

@@ -1,5 +1,9 @@
 // SPDX-License-Identifier: CC0-1.0
 
+/// Combines the given `$thing` field of two types by setting `self.thing` to be
+/// the value in `other.thing` iff `self.thing` is currently empty.
+///
+/// If `self.thing` already contains a value then this macro does nothing.
 #[allow(unused_macros)]
 macro_rules! combine {
     ($thing:ident, $slf:ident, $other:ident) => {

src/raw.rs

@@ -73,7 +73,7 @@ impl Deserialize for Pair {
 ///
 /// We do not carry the `keylen` around, we just create the `VarInt` length when serializing and
 /// deserializing.
-#[derive(Debug, PartialEq, Hash, Eq, Clone, Ord, PartialOrd)]
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
 pub struct Key {
@@ -136,7 +136,7 @@ pub type ProprietaryType = u8;
 
 /// Proprietary keys (i.e. keys starting with 0xFC byte) with their internal
 /// structure according to BIP 174.
-#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
 pub struct ProprietaryKey<Subtype = ProprietaryType>
@@ -171,7 +171,8 @@ where
     /// Constructs a [`ProprietaryKey`] from a [`Key`].
     ///
     /// # Errors
-    /// Returns [`Error::InvalidProprietaryKey`] if `key` does not start with `0xFC` byte.
+    ///
+    /// Returns [`serialize::Error::InvalidProprietaryKey`] if `key` does not start with `0xFC`.
     fn try_from(key: Key) -> Result<Self, Self::Error> {
         if key.type_value != 0xFC {
             return Err(serialize::Error::InvalidProprietaryKey);

src/serde_utils.rs

@@ -3,7 +3,6 @@
 //! Bitcoin serde utilities.
 //!
 //! This module is for special serde serializations.
-//!
 
 use crate::serde;
 

src/serialize.rs

@@ -4,15 +4,12 @@
 //!
 //! Traits to serialize PSBT values to and from raw bytes
 //! according to the BIP-174 specification.
-//!
 
 use core::convert::{TryFrom, TryInto};
 use core::fmt;
 
 use bitcoin::bip32::{ChildNumber, Fingerprint, KeySource};
-// TODO: This should be exposed like this in rust-bitcoin.
-use bitcoin::consensus::encode as consensus;
-use bitcoin::consensus::{deserialize_partial, serialize, Decodable, Encodable};
+use bitcoin::consensus::{self, Decodable, Encodable};
 use bitcoin::hashes::{self, hash160, ripemd160, sha256, sha256d, Hash};
 use bitcoin::key::PublicKey;
 use bitcoin::secp256k1::{self, XOnlyPublicKey};
@@ -128,7 +125,7 @@ impl Serialize for KeySource {
         rv.append(&mut self.0.to_bytes().to_vec());
 
         for cnum in self.1.into_iter() {
-            rv.append(&mut serialize(&u32::from(*cnum)))
+            rv.append(&mut consensus::serialize(&u32::from(*cnum)))
         }
 
         rv
@@ -157,7 +154,7 @@ impl Deserialize for KeySource {
 }
 
 impl Serialize for u32 {
-    fn serialize(&self) -> Vec<u8> { serialize(&self) }
+    fn serialize(&self) -> Vec<u8> { consensus::serialize(&self) }
 }
 
 impl Deserialize for u32 {
@@ -168,7 +165,7 @@ impl Deserialize for u32 {
 }
 
 impl Serialize for Sequence {
-    fn serialize(&self) -> Vec<u8> { serialize(&self) }
+    fn serialize(&self) -> Vec<u8> { consensus::serialize(&self) }
 }
 
 impl Deserialize for Sequence {
@@ -179,7 +176,7 @@ impl Deserialize for Sequence {
 }
 
 impl Serialize for absolute::Height {
-    fn serialize(&self) -> Vec<u8> { serialize(&self.to_consensus_u32()) }
+    fn serialize(&self) -> Vec<u8> { consensus::serialize(&self.to_consensus_u32()) }
 }
 
 impl Deserialize for absolute::Height {
@@ -191,7 +188,7 @@ impl Deserialize for absolute::Height {
 }
 
 impl Serialize for absolute::Time {
-    fn serialize(&self) -> Vec<u8> { serialize(&self.to_consensus_u32()) }
+    fn serialize(&self) -> Vec<u8> { consensus::serialize(&self.to_consensus_u32()) }
 }
 
 impl Deserialize for absolute::Time {
@@ -212,7 +209,7 @@ impl Deserialize for Vec<u8> {
 }
 
 impl Serialize for PsbtSighashType {
-    fn serialize(&self) -> Vec<u8> { serialize(&self.to_u32()) }
+    fn serialize(&self) -> Vec<u8> { consensus::serialize(&self.to_u32()) }
 }
 
 impl Deserialize for PsbtSighashType {
@@ -316,7 +313,7 @@ impl Serialize for (Vec<TapLeafHash>, KeySource) {
 
 impl Deserialize for (Vec<TapLeafHash>, KeySource) {
     fn deserialize(bytes: &[u8]) -> Result<Self, Error> {
-        let (leafhash_vec, consumed) = deserialize_partial::<Vec<TapLeafHash>>(bytes)?;
+        let (leafhash_vec, consumed) = consensus::deserialize_partial::<Vec<TapLeafHash>>(bytes)?;
         let key_source = KeySource::deserialize(&bytes[consumed..])?;
         Ok((leafhash_vec, key_source))
     }
@@ -352,7 +349,8 @@ impl Deserialize for TapTree {
         let mut bytes_iter = bytes.iter();
         while let Some(depth) = bytes_iter.next() {
             let version = bytes_iter.next().ok_or(Error::Taproot("Invalid Taproot Builder"))?;
-            let (script, consumed) = deserialize_partial::<ScriptBuf>(bytes_iter.as_slice())?;
+            let (script, consumed) =
+                consensus::deserialize_partial::<ScriptBuf>(bytes_iter.as_slice())?;
             if consumed > 0 {
                 bytes_iter.nth(consumed - 1);
             }
@@ -369,7 +367,8 @@ impl Deserialize for TapTree {
 // Helper function to compute key source len
 fn key_source_len(key_source: &KeySource) -> usize { 4 + 4 * (key_source.1).as_ref().len() }
 
-// TODO: Once all we use `DeserializePsbtError` and `DecodeError` remove all the decoding and PSBT top level variasts (magic, separator, etc.).
+// TODO: This error is still too general but splitting it up is
+// non-trivial because it is returned by the Deserialize trait.
 /// Ways that deserializing a PSBT might fail.
 #[derive(Debug)]
 #[non_exhaustive]
@@ -385,7 +384,7 @@ pub enum Error {
     /// Invalid hash when parsing slice.
     InvalidHash(hashes::FromSliceError),
     /// Serialization error in bitcoin consensus-encoded structures
-    ConsensusEncoding(consensus::Error),
+    ConsensusEncoding(consensus::encode::Error),
     /// Parsing error indicating invalid public keys
     InvalidPublicKey(bitcoin::key::Error),
     /// Parsing error indicating invalid secp256k1 public keys
@@ -474,8 +473,8 @@ impl From<hashes::FromSliceError> for Error {
     fn from(e: hashes::FromSliceError) -> Self { Self::InvalidHash(e) }
 }
 
-impl From<consensus::Error> for Error {
-    fn from(e: consensus::Error) -> Self { Self::ConsensusEncoding(e) }
+impl From<consensus::encode::Error> for Error {
+    fn from(e: consensus::encode::Error) -> Self { Self::ConsensusEncoding(e) }
 }
 
 impl From<absolute::Error> for Error {

src/v0/error.rs

@@ -7,6 +7,7 @@ use core::fmt;
 use bitcoin::{sighash, FeeRate, Transaction};
 
 use crate::error::{write_err, InconsistentKeySourcesError};
+use crate::prelude::Box;
 use crate::v0::map::{global, input, output};
 use crate::v0::Psbt;
 
@@ -309,9 +310,9 @@ pub enum CombineError {
     /// Attempting to combine with a PSBT describing a different unsigned transaction.
     UnexpectedUnsignedTx {
         /// Expected transaction.
-        expected: Transaction,
+        expected: Box<Transaction>,
         /// Actual transaction.
-        actual: Transaction,
+        actual: Box<Transaction>,
     },
     /// Global extended public key has inconsistent key sources.
     InconsistentKeySources(InconsistentKeySourcesError),

src/v0/map/global.rs

@@ -24,7 +24,7 @@ use crate::version::Version;
 use crate::{consts, raw, serialize, V0};
 
 /// The global key-value map.
-#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
 pub struct Global {
@@ -235,8 +235,8 @@ impl Global {
     pub fn combine(&mut self, other: Self) -> Result<(), CombineError> {
         if self.unsigned_tx != other.unsigned_tx {
             return Err(CombineError::UnexpectedUnsignedTx {
-                expected: self.unsigned_tx.clone(),
-                actual: other.unsigned_tx,
+                expected: Box::new(self.unsigned_tx.clone()),
+                actual: Box::new(other.unsigned_tx),
             });
         }
 

src/v0/map/input.rs

@@ -28,7 +28,7 @@ use crate::{consts, io, raw, serialize};
 
 /// A key-value map for an input of the corresponding index in the unsigned
 /// transaction.
-#[derive(Clone, Default, Debug, PartialEq, Eq, Hash)]
+#[derive(Debug, Default, Clone, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
 pub struct Input {

src/v0/map/output.rs

@@ -20,7 +20,7 @@ use crate::{consts, io, raw, serialize};
 
 /// A key-value map for an output of the corresponding index in the unsigned
 /// transaction.
-#[derive(Clone, Default, Debug, PartialEq, Eq, Hash)]
+#[derive(Debug, Default, Clone, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
 pub struct Output {

src/v0/miniscript/error.rs

@@ -209,7 +209,7 @@ impl From<bitcoin::key::Error> for InputError {
 }
 
 /// Return error type for [`Psbt::update_input_with_descriptor`]
-#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub enum UtxoUpdateError {
     /// Index out of bounds
     IndexOutOfBounds(usize, usize),
@@ -258,7 +258,7 @@ impl std::error::Error for UtxoUpdateError {
 }
 
 /// Return error type for [`Psbt::update_output_with_descriptor`]
-#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub enum OutputUpdateError {
     /// Index out of bounds
     IndexOutOfBounds(usize, usize),

src/v0/miniscript/mod.rs

@@ -1118,7 +1118,7 @@ fn update_item_with_descriptor_helper<F: PsbtFields>(
 }
 
 /// Sighash message(signing data) for a given psbt transaction input.
-#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub enum PsbtSighashMsg {
     /// Taproot Signature hash
     TapSighash(sighash::TapSighash),

src/v0/mod.rs

@@ -510,12 +510,6 @@ impl Psbt {
     ///
     /// 'Fee' being the amount that will be paid for mining a transaction with the current inputs
     /// and outputs i.e., the difference in value of the total inputs and the total outputs.
-    ///
-    /// ## Errors
-    ///
-    /// - [`Error::MissingUtxo`] when UTXO information for any input is not present or is invalid.
-    /// - [`Error::NegativeFee`] if calculated value is negative.
-    /// - [`Error::FeeOverflow`] if an integer overflow occurs.
     pub fn fee(&self) -> Result<Amount, FeeError> {
         use FeeError::*;
 
@@ -592,7 +586,7 @@ mod display_from_str {
 }
 
 /// Data required to call [`GetKey`] to get the private key to sign an input.
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 #[non_exhaustive]
 pub enum KeyRequest {
     /// Request a private key using the associated public key.
@@ -740,7 +734,7 @@ impl From<bip32::Error> for GetKeyError {
 }
 
 /// The various output types supported by the Bitcoin network.
-#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 #[non_exhaustive]
 pub enum OutputType {
     /// An output of type: pay-to-pubkey or pay-to-pubkey-hash.
@@ -772,7 +766,7 @@ impl OutputType {
 }
 
 /// Signing algorithms supported by the Bitcoin network.
-#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub enum SigningAlgorithm {
     /// The Elliptic Curve Digital Signature Algorithm (see [wikipedia]).
     ///

src/v2/map/global.rs

@@ -32,7 +32,7 @@ const OUTPUTS_MODIFIABLE: u8 = 0x01 << 1;
 const SIGHASH_SINGLE: u8 = 0x01 << 2;
 
 /// The global key-value map.
-#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
 pub struct Global {
@@ -127,6 +127,8 @@ impl Global {
         self.tx_modifiable_flags & OUTPUTS_MODIFIABLE > 0
     }
 
+    // TODO: Use this function?
+    #[allow(dead_code)]
     pub(crate) fn has_sighash_single(&self) -> bool {
         self.tx_modifiable_flags & SIGHASH_SINGLE > 0
     }
@@ -296,7 +298,7 @@ impl Global {
                         return Err(InsertPairError::InvalidKeyDataEmpty(pair.key));
                     },
                 v if v == PSBT_GLOBAL_UNSIGNED_TX =>
-                    return Err(InsertPairError::ExcludedKey { key_type_value: v }.into()),
+                    return Err(InsertPairError::ExcludedKey { key_type_value: v }),
                 _ => match unknowns.entry(pair.key) {
                     btree_map::Entry::Vacant(empty_key) => {
                         empty_key.insert(pair.value);

src/v2/map/input.rs

@@ -33,7 +33,7 @@ use crate::{io, raw, serialize, v0};
 
 /// A key-value map for an input of the corresponding index in the unsigned
 /// transaction.
-#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
 pub struct Input {
@@ -839,13 +839,15 @@ mod test {
         // assert_eq!(back.taproot_hash_ty(), Err(InvalidSighashTypeError(nonstd)));
     }
 
+    #[cfg(feature = "std")]
     fn out_point() -> OutPoint {
         let txid = Txid::hash(b"some arbitrary bytes");
         let vout = 0xab;
         OutPoint { txid, vout }
     }
 
     #[test]
+    #[cfg(feature = "std")]
     fn serialize_roundtrip() {
         let input = Input::new(out_point());
 

src/v2/map/output.rs

@@ -21,7 +21,7 @@ use crate::{io, raw, serialize, v0};
 
 /// A key-value map for an output of the corresponding index in the unsigned
 /// transaction.
-#[derive(Clone, Default, Debug, PartialEq, Eq, Hash)]
+#[derive(Debug, Default, Clone, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
 pub struct Output {
@@ -346,6 +346,7 @@ impl From<serialize::Error> for InsertPairError {
 }
 
 #[cfg(test)]
+#[cfg(feature = "std")]
 mod tests {
     use super::*;