From 9c015d9ce396ed8038634bc29d7c09ed42b567dd Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Mon, 29 Nov 2021 13:17:16 +1100 Subject: [PATCH 1/8] Add newline to end of file Idiomatic UNIX file handling leaves files with a newline at the end. Add newline to end of `schnorr` module. --- src/util/schnorr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/schnorr.rs b/src/util/schnorr.rs index c85e7258..a7bba40f 100644 --- a/src/util/schnorr.rs +++ b/src/util/schnorr.rs @@ -82,4 +82,4 @@ impl TweakedPublicKey { &self.0 } -} \ No newline at end of file +} From 402bd993b226c4642ef524b1b435c0cbcf1952eb Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Mon, 29 Nov 2021 13:18:22 +1100 Subject: [PATCH 2/8] Add standard derives to TweakedPublickKey All new types in `rust-bitcoin` should use our standard set of derives. Add said standard derives to `TweakedPublickKey`. --- src/util/schnorr.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/schnorr.rs b/src/util/schnorr.rs index a7bba40f..74c97042 100644 --- a/src/util/schnorr.rs +++ b/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 From b60db79a3b39e3060d65090d45f04166e05695f4 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Mon, 29 Nov 2021 13:19:21 +1100 Subject: [PATCH 3/8] Use un/tweaked public key types 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. --- src/util/taproot.rs | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/src/util/taproot.rs b/src/util/taproot.rs index 3c389d09..5be83cf7 100644 --- a/src/util/taproot.rs +++ b/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}; 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, ) -> TapTweakHash { let mut eng = TapTweakHash::engine(); @@ -171,13 +171,13 @@ type ScriptMerkleProofMap = BTreeMap<(Script, LeafVersion), BTreeSet, /// 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 @@ -204,7 +204,7 @@ impl TaprootSpendInfo { /// dealing with numbers close to 2^64. pub fn with_huffman_tree( secp: &Secp256k1, - internal_key: schnorr::PublicKey, + internal_key: UntweakedPublicKey, script_weights: I, ) -> Result where @@ -250,7 +250,7 @@ impl TaprootSpendInfo { /// pub fn new_key_spend( secp: &Secp256k1, - internal_key: schnorr::PublicKey, + internal_key: UntweakedPublicKey, merkle_root: Option, ) -> Self { let tweak = TapTweakHash::from_key_and_tweak(internal_key, merkle_root); @@ -268,7 +268,7 @@ impl TaprootSpendInfo { internal_key: internal_key, merkle_root: merkle_root, output_key_parity: parity, - output_key: output_key, + output_key: TweakedPublicKey::new(output_key), script_map: BTreeMap::new(), } } @@ -279,7 +279,7 @@ impl TaprootSpendInfo { } /// Obtain the internal key - pub fn internal_key(&self) -> schnorr::PublicKey { + pub fn internal_key(&self) -> UntweakedPublicKey { self.internal_key } @@ -290,7 +290,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 } @@ -302,7 +302,7 @@ impl TaprootSpendInfo { // Internal function to compute [`TaprootSpendInfo`] from NodeInfo fn from_node_info( secp: &Secp256k1, - internal_key: schnorr::PublicKey, + internal_key: UntweakedPublicKey, node: NodeInfo, ) -> TaprootSpendInfo { // Create as if it is a key spend path with the given merkle root @@ -430,7 +430,7 @@ impl TaprootBuilder { pub fn finalize( mut self, secp: &Secp256k1, - internal_key: schnorr::PublicKey, + internal_key: UntweakedPublicKey, ) -> Result { if self.branch.len() > 1 { return Err(TaprootBuilderError::IncompleteTree); @@ -652,7 +652,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, } @@ -674,7 +674,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 { @@ -719,7 +719,7 @@ impl ControlBlock { pub fn verify_taproot_commitment( &self, secp: &Secp256k1, - output_key: &schnorr::PublicKey, + output_key: &TweakedPublicKey, script: &Script, ) -> bool { // compute the script hash @@ -743,7 +743,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(), ) @@ -900,6 +900,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(); @@ -984,6 +985,7 @@ mod test { fn _verify_tap_commitments(secp: &Secp256k1, 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::new(out_pk); let script = Script::from_hex(script_hex).unwrap(); let control_block = ControlBlock::from_slice(&Vec::::from_hex(control_block_hex).unwrap()).unwrap(); assert_eq!(control_block_hex, control_block.serialize().to_hex()); @@ -1025,7 +1027,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 @@ -1075,7 +1077,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 From d8e42d153e10187229dfa8dd1b2ed416a59212db Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Thu, 2 Dec 2021 14:08:28 +1100 Subject: [PATCH 4/8] Remove 'what' comments When used, code comments should say _why_ we do something not _what_ we do, the code already says what we do. Remove 'what we do' style comments. --- src/util/schnorr.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/util/schnorr.rs b/src/util/schnorr.rs index 74c97042..d109e70a 100644 --- a/src/util/schnorr.rs +++ b/src/util/schnorr.rs @@ -50,10 +50,7 @@ pub trait TapTweak { impl TapTweak for UntweakedPublicKey { fn tap_tweak(self, secp: &Secp256k1, merkle_root: Option) -> TweakedPublicKey { - // Compute the tweak 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) { From 3c3cf0396bc9ff04c619df1806029c2c8475e791 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Thu, 2 Dec 2021 14:13:33 +1100 Subject: [PATCH 5/8] Remove use of unreachable in error branch We currently run `tweak_add_check` and use the result as a conditional branch, the error path of which uses `unreachable`. This usage of `unreachable` is non-typical. An 'unreachable' statement is by definition supposed to be unreachable, it is not clear why we would need to have a conditional branch to check an unreachable statement. Use `debug_assert!` so programmer errors get caught in un-optimised builds but in optimised builds the call to `tweak_add_check` is not even done. --- src/util/schnorr.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/schnorr.rs b/src/util/schnorr.rs index d109e70a..64fa3193 100644 --- a/src/util/schnorr.rs +++ b/src/util/schnorr.rs @@ -53,9 +53,9 @@ impl TapTweak for UntweakedPublicKey { let tweak_value = TapTweakHash::from_key_and_tweak(self, merkle_root).into_inner(); 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) } fn dangerous_assume_tweaked(self) -> TweakedPublicKey { From 7af0999745a7eef08acb96ab2eaedd195299fb22 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Thu, 2 Dec 2021 14:40:24 +1100 Subject: [PATCH 6/8] Re-name TweakedPublicKey constructor Keeping inline with the method on `UntweakedPublicKey` that outputs a `TweakedPublicKey` we can use the same name, for the same reasons. Use `dangerous_assume_tweaked` as the constructor name to highlight the fact that this constructor should probably not be being used. --- src/util/schnorr.rs | 5 +++-- src/util/taproot.rs | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/util/schnorr.rs b/src/util/schnorr.rs index 64fa3193..9a92afc1 100644 --- a/src/util/schnorr.rs +++ b/src/util/schnorr.rs @@ -65,8 +65,9 @@ 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) } diff --git a/src/util/taproot.rs b/src/util/taproot.rs index 5be83cf7..30375578 100644 --- a/src/util/taproot.rs +++ b/src/util/taproot.rs @@ -268,7 +268,7 @@ impl TaprootSpendInfo { internal_key: internal_key, merkle_root: merkle_root, output_key_parity: parity, - output_key: TweakedPublicKey::new(output_key), + output_key: TweakedPublicKey::dangerous_assume_tweaked(output_key), script_map: BTreeMap::new(), } } @@ -985,7 +985,7 @@ mod test { fn _verify_tap_commitments(secp: &Secp256k1, 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::new(out_pk); + let out_pk = TweakedPublicKey::dangerous_assume_tweaked(out_pk); let script = Script::from_hex(script_hex).unwrap(); let control_block = ControlBlock::from_slice(&Vec::::from_hex(control_block_hex).unwrap()).unwrap(); assert_eq!(control_block_hex, control_block.serialize().to_hex()); From a6d3514f2be87b6b472fa7332889c8ffcdb55d11 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Thu, 2 Dec 2021 14:43:38 +1100 Subject: [PATCH 7/8] Return parity when doing tap_tweak Currently we calculate the parity during `tap_tweak` but do not return it, this means others must re-do work done inside `tap_tweak` in order to calculate the parity. We can just return the parity along with the tweaked key. --- src/util/address.rs | 3 ++- src/util/schnorr.rs | 9 ++++++--- src/util/taproot.rs | 16 +++------------- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/src/util/address.rs b/src/util/address.rs index 6945c7fd..1b7c3bfe 100644 --- a/src/util/address.rs +++ b/src/util/address.rs @@ -527,11 +527,12 @@ impl Address { merkle_root: Option, 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() } } } diff --git a/src/util/schnorr.rs b/src/util/schnorr.rs index 9a92afc1..13e14794 100644 --- a/src/util/schnorr.rs +++ b/src/util/schnorr.rs @@ -39,7 +39,10 @@ pub trait TapTweak { /// * H is the hash function /// * c is the commitment data /// * G is the generator point - fn tap_tweak(self, secp: &Secp256k1, merkle_root: Option) -> TweakedPublicKey; + /// + /// # Returns + /// The tweaked key and its parity. + fn tap_tweak(self, secp: &Secp256k1, merkle_root: Option) -> (TweakedPublicKey, bool); /// Directly convert an UntweakedPublicKey to a TweakedPublicKey /// @@ -49,13 +52,13 @@ pub trait TapTweak { } impl TapTweak for UntweakedPublicKey { - fn tap_tweak(self, secp: &Secp256k1, merkle_root: Option) -> TweakedPublicKey { + fn tap_tweak(self, secp: &Secp256k1, merkle_root: Option) -> (TweakedPublicKey, bool) { let tweak_value = TapTweakHash::from_key_and_tweak(self, merkle_root).into_inner(); let mut output_key = self.clone(); let parity = output_key.tweak_add_assign(&secp, &tweak_value).expect("Tap tweak failed"); debug_assert!(self.tweak_add_check(&secp, &output_key, parity, tweak_value)); - TweakedPublicKey(output_key) + (TweakedPublicKey(output_key), parity) } fn dangerous_assume_tweaked(self) -> TweakedPublicKey { diff --git a/src/util/taproot.rs b/src/util/taproot.rs index 30375578..dc2988c2 100644 --- a/src/util/taproot.rs +++ b/src/util/taproot.rs @@ -25,7 +25,7 @@ use core::cmp::Reverse; use std::error; use hashes::{sha256, sha256t, Hash, HashEngine}; -use schnorr::{TweakedPublicKey, UntweakedPublicKey}; +use schnorr::{TweakedPublicKey, UntweakedPublicKey, TapTweak}; use Script; use consensus::Encodable; @@ -253,22 +253,12 @@ impl TaprootSpendInfo { internal_key: UntweakedPublicKey, merkle_root: Option, ) -> 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, output_key_parity: parity, - output_key: TweakedPublicKey::dangerous_assume_tweaked(output_key), + output_key: output_key, script_map: BTreeMap::new(), } } From b5bf6d73190c7c6a09b20917918ede28baeb5f9c Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Fri, 10 Dec 2021 11:42:43 +1100 Subject: [PATCH 8/8] Improve rustdocs on schnorr module Improve the docs by doing: - Use [`Foo`] for types - Use third person tense - Add trailing periods --- src/util/schnorr.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/schnorr.rs b/src/util/schnorr.rs index 13e14794..834bf338 100644 --- a/src/util/schnorr.rs +++ b/src/util/schnorr.rs @@ -44,7 +44,7 @@ pub trait TapTweak { /// The tweaked key and its parity. fn tap_tweak(self, secp: &Secp256k1, merkle_root: Option) -> (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. @@ -74,12 +74,12 @@ impl 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 }