Repository: rust-bitcoin/rust-bitcoin
Pull request #1678 (2023-02-27 22:51:35)

Improve the public API
We created the `crypto` crate as a container for cryptography modules with the idea that it may be split out into a separate crate. There is no reason for users of the lib to know about this module. Also, we have two `taproot` modules, one in `crypto` and one at the crate root, this makes for un-ergonomic usage of the lib.

Improve the public API by doing:

- Make the `crypto` module private (`pub(crate)`).
- Re-export `crypto::taproot::Signature` (and `Error`) from `crate::taproot`

Fix: #1668 

tcharding: Includes re-export of some secp types from `taproot` - this is in the name of "expose types that are used together from the same place".
tcharding: Ok, the additional pub re-exports that were commented upon were a mistake by me because they are scope kreep and unrelated to this PR, my bad - all removed. This PR now does _only_ what it claims to do. Good Tobin.

Patch:
diff --git a/bitcoin/examples/taproot-psbt.rs b/bitcoin/examples/taproot-psbt.rs
index 4b77ac05..6fd098eb 100644
--- a/bitcoin/examples/taproot-psbt.rs
+++ b/bitcoin/examples/taproot-psbt.rs
@@ -81,15 +81,12 @@
 use bitcoin::bip32::{ChildNumber, DerivationPath, ExtendedPrivKey, ExtendedPubKey, Fingerprint};
 use bitcoin::consensus::encode;
 use bitcoin::constants::COIN_VALUE;
-use bitcoin::crypto::taproot;
 use bitcoin::key::{TapTweak, XOnlyPublicKey};
 use bitcoin::opcodes::all::{OP_CHECKSIG, OP_CLTV, OP_DROP};
 use bitcoin::psbt::{self, Input, Output, Psbt, PsbtSighashType};
 use bitcoin::secp256k1::Secp256k1;
-use bitcoin::sighash::{self, TapSighashType, SighashCache};
-use bitcoin::taproot::{
-    LeafVersion, TapLeafHash, TapSighash, TaprootBuilder, TaprootSpendInfo,
-};
+use bitcoin::sighash::{self, SighashCache, TapSighash, TapSighashType};
+use bitcoin::taproot::{self, LeafVersion, TapLeafHash, TaprootBuilder, TaprootSpendInfo};
 use bitcoin::{
     absolute, script, Address, Amount, Network, OutPoint, ScriptBuf, Transaction, TxIn, TxOut,  Witness,
 };
diff --git a/bitcoin/src/blockdata/constants.rs b/bitcoin/src/blockdata/constants.rs
index eb0534c8..53fe47ca 100644
--- a/bitcoin/src/blockdata/constants.rs
+++ b/bitcoin/src/blockdata/constants.rs
@@ -54,8 +54,6 @@
 pub const SUBSIDY_HALVING_INTERVAL: u32 = 210_000;
 /// Maximum allowed value for an integer in Script.
 pub const MAX_SCRIPTNUM_VALUE: u32 = 0x80000000; // 2^31
-/// Number of blocks needed for an output from a coinbase transaction to be spendable.
-pub const COINBASE_MATURITY: u32 = 100;
 
 /// The maximum value allowed in an output (useful for sanity checking,
 /// since keeping everything below this value should prevent overflows
diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs
index c5885059..792906c5 100644
--- a/bitcoin/src/blockdata/transaction.rs
+++ b/bitcoin/src/blockdata/transaction.rs
@@ -589,8 +589,8 @@ impl<E> EncodeSigningDataResult<E> {
     ///
     /// ```rust
     /// # use bitcoin::consensus::deserialize;
+    /// # use bitcoin::sighash::{LegacySighash, SighashCache};
     /// # use bitcoin::Transaction;
-    /// # use bitcoin::crypto::sighash::{LegacySighash, SighashCache};
     /// # use bitcoin_hashes::{Hash, hex::FromHex};
     /// # let mut writer = LegacySighash::engine();
     /// # let input_index = 0;
diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs
index e50d8750..e1646864 100644
--- a/bitcoin/src/crypto/key.rs
+++ b/bitcoin/src/crypto/key.rs
@@ -578,7 +578,7 @@ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
 /// # Examples
 /// ```
 /// # #[cfg(feature = "rand-std")] {
-/// # use bitcoin::crypto::key::{KeyPair, TweakedKeyPair, TweakedPublicKey};
+/// # use bitcoin::key::{KeyPair, TweakedKeyPair, TweakedPublicKey};
 /// # use bitcoin::secp256k1::{rand, Secp256k1};
 /// # let secp = Secp256k1::new();
 /// # let keypair = TweakedKeyPair::dangerous_assume_tweaked(KeyPair::new(&secp, &mut rand::thread_rng()));
diff --git a/bitcoin/src/crypto/mod.rs b/bitcoin/src/crypto/mod.rs
index 4c4d5129..a567fc68 100644
--- a/bitcoin/src/crypto/mod.rs
+++ b/bitcoin/src/crypto/mod.rs
@@ -9,4 +9,5 @@
 pub mod ecdsa;
 pub mod key;
 pub mod sighash;
-pub mod taproot;
+// Contents re-exported in `bitcoin::taproot`.
+pub(crate) mod taproot;
diff --git a/bitcoin/src/lib.rs b/bitcoin/src/lib.rs
index 309ae9e8..75a3b69b 100644
--- a/bitcoin/src/lib.rs
+++ b/bitcoin/src/lib.rs
@@ -92,7 +92,8 @@
 pub mod bip32;
 pub mod blockdata;
 pub mod consensus;
-pub mod crypto;
+// Private until we either make this a crate or flatten it - still to be decided.
+pub(crate) mod crypto;
 pub mod error;
 pub mod hash_types;
 pub mod merkle_tree;
@@ -127,7 +128,7 @@
 pub use crate::blockdata::{constants, opcodes};
 pub use crate::consensus::encode::VarInt;
 pub use crate::crypto::key::{self, PrivateKey, PublicKey};
-pub use crate::crypto::sighash;
+pub use crate::crypto::{ecdsa, sighash};
 pub use crate::error::Error;
 pub use crate::hash_types::{Txid, Wtxid, BlockHash, PubkeyHash,  ScriptHash, WPubkeyHash, WScriptHash};
 pub use crate::merkle_tree::MerkleBlock;
diff --git a/bitcoin/src/network/constants.rs b/bitcoin/src/network/constants.rs
index e3bf8a65..aeb96a77 100644
--- a/bitcoin/src/network/constants.rs
+++ b/bitcoin/src/network/constants.rs
@@ -30,14 +30,12 @@
 use core::convert::TryFrom;
 use core::str::FromStr;
 use core::{fmt, ops};
-use core::fmt::Display;
 
 use bitcoin_internals::{debug_from_display, write_err};
 #[cfg(feature = "serde")]
 use serde::{Deserialize, Serialize};
 
 use crate::consensus::encode::{self, Decodable, Encodable};
-use crate::constants::ChainHash;
 use crate::error::impl_std_error;
 use crate::hashes::hex::{Error, FromHex};
 use crate::io;
@@ -143,34 +141,6 @@ pub fn from_core_arg(core_arg: &str) -> Result<Self, ParseNetworkError> {
         };
         Ok(network)
     }
-
-    /// Return the network's chain hash (genesis block hash).
-    ///
-    /// # Examples
-    ///
-    /// ```rust
-    /// use bitcoin::network::constants::Network;
-    /// use bitcoin::blockdata::constants::ChainHash;
-    ///
-    /// let network = Network::Bitcoin;
-    /// assert_eq!(network.chain_hash(), ChainHash::BITCOIN);
-    /// ```
-    pub fn chain_hash(self) -> ChainHash {
-        ChainHash::using_genesis_block(self)
-    }
-
-    /// Creates a `Network` from the chain hash (genesis block hash).
-    ///
-    /// # Examples
-    ///
-    /// ```rust
-    /// use bitcoin::network::constants::Network;
-    /// use bitcoin::blockdata::constants::ChainHash;
-    /// use std::convert::TryFrom;
-    ///
-    /// assert_eq!(Ok(Network::Bitcoin), Network::try_from(ChainHash::BITCOIN));
-    /// ```
-    pub fn from_chain_hash(chain_hash: ChainHash) -> Option<Network> { Network::try_from(chain_hash).ok() }
 }
 
 /// An error in parsing network string.
@@ -216,33 +186,6 @@ fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
     }
 }
 
-/// Error in parsing network from chain hash.
-#[derive(Debug, Clone, PartialEq, Eq)]
-pub struct UnknownChainHash(ChainHash);
-
-impl Display for UnknownChainHash {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "unknown chain hash: {}", self.0)
-    }
-}
-
-impl_std_error!(UnknownChainHash);
-
-impl TryFrom<ChainHash> for Network {
-    type Error = UnknownChainHash;
-
-    fn try_from(chain_hash: ChainHash) -> Result<Self, Self::Error> {
-        match chain_hash {
-            // Note: any new network entries must be matched against here.
-            ChainHash::BITCOIN => Ok(Network::Bitcoin),
-            ChainHash::TESTNET => Ok(Network::Testnet),
-            ChainHash::SIGNET => Ok(Network::Signet),
-            ChainHash::REGTEST => Ok(Network::Regtest),
-            _ => Err(UnknownChainHash(chain_hash)),
-        }
-    }
-}
-
 /// Network magic bytes to identify the cryptocurrency network the message was intended for.
 #[derive(Copy, PartialEq, Eq, PartialOrd, Ord, Clone, Hash)]
 pub struct Magic([u8; 4]);
diff --git a/bitcoin/src/taproot.rs b/bitcoin/src/taproot.rs
index 619db781..e16a41b2 100644
--- a/bitcoin/src/taproot.rs
+++ b/bitcoin/src/taproot.rs
@@ -13,12 +13,13 @@
 use secp256k1::{self, Scalar, Secp256k1};
 
 use crate::consensus::Encodable;
-use crate::crypto::key::{TapTweak, TweakedPublicKey, UntweakedPublicKey, XOnlyPublicKey};
 use crate::hashes::{sha256t_hash_newtype, Hash, HashEngine};
+use crate::crypto::key::{TapTweak, TweakedPublicKey, UntweakedPublicKey, XOnlyPublicKey};
 use crate::prelude::*;
 use crate::{io, Script, ScriptBuf};
 
-pub use crate::crypto::sighash::{TapSighash, TapSighashTag};
+// Re-export these so downstream only has to use one `taproot` module.
+pub use crate::crypto::taproot::{Signature, Error};
 
 /// The SHA-256 midstate value for the TapLeaf hash.
 const MIDSTATE_TAPLEAF: [u8; 32] = [
@@ -1133,6 +1134,7 @@ mod test {
     use crate::hashes::hex::FromHex;
     use crate::hashes::sha256t::Tag;
     use crate::hashes::{sha256, Hash, HashEngine};
+    use crate::sighash::{TapSighash, TapSighashTag};
     use crate::{Address, Network};
     extern crate serde_json;
 
diff --git a/hashes/src/util.rs b/hashes/src/util.rs
index c00f89be..62441a77 100644
--- a/hashes/src/util.rs
+++ b/hashes/src/util.rs
@@ -121,11 +121,11 @@ fn input(&mut self, inp: &[u8]) {
 /// }
 /// ```
 ///
-/// You can use any valid visibility specifier in place of `pub` or you can omit either or both, if
-/// you want the type or its field to be private.
+/// You can use any valid visibility specifier in place of `pub` or you can leave it out if you
+/// don't want the type or its field to be private.
 ///
-/// Whether the hash is reversed or not when displaying depends on the inner type. However you can
-/// override it like this:
+/// Whether the hash is reversed or not depends on the inner type. However you can override it like
+/// this:
 ///
 /// ```
 /// # use bitcoin_hashes::{hash_newtype, sha256};
@@ -156,40 +156,6 @@ fn input(&mut self, inp: &[u8]) {
 ///     struct Newtype2(hash160::Hash);
 /// }
 /// ```
-///
-/// Note: the macro is internally recursive. If you use too many attributes (> 256 tokens) you may
-/// hit recursion limit. If you have so many attributes for a good reason, just raising the limit
-/// should be OK. Note however that attribute-processing part has to use [TT muncher] which has
-/// quadratic complexity, so having many attributes may blow up compile time. This should be rare.
-///
-/// [TT muncher]: https://danielkeep.github.io/tlborm/book/pat-incremental-tt-munchers.html
-///
-// Ever heard of legendary comments warning developers to not touch the code? Yep, here's another
-// one. The following code is written the way it is for some specific reasons. If you think you can
-// simplify it, I suggest spending your time elsewhere.
-//
-// If you looks at the code carefully you might ask these questions:
-//
-// * Why are attributes using `tt` and not `meta`?!
-// * Why are the macros split like that?!
-// * Why use recursion instead of `$()*`?
-//
-// None of these are here by accident. For some reason unknown to me, if you accept an argument to
-// macro with any fragment specifier other than `tt` it will **not** match any of the rules
-// requiring a specific token. Yep, I tried it, I literally got error that `hash_newtype` doesn't
-// match `hash_newtype`. So all input attributes must be `tt`.
-//
-// Originally I wanted to define a bunch of macros that would filter-out hash_type attributes. Then
-// I remembered (by seeing compiler error) that calling macros is not allowed inside attributes.
-// And no, you can't bypass it by calling a helper macro and passing "output of another macro" into
-// it. The whole macro gets passed, not the resulting value. So we have to generate the entire
-// attributes. And you can't just place an attribute-producing macro above struct - they are
-// considered separate items. This is not C.
-//
-// Thus struct is generated in a separate macro together with attributes. And since the macro needs
-// attributes as the input and I didn't want to create confusion by using `#[]` syntax *after*
-// struct, I opted to use `{}` as a separator. Yes, a separator is required because an attribute
-// may be composed of multiple token trees - that's the point of "double repetition".
 #[macro_export]
 macro_rules! hash_newtype {
     ($($(#[$($type_attrs:tt)*])* $type_vis:vis struct $newtype:ident($(#[$field_attrs:tt])* $field_vis:vis $hash:path);)+) => {
@@ -314,16 +280,6 @@ fn index(&self, index: I) -> &Self::Output {
     };
 }
 
-// Generates the struct only (no impls)
-//
-// This is a separate macro to make it more readable and have a separate interface that allows for
-// two groups of type attributes: processed and not-yet-processed ones (think about it like
-// computation via recursion). The macro recursively matches unprocessed attributes, popping them
-// one at a time and either ignoring them (`hash_newtype`) or appending them to the list of
-// processed attributes to be added to the struct.
-//
-// Once the list of not-yet-processed attributes is empty the struct is generated with processed
-// attributes added.
 #[doc(hidden)]
 #[macro_export]
 macro_rules! hash_newtype_struct {
@@ -351,14 +307,13 @@ macro_rules! hash_newtype_struct {
     };
 }
 
-// Extracts `hash_newtype(forward)` and `hash_newtype(backward)` attributes if any and turns them
-// into bool, defaulting to `DISPLAY_BACKWARD` of the wrapped type if the attribute is omitted.
-//
-// Once an appropriate attribute is found we pass the remaining ones into another macro to detect
-// duplicates/conflicts and report an error.
-//
-// FYI, no, we can't use a helper macro to first filter all `hash_newtype` attributes. We would be
-// attempting to match on macros instead. So we must write `hashe_newtype` in each branch.
+#[doc(hidden)]
+#[macro_export]
+macro_rules! hash_newtype_our_attrs {
+    (hash_newtype($($attr:tt)*)) => { $($attr)* };
+    ($($ignore:tt)*) => {};
+}
+
 #[doc(hidden)]
 #[macro_export]
 macro_rules! hash_newtype_get_direction {
@@ -368,9 +323,6 @@ macro_rules! hash_newtype_get_direction {
     ($hash:ty, #[$($ignore:tt)*]  $($others:tt)*) => { $crate::hash_newtype_get_direction!($hash, $($others)*) };
 }
 
-// Reports an error if any of the attributes is `hash_newtype($direction)`.
-//
-// This is used for detection of duplicates/conflicts, see the macro above.
 #[doc(hidden)]
 #[macro_export]
 macro_rules! hash_newtype_forbid_direction {
@@ -386,11 +338,6 @@ macro_rules! hash_newtype_forbid_direction {
     };
 }
 
-// Checks (at compile time) that all `hash_newtype` attributes are known.
-//
-// An unknown attribute could be a typo that could cause problems - e.g. wrong display direction if
-// it's missing. To prevent this, we call this macro above. The macro produces nothing unless an
-// unknown attribute is found in which case it produces `compile_error!`.
 #[doc(hidden)]
 #[macro_export]
 macro_rules! hash_newtype_known_attrs {


Test patch:
diff --git a/bitcoin/tests/serde.rs b/bitcoin/tests/serde.rs
index 254a2016..9c5636d3 100644
--- a/bitcoin/tests/serde.rs
+++ b/bitcoin/tests/serde.rs
@@ -30,16 +30,15 @@
 use bitcoin::blockdata::locktime::{absolute, relative};
 use bitcoin::blockdata::witness::Witness;
 use bitcoin::consensus::encode::deserialize;
-use bitcoin::crypto::key::UntweakedPublicKey;
-use bitcoin::crypto::{ecdsa, taproot};
 use bitcoin::hashes::hex::FromHex;
 use bitcoin::hashes::{hash160, ripemd160, sha256, sha256d, Hash};
+use bitcoin::key::UntweakedPublicKey;
 use bitcoin::psbt::raw::{self, Key, Pair, ProprietaryKey};
 use bitcoin::psbt::{Input, Output, Psbt, PsbtSighashType};
 use bitcoin::sighash::{EcdsaSighashType, TapSighashType};
-use bitcoin::taproot::{ControlBlock, LeafVersion, TaprootBuilder, TaprootSpendInfo};
+use bitcoin::taproot::{self, ControlBlock, LeafVersion, TaprootBuilder, TaprootSpendInfo};
 use bitcoin::{
-    Address, Block, Network, OutPoint, PrivateKey, PublicKey, ScriptBuf, Sequence, Target,
+    ecdsa, Address, Block, Network, OutPoint, PrivateKey, PublicKey, ScriptBuf, Sequence, Target,
     Transaction, TxIn, TxOut, Txid, Work,
 };
 use secp256k1::Secp256k1;