From d61cb0aa3d5e4fee08590922fbbfee541884efca Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 15 Sep 2024 18:20:58 +0000 Subject: [PATCH] primitives: introduce InvalidResidueError 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. --- src/lib.rs | 12 ++++--- src/primitives/decode.rs | 66 ++++++++++++++++++++++++++++++----- src/primitives/fieldvec.rs | 19 +++++++++- tests/bip_173_test_vectors.rs | 14 ++++---- tests/bip_350_test_vectors.rs | 14 ++++---- 5 files changed, 97 insertions(+), 28 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 290f2f88f..e7638e4b9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -214,13 +214,15 @@ const BUF_LENGTH: usize = 10; pub fn decode(s: &str) -> Result<(Hrp, Vec), DecodeError> { let unchecked = UncheckedHrpstring::new(s)?; - if let Err(e) = unchecked.validate_checksum::() { - if !unchecked.has_valid_checksum::() { + match unchecked.validate_checksum::() { + 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::(); Ok((checked.hrp(), checked.byte_iter().collect())) diff --git a/src/primitives/decode.rs b/src/primitives/decode.rs index 7752ffdaf..f463f35f6 100644 --- a/src/primitives/decode.rs +++ b/src/primitives/decode.rs @@ -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). @@ -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(()) @@ -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, } @@ -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"), } } @@ -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, + target: Polynomial, +} + +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(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] @@ -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] @@ -1117,7 +1167,7 @@ mod tests { .expect("string parses correctly") .validate_checksum::() .unwrap_err(); - assert_eq!(err, InvalidResidue); + assert!(matches!(err, InvalidResidue(..))); } #[test] @@ -1178,7 +1228,7 @@ mod tests { .unwrap() .validate_checksum::() .unwrap_err(); - assert_eq!(err, InvalidResidue); + assert!(matches!(err, InvalidResidue(..))); } #[test] diff --git a/src/primitives/fieldvec.rs b/src/primitives/fieldvec.rs index f884019b4..72a3dda3a 100644 --- a/src/primitives/fieldvec.rs +++ b/src/primitives/fieldvec.rs @@ -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. /// @@ -77,6 +79,12 @@ pub struct FieldVec { inner_v: Vec, } +impl FieldVec { + pub fn from_residue(residue: R) -> Self { + (0..R::WIDTH).map(|i| Fe32(residue.unpack(i))).collect() + } +} + impl FieldVec { /// Determines whether the residue is representable, given the current /// compilation context. @@ -278,6 +286,15 @@ impl iter::FromIterator for FieldVec { } } +impl fmt::Display for FieldVec { + 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 { type Item = &'a F; type IntoIter = slice::Iter<'a, F>; diff --git a/tests/bip_173_test_vectors.rs b/tests/bip_173_test_vectors.rs index 74868f1d7..85e971549 100644 --- a/tests/bip_173_test_vectors.rs +++ b/tests/bip_173_test_vectors.rs @@ -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::(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 { @@ -35,7 +35,7 @@ macro_rules! check_valid_bech32 { let p = UncheckedHrpstring::new($valid_bech32).unwrap(); p.validate_checksum::().expect("valid bech32"); // Valid bech32 strings are by definition invalid bech32m. - assert_eq!(p.validate_checksum::().unwrap_err(), ChecksumError::InvalidResidue); + assert!(matches!(p.validate_checksum::(), Err(ChecksumError::InvalidResidue(..)))); } )* } diff --git a/tests/bip_350_test_vectors.rs b/tests/bip_350_test_vectors.rs index 546beaab7..6e310d9b7 100644 --- a/tests/bip_350_test_vectors.rs +++ b/tests/bip_350_test_vectors.rs @@ -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::(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 { @@ -34,7 +34,7 @@ macro_rules! check_valid_bech32m { let p = UncheckedHrpstring::new($valid_bech32m).unwrap(); p.validate_checksum::().expect("valid bech32m"); // Valid bech32m strings are by definition invalid bech32. - assert_eq!(p.validate_checksum::().unwrap_err(), ChecksumError::InvalidResidue); + assert!(matches!(p.validate_checksum::().unwrap_err(), ChecksumError::InvalidResidue(..))); } )* }