Merge rust-bitcoin#204: Introduce FieldVec and improve Polynomial and InvalidResidueError

c209ce8 polynomial: expand unit tests (Andrew Poelstra)
dfc29ad fieldvec: add a bunch of unit tests (Andrew Poelstra)
0e3d954 polynomial: make Eq/PartialEq take leading zeros into account (Andrew Poelstra)
d08da46 correction: introduce CorrectableError trait (Andrew Poelstra)
4c6010e primitives: introduce InvalidResidueError (Andrew Poelstra)
b76adbe polynomial: add some extra functionality enabled by FieldVec (Andrew Poelstra)
690fc7d polynomial: use FieldVec in Polynomial (Andrew Poelstra)
f2ec582 primitives: introduce FieldVec type (Andrew Poelstra)

Pull request description:

  This introduces a new internal type `FieldVec` which is basically a backing array for checksum residues with some fiddly logic to work in a noalloc context.

  Unlike ArrayVec or other similar types, this one's semantics are "an unbounded vector if you have an allocator, a bounded vector otherwise". If you go outside of the bounds without an allocator the code will panic, but our use of it ensures that this is never exposed to a user.

  It also assumes that it's holding a `Default` type which lets us avoid unsafety (and dismisses the potential performance cost because this array is never expected to have more than 10 or 20 elements, even when it is "unbounded").

  Using this backing, it improves `Polynomial` and removes the alloc bound from it; using this, it puts `Polynomial` into a new `InvalidResidueError` type, which will allow users to extract the necessary information from a checksum error to run the error correction algorithm. (The resulting change to an error type makes this an API-breaking change, though I don't think that anything else here is API-breaking.)

  The next PR will actually implement correction :).

ACKs for top commit:
  tcharding:
    ACK c209ce8
  clarkmoody:
    ACK c209ce8

Tree-SHA512: 16a23bd88101be549c9d50f6b32f669688dfb1efbd6e842c1c3db2203d1775e647f0909c4a65191fd332cd07778c0431ae7b4635909a00f7c2d86135fae042e2

Author: apoelstra

Diff for: src/lib.rs

@@ -134,6 +134,8 @@
 #![cfg_attr(docsrs, feature(doc_auto_cfg))]
 // Coding conventions
 #![deny(missing_docs)]
+#![allow(clippy::suspicious_arithmetic_impl)] // this lint is literally always wrong
+#![allow(clippy::suspicious_op_assign_impl)] // ...and "always wrong" loves company
 
 #[cfg(bench)]
 extern crate test;
@@ -164,6 +166,7 @@ use crate::primitives::decode::{ChecksumError, UncheckedHrpstring, UncheckedHrps
 #[doc(inline)]
 pub use {
     crate::primitives::checksum::Checksum,
+    crate::primitives::correction::CorrectableError,
     crate::primitives::gf32::Fe32,
     crate::primitives::gf32_ext::{Fe1024, Fe32768},
     crate::primitives::hrp::Hrp,
@@ -216,13 +219,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()))

Diff for: src/primitives/checksum.rs

@@ -4,20 +4,14 @@
 //!
 //! [BCH]: <https://en.wikipedia.org/wiki/BCH_code>
 
-#[cfg(all(feature = "alloc", not(feature = "std"), not(test)))]
+#[cfg(all(feature = "alloc", not(feature = "std")))]
 use alloc::vec::Vec;
-#[cfg(feature = "alloc")]
-use core::fmt;
-#[cfg(feature = "alloc")]
 use core::marker::PhantomData;
-use core::{mem, ops};
+use core::{fmt, mem, ops};
 
-#[cfg(feature = "alloc")]
 use super::Polynomial;
 use crate::primitives::hrp::Hrp;
-#[cfg(feature = "alloc")]
-use crate::Fe1024;
-use crate::Fe32;
+use crate::{Fe1024, Fe32};
 
 /// Trait defining a particular checksum.
 ///
@@ -169,18 +163,16 @@ pub trait Checksum {
 /// to compute the values yourself. The reason is that the specific values
 /// used depend on the representation of extension fields, which may differ
 /// between implementations (and between specifications) of your BCH code.
-#[cfg(feature = "alloc")]
 pub struct PrintImpl<'a, ExtField = Fe1024> {
     name: &'a str,
-    generator: &'a [Fe32],
+    generator: Polynomial<Fe32>,
     target: &'a [Fe32],
     bit_len: usize,
     hex_width: usize,
     midstate_repr: &'static str,
     phantom: PhantomData<ExtField>,
 }
 
-#[cfg(feature = "alloc")]
 impl<'a, ExtField> PrintImpl<'a, ExtField> {
     /// Constructor for an object to print an impl-block for the [`Checksum`] trait.
     ///
@@ -225,7 +217,7 @@ impl<'a, ExtField> PrintImpl<'a, ExtField> {
         // End sanity checks.
         PrintImpl {
             name,
-            generator,
+            generator: Polynomial::with_monic_leading_term(generator),
             target,
             bit_len,
             hex_width,
@@ -235,23 +227,16 @@ impl<'a, ExtField> PrintImpl<'a, ExtField> {
     }
 }
 
-#[cfg(feature = "alloc")]
 impl<'a, ExtField> fmt::Display for PrintImpl<'a, ExtField>
 where
     ExtField: super::Bech32Field + super::ExtensionField<BaseField = Fe32>,
 {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         // Generator polynomial as a polynomial over GF1024
-        let gen_poly = {
-            let mut v = Vec::with_capacity(self.generator.len() + 1);
-            v.push(ExtField::ONE);
-            v.extend(self.generator.iter().cloned().map(ExtField::from));
-            Polynomial::new(v)
-        };
-        let (gen, length, exponents) = gen_poly.bch_generator_primitive_element();
+        let (gen, length, exponents) = self.generator.bch_generator_primitive_element::<ExtField>();
 
         write!(f, "// Code block generated by Checksum::print_impl polynomial ")?;
-        for fe in self.generator {
+        for fe in self.generator.as_inner() {
             write!(f, "{}", fe)?;
         }
         write!(f, " target ")?;
@@ -278,9 +263,9 @@ where
         )?;
         f.write_str("\n")?;
         writeln!(f, "    const CODE_LENGTH: usize = {};", length)?;
-        writeln!(f, "    const CHECKSUM_LENGTH: usize = {};", gen_poly.degree())?;
+        writeln!(f, "    const CHECKSUM_LENGTH: usize = {};", self.generator.degree())?;
         writeln!(f, "    const GENERATOR_SH: [{}; 5] = [", self.midstate_repr)?;
-        let mut gen5 = self.generator.to_vec();
+        let mut gen5 = self.generator.clone().into_inner();
         for _ in 0..5 {
             let gen_packed = u128::pack(gen5.iter().copied().map(From::from));
             writeln!(f, "        0x{:0width$x},", gen_packed, width = self.hex_width)?;

Diff for: src/primitives/correction.rs

@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: MIT
+
+//! Error Correction
+//!
+//! Implements the Berlekamp-Massey algorithm to locate errors, with Forney's
+//! equation to identify the error values, in a BCH-encoded string.
+//!
+
+use crate::primitives::decode::{
+    CheckedHrpstringError, ChecksumError, InvalidResidueError, SegwitHrpstringError,
+};
+#[cfg(feature = "alloc")]
+use crate::DecodeError;
+
+/// **One more than** the maximum length (in characters) of a checksum which
+/// can be error-corrected without an allocator.
+///
+/// When the **alloc** feature is enabled, this constant is practically irrelevant.
+/// When the feature is disabled, it represents a length beyond which this library
+/// does not support error correction.
+///
+/// If you need this value to be increased, please file an issue describing your
+/// usecase. Bear in mind that an increased value will increase memory usage for
+/// all users, and the focus of this library is the Bitcoin ecosystem, so we may
+/// not be able to accept your request.
+// This constant is also used when comparing bech32 residues against the
+// bech32/bech32m targets, which should work with no-alloc. Therefore this
+// constant must be > 6 (the length of the bech32(m) checksum).
+//
+// Otherwise it basically represents a tradeoff between stack usage and the
+// size of error types, vs functionality in a no-alloc setting. The value
+// of 7 covers bech32 and bech32m. To get the descriptor checksum we need a
+// value and the descriptor checksum. To also get codex32 it should be >13,
+// and for "long codex32" >15 ... but consider that no-alloc contexts are
+// likely to be underpowered and will struggle to do correction on these
+// big codes anyway.
+//
+// Perhaps we will want to add a feature gate, off by default, that boosts
+// this to 16, or maybe even higher. But we will wait for implementors who
+// complain.
+pub const NO_ALLOC_MAX_LENGTH: usize = 7;
+
+/// Trait describing an error for which an error correction algorithm is applicable.
+///
+/// Essentially, this trait represents an error which contains an [`InvalidResidueError`]
+/// variant.
+pub trait CorrectableError {
+    /// Given a decoding error, if this is a "checksum failed" error, extract
+    /// that specific error type.
+    ///
+    /// There are many ways in which decoding a checksummed string might fail.
+    /// If the string was well-formed in all respects except that the final
+    /// checksum characters appear to be wrong, it is possible to run an
+    /// error correction algorithm to attempt to extract errors.
+    ///
+    /// In all other cases we do not have enough information to do correction.
+    ///
+    /// This is the function that implementors should implement.
+    fn residue_error(&self) -> Option<&InvalidResidueError>;
+}
+
+impl CorrectableError for InvalidResidueError {
+    fn residue_error(&self) -> Option<&InvalidResidueError> { Some(self) }
+}
+
+impl CorrectableError for ChecksumError {
+    fn residue_error(&self) -> Option<&InvalidResidueError> {
+        match self {
+            ChecksumError::InvalidResidue(ref e) => Some(e),
+            _ => None,
+        }
+    }
+}
+
+impl CorrectableError for SegwitHrpstringError {
+    fn residue_error(&self) -> Option<&InvalidResidueError> {
+        match self {
+            SegwitHrpstringError::Checksum(ref e) => e.residue_error(),
+            _ => None,
+        }
+    }
+}
+
+impl CorrectableError for CheckedHrpstringError {
+    fn residue_error(&self) -> Option<&InvalidResidueError> {
+        match self {
+            CheckedHrpstringError::Checksum(ref e) => e.residue_error(),
+            _ => None,
+        }
+    }
+}
+
+#[cfg(feature = "alloc")]
+impl CorrectableError for crate::segwit::DecodeError {
+    fn residue_error(&self) -> Option<&InvalidResidueError> { self.0.residue_error() }
+}
+
+#[cfg(feature = "alloc")]
+impl CorrectableError for DecodeError {
+    fn residue_error(&self) -> Option<&InvalidResidueError> {
+        match self {
+            DecodeError::Checksum(ref e) => e.residue_error(),
+            _ => None,
+        }
+    }
+}

Diff for: 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::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<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: Polynomial::from_residue(residue),
+            target: Polynomial::from_residue(target_residue),
+        }
+    }
+
+    /// 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 == Polynomial::from_residue(Bech32::TARGET_RESIDUE)
+    }
+}
+
+#[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::<Bech32>()
             .unwrap_err();
-        assert_eq!(err, InvalidResidue);
+        assert!(matches!(err, InvalidResidue(..)));
     }
 
     #[test]
@@ -1178,7 +1228,7 @@ mod tests {
             .unwrap()
             .validate_checksum::<Bech32m>()
             .unwrap_err();
-        assert_eq!(err, InvalidResidue);
+        assert!(matches!(err, InvalidResidue(..)));
     }
 
     #[test]
@@ -1212,6 +1262,15 @@ mod tests {
         }
     }
 
+    #[test]
+    #[allow(clippy::assertions_on_constants)]
+    fn constant_sanity() {
+        assert!(
+            crate::primitives::correction::NO_ALLOC_MAX_LENGTH > 6,
+            "The NO_ALLOC_MAX_LENGTH constant must be > 6. See its documentation for why.",
+        );
+    }
+
     macro_rules! check_invalid_segwit_addresses {
         ($($test_name:ident, $reason:literal, $address:literal);* $(;)?) => {
             $(