From 492073f28876406f8fe5a07a8a2495c8e0ba1fb3 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Mon, 24 Mar 2025 15:55:42 +0100 Subject: [PATCH] Strengthen the type of `taproot_control_block()` The type returned by `Witness::taproot_control_block()` was just `&[u8]` which wasn't very nice since users then had to manually decode it which so far also required allocation. Thanks to previous improvements to `ControlBlock` it is now possible to return a `ControlBlock` type directly. To avoid expensive checks, this change adds a new type `SerializedXOnlyPublicKey` which is a wrapper around `[u8; 32]` that is used in `ControlBlock` if complete checking is undesirable. It is then used in the `ControlBlock` returned from `Witness::taproot_control_block`. Users can still conveniently validate the key using `to_validated` method. It then uses this type in the recently-added `P2TrSpend` type. As a side effect this checks more properties of `Witness` when calling unrelated methods on `Witness`. From correctness perspective this should be OK: a witness obtained from a verified source will be correct anyway and, if these checks were done by the caller, they can be removed. From performance perspective, if the `Witness` was obtained from a verified source (e.g. using Bitcoin Core RPC) these checks are wasted CPU time. But they shouldn't be too expensive, we already avoid `secp256k1` overhead and, given that they always succeed in such case, they should be easy to branch-predict. --- bitcoin/src/blockdata/witness.rs | 37 +++++++++++--------- bitcoin/src/crypto/key.rs | 58 ++++++++++++++++++++++++++++++++ bitcoin/src/taproot/mod.rs | 29 ++++++++++++---- 3 files changed, 102 insertions(+), 22 deletions(-) diff --git a/bitcoin/src/blockdata/witness.rs b/bitcoin/src/blockdata/witness.rs index fd57da387..9f276d7fc 100644 --- a/bitcoin/src/blockdata/witness.rs +++ b/bitcoin/src/blockdata/witness.rs @@ -10,15 +10,15 @@ use io::{BufRead, Write}; use crate::consensus::encode::{self, Error, ReadExt, WriteExt, MAX_VEC_SIZE}; use crate::consensus::{Decodable, Encodable}; use crate::crypto::ecdsa; +use crate::crypto::key::SerializedXOnlyPublicKey; use crate::prelude::Vec; #[cfg(doc)] use crate::script::ScriptExt as _; -use crate::taproot::{ - self, ControlBlock, LeafScript, LeafVersion, TAPROOT_ANNEX_PREFIX, TAPROOT_CONTROL_BASE_SIZE, - TAPROOT_LEAF_MASK, TaprootMerkleBranch, -}; +use crate::taproot::{self, ControlBlock, LeafScript, TAPROOT_ANNEX_PREFIX, TaprootMerkleBranch}; use crate::Script; +type BorrowedControlBlock<'a> = ControlBlock<&'a TaprootMerkleBranch, &'a SerializedXOnlyPublicKey>; + #[rustfmt::skip] // Keep public re-exports separate. #[doc(inline)] pub use primitives::witness::{Iter, Witness}; @@ -176,9 +176,8 @@ crate::internal_macros::define_extension_trait! { /// version. fn taproot_leaf_script(&self) -> Option> { match P2TrSpend::from_witness(self) { - Some(P2TrSpend::Script { leaf_script, control_block, .. }) if control_block.len() >= TAPROOT_CONTROL_BASE_SIZE => { - let version = LeafVersion::from_consensus(control_block[0] & TAPROOT_LEAF_MASK).ok()?; - Some(LeafScript { version, script: leaf_script, }) + Some(P2TrSpend::Script { leaf_script, control_block, .. }) => { + Some(LeafScript { version: control_block.leaf_version, script: leaf_script, }) }, _ => None, } @@ -191,7 +190,7 @@ crate::internal_macros::define_extension_trait! { /// byte of the last element being equal to 0x50. /// /// See [`Script::is_p2tr`] to check whether this is actually a Taproot witness. - fn taproot_control_block(&self) -> Option<&[u8]> { + fn taproot_control_block(&self) -> Option> { match P2TrSpend::from_witness(self) { Some(P2TrSpend::Script { control_block, .. }) => Some(control_block), _ => None, @@ -236,7 +235,7 @@ enum P2TrSpend<'a> { }, Script { leaf_script: &'a Script, - control_block: &'a [u8], + control_block: BorrowedControlBlock<'a>, annex: Option<&'a [u8]>, }, } @@ -275,17 +274,21 @@ impl<'a> P2TrSpend<'a> { // last one does NOT start with TAPROOT_ANNEX_PREFIX. This is handled in the catchall // arm. 3.. if witness.last().expect("len > 0").starts_with(&[TAPROOT_ANNEX_PREFIX]) => { + let control_block = witness.get_back(1).expect("len > 1"); + let control_block = BorrowedControlBlock::decode_borrowed(control_block).ok()?; let spend = P2TrSpend::Script { leaf_script: Script::from_bytes(witness.get_back(2).expect("len > 2")), - control_block: witness.get_back(1).expect("len > 1"), + control_block, annex: witness.last(), }; Some(spend) } _ => { + let control_block = witness.last().expect("len > 0"); + let control_block = BorrowedControlBlock::decode_borrowed(control_block).ok()?; let spend = P2TrSpend::Script { leaf_script: Script::from_bytes(witness.get_back(1).expect("len > 1")), - control_block: witness.last().expect("len > 0"), + control_block, annex: None, }; Some(spend) @@ -324,6 +327,7 @@ mod test { use crate::consensus::{deserialize, encode, serialize}; use crate::hex::DisplayHex; use crate::sighash::EcdsaSighashType; + use crate::taproot::LeafVersion; use crate::Transaction; #[test] @@ -383,7 +387,7 @@ mod test { #[test] fn get_tapscript() { let tapscript = hex!("deadbeef"); - let control_block = hex!("02"); + let control_block = hex!("c0ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"); // annex starting with 0x50 causes the branching logic. let annex = hex!("50"); @@ -449,7 +453,8 @@ mod test { #[test] fn get_control_block() { let tapscript = hex!("deadbeef"); - let control_block = hex!("02"); + let control_block = hex!("c0ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"); + let expected_control_block = BorrowedControlBlock::decode_borrowed(&control_block).unwrap(); // annex starting with 0x50 causes the branching logic. let annex = hex!("50"); let signature = vec![0xff; 64]; @@ -468,15 +473,15 @@ mod test { deserialize::(&witness_serialized_key_spend_annex[..]).unwrap(); // With or without annex, the tapscript should be returned. - assert_eq!(witness.taproot_control_block(), Some(&control_block[..])); - assert_eq!(witness_annex.taproot_control_block(), Some(&control_block[..])); + assert_eq!(witness.taproot_control_block().unwrap(), expected_control_block); + assert_eq!(witness_annex.taproot_control_block().unwrap(), expected_control_block); assert!(witness_key_spend_annex.taproot_control_block().is_none()) } #[test] fn get_annex() { let tapscript = hex!("deadbeef"); - let control_block = hex!("02"); + let control_block = hex!("c0ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"); // annex starting with 0x50 causes the branching logic. let annex = hex!("50"); diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index 1c4dc32ae..279d97357 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -26,6 +26,7 @@ use crate::taproot::{TapNodeHash, TapTweakHash}; #[rustfmt::skip] // Keep public re-exports separate. pub use secp256k1::{constants, Keypair, Parity, Secp256k1, Verification, XOnlyPublicKey}; +pub use serialized_x_only::SerializedXOnlyPublicKey; #[cfg(feature = "rand-std")] pub use secp256k1::rand; @@ -1208,6 +1209,63 @@ impl fmt::Display for InvalidWifCompressionFlagError { #[cfg(feature = "std")] impl std::error::Error for InvalidWifCompressionFlagError {} +mod serialized_x_only { + internals::transparent_newtype! { + /// An array of bytes that's semantically an x-only public but was **not** validated. + /// + /// This can be useful when validation is not desired but semantics of the bytes should be + /// preserved. The validation can still happen using `to_validated()` method. + #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] + pub struct SerializedXOnlyPublicKey([u8; 32]); + + impl SerializedXOnlyPublicKey { + pub(crate) fn from_bytes_ref(bytes: &_) -> Self; + } + } + + impl SerializedXOnlyPublicKey { + /// Marks the supplied bytes as a serialized x-only public key. + pub const fn from_byte_array(bytes: [u8; 32]) -> Self { + Self(bytes) + } + + /// Returns the raw bytes. + pub const fn to_byte_array(self) -> [u8; 32] { + self.0 + } + + /// Returns a reference to the raw bytes. + pub const fn as_byte_array(&self) -> &[u8; 32] { + &self.0 + } + } +} + +impl SerializedXOnlyPublicKey { + /// Returns `XOnlyPublicKey` if the bytes are valid. + pub fn to_validated(self) -> Result { + XOnlyPublicKey::from_byte_array(self.as_byte_array()) + } +} + +impl AsRef<[u8; 32]> for SerializedXOnlyPublicKey { + fn as_ref(&self) -> &[u8; 32] { + self.as_byte_array() + } +} + +impl From<&SerializedXOnlyPublicKey> for SerializedXOnlyPublicKey { + fn from(borrowed: &SerializedXOnlyPublicKey) -> Self { + *borrowed + } +} + +impl fmt::Debug for SerializedXOnlyPublicKey { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Debug::fmt(&self.as_byte_array().as_hex(), f) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/bitcoin/src/taproot/mod.rs b/bitcoin/src/taproot/mod.rs index b14264f24..15f188a10 100644 --- a/bitcoin/src/taproot/mod.rs +++ b/bitcoin/src/taproot/mod.rs @@ -22,7 +22,7 @@ use io::Write; use secp256k1::{Scalar, Secp256k1}; use crate::consensus::Encodable; -use crate::crypto::key::{TapTweak, TweakedPublicKey, UntweakedPublicKey, XOnlyPublicKey}; +use crate::crypto::key::{SerializedXOnlyPublicKey, TapTweak, TweakedPublicKey, UntweakedPublicKey, XOnlyPublicKey}; use crate::prelude::{BTreeMap, BTreeSet, BinaryHeap, Vec}; use crate::{Script, ScriptBuf}; @@ -1141,13 +1141,13 @@ impl<'leaf> ScriptLeaf<'leaf> { /// Control block data structure used in Tapscript satisfaction. #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -pub struct ControlBlock where Branch: ?Sized { +pub struct ControlBlock where Branch: ?Sized { /// The tapleaf version. pub leaf_version: LeafVersion, /// The parity of the output key (NOT THE INTERNAL KEY WHICH IS ALWAYS XONLY). pub output_key_parity: secp256k1::Parity, /// The internal key. - pub internal_key: UntweakedPublicKey, + pub internal_key: Key, /// The Merkle proof of a script associated with this leaf. pub merkle_branch: Branch, } @@ -1166,6 +1166,24 @@ impl ControlBlock { /// - [`TaprootError::InvalidInternalKey`] if internal key is invalid (first 32 bytes after the parity byte). /// - [`TaprootError::InvalidMerkleTreeDepth`] if Merkle tree is too deep (more than 128 levels). pub fn decode(sl: &[u8]) -> Result { + use alloc::borrow::ToOwned; + + let ControlBlock { + leaf_version, + output_key_parity, + internal_key, + merkle_branch, + } = ControlBlock::<&TaprootMerkleBranch, &SerializedXOnlyPublicKey>::decode_borrowed(sl)?; + + let internal_key = internal_key.to_validated().map_err(TaprootError::InvalidInternalKey)?; + let merkle_branch = merkle_branch.to_owned(); + + Ok(ControlBlock { leaf_version, output_key_parity, internal_key, merkle_branch }) + } +} + +impl ControlBlock { + pub(crate) fn decode_borrowed<'a>(sl: &'a [u8]) -> Result where B: From<&'a TaprootMerkleBranch>, K: From<&'a SerializedXOnlyPublicKey> { let (base, merkle_branch) = sl.split_first_chunk::() .ok_or(InvalidControlBlockSizeError(sl.len()))?; @@ -1177,9 +1195,8 @@ impl ControlBlock { }; let leaf_version = LeafVersion::from_consensus(first & TAPROOT_LEAF_MASK)?; - let internal_key = UntweakedPublicKey::from_byte_array(internal_key) - .map_err(TaprootError::InvalidInternalKey)?; - let merkle_branch = TaprootMerkleBranchBuf::decode(merkle_branch)?; + let internal_key = SerializedXOnlyPublicKey::from_bytes_ref(internal_key).into(); + let merkle_branch = TaprootMerkleBranch::decode(merkle_branch)?.into(); Ok(ControlBlock { leaf_version, output_key_parity, internal_key, merkle_branch }) } }