From 5dd0c9253d207facc954a9b7f30ec7b2a7f410d8 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Wed, 5 Mar 2025 11:45:38 +0100 Subject: [PATCH 1/2] Remove a bunch of `try_into().expect()` Previously we've used `try_into().expect()` because const generics were unavailable. Then they became available but we didn't realize we could already convert a bunch of code to not use panicking conversions. But we can (and could for a while). This adds an extension trait for arrays to provide basic non-panicking operations returning arrays, so they can be composed with other functions accepting arrays without any conversions. It also refactors a bunch of code to use the non-panicking constructs but it's certainly not all of it. That could be done later. This just aims at removing the ugliest offenders and demonstrate the usefulness of this approach. Aside from this, to avoid a bunch of duplicated work, this refactors BIP32 key parsing to use a common method where xpub and xpriv are encoded the same. Not doing this already led to a mistake where xpriv implemented some additional checks that were missing in xpub. Thus this change also indirectly fixes that bug. --- base58/src/lib.rs | 16 ++- bitcoin/src/address/mod.rs | 8 +- bitcoin/src/address/script_pubkey.rs | 19 ++-- bitcoin/src/bip152.rs | 6 +- bitcoin/src/bip158.rs | 10 +- bitcoin/src/bip32.rs | 147 ++++++++++++++++----------- bitcoin/src/crypto/key.rs | 27 +++-- bitcoin/src/crypto/taproot.rs | 33 +++--- bitcoin/src/lib.rs | 1 + bitcoin/src/psbt/serialize.rs | 10 +- bitcoin/src/taproot/mod.rs | 26 ++--- internals/src/array.rs | 105 +++++++++++++++++++ internals/src/lib.rs | 1 + internals/src/slice.rs | 40 ++++++++ primitives/src/witness.rs | 8 +- 15 files changed, 309 insertions(+), 148 deletions(-) create mode 100644 internals/src/array.rs diff --git a/base58/src/lib.rs b/base58/src/lib.rs index 312e9c564..13db9fd1f 100644 --- a/base58/src/lib.rs +++ b/base58/src/lib.rs @@ -20,6 +20,7 @@ // Exclude lints we don't think are valuable. #![allow(clippy::needless_question_mark)] // https://github.com/rust-bitcoin/rust-bitcoin/pull/2134 #![allow(clippy::manual_range_contains)] // More readable than clippy's format. +#![allow(clippy::incompatible_msrv)] // Has FPs and we're testing it which is more reliable anyway. extern crate alloc; @@ -40,7 +41,10 @@ use core::fmt; pub use std::{string::String, vec::Vec}; use hashes::sha256d; +use internals::array::ArrayExt; use internals::array_vec::ArrayVec; +#[allow(unused)] // MSRV polyfill +use internals::slice::SliceExt; use crate::error::{IncorrectChecksumError, TooShortError}; @@ -109,15 +113,9 @@ pub fn decode(data: &str) -> Result, InvalidCharacterError> { /// Decodes a base58check-encoded string into a byte vector verifying the checksum. pub fn decode_check(data: &str) -> Result, Error> { let mut ret: Vec = decode(data)?; - if ret.len() < 4 { - return Err(TooShortError { length: ret.len() }.into()); - } - let check_start = ret.len() - 4; + let (remaining, &data_check) = ret.split_last_chunk::<4>().ok_or(TooShortError { length: ret.len() })?; - let hash_check = sha256d::Hash::hash(&ret[..check_start]).as_byte_array()[..4] - .try_into() - .expect("4 byte slice"); - let data_check = ret[check_start..].try_into().expect("4 byte slice"); + let hash_check = *sha256d::Hash::hash(remaining).as_byte_array().sub_array::<0, 4>(); let expected = u32::from_le_bytes(hash_check); let actual = u32::from_le_bytes(data_check); @@ -126,7 +124,7 @@ pub fn decode_check(data: &str) -> Result, Error> { return Err(IncorrectChecksumError { incorrect: actual, expected }.into()); } - ret.truncate(check_start); + ret.truncate(remaining.len()); Ok(ret) } diff --git a/bitcoin/src/address/mod.rs b/bitcoin/src/address/mod.rs index 36b6a553c..93bc290d8 100644 --- a/bitcoin/src/address/mod.rs +++ b/bitcoin/src/address/mod.rs @@ -49,6 +49,7 @@ use core::str::FromStr; use bech32::primitives::gf32::Fe32; use bech32::primitives::hrp::Hrp; use hashes::{hash160, HashEngine}; +use internals::array::ArrayExt; use secp256k1::{Secp256k1, Verification, XOnlyPublicKey}; use crate::address::script_pubkey::ScriptBufExt as _; @@ -902,12 +903,9 @@ impl Address { return Err(LegacyAddressTooLongError { length: s.len() }.into()); } let data = base58::decode_check(s)?; - if data.len() != 21 { - return Err(InvalidBase58PayloadLengthError { length: s.len() }.into()); - } + let data: &[u8; 21] = (&*data).try_into().map_err(|_| InvalidBase58PayloadLengthError { length: s.len() })?; - let (prefix, data) = data.split_first().expect("length checked above"); - let data: [u8; 20] = data.try_into().expect("length checked above"); + let (prefix, &data) = data.split_first(); let inner = match *prefix { PUBKEY_ADDRESS_PREFIX_MAIN => { diff --git a/bitcoin/src/address/script_pubkey.rs b/bitcoin/src/address/script_pubkey.rs index 7c048680e..b89008ca3 100644 --- a/bitcoin/src/address/script_pubkey.rs +++ b/bitcoin/src/address/script_pubkey.rs @@ -2,6 +2,7 @@ //! Bitcoin scriptPubkey script extensions. +use internals::array::ArrayExt; use secp256k1::{Secp256k1, Verification}; use crate::internal_macros::define_extension_trait; @@ -99,14 +100,16 @@ define_extension_trait! { pub(crate) trait ScriptExtPrivate impl for Script { /// Returns the bytes of the (possibly invalid) public key if this script is P2PK. fn p2pk_pubkey_bytes(&self) -> Option<&[u8]> { - match self.len() { - 67 if self.as_bytes()[0] == OP_PUSHBYTES_65.to_u8() - && self.as_bytes()[66] == OP_CHECKSIG.to_u8() => - Some(&self.as_bytes()[1..66]), - 35 if self.as_bytes()[0] == OP_PUSHBYTES_33.to_u8() - && self.as_bytes()[34] == OP_CHECKSIG.to_u8() => - Some(&self.as_bytes()[1..34]), - _ => None, + if let Ok(bytes) = <&[u8; 67]>::try_from(self.as_bytes()) { + let (&first, bytes) = bytes.split_first::<66>(); + let (&last, pubkey) = bytes.split_last::<65>(); + (first == OP_PUSHBYTES_65.to_u8() && last == OP_CHECKSIG.to_u8()).then_some(pubkey) + } else if let Ok(bytes) = <&[u8; 35]>::try_from(self.as_bytes()) { + let (&first, bytes) = bytes.split_first::<34>(); + let (&last, pubkey) = bytes.split_last::<33>(); + (first == OP_PUSHBYTES_33.to_u8() && last == OP_CHECKSIG.to_u8()).then_some(pubkey) + } else { + None } } } diff --git a/bitcoin/src/bip152.rs b/bitcoin/src/bip152.rs index 979dabc5e..bb25066d1 100644 --- a/bitcoin/src/bip152.rs +++ b/bitcoin/src/bip152.rs @@ -10,7 +10,7 @@ use core::{convert, fmt, mem}; use std::error; use hashes::{sha256, siphash24}; -use internals::ToU64 as _; +use internals::{ToU64 as _, array::ArrayExt as _}; use io::{BufRead, Write}; use crate::consensus::encode::{self, Decodable, Encodable, ReadExt, WriteExt}; @@ -117,8 +117,8 @@ impl ShortId { // 2. Running SipHash-2-4 with the input being the transaction ID and the keys (k0/k1) // set to the first two little-endian 64-bit integers from the above hash, respectively. ( - u64::from_le_bytes(h.as_byte_array()[0..8].try_into().expect("8 byte slice")), - u64::from_le_bytes(h.as_byte_array()[8..16].try_into().expect("8 byte slice")), + u64::from_le_bytes(*h.as_byte_array().sub_array::<0, 8>()), + u64::from_le_bytes(*h.as_byte_array().sub_array::<8, 8>()), ) } diff --git a/bitcoin/src/bip158.rs b/bitcoin/src/bip158.rs index a0d57c1a8..abdde2a9c 100644 --- a/bitcoin/src/bip158.rs +++ b/bitcoin/src/bip158.rs @@ -42,7 +42,7 @@ use core::convert::Infallible; use core::fmt; use hashes::{sha256d, siphash24, HashEngine as _}; -use internals::{write_err, ToU64 as _}; +use internals::{write_err, ToU64 as _, array::ArrayExt as _}; use io::{BufRead, Write}; use crate::block::{Block, BlockHash, Checked}; @@ -195,8 +195,8 @@ impl<'a, W: Write> BlockFilterWriter<'a, W> { /// Constructs a new [`BlockFilterWriter`] from `block`. pub fn new(writer: &'a mut W, block: &'a Block) -> BlockFilterWriter<'a, W> { let block_hash_as_int = block.block_hash().to_byte_array(); - let k0 = u64::from_le_bytes(block_hash_as_int[0..8].try_into().expect("8 byte slice")); - let k1 = u64::from_le_bytes(block_hash_as_int[8..16].try_into().expect("8 byte slice")); + let k0 = u64::from_le_bytes(*block_hash_as_int.sub_array::<0, 8>()); + let k1 = u64::from_le_bytes(*block_hash_as_int.sub_array::<8, 8>()); let writer = GcsFilterWriter::new(writer, k0, k1, M, P); BlockFilterWriter { block, writer } } @@ -250,8 +250,8 @@ impl BlockFilterReader { /// Constructs a new [`BlockFilterReader`] from `block_hash`. pub fn new(block_hash: BlockHash) -> BlockFilterReader { let block_hash_as_int = block_hash.to_byte_array(); - let k0 = u64::from_le_bytes(block_hash_as_int[0..8].try_into().expect("8 byte slice")); - let k1 = u64::from_le_bytes(block_hash_as_int[8..16].try_into().expect("8 byte slice")); + let k0 = u64::from_le_bytes(*block_hash_as_int.sub_array::<0, 8>()); + let k1 = u64::from_le_bytes(*block_hash_as_int.sub_array::<8, 8>()); BlockFilterReader { reader: GcsFilterReader::new(k0, k1, M, P) } } diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 382d85554..057efe451 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -10,7 +10,8 @@ use core::ops::Index; use core::str::FromStr; use core::{fmt, slice}; -use hashes::{hash160, hash_newtype, sha512, HashEngine, Hmac, HmacEngine}; +use hashes::{hash160, hash_newtype, sha512, Hash, HashEngine, Hmac, HmacEngine}; +use internals::array::ArrayExt; use internals::write_err; use secp256k1::{Secp256k1, XOnlyPublicKey}; @@ -44,7 +45,7 @@ impl_array_newtype_stringify!(ChainCode, 32); impl ChainCode { fn from_hmac(hmac: Hmac) -> Self { - hmac.as_ref()[32..].try_into().expect("half of hmac is guaranteed to be 32 bytes") + ChainCode(*hmac.as_byte_array().split_array::<32, 32>().1) } } @@ -608,7 +609,7 @@ impl Xpriv { parent_fingerprint: Default::default(), child_number: ChildNumber::ZERO_NORMAL, private_key: secp256k1::SecretKey::from_byte_array( - &hmac.as_ref()[..32].try_into().expect("Slice should be exactly 32 bytes"), + hmac.as_byte_array().split_array::<32, 32>().0, )?, chain_code: ChainCode::from_hmac(hmac), }) @@ -682,7 +683,7 @@ impl Xpriv { engine.input(&u32::from(i).to_be_bytes()); let hmac: Hmac = engine.finalize(); let sk = secp256k1::SecretKey::from_byte_array( - &hmac.as_ref()[..32].try_into().expect("statistically impossible to hit"), + hmac.as_byte_array().split_array::<32, 32>().0, ) .expect("statistically impossible to hit"); let tweaked = @@ -700,49 +701,33 @@ impl Xpriv { /// Decoding extended private key from binary data according to BIP 32 pub fn decode(data: &[u8]) -> Result { - if data.len() != 78 { - return Err(Error::WrongExtendedKeyLength(data.len())); - } + let Common { + network, + depth, + parent_fingerprint, + child_number, + chain_code, + key, + } = Common::decode(data)?; - let network = if data.starts_with(&VERSION_BYTES_MAINNET_PRIVATE) { - NetworkKind::Main - } else if data.starts_with(&VERSION_BYTES_TESTNETS_PRIVATE) { - NetworkKind::Test - } else { - let (b0, b1, b2, b3) = (data[0], data[1], data[2], data[3]); - return Err(Error::UnknownVersion([b0, b1, b2, b3])); + let network = match network { + VERSION_BYTES_MAINNET_PRIVATE => NetworkKind::Main, + VERSION_BYTES_TESTNETS_PRIVATE => NetworkKind::Test, + unknown => return Err(Error::UnknownVersion(unknown)), }; - // Check that byte 45 is 0 as required by BIP-32 for private keys - if data[45] != 0 { + let (&zero, private_key) = key.split_first(); + if zero != 0 { return Err(Error::InvalidPrivateKeyPrefix); } - // For master keys (depth=0), both parent fingerprint and child number must be zero - let depth = data[4]; - if depth == 0 { - if !data[5..9].iter().all(|&b| b == 0) { - return Err(Error::NonZeroParentFingerprintForMasterKey); - } - - if !data[9..13].iter().all(|&b| b == 0) { - return Err(Error::NonZeroChildNumberForMasterKey); - } - } - Ok(Xpriv { network, - depth: data[4], - parent_fingerprint: data[5..9] - .try_into() - .expect("9 - 5 == 4, which is the Fingerprint length"), - child_number: u32::from_be_bytes(data[9..13].try_into().expect("4 byte slice")).into(), - chain_code: data[13..45] - .try_into() - .expect("45 - 13 == 32, which is the ChainCode length"), - private_key: secp256k1::SecretKey::from_byte_array( - &data[46..78].try_into().expect("Slice should be exactly 32 bytes"), - )?, + depth, + parent_fingerprint, + child_number, + chain_code, + private_key: secp256k1::SecretKey::from_byte_array(private_key)?, }) } @@ -769,7 +754,7 @@ impl Xpriv { /// Returns the first four bytes of the identifier pub fn fingerprint(&self, secp: &Secp256k1) -> Fingerprint { - self.identifier(secp).as_byte_array()[0..4].try_into().expect("4 is the fingerprint length") + self.identifier(secp).as_byte_array().sub_array::<0, 4>().into() } } @@ -849,7 +834,7 @@ impl Xpub { let hmac = engine.finalize(); let private_key = secp256k1::SecretKey::from_byte_array( - &hmac.as_ref()[..32].try_into().expect("Slice should be exactly 32 bytes"), + hmac.as_byte_array().split_array::<32, 32>().0 )?; let chain_code = ChainCode::from_hmac(hmac); Ok((private_key, chain_code)) @@ -878,30 +863,28 @@ impl Xpub { /// Decoding extended public key from binary data according to BIP 32 pub fn decode(data: &[u8]) -> Result { - if data.len() != 78 { - return Err(Error::WrongExtendedKeyLength(data.len())); - } + let Common { + network, + depth, + parent_fingerprint, + child_number, + chain_code, + key, + } = Common::decode(data)?; - let network = if data.starts_with(&VERSION_BYTES_MAINNET_PUBLIC) { - NetworkKind::Main - } else if data.starts_with(&VERSION_BYTES_TESTNETS_PUBLIC) { - NetworkKind::Test - } else { - let (b0, b1, b2, b3) = (data[0], data[1], data[2], data[3]); - return Err(Error::UnknownVersion([b0, b1, b2, b3])); + let network = match network { + VERSION_BYTES_MAINNET_PUBLIC => NetworkKind::Main, + VERSION_BYTES_TESTNETS_PUBLIC => NetworkKind::Test, + unknown => return Err(Error::UnknownVersion(unknown)), }; Ok(Xpub { network, - depth: data[4], - parent_fingerprint: data[5..9] - .try_into() - .expect("9 - 5 == 4, which is the Fingerprint length"), - child_number: u32::from_be_bytes(data[9..13].try_into().expect("4 byte slice")).into(), - chain_code: data[13..45] - .try_into() - .expect("45 - 13 == 32, which is the ChainCode length"), - public_key: secp256k1::PublicKey::from_slice(&data[45..78])?, + depth, + parent_fingerprint, + child_number, + chain_code, + public_key: secp256k1::PublicKey::from_slice(&key)?, }) } @@ -927,7 +910,7 @@ impl Xpub { /// Returns the first four bytes of the identifier pub fn fingerprint(&self) -> Fingerprint { - self.identifier().as_byte_array()[0..4].try_into().expect("4 is the fingerprint length") + self.identifier().as_byte_array().sub_array::<0, 4>().into() } } @@ -1004,6 +987,48 @@ impl fmt::Display for InvalidBase58PayloadLengthError { #[cfg(feature = "std")] impl std::error::Error for InvalidBase58PayloadLengthError {} +// Helps unify decoding +struct Common { + network: [u8; 4], + depth: u8, + parent_fingerprint: Fingerprint, + child_number: ChildNumber, + chain_code: ChainCode, + // public key (compressed) or 0 byte followed by a private key + key: [u8; 33], +} + +impl Common { + fn decode(data: &[u8]) -> Result { + let data: &[u8; 78] = data.try_into().map_err(|_| Error::WrongExtendedKeyLength(data.len()))?; + + let (&network, data) = data.split_array::<4, 74>(); + let (&depth, data) = data.split_first::<73>(); + let (&parent_fingerprint, data) = data.split_array::<4, 69>(); + let (&child_number, data) = data.split_array::<4, 65>(); + let (&chain_code, &key) = data.split_array::<32, 33>(); + + if depth == 0 { + if parent_fingerprint != [0u8; 4] { + return Err(Error::NonZeroParentFingerprintForMasterKey); + } + + if child_number != [0u8; 4] { + return Err(Error::NonZeroChildNumberForMasterKey); + } + } + + Ok(Common { + network, + depth, + parent_fingerprint: parent_fingerprint.into(), + child_number: u32::from_be_bytes(child_number).into(), + chain_code: chain_code.into(), + key, + }) + } +} + #[cfg(test)] mod tests { use hex::test_hex_unwrap as hex; diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index 862ed1fc7..1c4dc32ae 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -12,6 +12,7 @@ use core::str::FromStr; use hashes::hash160; use hex::{FromHex, HexToArrayError}; +use internals::array::ArrayExt; use internals::array_vec::ArrayVec; use internals::{impl_to_hex_from_lower_hex, write_err}; use io::{Read, Write}; @@ -505,20 +506,20 @@ impl PrivateKey { pub fn from_wif(wif: &str) -> Result { let data = base58::decode_check(wif)?; - let compressed = match data.len() { - 33 => false, - 34 => { - if data[33] != 1 { - return Err(InvalidWifCompressionFlagError { invalid: data[33] }.into()); - } - true - } - length => { - return Err(InvalidBase58PayloadLengthError { length }.into()); + let (compressed, data) = if let Ok(data) = <&[u8; 33]>::try_from(&*data) { + (false, data) + } else if let Ok(data) = <&[u8; 34]>::try_from(&*data) { + let (compressed_flag, data) = data.split_last::<33>(); + if *compressed_flag != 1 { + return Err(InvalidWifCompressionFlagError { invalid: *compressed_flag }.into()); } + (true, data) + } else { + return Err(InvalidBase58PayloadLengthError { length: data.len() }.into()); }; - let network = match data[0] { + let (network, key) = data.split_first(); + let network = match *network { 128 => NetworkKind::Main, 239 => NetworkKind::Test, invalid => { @@ -529,9 +530,7 @@ impl PrivateKey { Ok(PrivateKey { compressed, network, - inner: secp256k1::SecretKey::from_byte_array( - &data[1..33].try_into().expect("Slice should be exactly 32 bytes"), - )?, + inner: secp256k1::SecretKey::from_byte_array(key)?, }) } } diff --git a/bitcoin/src/crypto/taproot.rs b/bitcoin/src/crypto/taproot.rs index 7a19f579f..fe66028b8 100644 --- a/bitcoin/src/crypto/taproot.rs +++ b/bitcoin/src/crypto/taproot.rs @@ -10,6 +10,7 @@ use core::fmt; #[cfg(feature = "arbitrary")] use arbitrary::{Arbitrary, Unstructured}; use internals::write_err; +use internals::array::ArrayExt; use io::Write; use crate::prelude::Vec; @@ -29,23 +30,17 @@ pub struct Signature { impl Signature { /// Deserializes the signature from a slice. pub fn from_slice(sl: &[u8]) -> Result { - match sl.len() { - 64 => { - // default type - let signature = secp256k1::schnorr::Signature::from_byte_array( - sl[0..64].try_into().expect("Slice should be exactly 64 bytes"), - ); - Ok(Signature { signature, sighash_type: TapSighashType::Default }) - } - 65 => { - let (sighash_type, signature) = sl.split_last().expect("slice len checked == 65"); - let sighash_type = TapSighashType::from_consensus_u8(*sighash_type)?; - let signature = secp256k1::schnorr::Signature::from_byte_array( - signature[0..64].try_into().expect("Slice should be exactly 64 bytes"), - ); - Ok(Signature { signature, sighash_type }) - } - len => Err(SigFromSliceError::InvalidSignatureSize(len)), + if let Ok(signature) = <[u8; 64]>::try_from(sl) { + // default type + let signature = secp256k1::schnorr::Signature::from_byte_array(signature); + Ok(Signature { signature, sighash_type: TapSighashType::Default }) + } else if let Ok(signature) = <[u8; 65]>::try_from(sl) { + let (sighash_type, signature) = signature.split_last(); + let sighash_type = TapSighashType::from_consensus_u8(*sighash_type)?; + let signature = secp256k1::schnorr::Signature::from_byte_array(*signature); + Ok(Signature { signature, sighash_type }) + } else { + Err(SigFromSliceError::InvalidSignatureSize(sl.len())) } } @@ -144,9 +139,7 @@ impl<'a> Arbitrary<'a> for Signature { let arbitrary_bytes: [u8; secp256k1::constants::SCHNORR_SIGNATURE_SIZE] = u.arbitrary()?; Ok(Signature { - signature: secp256k1::schnorr::Signature::from_byte_array( - arbitrary_bytes[0..64].try_into().expect("Slice should be exactly 64 bytes"), - ), + signature: secp256k1::schnorr::Signature::from_byte_array(arbitrary_bytes), sighash_type: TapSighashType::arbitrary(u)?, }) } diff --git a/bitcoin/src/lib.rs b/bitcoin/src/lib.rs index 545604397..ed14a8bad 100644 --- a/bitcoin/src/lib.rs +++ b/bitcoin/src/lib.rs @@ -38,6 +38,7 @@ // Exclude lints we don't think are valuable. #![allow(clippy::needless_question_mark)] // https://github.com/rust-bitcoin/rust-bitcoin/pull/2134 #![allow(clippy::manual_range_contains)] // More readable than clippy's format. +#![allow(clippy::incompatible_msrv)] // Has FPs and we're testing it which is more reliable anyway. // We only support machines with index size of 4 bytes or more. // diff --git a/bitcoin/src/psbt/serialize.rs b/bitcoin/src/psbt/serialize.rs index 34086afab..4c2b0f481 100644 --- a/bitcoin/src/psbt/serialize.rs +++ b/bitcoin/src/psbt/serialize.rs @@ -7,6 +7,8 @@ use hashes::{hash160, ripemd160, sha256, sha256d}; use internals::compact_size; +#[allow(unused)] // MSRV polyfill +use internals::slice::SliceExt; use secp256k1::XOnlyPublicKey; use super::map::{Input, Map, Output, PsbtSighashType}; @@ -214,14 +216,12 @@ impl Serialize for KeySource { impl Deserialize for KeySource { fn deserialize(bytes: &[u8]) -> Result { - if bytes.len() < 4 { - return Err(io::Error::from(io::ErrorKind::UnexpectedEof).into()); - } + let (fingerprint, mut d) = bytes.split_first_chunk::<4>() + .ok_or(io::Error::from(io::ErrorKind::UnexpectedEof))?; - let fprint: Fingerprint = bytes[0..4].try_into().expect("4 is the fingerprint length"); + let fprint: Fingerprint = fingerprint.into(); let mut dpath: Vec = Default::default(); - let mut d = &bytes[4..]; while !d.is_empty() { match u32::consensus_decode(&mut d) { Ok(index) => dpath.push(index.into()), diff --git a/bitcoin/src/taproot/mod.rs b/bitcoin/src/taproot/mod.rs index a3358964a..b14264f24 100644 --- a/bitcoin/src/taproot/mod.rs +++ b/bitcoin/src/taproot/mod.rs @@ -13,7 +13,11 @@ use core::fmt; use core::iter::FusedIterator; use hashes::{hash_newtype, sha256t, sha256t_tag, HashEngine}; +use internals::array::ArrayExt; use internals::{impl_to_hex_from_lower_hex, write_err}; +#[allow(unused)] // MSRV polyfill +use internals::slice::SliceExt; + use io::Write; use secp256k1::{Scalar, Secp256k1}; @@ -1162,22 +1166,20 @@ 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 { - if sl.len() < TAPROOT_CONTROL_BASE_SIZE - || (sl.len() - TAPROOT_CONTROL_BASE_SIZE) % TAPROOT_CONTROL_NODE_SIZE != 0 - { - return Err(InvalidControlBlockSizeError(sl.len()).into()); - } - let output_key_parity = match sl[0] & 1 { + let (base, merkle_branch) = sl.split_first_chunk::() + .ok_or(InvalidControlBlockSizeError(sl.len()))?; + + let (&first, internal_key) = base.split_first(); + + let output_key_parity = match first & 1 { 0 => secp256k1::Parity::Even, _ => secp256k1::Parity::Odd, }; - let leaf_version = LeafVersion::from_consensus(sl[0] & TAPROOT_LEAF_MASK)?; - let internal_key = UntweakedPublicKey::from_byte_array( - &sl[1..TAPROOT_CONTROL_BASE_SIZE].try_into().expect("Slice should be exactly 32 bytes"), - ) - .map_err(TaprootError::InvalidInternalKey)?; - let merkle_branch = TaprootMerkleBranchBuf::decode(&sl[TAPROOT_CONTROL_BASE_SIZE..])?; + 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)?; Ok(ControlBlock { leaf_version, output_key_parity, internal_key, merkle_branch }) } } diff --git a/internals/src/array.rs b/internals/src/array.rs new file mode 100644 index 000000000..becbf6b64 --- /dev/null +++ b/internals/src/array.rs @@ -0,0 +1,105 @@ +//! Contains extensions related to arrays. + +/// Extension trait for arrays. +pub trait ArrayExt { + /// The item type the array is storing. + type Item; + + /// Just like the slicing operation, this returns an array `LEN` items long at position + /// `OFFSET`. + /// + /// The correctness of this operation is compile-time checked. + /// + /// Note that unlike slicing where the second number is the end index, here the second number + /// is array length! + fn sub_array(&self) -> &[Self::Item; LEN]; + + /// Returns an item at given statically-known index. + /// + /// This is just like normal indexing except the check happens at compile time. + fn get_static(&self) -> &Self::Item { + &self.sub_array::()[0] + } + + /// Returns the first item in an array. + /// + /// Fails to compile if the array is empty. + /// + /// Note that this method's name is intentionally shadowing the `std`'s `first` method which + /// returns `Option`. The rationale is that given the known length of the array, we always know + /// that this will not return `None` so trying to keep the `std` method around is pointless. + /// Importing the trait will also cause compile failures - that's also intentional to expose + /// the places where useless checks are made. + fn first(&self) -> &Self::Item { + self.get_static::<0>() + } + + /// Splits the array into two, non-overlaping smaller arrays covering the entire range. + /// + /// This is almost equivalent to just calling [`sub_array`](Self::sub_array) twice, except it also + /// checks that the arrays don't overlap and that they cover the full range. This is very useful + /// for demonstrating correctness, especially when chained. Using this technique even revealed + /// a bug in the past. ([#4195](https://github.com/rust-bitcoin/rust-bitcoin/issues/4195)) + fn split_array(&self) -> (&[Self::Item; LEFT], &[Self::Item; RIGHT]); + + /// Splits the array into the first element and the remaining, one element shorter, array. + /// + /// Fails to compile if the array is empty. + /// + /// Note that this method's name is intentionally shadowing the `std`'s `split_first` method which + /// returns `Option`. The rationale is that given the known length of the array, we always know + /// that this will not return `None` so trying to keep the `std` method around is pointless. + /// Importing the trait will also cause compile failures - that's also intentional to expose + /// the places where useless checks are made. + fn split_first(&self) -> (&Self::Item, &[Self::Item; RIGHT]) { + let (first, remaining) = self.split_array::<1, RIGHT>(); + (&first[0], remaining) + } + + /// Splits the array into the last element and the remaining, one element shorter, array. + /// + /// Fails to compile if the array is empty. + /// + /// Note that this method's name is intentionally shadowing the `std`'s `split_last` method which + /// returns `Option`. The rationale is that given the known length of the array, we always know + /// that this will not return `None` so trying to keep the `std` method around is pointless. + /// Importing the trait will also cause compile failures - that's also intentional to expose + /// the places where useless checks are made. + /// + /// The returned tuple is also reversed just as `std` for consistency and simpler diffs when + /// migrating. + fn split_last(&self) -> (&Self::Item, &[Self::Item; LEFT]) { + let (remaining, last) = self.split_array::(); + (&last[0], remaining) + } +} + +impl ArrayExt for [T; N] { + type Item = T; + + fn sub_array(&self) -> &[Self::Item; LEN] { + #[allow(clippy::let_unit_value)] + let _ = Hack::::IS_VALID_RANGE; + + self[OFFSET..(OFFSET + LEN)].try_into().expect("this is also compiler-checked above") + } + + fn split_array(&self) -> (&[Self::Item; LEFT], &[Self::Item; RIGHT]) { + #[allow(clippy::let_unit_value)] + let _ = Hack2::::IS_FULL_RANGE; + + (self.sub_array::<0, LEFT>(), self.sub_array::()) + } +} + +struct Hack; + +impl Hack { + const IS_VALID_RANGE: () = assert!(OFFSET + LEN <= N); +} + +struct Hack2; + +impl Hack2 { + const IS_FULL_RANGE: () = assert!(LEFT + RIGHT == N); +} diff --git a/internals/src/lib.rs b/internals/src/lib.rs index bed67bd2f..c2e2ce708 100644 --- a/internals/src/lib.rs +++ b/internals/src/lib.rs @@ -35,6 +35,7 @@ pub mod rust_version { include!(concat!(env!("OUT_DIR"), "/rust_version.rs")); } +pub mod array; pub mod array_vec; pub mod compact_size; pub mod const_tools; diff --git a/internals/src/slice.rs b/internals/src/slice.rs index 4c32126d0..b703238e4 100644 --- a/internals/src/slice.rs +++ b/internals/src/slice.rs @@ -26,6 +26,25 @@ pub trait SliceExt { fn bitcoin_as_chunks_mut( &mut self, ) -> (&mut [[Self::Item; N]], &mut [Self::Item]); + + /// Tries to access a sub-array of length `ARRAY_LEN` at the specified `offset`. + /// + /// Returns `None` in case of out-of-bounds access. + fn get_array(&self, offset: usize) -> Option<&[Self::Item; ARRAY_LEN]>; + + /// Splits the slice into an array and remainder if it's long enough. + /// + /// Returns `None` if the slice is shorter than `ARRAY_LEN` + #[allow(clippy::type_complexity)] // it's not really complex and redefining would make it + // harder to understand + fn split_first_chunk(&self) -> Option<(&[Self::Item; ARRAY_LEN], &[Self::Item])>; + + /// Splits the slice into a remainder and an array if it's long enough. + /// + /// Returns `None` if the slice is shorter than `ARRAY_LEN` + #[allow(clippy::type_complexity)] // it's not really complex and redefining would make it + // harder to understand + fn split_last_chunk(&self) -> Option<(&[Self::Item], &[Self::Item; ARRAY_LEN])>; } impl SliceExt for [T] { @@ -69,6 +88,27 @@ impl SliceExt for [T] { }; (left, right) } + + fn get_array(&self, offset: usize) -> Option<&[Self::Item; ARRAY_LEN]> { + self.get(offset..(offset + ARRAY_LEN)) + .map(|slice| slice.try_into().expect("the arguments to `get` evaluate to the same length the return type uses")) + } + + fn split_first_chunk(&self) -> Option<(&[Self::Item; ARRAY_LEN], &[Self::Item])> { + if self.len() < ARRAY_LEN { + return None; + } + let (first, remainder) = self.split_at(ARRAY_LEN); + Some((first.try_into().expect("we're passing `ARRAY_LEN` to `split_at` above"), remainder)) + } + + fn split_last_chunk(&self) -> Option<(&[Self::Item], &[Self::Item; ARRAY_LEN])> { + if self.len() < ARRAY_LEN { + return None; + } + let (remainder, last) = self.split_at(self.len() - ARRAY_LEN); + Some((remainder, last.try_into().expect("we're passing `self.len() - ARRAY_LEN` to `split_at` above"))) + } } struct Hack; diff --git a/primitives/src/witness.rs b/primitives/src/witness.rs index bf2a8f9f8..101c37caa 100644 --- a/primitives/src/witness.rs +++ b/primitives/src/witness.rs @@ -12,6 +12,7 @@ use arbitrary::{Arbitrary, Unstructured}; use hex::DisplayHex; use internals::compact_size; use internals::wrap_debug::WrapDebug; +use internals::slice::SliceExt; use crate::prelude::Vec; @@ -234,12 +235,7 @@ fn encode_cursor(bytes: &mut [u8], start_of_indices: usize, index: usize, value: #[inline] fn decode_cursor(bytes: &[u8], start_of_indices: usize, index: usize) -> Option { let start = start_of_indices + index * 4; - let end = start + 4; - if end > bytes.len() { - None - } else { - Some(u32::from_ne_bytes(bytes[start..end].try_into().expect("is u32 size")) as usize) - } + bytes.get_array::<4>(start).map(|index_bytes| u32::from_ne_bytes(*index_bytes) as usize) } /// Debug implementation that displays the witness as a structured output containing: From 437562e71cbb1e18179df3f5d01d2cc86961a150 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Sat, 15 Mar 2025 15:38:30 +0100 Subject: [PATCH 2/2] Add official BIP32 test vectors for invalid keys These are defined in the BIP as invalid. The previous commit fixed a bug where invalid key was parsed as valid and this bug can be caught by these vectors. Therefore, if this commit is ordered before the last one the test will fail. --- bitcoin/src/bip32.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 057efe451..73d74ffc8 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -1410,4 +1410,33 @@ mod tests { let xpriv_str = "xprv9s21ZrQH143K24Mfq5zL5MhWK9hUhhGbd45hLXo2Pq2oqzMMo63oStZzFAzHGBP2UuGCqWLTAPLcMtD9y5gkZ6Eq3Rjuahrv17fENZ3QzxW"; xpriv_str.parse::().unwrap(); } + + #[test] + fn official_vectors_5() { + let invalid_keys = [ + "xpub661MyMwAqRbcEYS8w7XLSVeEsBXy79zSzH1J8vCdxAZningWLdN3zgtU6LBpB85b3D2yc8sfvZU521AAwdZafEz7mnzBBsz4wKY5fTtTQBm", + "xprv9s21ZrQH143K24Mfq5zL5MhWK9hUhhGbd45hLXo2Pq2oqzMMo63oStZzFGTQQD3dC4H2D5GBj7vWvSQaaBv5cxi9gafk7NF3pnBju6dwKvH", + "xpub661MyMwAqRbcEYS8w7XLSVeEsBXy79zSzH1J8vCdxAZningWLdN3zgtU6Txnt3siSujt9RCVYsx4qHZGc62TG4McvMGcAUjeuwZdduYEvFn", + "xprv9s21ZrQH143K24Mfq5zL5MhWK9hUhhGbd45hLXo2Pq2oqzMMo63oStZzFGpWnsj83BHtEy5Zt8CcDr1UiRXuWCmTQLxEK9vbz5gPstX92JQ", + "xpub661MyMwAqRbcEYS8w7XLSVeEsBXy79zSzH1J8vCdxAZningWLdN3zgtU6N8ZMMXctdiCjxTNq964yKkwrkBJJwpzZS4HS2fxvyYUA4q2Xe4", + "xprv9s21ZrQH143K24Mfq5zL5MhWK9hUhhGbd45hLXo2Pq2oqzMMo63oStZzFAzHGBP2UuGCqWLTAPLcMtD9y5gkZ6Eq3Rjuahrv17fEQ3Qen6J", + "xprv9s2SPatNQ9Vc6GTbVMFPFo7jsaZySyzk7L8n2uqKXJen3KUmvQNTuLh3fhZMBoG3G4ZW1N2kZuHEPY53qmbZzCHshoQnNf4GvELZfqTUrcv", + "xpub661no6RGEX3uJkY4bNnPcw4URcQTrSibUZ4NqJEw5eBkv7ovTwgiT91XX27VbEXGENhYRCf7hyEbWrR3FewATdCEebj6znwMfQkhRYHRLpJ", + "xprv9s21ZrQH4r4TsiLvyLXqM9P7k1K3EYhA1kkD6xuquB5i39AU8KF42acDyL3qsDbU9NmZn6MsGSUYZEsuoePmjzsB3eFKSUEh3Gu1N3cqVUN", + "xpub661MyMwAuDcm6CRQ5N4qiHKrJ39Xe1R1NyfouMKTTWcguwVcfrZJaNvhpebzGerh7gucBvzEQWRugZDuDXjNDRmXzSZe4c7mnTK97pTvGS8", + "DMwo58pR1QLEFihHiXPVykYB6fJmsTeHvyTp7hRThAtCX8CvYzgPcn8XnmdfHGMQzT7ayAmfo4z3gY5KfbrZWZ6St24UVf2Qgo6oujFktLHdHY4", + "DMwo58pR1QLEFihHiXPVykYB6fJmsTeHvyTp7hRThAtCX8CvYzgPcn8XnmdfHPmHJiEDXkTiJTVV9rHEBUem2mwVbbNfvT2MTcAqj3nesx8uBf9", + "xprv9s21ZrQH143K24Mfq5zL5MhWK9hUhhGbd45hLXo2Pq2oqzMMo63oStZzF93Y5wvzdUayhgkkFoicQZcP3y52uPPxFnfoLZB21Teqt1VvEHx", + "xprv9s21ZrQH143K24Mfq5zL5MhWK9hUhhGbd45hLXo2Pq2oqzMMo63oStZzFAzHGBP2UuGCqWLTAPLcMtD5SDKr24z3aiUvKr9bJpdrcLg1y3G", + "xpub661MyMwAqRbcEYS8w7XLSVeEsBXy79zSzH1J8vCdxAZningWLdN3zgtU6Q5JXayek4PRsn35jii4veMimro1xefsM58PgBMrvdYre8QyULY", + "xprv9s21ZrQH143K3QTDL4LXw2F7HEK3wJUD2nW2nRk4stbPy6cq3jPPqjiChkVvvNKmPGJxWUtg6LnF5kejMRNNU3TGtRBeJgk33yuGBxrMPHL", + ]; + for key in invalid_keys { + if key.starts_with("xpub") { + key.parse::().unwrap_err(); + } else { + key.parse::().unwrap_err(); + } + } + } }