Skip to content

Commit

Permalink
primitives: introduce InvalidResidueError
Browse files Browse the repository at this point in the history
We are going to want to extend the ChecksumError::InvalidResidue error
variant to allow error correction, and in order to do so, we need to know
the actual residue computed from a string, not just that the residue failed
to match the target.

Uses the `Polynomial` type; see the previous commits for some caveats
about this type when alloc is disabled. (Prior to this PR, you just
couldn't use Polynomial at all without alloc.)

As an initial application of the new error, avoid re-validating
a bech32 address to handle the distinction between bech32m and
bech32. Instead, if bech32m validation fails, check if the
"bad" residue actually matches the bech32 target. If so, accept.

On my system this reduces the `bech32_parse_address` benchmark
from 610ns to 455ns, more than a 33% speedup.
  • Loading branch information
apoelstra committed Sep 18, 2024
1 parent 4cc66fb commit d61cb0a
Show file tree
Hide file tree
Showing 5 changed files with 97 additions and 28 deletions.
12 changes: 7 additions & 5 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,13 +214,15 @@ const BUF_LENGTH: usize = 10;
pub fn decode(s: &str) -> Result<(Hrp, Vec<u8>), DecodeError> {
let unchecked = UncheckedHrpstring::new(s)?;

if let Err(e) = unchecked.validate_checksum::<Bech32m>() {
if !unchecked.has_valid_checksum::<Bech32>() {
match unchecked.validate_checksum::<Bech32m>() {
Ok(_) => {}
Err(ChecksumError::InvalidResidue(ref res_err)) if res_err.matches_bech32_checksum() => {}
Err(e) => {
return Err(DecodeError::Checksum(e));
}
};
// One of the checksums was valid, Ck is only for length and since
// they are both the same we can use either here.
}
// One of the checksums was valid. `Bech32m` is only used for its
// length and since it is the same as `Bech32` we can use either here.
let checked = unchecked.remove_checksum::<Bech32m>();

Ok((checked.hrp(), checked.byte_iter().collect()))
Expand Down
66 changes: 58 additions & 8 deletions src/primitives/decode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,12 @@
use core::{fmt, iter, slice, str};

use crate::error::write_err;
use crate::primitives::checksum::{self, Checksum};
use crate::primitives::checksum::{self, Checksum, PackedFe32};
use crate::primitives::gf32::Fe32;
use crate::primitives::hrp::{self, Hrp};
use crate::primitives::iter::{Fe32IterExt, FesToBytes};
use crate::primitives::segwit::{self, WitnessLengthError, VERSION_0};
use crate::primitives::{FieldVec, Polynomial};
use crate::{Bech32, Bech32m};

/// Separator between the hrp and payload (as defined by BIP-173).
Expand Down Expand Up @@ -277,8 +278,9 @@ impl<'s> UncheckedHrpstring<'s> {
checksum_eng.input_fe(fe);
}

if checksum_eng.residue() != &Ck::TARGET_RESIDUE {
return Err(InvalidResidue);
let residue = *checksum_eng.residue();
if residue != Ck::TARGET_RESIDUE {
return Err(InvalidResidue(InvalidResidueError::new(residue, Ck::TARGET_RESIDUE)));
}

Ok(())
Expand Down Expand Up @@ -952,7 +954,7 @@ pub enum ChecksumError {
/// String exceeds maximum allowed length.
CodeLength(CodeLengthError),
/// The checksum residue is not valid for the data.
InvalidResidue,
InvalidResidue(InvalidResidueError),
/// The checksummed string is not a valid length.
InvalidLength,
}
Expand All @@ -963,7 +965,7 @@ impl fmt::Display for ChecksumError {

match *self {
CodeLength(ref e) => write_err!(f, "string exceeds maximum allowed length"; e),
InvalidResidue => write!(f, "the checksum residue is not valid for the data"),
InvalidResidue(ref e) => write_err!(f, "checksum failed"; e),
InvalidLength => write!(f, "the checksummed string is not a valid length"),
}
}
Expand All @@ -976,11 +978,56 @@ impl std::error::Error for ChecksumError {

match *self {
CodeLength(ref e) => Some(e),
InvalidResidue | InvalidLength => None,
InvalidResidue(ref e) => Some(e),
InvalidLength => None,
}
}
}

/// Residue mismatch validating the checksum. That is, "the checksum failed".
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct InvalidResidueError {
actual: Polynomial<Fe32>,
target: Polynomial<Fe32>,
}

impl fmt::Display for InvalidResidueError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
if self.actual.has_data() {
write!(f, "residue {} did not match target {}", self.actual, self.target)
} else {
f.write_str("residue mismatch")
}
}
}

impl InvalidResidueError {
/// Constructs a new "invalid residue" error.
fn new<F: PackedFe32>(residue: F, target_residue: F) -> Self {
Self {
actual: FieldVec::from_residue(residue).into(),
target: FieldVec::from_residue(target_residue).into(),
}
}

/// Whether this "invalid residue" error actually represents a valid residue
/// for the bech32 checksum.
///
/// This method could in principle be made generic over the intended checksum,
/// but it is not clear what the purpose would be (checking bech32 vs bech32m
/// is a special case), and the method would necessarily panic if called with
/// too large a checksum without an allocator. We would like to better understand
/// the usecase for this before exposing such a footgun.
pub fn matches_bech32_checksum(&self) -> bool {
self.actual == FieldVec::from_residue(Bech32::TARGET_RESIDUE).into()
}
}

#[cfg(feature = "std")]
impl std::error::Error for InvalidResidueError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None }
}

/// Encoding HRP and data into a bech32 string exceeds the checksum code length.
#[derive(Debug, Clone, PartialEq, Eq)]
#[non_exhaustive]
Expand Down Expand Up @@ -1065,6 +1112,9 @@ impl std::error::Error for PaddingError {

#[cfg(test)]
mod tests {
#[cfg(all(feature = "alloc", not(feature = "std")))]
use alloc::vec::Vec;

use super::*;

#[test]
Expand Down Expand Up @@ -1117,7 +1167,7 @@ mod tests {
.expect("string parses correctly")
.validate_checksum::<Bech32>()
.unwrap_err();
assert_eq!(err, InvalidResidue);
assert!(matches!(err, InvalidResidue(..)));
}

#[test]
Expand Down Expand Up @@ -1178,7 +1228,7 @@ mod tests {
.unwrap()
.validate_checksum::<Bech32m>()
.unwrap_err();
assert_eq!(err, InvalidResidue);
assert!(matches!(err, InvalidResidue(..)));
}

#[test]
Expand Down
19 changes: 18 additions & 1 deletion src/primitives/fieldvec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,12 @@

#[cfg(all(feature = "alloc", not(feature = "std")))]
use alloc::vec::Vec;
use core::{iter, mem, ops, slice};
use core::{fmt, iter, mem, ops, slice};

use super::checksum::PackedFe32;
use super::Field;
use crate::primitives::correction::NO_ALLOC_MAX_LENGTH;
use crate::Fe32;

/// A vector of field elements.
///
Expand All @@ -77,6 +79,12 @@ pub struct FieldVec<F> {
inner_v: Vec<F>,
}

impl FieldVec<Fe32> {
pub fn from_residue<R: PackedFe32>(residue: R) -> Self {
(0..R::WIDTH).map(|i| Fe32(residue.unpack(i))).collect()
}
}

impl<F> FieldVec<F> {
/// Determines whether the residue is representable, given the current
/// compilation context.
Expand Down Expand Up @@ -278,6 +286,15 @@ impl<F: Clone + Default> iter::FromIterator<F> for FieldVec<F> {
}
}

impl<F: fmt::Display> fmt::Display for FieldVec<F> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
for fe in self.iter() {
fe.fmt(f)?;
}
Ok(())
}
}

impl<'a, F> IntoIterator for &'a FieldVec<F> {
type Item = &'a F;
type IntoIter = slice::Iter<'a, F>;
Expand Down
14 changes: 7 additions & 7 deletions tests/bip_173_test_vectors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@ fn bip_173_checksum_calculated_with_uppercase_form() {
// BIP-173 states reason for error should be: "checksum calculated with uppercase form of HRP".
let s = "A1G7SGD8";

assert_eq!(
assert!(matches!(
CheckedHrpstring::new::<Bech32>(s).unwrap_err(),
CheckedHrpstringError::Checksum(ChecksumError::InvalidResidue)
);
CheckedHrpstringError::Checksum(ChecksumError::InvalidResidue(..))
));

assert_eq!(
assert!(matches!(
SegwitHrpstring::new(s).unwrap_err(),
SegwitHrpstringError::Checksum(ChecksumError::InvalidResidue)
);
SegwitHrpstringError::Checksum(ChecksumError::InvalidResidue(..))
));
}

macro_rules! check_valid_bech32 {
Expand All @@ -35,7 +35,7 @@ macro_rules! check_valid_bech32 {
let p = UncheckedHrpstring::new($valid_bech32).unwrap();
p.validate_checksum::<Bech32>().expect("valid bech32");
// Valid bech32 strings are by definition invalid bech32m.
assert_eq!(p.validate_checksum::<Bech32m>().unwrap_err(), ChecksumError::InvalidResidue);
assert!(matches!(p.validate_checksum::<Bech32m>(), Err(ChecksumError::InvalidResidue(..))));
}
)*
}
Expand Down
14 changes: 7 additions & 7 deletions tests/bip_350_test_vectors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ fn bip_350_checksum_calculated_with_uppercase_form() {
// BIP-350 states reason for error should be: "checksum calculated with uppercase form of HRP".
let s = "M1VUXWEZ";

assert_eq!(
assert!(matches!(
CheckedHrpstring::new::<Bech32m>(s).unwrap_err(),
CheckedHrpstringError::Checksum(ChecksumError::InvalidResidue)
);
CheckedHrpstringError::Checksum(ChecksumError::InvalidResidue(..))
));

assert_eq!(
assert!(matches!(
SegwitHrpstring::new(s).unwrap_err(),
SegwitHrpstringError::Checksum(ChecksumError::InvalidResidue)
);
SegwitHrpstringError::Checksum(ChecksumError::InvalidResidue(..))
));
}

macro_rules! check_valid_bech32m {
Expand All @@ -34,7 +34,7 @@ macro_rules! check_valid_bech32m {
let p = UncheckedHrpstring::new($valid_bech32m).unwrap();
p.validate_checksum::<Bech32m>().expect("valid bech32m");
// Valid bech32m strings are by definition invalid bech32.
assert_eq!(p.validate_checksum::<Bech32>().unwrap_err(), ChecksumError::InvalidResidue);
assert!(matches!(p.validate_checksum::<Bech32>().unwrap_err(), ChecksumError::InvalidResidue(..)));
}
)*
}
Expand Down

0 comments on commit d61cb0a

Please sign in to comment.