Skip to content

Commit 4a38059

Browse files
committed
Merge #280: Backport slicing improvements to 0.25.x
982a50f Disable fuzztest in CI (Philip Robinson) ad00563 Bump version to 0.25.3 (Philip Robinson) 6275989 Fix docstring in blech32 (Philip Robinson) 675ca26 Pin deps for Fuzztest job (Philip Robinson) ea42e11 add `--no-deps` to doc test (Philip Robinson) 70276cc pin bitcoin crates in test.sh (Philip Robinson) 2941a39 Remove `-- -D warnings` from test.sh (Philip Robinson) 250c4f6 pset: fix slicing in KeySource::deserialize (Philip Robinson) b06dac9 address: do a better job slicing bech32 data (Philip Robinson) 2a8b46f transaction: use better error typing for pegin destructuring (Philip Robinson) 8b15649 blind: use array splitting in TxOut::unblind (fix potential DoS?) (Philip Robinson) 179c6da rewrite Address::from_base58 to eliminate all the unwraps (Philip Robinson) 8cd2a4d Add arrays.rs and slices.rs from bitcoin-internals 0.5 (Philip Robinson) Pull request description: There are a number of useful improvements from #279 we would like to include in 0.25.x. ACKs for top commit: apoelstra: utACK 982a50f Tree-SHA512: 51f786c01982d107193f9e75be13a1087a24372664eb70fbbb0eb3ee3d2bfbf64b94ac573a22fe20d412c717f54d5c3a37a4010099fe6d56892fdd486e026c62
2 parents 378e753 + 982a50f commit 4a38059

15 files changed

Lines changed: 422 additions & 62 deletions

File tree

.github/workflows/rust.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ jobs:
8484
env:
8585
DO_DOCS: false
8686
DO_DOCSRS: false
87-
DO_FUZZ: true
87+
DO_FUZZ: false
8888
DO_INTEGRATION: false
8989
DO_LINT: false
9090
DO_FEATURE_MATRIX: false

Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "elements"
3-
version = "0.25.2"
3+
version = "0.25.3"
44
authors = ["Andrew Poelstra <apoelstra@blockstream.com>"]
55
description = "Library with support for de/serialization, parsing and executing on data structures and network messages related to Elements"
66
license = "CC0-1.0"

contrib/test.sh

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,17 @@ if cargo --version | grep "1\.56"; then
1313
cargo update -p serde_json --precise 1.0.98
1414
cargo update -p serde --precise 1.0.156
1515
cargo update -p ppv-lite86 --precise 0.2.8
16+
cargo update -p bitcoin --precise 0.32.2
17+
cargo update -p bitcoin-units --precise 0.1.2
18+
cargo update -p bitcoin-io --precise 0.1.2
19+
cargo update -p bitcoin_hashes --precise 0.14.2
20+
cargo update -p base58ck --precise 0.1.0
21+
cargo update -p itoa --precise 1.0.6
22+
cargo update -p ryu --precise 1.0.9
23+
cargo update -p getrandom --precise 0.2.3
24+
cargo update -p quote --precise 1.0.35
25+
cargo update -p proc-macro2 --precise 1.0.80
26+
cargo update -p unicode-ident --precise 1.0.6
1627
fi
1728

1829
if [ "$DO_FEATURE_MATRIX" = true ]
@@ -38,18 +49,18 @@ fi
3849

3950
if [ "$DO_LINT" = true ]
4051
then
41-
cargo clippy --all-features --all-targets -- -D warnings
52+
cargo clippy --all-features --all-targets
4253
fi
4354

4455
# Build the docs if told to (this only works with the nightly toolchain)
4556
if [ "$DO_DOCSRS" = true ]; then
46-
RUSTDOCFLAGS="--cfg docsrs -D warnings -D rustdoc::broken-intra-doc-links" cargo +nightly doc --all-features
57+
RUSTDOCFLAGS="--cfg docsrs -D rustdoc::broken-intra-doc-links" cargo +nightly doc --no-deps --all-features
4758
fi
4859

4960
# Build the docs with a stable toolchain, in unison with the DO_DOCSRS command
5061
# above this checks that we feature guarded docs imports correctly.
5162
if [ "$DO_DOCS" = true ]; then
52-
RUSTDOCFLAGS="-D warnings" cargo +stable doc --all-features
63+
cargo +stable doc --all-features
5364
fi
5465

5566

@@ -58,8 +69,23 @@ if [ "$DO_FUZZ" = true ]
5869
then
5970
(
6071
cd fuzz
61-
if cargo --version | grep "1\.58"; then
72+
if cargo --version | grep "1\.63"; then
6273
cargo update -p cc --precise 1.0.94
74+
cargo update -p bitcoin --precise 0.32.2
75+
cargo update -p bitcoin-units --precise 0.1.2
76+
cargo update -p bitcoin-io --precise 0.1.2
77+
cargo update -p bitcoin_hashes --precise 0.14.2
78+
cargo update -p base58ck --precise 0.1.0
79+
cargo update -p itoa --precise 1.0.6
80+
cargo update -p unicode-ident --precise 1.0.6
81+
cargo update -p serde_json --precise 1.0.98
82+
cargo update -p serde --precise 1.0.156
83+
cargo update -p ppv-lite86 --precise 0.2.8
84+
cargo update -p quote --precise 1.0.28
85+
cargo update -p proc-macro2 --precise 1.0.66
86+
cargo update -p libc --precise 0.2.163
87+
cargo update -p ryu --precise 1.0.9
88+
cargo update -p honggfuzz --precise 0.5.54
6389
fi
6490
cargo test --verbose
6591
./travis-fuzz.sh

fuzz/travis-fuzz.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
#!/bin/bash
22
set -e
3-
cargo install --force honggfuzz --no-default-features
3+
cargo install --force honggfuzz --no-default-features --version 0.5.54
44
for TARGET in fuzz_targets/*; do
55
FILENAME=$(basename $TARGET)
66
FILE="${FILENAME%.*}"

src/address.rs

Lines changed: 36 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ use crate::blech32::{Blech32, Blech32m};
2626
use crate::hashes::Hash;
2727
use bitcoin::base58;
2828
use bitcoin::PublicKey;
29+
use crate::internals::array::ArrayExt as _;
30+
use crate::internals::slice::SliceExt;
2931
use secp256k1_zkp;
3032
use secp256k1_zkp::Secp256k1;
3133
use secp256k1_zkp::Verification;
@@ -468,13 +470,17 @@ impl Address {
468470
};
469471

470472
let (blinding_pubkey, program) = match blinded {
471-
true => (
473+
true => {
474+
let (pk, rest) = SliceExt::split_first_chunk::<33>(data.as_slice())
475+
.ok_or(AddressError::InvalidSegwitV0Encoding)?;
476+
(
472477
Some(
473-
secp256k1_zkp::PublicKey::from_slice(&data[..33])
478+
secp256k1_zkp::PublicKey::from_slice(pk)
474479
.map_err(AddressError::InvalidBlindingPubKey)?,
475-
),
476-
data[33..].to_vec(),
477-
),
480+
),
481+
rest.to_vec(),
482+
)
483+
},
478484
false => (None, data),
479485
};
480486

@@ -487,43 +493,41 @@ impl Address {
487493

488494
// data.len() should be >= 1 when this method is called
489495
fn from_base58(data: &[u8], params: &'static AddressParams) -> Result<Address, AddressError> {
496+
let len_error = AddressError::InvalidLength(data.len());
490497
// When unblinded, the structure is:
491498
// <1: regular prefix> <20: hash160>
492499
// When blinded, the structure is:
493500
// <1: blinding prefix> <1: regular prefix> <33: blinding pubkey> <20: hash160>
494501

495-
let (blinded, prefix) = match data[0] == params.blinded_prefix {
496-
true => {
497-
if data.len() != 55 {
498-
return Err(AddressError::InvalidLength(data.len()));
499-
}
500-
(true, data[1])
501-
}
502-
false => {
503-
if data.len() != 21 {
504-
return Err(AddressError::InvalidLength(data.len()));
505-
}
506-
(false, data[0])
507-
}
502+
let (blinding_prefix, blinded_data) = match data.split_first() {
503+
Some(v) => v,
504+
None => return Err(len_error),
508505
};
509506

510-
let (blinding_pubkey, payload_data) = match blinded {
511-
true => (
512-
Some(
513-
secp256k1_zkp::PublicKey::from_slice(&data[2..35])
514-
.map_err(AddressError::InvalidBlindingPubKey)?,
515-
),
516-
&data[35..],
517-
),
518-
false => (None, &data[1..]),
507+
let (prefix, blinding_pubkey, hash) = if *blinding_prefix == params.blinded_prefix {
508+
let (prefix, pubkey_and_hash) = match blinded_data.split_first() {
509+
Some(v) => v,
510+
None => return Err(len_error),
511+
};
512+
513+
let pubkey_and_hash = <&[u8; 53]>::try_from(pubkey_and_hash).map_err(|_| len_error)?;
514+
let (pubkey, hash) = pubkey_and_hash.split_array::<33, 20>();
515+
516+
let blinding_pubkey = secp256k1_zkp::PublicKey::from_slice(pubkey)
517+
.map_err(AddressError::InvalidBlindingPubKey)?;
518+
519+
(prefix, Some(blinding_pubkey), hash)
520+
} else {
521+
let hash = <&[u8; 20]>::try_from(blinded_data).map_err(|_| len_error)?;
522+
(blinding_prefix, None, hash)
519523
};
520524

521-
let payload = if prefix == params.p2pkh_prefix {
522-
Payload::PubkeyHash(PubkeyHash::from_slice(payload_data).unwrap())
523-
} else if prefix == params.p2sh_prefix {
524-
Payload::ScriptHash(ScriptHash::from_slice(payload_data).unwrap())
525+
let payload = if *prefix == params.p2pkh_prefix {
526+
Payload::PubkeyHash(PubkeyHash::from_byte_array(*hash))
527+
} else if *prefix == params.p2sh_prefix {
528+
Payload::ScriptHash(ScriptHash::from_byte_array(*hash))
525529
} else {
526-
return Err(AddressError::InvalidAddressVersion(prefix));
530+
return Err(AddressError::InvalidAddressVersion(*prefix));
527531
};
528532

529533
Ok(Address {

src/blech32/decode.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ const SEP: char = '1';
8282
/// checksum in any way.
8383
///
8484
/// Unless you are attempting to validate a string with multiple checksums then you likely do not
85-
/// want to use this type directly, instead use [`CheckedHrpstring::new(s)`].
85+
/// want to use this type directly, instead use [`CheckedHrpstring::new`].
8686
#[derive(Debug)]
8787
pub struct UncheckedHrpstring<'s> {
8888
/// The human-readable part, guaranteed to be lowercase ASCII characters.

src/blind.rs

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
//! # Transactions Blinding
1616
//!
1717
18+
use crate::internals::array::ArrayExt as _;
19+
use crate::internals::slice::SliceExt;
1820
use std::{self, collections::BTreeMap, fmt};
1921

2022
use secp256k1_zkp::{
@@ -774,15 +776,31 @@ impl TxOut {
774776
additional_generator,
775777
)?;
776778

777-
let (asset, asset_bf) = opening.message.as_ref().split_at(32);
778-
let asset = AssetId::from_slice(asset)?;
779-
let asset_bf = AssetBlindingFactor::from_slice(&asset_bf[..32])?;
779+
// Use `MissingRangeproof` error because it's available so does not require
780+
// API breaks. In a later PR we should extend that enum and add #[non_exhaustive]
781+
// to it. The maybe-better `MalformedAssetId` error requires we start with a
782+
// std `FromSliceError` which we don't have.
783+
let asset_and_bf = SliceExt::split_first_chunk::<64>(opening.message.as_ref())
784+
.ok_or(UnblindError::MissingRangeproof)?
785+
.0;
786+
let (asset_id, asset_bf) = asset_and_bf.split_array();
787+
788+
let asset_id = AssetId::from_byte_array(*asset_id);
789+
let asset_bf = AssetBlindingFactor::from_byte_array(*asset_bf)?;
790+
if let Asset::Confidential(own_asset) = self.asset {
791+
let secp = Secp256k1::signing_only(); // needed to avoid API break
792+
let asset = Generator::new_blinded(&secp, asset_id.into_tag(), asset_bf.into_inner());
793+
if asset != own_asset {
794+
// See above about use of MissingRangeproof.
795+
return Err(UnblindError::MissingRangeproof);
796+
}
797+
}
780798

781799
let value = opening.value;
782800
let value_bf = ValueBlindingFactor(opening.blinding_factor);
783801

784802
Ok(TxOutSecrets {
785-
asset,
803+
asset: asset_id,
786804
asset_bf,
787805
value,
788806
value_bf,

src/confidential.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -746,6 +746,11 @@ impl AssetBlindingFactor {
746746
AssetBlindingFactor(Tweak::new(rng))
747747
}
748748

749+
/// Create from bytes.
750+
pub fn from_byte_array(bytes: [u8; 32]) -> Result<Self, secp256k1_zkp::Error> {
751+
Ok(AssetBlindingFactor(Tweak::from_inner(bytes)?))
752+
}
753+
749754
/// Create from bytes.
750755
pub fn from_slice(bytes: &[u8]) -> Result<Self, secp256k1_zkp::Error> {
751756
Ok(AssetBlindingFactor(Tweak::from_slice(bytes)?))

src/internals/array.rs

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
//! Contains extensions related to arrays.
2+
3+
use std::convert::TryInto;
4+
5+
/// Extension trait for arrays.
6+
pub trait ArrayExt {
7+
/// The item type the array is storing.
8+
type Item;
9+
10+
/// Just like the slicing operation, this returns an array `LEN` items long at position
11+
/// `OFFSET`.
12+
///
13+
/// The correctness of this operation is compile-time checked.
14+
///
15+
/// Note that unlike slicing where the second number is the end index, here the second number
16+
/// is array length!
17+
fn sub_array<const OFFSET: usize, const LEN: usize>(&self) -> &[Self::Item; LEN];
18+
19+
/// Returns an item at given statically-known index.
20+
///
21+
/// This is just like normal indexing except the check happens at compile time.
22+
fn get_static<const INDEX: usize>(&self) -> &Self::Item { &self.sub_array::<INDEX, 1>()[0] }
23+
24+
/// Returns the first item in an array.
25+
///
26+
/// Fails to compile if the array is empty.
27+
///
28+
/// Note that this method's name intentionally shadows the `std`'s `first` method which
29+
/// returns `Option`. The rationale is that given the known length of the array, we always know
30+
/// that this will not return `None` so trying to keep the `std` method around is pointless.
31+
/// Importing the trait will also cause compile failures - that's also intentional to expose
32+
/// the places where useless checks are made.
33+
fn first(&self) -> &Self::Item { self.get_static::<0>() }
34+
35+
/// Splits the array into two, non-overlapping smaller arrays covering the entire range.
36+
///
37+
/// This is almost equivalent to just calling [`sub_array`](Self::sub_array) twice, except it also
38+
/// checks that the arrays don't overlap and that they cover the full range. This is very useful
39+
/// for demonstrating correctness, especially when chained. Using this technique even revealed
40+
/// a bug in the past. ([#4195](https://github.com/rust-bitcoin/rust-bitcoin/issues/4195))
41+
fn split_array<const LEFT: usize, const RIGHT: usize>(
42+
&self,
43+
) -> (&[Self::Item; LEFT], &[Self::Item; RIGHT]);
44+
45+
/// Splits the array into the first element and the remaining, one element shorter, array.
46+
///
47+
/// Fails to compile if the array is empty.
48+
///
49+
/// Note that this method's name intentionally shadows the `std`'s `split_first` method which
50+
/// returns `Option`. The rationale is that given the known length of the array, we always know
51+
/// that this will not return `None` so trying to keep the `std` method around is pointless.
52+
/// Importing the trait will also cause compile failures - that's also intentional to expose
53+
/// the places where useless checks are made.
54+
fn split_first<const RIGHT: usize>(&self) -> (&Self::Item, &[Self::Item; RIGHT]) {
55+
let (first, remaining) = self.split_array::<1, RIGHT>();
56+
(&first[0], remaining)
57+
}
58+
59+
/// Splits the array into the last element and the remaining, one element shorter, array.
60+
///
61+
/// Fails to compile if the array is empty.
62+
///
63+
/// Note that this method's name intentionally shadows the `std`'s `split_last` method which
64+
/// returns `Option`. The rationale is that given the known length of the array, we always know
65+
/// that this will not return `None` so trying to keep the `std` method around is pointless.
66+
/// Importing the trait will also cause compile failures - that's also intentional to expose
67+
/// the places where useless checks are made.
68+
///
69+
/// The returned tuple is also reversed just as `std` for consistency and simpler diffs when
70+
/// migrating.
71+
fn split_last<const LEFT: usize>(&self) -> (&Self::Item, &[Self::Item; LEFT]) {
72+
let (remaining, last) = self.split_array::<LEFT, 1>();
73+
(&last[0], remaining)
74+
}
75+
}
76+
77+
impl<T, const N: usize> ArrayExt for [T; N] {
78+
type Item = T;
79+
80+
fn sub_array<const OFFSET: usize, const LEN: usize>(&self) -> &[Self::Item; LEN] {
81+
#[allow(clippy::let_unit_value)]
82+
let () = Hack::<N, OFFSET, LEN>::IS_VALID_RANGE;
83+
84+
self[OFFSET..(OFFSET + LEN)].try_into().expect("this is also compiler-checked above")
85+
}
86+
87+
fn split_array<const LEFT: usize, const RIGHT: usize>(
88+
&self,
89+
) -> (&[Self::Item; LEFT], &[Self::Item; RIGHT]) {
90+
#[allow(clippy::let_unit_value)]
91+
let () = Hack2::<N, LEFT, RIGHT>::IS_FULL_RANGE;
92+
93+
(self.sub_array::<0, LEFT>(), self.sub_array::<LEFT, RIGHT>())
94+
}
95+
}
96+
97+
struct Hack<const N: usize, const OFFSET: usize, const LEN: usize>;
98+
99+
impl<const N: usize, const OFFSET: usize, const LEN: usize> Hack<N, OFFSET, LEN> {
100+
const IS_VALID_RANGE: () = [()][(OFFSET + LEN > N) as usize];
101+
}
102+
103+
struct Hack2<const N: usize, const LEFT: usize, const RIGHT: usize>;
104+
105+
impl<const N: usize, const LEFT: usize, const RIGHT: usize> Hack2<N, LEFT, RIGHT> {
106+
const IS_FULL_RANGE: () = [()][(LEFT + RIGHT != N) as usize];
107+
}

src/internals/mod.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
// SPDX-License-Identifier: CC0-1.0
2+
3+
//! These modules are copied from the bitcoin-internals 0.5 crate from rust-bitcoin
4+
pub mod array;
5+
pub mod slice;

0 commit comments

Comments
 (0)