From f9265e616fadf642d43ebd0abce02a444ba95e39 Mon Sep 17 00:00:00 2001 From: Arvind Mukund Date: Thu, 2 Nov 2023 14:04:52 -0700 Subject: [PATCH 1/3] p521: Improve `Debug` for unsaturated math When using p521, the limbs can be populated differently which when deriving [`Debug`] shows up as different values for equal FieldElements, this can lead to confusion. See: - https://github.com/RustCrypto/elliptic-curves/pull/946#issuecomment-1789749836, equal field elements were considered as unequal. - https://github.com/RustCrypto/elliptic-curves/pull/946#discussion_r1380388316, assert_eq! wasn't meaningful. Signed-off-by: Arvind Mukund --- p521/src/arithmetic/field.rs | 38 +++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/p521/src/arithmetic/field.rs b/p521/src/arithmetic/field.rs index 5230e879..7e50e78d 100644 --- a/p521/src/arithmetic/field.rs +++ b/p521/src/arithmetic/field.rs @@ -51,9 +51,45 @@ const MODULUS_HEX: &str = "00000000000001fffffffffffffffffffffffffffffffffffffff const MODULUS: U576 = U576::from_be_hex(MODULUS_HEX); /// Element of the secp521r1 base field used for curve coordinates. -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy)] pub struct FieldElement(fiat_p521_tight_field_element); +impl core::fmt::Debug for FieldElement { + /// Formatting machinery for [`FieldElement`] + /// + /// # Why + /// ``` + /// let fe1 = FieldElement([9, 0, 0, 0, 0, 0, 0, 0, 0]); + /// let fe2 = FieldElement([ + /// 8, + /// 0, + /// 288230376151711744, + /// 288230376151711743, + /// 288230376151711743, + /// 288230376151711743, + /// 288230376151711743, + /// 288230376151711743, + /// 144115188075855871, + /// ]); + /// ``` + /// + /// For the above example, deriving [`core::fmt::Debug`] will result in returning 2 different strings, + /// which are in reality the same due to p521's unsaturated math, instead print the output as a hex string in + /// big-endian + /// + /// This makes debugging easier. + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + let mut bytes = fiat_p521_to_bytes(&self.0); + bytes.reverse(); + + write!(f, "0x")?; + for b in bytes { + write!(f, "{b:02X}")?; + } + Ok(()) + } +} + impl FieldElement { /// Zero element. pub const ZERO: Self = Self::from_u64(0); From 1ad3494d0692aaac7ce9222f2c9355a9b426eeac Mon Sep 17 00:00:00 2001 From: Arvind Mukund Date: Thu, 2 Nov 2023 14:14:13 -0700 Subject: [PATCH 2/3] rustdoc: Ignore the codeblock for doctests Signed-off-by: Arvind Mukund --- p521/src/arithmetic/field.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/p521/src/arithmetic/field.rs b/p521/src/arithmetic/field.rs index 7e50e78d..6f387d53 100644 --- a/p521/src/arithmetic/field.rs +++ b/p521/src/arithmetic/field.rs @@ -58,7 +58,7 @@ impl core::fmt::Debug for FieldElement { /// Formatting machinery for [`FieldElement`] /// /// # Why - /// ``` + /// ```ignore /// let fe1 = FieldElement([9, 0, 0, 0, 0, 0, 0, 0, 0]); /// let fe2 = FieldElement([ /// 8, From f3847272ef1bcbea430f089e5bd6b5b6f67a1657 Mon Sep 17 00:00:00 2001 From: Arvind Mukund Date: Thu, 2 Nov 2023 14:43:32 -0700 Subject: [PATCH 3/3] Use `base16ct` to do the formatting Signed-off-by: Arvind Mukund --- Cargo.lock | 1 + p521/Cargo.toml | 1 + p521/src/arithmetic/field.rs | 10 ++++------ 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ab0300e2..1010cf48 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -778,6 +778,7 @@ dependencies = [ name = "p521" version = "0.13.0" dependencies = [ + "base16ct", "elliptic-curve", "primeorder", "sha2", diff --git a/p521/Cargo.toml b/p521/Cargo.toml index 5b2a2dea..0e5544ad 100644 --- a/p521/Cargo.toml +++ b/p521/Cargo.toml @@ -21,6 +21,7 @@ sha2 = { version = "0.10", optional = true, default-features = false } # optional dependencies primeorder = { version = "0.13.1", optional = true, path = "../primeorder" } +base16ct = "0.2.0" [features] default = ["pem", "std"] diff --git a/p521/src/arithmetic/field.rs b/p521/src/arithmetic/field.rs index 6f387d53..037425a3 100644 --- a/p521/src/arithmetic/field.rs +++ b/p521/src/arithmetic/field.rs @@ -81,12 +81,10 @@ impl core::fmt::Debug for FieldElement { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { let mut bytes = fiat_p521_to_bytes(&self.0); bytes.reverse(); - - write!(f, "0x")?; - for b in bytes { - write!(f, "{b:02X}")?; - } - Ok(()) + let formatter = base16ct::HexDisplay(&bytes); + f.debug_tuple("FieldElement") + .field(&format_args!("0x{formatter:X}")) + .finish() } }