Merge #609: Do some checksum module improvements

5018673 Remove magic number (Tobin C. Harding)
535cd17 Improve docs in the checksum module (Tobin C. Harding)
fec700a Add bip 380 checksum test vectors (Tobin C. Harding)
73f4892 Add output descriptor bip referenece (Tobin C. Harding)

Pull request description:

  The first 4 patches from #608, no changes.

  Clean up and add some test vector unit tests.

ACKs for top commit:
  apoelstra:
    ACK 5018673

Tree-SHA512: f50f64bf9a1fc28eebca809379e02580cab96e7e41228aab6045441eb71702bef1b1979e497a6dcb1e1bce082965e5c93e78dba6e8fbd78c7a0ae2e3c8035660

Author: apoelstra

src/descriptor/checksum.rs

@@ -3,7 +3,9 @@
 //! Descriptor checksum
 //!
 //! This module contains a re-implementation of the function used by Bitcoin Core to calculate the
-//! checksum of a descriptor
+//! checksum of a descriptor. The checksum algorithm is specified in [BIP-380].
+//!
+//! [BIP-380]: <https://github.com/bitcoin/bips/blob/master/bip-0380.mediawiki>
 
 use core::fmt;
 use core::iter::FromIterator;
@@ -14,6 +16,8 @@ use crate::Error;
 
 const CHECKSUM_CHARSET: &[u8] = b"qpzry9x8gf2tvdw0s3jn54khce6mua7l";
 
+const CHECKSUM_LENGTH: usize = 8;
+
 fn poly_mod(mut c: u64, val: u64) -> u64 {
     let c0 = c >> 35;
 
@@ -37,20 +41,20 @@ fn poly_mod(mut c: u64, val: u64) -> u64 {
     c
 }
 
-/// Compute the checksum of a descriptor
-/// Note that this function does not check if the
-/// descriptor string is syntactically correct or not.
-/// This only computes the checksum
+/// Compute the checksum of a descriptor.
+///
+/// Note that this function does not check if the descriptor string is
+/// syntactically correct or not. This only computes the checksum.
 pub fn desc_checksum(desc: &str) -> Result<String, Error> {
     let mut eng = Engine::new();
     eng.input(desc)?;
     Ok(eng.checksum())
 }
 
-/// Helper function for FromStr for various
-/// descriptor types. Checks and verifies the checksum
-/// if it is present and returns the descriptor string
-/// without the checksum
+/// Helper function for `FromStr` for various descriptor types.
+///
+/// Checks and verifies the checksum if it is present and returns the descriptor
+/// string without the checksum.
 pub(super) fn verify_checksum(s: &str) -> Result<&str, Error> {
     for ch in s.as_bytes() {
         if *ch < 20 || *ch > 127 {
@@ -72,7 +76,7 @@ pub(super) fn verify_checksum(s: &str) -> Result<&str, Error> {
     Ok(desc_str)
 }
 
-/// An engine to compute a checksum from a string
+/// An engine to compute a checksum from a string.
 pub struct Engine {
     c: u64,
     cls: u64,
@@ -84,10 +88,10 @@ impl Default for Engine {
 }
 
 impl Engine {
-    /// Construct an engine with no input
+    /// Constructs an engine with no input.
     pub fn new() -> Self { Engine { c: 1, cls: 0, clscount: 0 } }
 
-    /// Checksum some data
+    /// Inputs some data into the checksum engine.
     ///
     /// If this function returns an error, the `Engine` will be left in an indeterminate
     /// state! It is safe to continue feeding it data but the result will not be meaningful.
@@ -113,38 +117,39 @@ impl Engine {
         Ok(())
     }
 
-    /// Obtain the checksum of all the data thus-far fed to the engine
-    pub fn checksum_chars(&mut self) -> [char; 8] {
+    /// Obtains the checksum characters of all the data thus-far fed to the
+    /// engine without allocating, to get a string use [`Self::checksum`].
+    pub fn checksum_chars(&mut self) -> [char; CHECKSUM_LENGTH] {
         if self.clscount > 0 {
             self.c = poly_mod(self.c, self.cls);
         }
-        (0..8).for_each(|_| self.c = poly_mod(self.c, 0));
+        (0..CHECKSUM_LENGTH).for_each(|_| self.c = poly_mod(self.c, 0));
         self.c ^= 1;
 
-        let mut chars = [0 as char; 8];
-        for j in 0..8 {
+        let mut chars = [0 as char; CHECKSUM_LENGTH];
+        for j in 0..CHECKSUM_LENGTH {
             chars[j] = CHECKSUM_CHARSET[((self.c >> (5 * (7 - j))) & 31) as usize] as char;
         }
         chars
     }
 
-    /// Obtain the checksum of all the data thus-far fed to the engine
+    /// Obtains the checksum of all the data thus-far fed to the engine.
     pub fn checksum(&mut self) -> String {
         String::from_iter(self.checksum_chars().iter().copied())
     }
 }
 
-/// A wrapper around a `fmt::Formatter` which provides checksumming ability
+/// A wrapper around a `fmt::Formatter` which provides checksumming ability.
 pub struct Formatter<'f, 'a> {
     fmt: &'f mut fmt::Formatter<'a>,
     eng: Engine,
 }
 
 impl<'f, 'a> Formatter<'f, 'a> {
-    /// Contruct a new `Formatter`, wrapping a given `fmt::Formatter`
+    /// Contructs a new `Formatter`, wrapping a given `fmt::Formatter`.
     pub fn new(f: &'f mut fmt::Formatter<'a>) -> Self { Formatter { fmt: f, eng: Engine::new() } }
 
-    /// Writes the checksum into the underlying `fmt::Formatter`
+    /// Writes the checksum into the underlying `fmt::Formatter`.
     pub fn write_checksum(&mut self) -> fmt::Result {
         use fmt::Write;
         self.fmt.write_char('#')?;
@@ -154,7 +159,7 @@ impl<'f, 'a> Formatter<'f, 'a> {
         Ok(())
     }
 
-    /// Writes the checksum into the underlying `fmt::Formatter`, unless it has "alternate" display on
+    /// Writes the checksum into the underlying `fmt::Formatter`, unless it has "alternate" display on.
     pub fn write_checksum_if_not_alt(&mut self) -> fmt::Result {
         if !self.fmt.alternate() {
             self.write_checksum()?;
@@ -219,4 +224,34 @@ mod test {
             format!("Invalid descriptor: Invalid character in checksum: '{}'", sparkle_heart)
         );
     }
+
+    #[test]
+    fn bip_380_test_vectors_checksum_and_character_set_valid() {
+        let tcs = vec![
+            "raw(deadbeef)#89f8spxm", // Valid checksum.
+            "raw(deadbeef)",          // No checksum.
+        ];
+        for tc in tcs {
+            if verify_checksum(tc).is_err() {
+                panic!("false negative: {}", tc)
+            }
+        }
+    }
+
+    #[test]
+    fn bip_380_test_vectors_checksum_and_character_set_invalid() {
+        let tcs = vec![
+            "raw(deadbeef)#",          // Missing checksum.
+            "raw(deadbeef)#89f8spxmx", // Too long checksum.
+            "raw(deadbeef)#89f8spx",   // Too short checksum.
+            "raw(dedbeef)#89f8spxm",   // Error in payload.
+            "raw(deadbeef)##9f8spxm",  // Error in checksum.
+            "raw(Ü)#00000000",         // Invalid characters in payload.
+        ];
+        for tc in tcs {
+            if verify_checksum(tc).is_ok() {
+                panic!("false positive: {}", tc)
+            }
+        }
+    }
 }