Merge rust-bitcoin/rust-bitcoin#728: Use un/tweaked public key types

b5bf6d7 Improve rustdocs on schnorr module (Tobin Harding)
a6d3514 Return parity when doing tap_tweak (Tobin Harding)
7af0999 Re-name TweakedPublicKey constructor (Tobin Harding)
3c3cf03 Remove use of unreachable in error branch (Tobin Harding)
d8e42d1 Remove 'what' comments (Tobin Harding)
b60db79 Use un/tweaked public key types (Tobin Harding)
402bd99 Add standard derives to TweakedPublickKey (Tobin Harding)
9c015d9 Add newline to end of file (Tobin Harding)

Pull request description:

  We have two types for tweaked/untweaked schnorr public keys to help users of the taproot API not mix these two keys up. Currently the `taproot` module uses 'raw' `schnoor::PublicKey`s.

  Use the `schnoor` module's tweak/untweaked public key types for the `taproot` API.

  Fixes: #725

  Please note, I saw this was labeled 'good-first-issue' but I ignored that and greedily implemented a solution because of two reasons
  1. We want to get taproot stuff done post haste.
  2. I'm struggling to follow what is going on with all the taproot work so this seemed like a way to get my hands dirty.

ACKs for top commit:
  dr-orlovsky:
    utACK b5bf6d7
  sanket1729:
    ACK b5bf6d7

Tree-SHA512: e3e0480e0d193877c33ac11d0e3a288b0393d9475b26056914e439cb3f19583c1936e70d048df8d2120a36a63b6b592d12e21ca3ab7e058dce6f8f873c3b598b

Author: dr-orlovsky

src/util/address.rs

@@ -527,11 +527,12 @@ impl Address {
         merkle_root: Option<TapBranchHash>,
         network: Network
     ) -> Address {
+        let (output_key, _parity) = internal_key.tap_tweak(secp, merkle_root);
         Address {
             network: network,
             payload: Payload::WitnessProgram {
                 version: WitnessVersion::V1,
-                program: internal_key.tap_tweak(secp, merkle_root).into_inner().serialize().to_vec()
+                program: output_key.into_inner().serialize().to_vec()
             }
         }
     }

src/util/schnorr.rs

@@ -26,6 +26,7 @@ use util::taproot::{TapBranchHash, TapTweakHash};
 pub type UntweakedPublicKey = PublicKey;
 
 /// Tweaked Schnorr public key
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct TweakedPublicKey(PublicKey);
 
 /// A trait for tweaking Schnorr public keys
@@ -38,26 +39,26 @@ pub trait TapTweak {
     ///  * H is the hash function
     ///  * c is the commitment data
     ///  * G is the generator point
-    fn tap_tweak<C: Verification>(self, secp: &Secp256k1<C>, merkle_root: Option<TapBranchHash>) -> TweakedPublicKey;
+    ///
+    /// # Returns
+    /// The tweaked key and its parity.
+    fn tap_tweak<C: Verification>(self, secp: &Secp256k1<C>, merkle_root: Option<TapBranchHash>) -> (TweakedPublicKey, bool);
 
-    /// Directly convert an UntweakedPublicKey to a TweakedPublicKey
+    /// Directly converts an [`UntweakedPublicKey`] to a [`TweakedPublicKey`]
     ///
     /// This method is dangerous and can lead to loss of funds if used incorrectly.
     /// Specifically, in multi-party protocols a peer can provide a value that allows them to steal.
     fn dangerous_assume_tweaked(self) -> TweakedPublicKey;
 }
 
 impl TapTweak for UntweakedPublicKey {
-    fn tap_tweak<C: Verification>(self, secp: &Secp256k1<C>, merkle_root: Option<TapBranchHash>) -> TweakedPublicKey {
-        // Compute the tweak
+    fn tap_tweak<C: Verification>(self, secp: &Secp256k1<C>, merkle_root: Option<TapBranchHash>) -> (TweakedPublicKey, bool) {
         let tweak_value = TapTweakHash::from_key_and_tweak(self, merkle_root).into_inner();
-
-        //Tweak the internal key by the tweak value
         let mut output_key = self.clone();
         let parity = output_key.tweak_add_assign(&secp, &tweak_value).expect("Tap tweak failed");
-        if self.tweak_add_check(&secp, &output_key, parity, tweak_value) {
-            return TweakedPublicKey(output_key);
-        } else { unreachable!("Tap tweak failed") }
+
+        debug_assert!(self.tweak_add_check(&secp, &output_key, parity, tweak_value));
+        (TweakedPublicKey(output_key), parity)
     }
 
     fn dangerous_assume_tweaked(self) -> TweakedPublicKey {
@@ -67,19 +68,20 @@ impl TapTweak for UntweakedPublicKey {
 
 
 impl TweakedPublicKey {
-    /// Create a new [TweakedPublicKey] from a [PublicKey]. No tweak is applied.
-    pub fn new(key: PublicKey) -> TweakedPublicKey {
+    /// Creates a new [`TweakedPublicKey`] from a [`PublicKey`]. No tweak is applied, consider
+    /// calling `tap_tweak` on an [`UntweakedPublicKey`] instead of using this constructor.
+    pub fn dangerous_assume_tweaked(key: PublicKey) -> TweakedPublicKey {
         TweakedPublicKey(key)
     }
 
-    /// Returns the underlying public key
+    /// Returns the underlying public key.
     pub fn into_inner(self) -> PublicKey {
         self.0
     }
 
-    /// Returns a reference to underlying public key
+    /// Returns a reference to underlying public key.
     pub fn as_inner(&self) -> &PublicKey {
         &self.0
     }
 
-}
+}

src/util/taproot.rs

@@ -25,7 +25,7 @@ use core::cmp::Reverse;
 use std::error;
 
 use hashes::{sha256, sha256t, Hash, HashEngine};
-use schnorr;
+use schnorr::{TweakedPublicKey, UntweakedPublicKey, TapTweak};
 use Script;
 
 use consensus::Encodable;
@@ -101,7 +101,7 @@ impl TapTweakHash {
     /// Create a new BIP341 [`TapTweakHash`] from key and tweak
     /// Produces H_taptweak(P||R) where P is internal key and R is the merkle root
     pub fn from_key_and_tweak(
-        internal_key: schnorr::PublicKey,
+        internal_key: UntweakedPublicKey,
         merkle_root: Option<TapBranchHash>,
     ) -> TapTweakHash {
         let mut eng = TapTweakHash::engine();
@@ -171,13 +171,13 @@ type ScriptMerkleProofMap = BTreeMap<(Script, LeafVersion), BTreeSet<TaprootMerk
 #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct TaprootSpendInfo {
     /// The BIP341 internal key.
-    internal_key: schnorr::PublicKey,
+    internal_key: UntweakedPublicKey,
     /// The Merkle root of the script tree (None if there are no scripts)
     merkle_root: Option<TapBranchHash>,
     /// The sign final output pubkey as per BIP 341
     output_key_parity: bool,
     /// The tweaked output key
-    output_key: schnorr::PublicKey,
+    output_key: TweakedPublicKey,
     /// Map from (script, leaf_version) to (sets of) [`TaprootMerkleBranch`].
     /// More than one control block for a given script is only possible if it
     /// appears in multiple branches of the tree. In all cases, keeping one should
@@ -207,7 +207,7 @@ impl TaprootSpendInfo {
     ///   2^32.
     pub fn with_huffman_tree<C, I>(
         secp: &Secp256k1<C>,
-        internal_key: schnorr::PublicKey,
+        internal_key: UntweakedPublicKey,
         script_weights: I,
     ) -> Result<Self, TaprootBuilderError>
     where
@@ -253,20 +253,10 @@ impl TaprootSpendInfo {
     ///
     pub fn new_key_spend<C: secp256k1::Verification>(
         secp: &Secp256k1<C>,
-        internal_key: schnorr::PublicKey,
+        internal_key: UntweakedPublicKey,
         merkle_root: Option<TapBranchHash>,
     ) -> Self {
-        let tweak = TapTweakHash::from_key_and_tweak(internal_key, merkle_root);
-        let mut output_key = internal_key;
-        // # Panics:
-        //
-        // This would return Err if the merkle root hash is the negation of the secret
-        // key corresponding to the internal key.
-        // Because the tweak is derived as specified in BIP341 (hash output of a function),
-        // this is unlikely to occur (1/2^128) in real life usage, it is safe to unwrap this
-        let parity = output_key
-            .tweak_add_assign(&secp, &tweak)
-            .expect("TapTweakHash::from_key_and_tweak is broken");
+        let (output_key, parity) = internal_key.tap_tweak(secp, merkle_root);
         Self {
             internal_key: internal_key,
             merkle_root: merkle_root,
@@ -282,7 +272,7 @@ impl TaprootSpendInfo {
     }
 
     /// Obtain the internal key
-    pub fn internal_key(&self) -> schnorr::PublicKey {
+    pub fn internal_key(&self) -> UntweakedPublicKey {
         self.internal_key
     }
 
@@ -293,7 +283,7 @@ impl TaprootSpendInfo {
 
     /// Output key(the key used in script pubkey) from Spend data. See also
     /// [`TaprootSpendInfo::output_key_parity`]
-    pub fn output_key(&self) -> schnorr::PublicKey {
+    pub fn output_key(&self) -> TweakedPublicKey {
         self.output_key
     }
 
@@ -305,7 +295,7 @@ impl TaprootSpendInfo {
     // Internal function to compute [`TaprootSpendInfo`] from NodeInfo
     fn from_node_info<C: secp256k1::Verification>(
         secp: &Secp256k1<C>,
-        internal_key: schnorr::PublicKey,
+        internal_key: UntweakedPublicKey,
         node: NodeInfo,
     ) -> TaprootSpendInfo {
         // Create as if it is a key spend path with the given merkle root
@@ -433,7 +423,7 @@ impl TaprootBuilder {
     pub fn finalize<C: secp256k1::Verification>(
         mut self,
         secp: &Secp256k1<C>,
-        internal_key: schnorr::PublicKey,
+        internal_key: UntweakedPublicKey,
     ) -> Result<TaprootSpendInfo, TaprootBuilderError> {
         if self.branch.len() > 1 {
             return Err(TaprootBuilderError::IncompleteTree);
@@ -655,7 +645,7 @@ pub struct ControlBlock {
     /// The parity of the output key (NOT THE INTERNAL KEY WHICH IS ALWAYS XONLY)
     pub output_key_parity: bool,
     /// The internal key
-    pub internal_key: schnorr::PublicKey,
+    pub internal_key: UntweakedPublicKey,
     /// The merkle proof of a script associated with this leaf
     pub merkle_branch: TaprootMerkleBranch,
 }
@@ -677,7 +667,7 @@ impl ControlBlock {
         }
         let output_key_parity = (sl[0] & 1) == 1;
         let leaf_version = LeafVersion::from_u8(sl[0] & TAPROOT_LEAF_MASK)?;
-        let internal_key = schnorr::PublicKey::from_slice(&sl[1..TAPROOT_CONTROL_BASE_SIZE])
+        let internal_key = UntweakedPublicKey::from_slice(&sl[1..TAPROOT_CONTROL_BASE_SIZE])
             .map_err(TaprootError::InvalidInternalKey)?;
         let merkle_branch = TaprootMerkleBranch::from_slice(&sl[TAPROOT_CONTROL_BASE_SIZE..])?;
         Ok(ControlBlock {
@@ -722,7 +712,7 @@ impl ControlBlock {
     pub fn verify_taproot_commitment<C: secp256k1::Verification>(
         &self,
         secp: &Secp256k1<C>,
-        output_key: &schnorr::PublicKey,
+        output_key: &TweakedPublicKey,
         script: &Script,
     ) -> bool {
         // compute the script hash
@@ -746,7 +736,7 @@ impl ControlBlock {
         let tweak = TapTweakHash::from_key_and_tweak(self.internal_key, Some(curr_hash));
         self.internal_key.tweak_add_check(
             secp,
-            output_key,
+            output_key.as_inner(),
             self.output_key_parity,
             tweak.into_inner(),
         )
@@ -903,6 +893,7 @@ mod test {
     use hashes::{sha256, Hash, HashEngine};
     use secp256k1::VerifyOnly;
     use core::str::FromStr;
+    use schnorr;
 
     fn tag_engine(tag_name: &str) -> sha256::HashEngine {
         let mut engine = sha256::Hash::engine();
@@ -987,6 +978,7 @@ mod test {
 
     fn _verify_tap_commitments(secp: &Secp256k1<VerifyOnly>, out_spk_hex: &str, script_hex : &str, control_block_hex: &str) {
         let out_pk = schnorr::PublicKey::from_str(&out_spk_hex[4..]).unwrap();
+        let out_pk = TweakedPublicKey::dangerous_assume_tweaked(out_pk);
         let script = Script::from_hex(script_hex).unwrap();
         let control_block = ControlBlock::from_slice(&Vec::<u8>::from_hex(control_block_hex).unwrap()).unwrap();
         assert_eq!(control_block_hex, control_block.serialize().to_hex());
@@ -1028,7 +1020,7 @@ mod test {
     #[test]
     fn build_huffman_tree() {
         let secp = Secp256k1::verification_only();
-        let internal_key = schnorr::PublicKey::from_str("93c7378d96518a75448821c4f7c8f4bae7ce60f804d03d1f0628dd5dd0f5de51").unwrap();
+        let internal_key = UntweakedPublicKey::from_str("93c7378d96518a75448821c4f7c8f4bae7ce60f804d03d1f0628dd5dd0f5de51").unwrap();
 
         let script_weights = vec![
             (10, Script::from_hex("51").unwrap()), // semantics of script don't matter for this test
@@ -1078,7 +1070,7 @@ mod test {
     #[test]
     fn taptree_builder() {
         let secp = Secp256k1::verification_only();
-        let internal_key = schnorr::PublicKey::from_str("93c7378d96518a75448821c4f7c8f4bae7ce60f804d03d1f0628dd5dd0f5de51").unwrap();
+        let internal_key = UntweakedPublicKey::from_str("93c7378d96518a75448821c4f7c8f4bae7ce60f804d03d1f0628dd5dd0f5de51").unwrap();
 
         let builder = TaprootBuilder::new();
         // Create a tree as shown below