Backport slicing improvements to 0.25.x

.github/workflows/rust.yml

@@ -84,7 +84,7 @@ jobs:
         env:
           DO_DOCS: false
           DO_DOCSRS: false
-          DO_FUZZ: true
+          DO_FUZZ: false
           DO_INTEGRATION: false
           DO_LINT: false
           DO_FEATURE_MATRIX: false

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "elements"
-version = "0.25.2"
+version = "0.25.3"
 authors = ["Andrew Poelstra <apoelstra@blockstream.com>"]
 description = "Library with support for de/serialization, parsing and executing on data structures and network messages related to Elements"
 license = "CC0-1.0"

contrib/test.sh

@@ -13,6 +13,17 @@ if cargo --version | grep "1\.56"; then
     cargo update -p serde_json --precise 1.0.98
     cargo update -p serde --precise 1.0.156
     cargo update -p ppv-lite86 --precise 0.2.8
+    cargo update -p bitcoin --precise 0.32.2
+    cargo update -p bitcoin-units --precise 0.1.2
+    cargo update -p bitcoin-io --precise 0.1.2
+    cargo update -p bitcoin_hashes --precise 0.14.2
+    cargo update -p base58ck --precise 0.1.0
+    cargo update -p itoa --precise 1.0.6
+    cargo update -p ryu --precise 1.0.9
+    cargo update -p getrandom --precise 0.2.3
+    cargo update -p quote --precise 1.0.35
+    cargo update -p proc-macro2 --precise 1.0.80
+    cargo update -p unicode-ident --precise 1.0.6
 fi
 
 if [ "$DO_FEATURE_MATRIX" = true ]
@@ -38,18 +49,18 @@ fi
 
 if [ "$DO_LINT" = true ]
 then
-    cargo clippy --all-features --all-targets -- -D warnings
+    cargo clippy --all-features --all-targets
 fi
 
 # Build the docs if told to (this only works with the nightly toolchain)
 if [ "$DO_DOCSRS" = true ]; then
-    RUSTDOCFLAGS="--cfg docsrs -D warnings -D rustdoc::broken-intra-doc-links" cargo +nightly doc --all-features
+    RUSTDOCFLAGS="--cfg docsrs -D rustdoc::broken-intra-doc-links" cargo +nightly doc --no-deps --all-features
 fi
 
 # Build the docs with a stable toolchain, in unison with the DO_DOCSRS command
 # above this checks that we feature guarded docs imports correctly.
 if [ "$DO_DOCS" = true ]; then
-    RUSTDOCFLAGS="-D warnings" cargo +stable doc --all-features
+    cargo +stable doc --all-features
 fi
 
 
@@ -58,8 +69,23 @@ if [ "$DO_FUZZ" = true ]
 then
     (
         cd fuzz
-        if cargo --version | grep "1\.58"; then
+        if cargo --version | grep "1\.63"; then
             cargo update -p cc --precise 1.0.94
+            cargo update -p bitcoin --precise 0.32.2
+            cargo update -p bitcoin-units --precise 0.1.2
+            cargo update -p bitcoin-io --precise 0.1.2
+            cargo update -p bitcoin_hashes --precise 0.14.2
+            cargo update -p base58ck --precise 0.1.0
+            cargo update -p itoa --precise 1.0.6
+            cargo update -p unicode-ident --precise 1.0.6
+            cargo update -p serde_json --precise 1.0.98
+            cargo update -p serde --precise 1.0.156
+            cargo update -p ppv-lite86 --precise 0.2.8
+            cargo update -p quote --precise 1.0.28
+            cargo update -p proc-macro2 --precise 1.0.66
+            cargo update -p libc --precise 0.2.163
+            cargo update -p ryu --precise 1.0.9
+            cargo update -p honggfuzz --precise 0.5.54
         fi
         cargo test --verbose
         ./travis-fuzz.sh

fuzz/travis-fuzz.sh

@@ -1,6 +1,6 @@
 #!/bin/bash
 set -e
-cargo install --force honggfuzz --no-default-features
+cargo install --force honggfuzz --no-default-features --version 0.5.54
 for TARGET in fuzz_targets/*; do
     FILENAME=$(basename $TARGET)
 	FILE="${FILENAME%.*}"

src/address.rs

@@ -26,6 +26,8 @@ use crate::blech32::{Blech32, Blech32m};
 use crate::hashes::Hash;
 use bitcoin::base58;
 use bitcoin::PublicKey;
+use crate::internals::array::ArrayExt as _;
+use crate::internals::slice::SliceExt;
 use secp256k1_zkp;
 use secp256k1_zkp::Secp256k1;
 use secp256k1_zkp::Verification;
@@ -468,13 +470,17 @@ impl Address {
         };
 
         let (blinding_pubkey, program) = match blinded {
-            true => (
+            true => {
+                let (pk, rest) = SliceExt::split_first_chunk::<33>(data.as_slice())
+                    .ok_or(AddressError::InvalidSegwitV0Encoding)?;
+                (
                 Some(
-                    secp256k1_zkp::PublicKey::from_slice(&data[..33])
+                    secp256k1_zkp::PublicKey::from_slice(pk)
                         .map_err(AddressError::InvalidBlindingPubKey)?,
-                ),
-                data[33..].to_vec(),
-            ),
+                    ),
+                    rest.to_vec(),
+                )
+            },
             false => (None, data),
         };
 
@@ -487,43 +493,41 @@ impl Address {
 
     // data.len() should be >= 1 when this method is called
     fn from_base58(data: &[u8], params: &'static AddressParams) -> Result<Address, AddressError> {
+        let len_error = AddressError::InvalidLength(data.len());
         // When unblinded, the structure is:
         // <1: regular prefix> <20: hash160>
         // When blinded, the structure is:
         // <1: blinding prefix> <1: regular prefix> <33: blinding pubkey> <20: hash160>
 
-        let (blinded, prefix) = match data[0] == params.blinded_prefix {
-            true => {
-                if data.len() != 55 {
-                    return Err(AddressError::InvalidLength(data.len()));
-                }
-                (true, data[1])
-            }
-            false => {
-                if data.len() != 21 {
-                    return Err(AddressError::InvalidLength(data.len()));
-                }
-                (false, data[0])
-            }
+        let (blinding_prefix, blinded_data) = match data.split_first() {
+            Some(v) => v,
+            None => return Err(len_error),
         };
 
-        let (blinding_pubkey, payload_data) = match blinded {
-            true => (
-                Some(
-                    secp256k1_zkp::PublicKey::from_slice(&data[2..35])
-                        .map_err(AddressError::InvalidBlindingPubKey)?,
-                ),
-                &data[35..],
-            ),
-            false => (None, &data[1..]),
+        let (prefix, blinding_pubkey, hash) = if *blinding_prefix == params.blinded_prefix {
+            let (prefix, pubkey_and_hash) = match blinded_data.split_first() {
+                Some(v) => v,
+                None => return Err(len_error),
+            };
+
+            let pubkey_and_hash = <&[u8; 53]>::try_from(pubkey_and_hash).map_err(|_| len_error)?;
+            let (pubkey, hash) = pubkey_and_hash.split_array::<33, 20>();
+
+            let blinding_pubkey = secp256k1_zkp::PublicKey::from_slice(pubkey)
+                .map_err(AddressError::InvalidBlindingPubKey)?;
+
+            (prefix, Some(blinding_pubkey), hash)
+        } else {
+            let hash = <&[u8; 20]>::try_from(blinded_data).map_err(|_| len_error)?;
+            (blinding_prefix, None, hash)
         };
 
-        let payload = if prefix == params.p2pkh_prefix {
-            Payload::PubkeyHash(PubkeyHash::from_slice(payload_data).unwrap())
-        } else if prefix == params.p2sh_prefix {
-            Payload::ScriptHash(ScriptHash::from_slice(payload_data).unwrap())
+        let payload = if *prefix == params.p2pkh_prefix {
+            Payload::PubkeyHash(PubkeyHash::from_byte_array(*hash))
+        } else if *prefix == params.p2sh_prefix {
+            Payload::ScriptHash(ScriptHash::from_byte_array(*hash))
         } else {
-            return Err(AddressError::InvalidAddressVersion(prefix));
+            return Err(AddressError::InvalidAddressVersion(*prefix));
         };
 
         Ok(Address {

src/blech32/decode.rs

@@ -82,7 +82,7 @@ const SEP: char = '1';
 /// checksum in any way.
 ///
 /// Unless you are attempting to validate a string with multiple checksums then you likely do not
-/// want to use this type directly, instead use [`CheckedHrpstring::new(s)`].
+/// want to use this type directly, instead use [`CheckedHrpstring::new`].
 #[derive(Debug)]
 pub struct UncheckedHrpstring<'s> {
     /// The human-readable part, guaranteed to be lowercase ASCII characters.

src/blind.rs

@@ -15,6 +15,8 @@
 //! # Transactions Blinding
 //!
 
+use crate::internals::array::ArrayExt as _;
+use crate::internals::slice::SliceExt;
 use std::{self, collections::BTreeMap, fmt};
 
 use secp256k1_zkp::{
@@ -774,15 +776,31 @@ impl TxOut {
             additional_generator,
         )?;
 
-        let (asset, asset_bf) = opening.message.as_ref().split_at(32);
-        let asset = AssetId::from_slice(asset)?;
-        let asset_bf = AssetBlindingFactor::from_slice(&asset_bf[..32])?;
+        // Use `MissingRangeproof` error because it's available so does not require
+        // API breaks. In a later PR we should extend that enum and add #[non_exhaustive]
+        // to it. The maybe-better `MalformedAssetId` error requires we start with a
+        // std `FromSliceError` which we don't have.
+        let asset_and_bf = SliceExt::split_first_chunk::<64>(opening.message.as_ref())
+            .ok_or(UnblindError::MissingRangeproof)?
+            .0;
+        let (asset_id, asset_bf) = asset_and_bf.split_array();
+
+        let asset_id = AssetId::from_byte_array(*asset_id);
+        let asset_bf = AssetBlindingFactor::from_byte_array(*asset_bf)?;
+        if let Asset::Confidential(own_asset) = self.asset {
+            let secp = Secp256k1::signing_only(); // needed to avoid API break
+            let asset = Generator::new_blinded(&secp, asset_id.into_tag(), asset_bf.into_inner());
+            if asset != own_asset {
+                // See above about use of MissingRangeproof.
+                return Err(UnblindError::MissingRangeproof);
+            }
+        }
 
         let value = opening.value;
         let value_bf = ValueBlindingFactor(opening.blinding_factor);
 
         Ok(TxOutSecrets {
-            asset,
+            asset: asset_id,
             asset_bf,
             value,
             value_bf,

src/confidential.rs

@@ -746,6 +746,11 @@ impl AssetBlindingFactor {
         AssetBlindingFactor(Tweak::new(rng))
     }
 
+    /// Create from bytes.
+    pub fn from_byte_array(bytes: [u8; 32]) -> Result<Self, secp256k1_zkp::Error> {
+        Ok(AssetBlindingFactor(Tweak::from_inner(bytes)?))
+    }
+
     /// Create from bytes.
     pub fn from_slice(bytes: &[u8]) -> Result<Self, secp256k1_zkp::Error> {
         Ok(AssetBlindingFactor(Tweak::from_slice(bytes)?))

src/internals/array.rs

@@ -0,0 +1,107 @@
+//! Contains extensions related to arrays.
+
+use std::convert::TryInto;
+
+/// 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<const OFFSET: usize, const LEN: usize>(&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<const INDEX: usize>(&self) -> &Self::Item { &self.sub_array::<INDEX, 1>()[0] }
+
+    /// Returns the first item in an array.
+    ///
+    /// Fails to compile if the array is empty.
+    ///
+    /// Note that this method's name intentionally shadows 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-overlapping 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<const LEFT: usize, const RIGHT: usize>(
+        &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 intentionally shadows 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<const RIGHT: usize>(&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 intentionally shadows 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<const LEFT: usize>(&self) -> (&Self::Item, &[Self::Item; LEFT]) {
+        let (remaining, last) = self.split_array::<LEFT, 1>();
+        (&last[0], remaining)
+    }
+}
+
+impl<T, const N: usize> ArrayExt for [T; N] {
+    type Item = T;
+
+    fn sub_array<const OFFSET: usize, const LEN: usize>(&self) -> &[Self::Item; LEN] {
+        #[allow(clippy::let_unit_value)]
+        let () = Hack::<N, OFFSET, LEN>::IS_VALID_RANGE;
+
+        self[OFFSET..(OFFSET + LEN)].try_into().expect("this is also compiler-checked above")
+    }
+
+    fn split_array<const LEFT: usize, const RIGHT: usize>(
+        &self,
+    ) -> (&[Self::Item; LEFT], &[Self::Item; RIGHT]) {
+        #[allow(clippy::let_unit_value)]
+        let () = Hack2::<N, LEFT, RIGHT>::IS_FULL_RANGE;
+
+        (self.sub_array::<0, LEFT>(), self.sub_array::<LEFT, RIGHT>())
+    }
+}
+
+struct Hack<const N: usize, const OFFSET: usize, const LEN: usize>;
+
+impl<const N: usize, const OFFSET: usize, const LEN: usize> Hack<N, OFFSET, LEN> {
+    const IS_VALID_RANGE: () = [()][(OFFSET + LEN > N) as usize];
+}
+
+struct Hack2<const N: usize, const LEFT: usize, const RIGHT: usize>;
+
+impl<const N: usize, const LEFT: usize, const RIGHT: usize> Hack2<N, LEFT, RIGHT> {
+    const IS_FULL_RANGE: () = [()][(LEFT + RIGHT != N) as usize];
+}

src/internals/mod.rs

@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: CC0-1.0
+
+//! These modules are copied from the bitcoin-internals 0.5 crate from rust-bitcoin
+pub mod array;
+pub mod slice;