checksum: introduce `PrintImpl` object that can compute various code properties #180

apoelstra · 2024-03-25T01:29:53Z

This PR does a couple things:

Bumps the MSRV to 1.56.1 to match rust-bitcoin (and get us const generics which are useful for defining extension fields) as well as cleaning up a couple warnings.
Introduces the GF1024 field, a simple polynomial type, the Chein root-finding algorithm, and a couple other misc algorithms that will be useful for error correction. (All this is internal and not reflected in the public API.)
Introduces the GF32768 field which will be needed for error correction of the descriptor checksum.
To demonstrate this new functionality, adds an object to the API which can produce the impl Checksum block for codes which can be corrected using a quadratic or cubic field extension. This will hopefully provide some context for people not super familiar with BCH code about where these magic numbers come from.

We also add unit tests that produce the parameters for codex32 and the descriptor checksum. A later PR will introduce more parameters which will be needed for error correction.

apoelstra · 2024-03-25T01:48:55Z

I updated the API files inline with each commit so it's clear what commits change the API in what ways. Let me know if it'd be better to pull these out into their own commit(s).

…ent nightly

apoelstra · 2024-03-26T16:56:07Z

This is ready for review. I extended it a bit to support the descriptor checksum but I'll stop extending it and do future changes in a later PR.

tcharding

That was a pleasure to review, the code is super clean. Code review only, no clue if the math is right :)

tcharding · 2024-03-26T17:59:06Z

.github/workflows/rust.yml

-    name: Test - 1.48.0 toolchain
+    name: Test - 1.56.1 toolchain


We need update to the README also man.

Can you go through and resolve all the comments (at least the nits).

tcharding · 2024-03-26T18:03:46Z

src/primitives/field.rs

+    + ops::Add<Self, Output = Self>
+    + for<'a> ops::Add<&'a Self, Output = Self>
+    + ops::Sub<Self, Output = Self>
+    + ops::AddAssign
+    + for<'a> ops::AddAssign<&'a Self>
+    + for<'a> ops::Sub<&'a Self, Output = Self>
+    + ops::SubAssign
+    + for<'a> ops::SubAssign<&'a Self>
+    + ops::Mul<Self, Output = Self>
+    + for<'a> ops::Mul<&'a Self, Output = Self>
+    + ops::MulAssign
+    + for<'a> ops::MulAssign<&'a Self>
+    + ops::Div<Self, Output = Self>
+    + for<'a> ops::Div<&'a Self, Output = Self>
+    + ops::DivAssign
+    + for<'a> ops::DivAssign<&'a Self>
+    + ops::Neg


All the references are a bit noisy, I found this order is easier to read:

+ ops::Add<Self, Output = Self> + ops::Sub<Self, Output = Self> + ops::AddAssign + ops::SubAssign + ops::Mul<Self, Output = Self> + ops::MulAssign + ops::Div<Self, Output = Self> + ops::DivAssign + for<'a> ops::Add<&'a Self, Output = Self> + for<'a> ops::Sub<&'a Self, Output = Self> + for<'a> ops::AddAssign<&'a Self> + for<'a> ops::SubAssign<&'a Self> + for<'a> ops::Mul<&'a Self, Output = Self> + for<'a> ops::MulAssign<&'a Self> + for<'a> ops::Div<&'a Self, Output = Self> + for<'a> ops::DivAssign<&'a Self> + ops::Neg

tcharding · 2024-03-26T18:08:42Z

src/primitives/field.rs

+    /// instead of calling this.
+    fn _mul(&self, other: &Self) -> Self;
+
+    /// Divides a value from `self`. This is a helper funcion for implementing the


Is this real English? If so awesome and TIL, I have, in the past, written "divides self by value" and wished I could have the operands in the other order.

Heh, I think the English is a bit sketchy ... but as you say, if you swap the operands there isn't really a good way to phrase it.

tcharding · 2024-03-26T18:13:49Z

src/primitives/field.rs

+    fn multiplicative_order(&self) -> usize {
+        for &ord in Self::MULTIPLICATIVE_ORDER_FACTORS {
+            if self.powi(ord) == Self::ONE {
+                return ord as usize;


Why is this cast ok? Or said another way, why are multiplicative order factors i64s but we return a usize?

I think I should make the factors all be usizes.

But all these casts are OK because in practice none of these values will ever exceed 32 bits (or even 16 bits, probably).

tcharding · 2024-03-26T18:26:32Z

src/primitives/field.rs

+}
+
+macro_rules! impl_ops_for_fe {
+    (impl$(<$params1:tt>)? for $op:ident$(<$param:tt>)?) => {


I'm not understanding what params1 is for, it doesn't appear in any function except on the impl keyword. Also its not used in any of the macro call sites?

I should probably drop it. It was going to be for if I could implement this generically on Fe32Ext<DEG> but then I couldn't figure out how to write a generic-over-DEG division algo so I couldn't do it.

tcharding · 2024-03-26T18:30:11Z

src/primitives/gf32_ext.rs

+//!
+//! We support specifically the fields GF1024 and GF32768 (the extension
+//! fields of degree 2 and 3, respectively), though we have tried to write
+//! the code in such a way that more can be added if codes require thhem.


Suggested change

//! the code in such a way that more can be added if codes require thhem.

//! the code in such a way that more can be added if codes require them.

tcharding · 2024-03-26T18:31:50Z

src/primitives/gf32_ext.rs

+#[derive(Copy, Clone, PartialEq, Eq, Hash)]
+pub struct Fe32Ext<const DEG: usize> {
+    /// The polynomial representation of the element in "little-endian" order;
+    /// that is, the element is the sum inner[i] * EXT_ELEM^i.


Square brackets need escaping like you do in other places.

3 instances in the PR, find with cargo rustdoc --all-features -- --document-private-items

Note please, before you change the PR, I make a further contradictory comment below.

tcharding · 2024-03-26T18:50:04Z

src/primitives/checksum.rs

@@ -154,6 +319,18 @@ pub trait PackedFe32: Copy + PartialEq + Eq + ops::BitXor<Self, Output = Self> {
    /// The number of fe32s that can fit into the type; computed as floor(bitwidth / 5).
    const WIDTH: usize = mem::size_of::<Self>() * 8 / 5;

+    /// Takes an iterator of `u8`s (or [`Fe32`]s converted to `u8`s) and packs
+    /// them into a Self.


nit, since the rest of the PR is so clean, lets use Self here.

tcharding · 2024-03-26T18:51:32Z

src/primitives/checksum.rs

+    /// # Panics
+    ///
+    /// If the iterator yields more items than can fit into the bit-packed
+    /// type, will panic.


Should this be "may panic" since its on a trait and its up to the implementor?

tcharding · 2024-03-26T19:00:02Z

src/primitives/gf32_ext.rs

+
+    /// The element zeta such that the extension field is defined as GF32\[zeta\].
+    ///
+    /// Alternately, the image of x in the mapping GF32\[x\]/p(x) -> the field


nit: The escapes make it pretty noisy in the source code, perhaps we just put backtics around the whole expression?

/// Alternately, the image of x in the mapping `GF32[x]/p(x)` -> the field

(I built the docs and looked at it, looks ok in HTML.)

tcharding · 2024-03-27T00:04:21Z

Meant to mention before, I found the API files very useful when reviewing.

…s on

I want to use const generics and I need 1.56 to do it. The rest of the rust-bitcoin ecosystem has moved to 1.56.1.

This new trait covers a larger matrix of ops, and implements them all with a single macro. Will allow us to introduce new fields (in particular, extension fields of GF32) in a future commit with more consistent/less boilerplate heavy implementations of basic arithmetic. We run the API checker in this commit so you can see that the API changes are strictly additive and only add missing op implementations.

Again, strictly adding API surface, not removing anything.

This introduces a type `Polynomial` which wraps a vector of field elements which it treats as the coefficients of a polynomial. It exposes only an ad-hoc set of functionality needed for error correction and is not intended to be a general-purpose polynomial type, so it is not exposed in the public API. Because it is backed by a `Vec` the type and its module are only available when the alloc feature is available.

Adds a couple elements to the API. Note that this *is* a breaking change because we are adding a method to the `PackedFe32` trait, which is not sealed, so any downstream implementors will be broken.

For the most part, BCH codes used in the ecosystem are defined as generator polynomials over GF32 and a target residue polynomial, and the remaining code parameters are not specified, or are only specified indirectly. It is difficult for non-experts to compute or even validate parameters such as the length of the code or the shifted+packed generator polynomials. It will be even more difficult in the sequel when we extend the Checksum trait to also cover error-correction parameters (some of which will depend on our particular choice of representation for GF1024 elements, which is not canonical and differs between different documents). So we introduce an object `PrintImpl` and a unit test and doctest demonstrating its use, which will generate all of the needed parameters and output a block of Rust code.

apoelstra · 2024-03-27T19:57:30Z

Addressed all comments.

tcharding

ACK 02282fa

apoelstra · 2024-03-30T19:19:21Z

@clarkmoody what are next steps for this PR?

tcharding · 2024-04-25T05:33:35Z

I rekon we can merge this, its been open for a while now. @clarkmoody speak now or forever hold your peace :)

apoelstra · 2024-04-25T12:02:13Z

It's a nontrivial change and unlike rust-bitcoin, I don't have a mandate to steer this crate (and actually even Github won't let me because you're not an approved approver), so I think we've gotta wait for Clark.

tcharding · 2024-04-25T21:25:00Z

Fair.

apoelstra force-pushed the 2024-03--error-correction branch 2 times, most recently from 6ea3734 to eea45d6 Compare March 25, 2024 01:48

api: add the Freeze marker which apparently was introduced in a rec…

0aeea2e

…ent nightly

apoelstra force-pushed the 2024-03--error-correction branch 2 times, most recently from a929a57 to 6c386ab Compare March 26, 2024 16:54

tcharding reviewed Mar 26, 2024

View reviewed changes

apoelstra added 10 commits March 27, 2024 19:49

docs: fix a couple unescaped [ instances which cargo doc complain…

b5a019b

…s on

no-std/alloc: fix a couple "unused import" warnings

a70e3ca

bump MSRV to 1.56.1.

be02165

I want to use const generics and I need 1.56 to do it. The rest of the rust-bitcoin ecosystem has moved to 1.56.1.

primitives: add extension field module, define GF1024

b54f78a

Again, strictly adding API surface, not removing anything.

PackedFe32: introduce ability to re-pack elements

fa8b70f

Adds a couple elements to the API. Note that this *is* a breaking change because we are adding a method to the `PackedFe32` trait, which is not sealed, so any downstream implementors will be broken.

gf_ext: add cubic extension field of GF32, GF32768

25bb6f9

checksum: add unit test to print parameters of the descriptor checksum

02282fa

apoelstra force-pushed the 2024-03--error-correction branch from 6c386ab to 02282fa Compare March 27, 2024 19:57

tcharding approved these changes Mar 28, 2024

View reviewed changes

apoelstra mentioned this pull request Mar 29, 2024

benchmarks: restore benchmarks #182

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checksum: introduce `PrintImpl` object that can compute various code properties #180

checksum: introduce `PrintImpl` object that can compute various code properties #180

apoelstra commented Mar 25, 2024 •

edited

apoelstra commented Mar 25, 2024

apoelstra commented Mar 26, 2024

tcharding left a comment

tcharding Mar 26, 2024

apoelstra Mar 31, 2024

tcharding Mar 26, 2024

tcharding Mar 26, 2024

apoelstra Mar 26, 2024

tcharding Mar 26, 2024

apoelstra Mar 26, 2024

apoelstra Mar 26, 2024

tcharding Mar 26, 2024

apoelstra Mar 26, 2024

tcharding Mar 26, 2024

tcharding Mar 26, 2024

tcharding Mar 26, 2024

tcharding Mar 26, 2024

tcharding Mar 26, 2024

tcharding Mar 26, 2024

tcharding Mar 26, 2024

tcharding commented Mar 27, 2024

apoelstra commented Mar 27, 2024

tcharding left a comment

apoelstra commented Mar 30, 2024

tcharding commented Apr 25, 2024 •

edited

apoelstra commented Apr 25, 2024

tcharding commented Apr 25, 2024

	//! the code in such a way that more can be added if codes require thhem.
	//! the code in such a way that more can be added if codes require them.

checksum: introduce PrintImpl object that can compute various code properties #180

Are you sure you want to change the base?

checksum: introduce PrintImpl object that can compute various code properties #180

Conversation

apoelstra commented Mar 25, 2024 • edited

apoelstra commented Mar 25, 2024

apoelstra commented Mar 26, 2024

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Mar 27, 2024

apoelstra commented Mar 27, 2024

tcharding left a comment

Choose a reason for hiding this comment

apoelstra commented Mar 30, 2024

tcharding commented Apr 25, 2024 • edited

apoelstra commented Apr 25, 2024

tcharding commented Apr 25, 2024

checksum: introduce `PrintImpl` object that can compute various code properties #180

checksum: introduce `PrintImpl` object that can compute various code properties #180

apoelstra commented Mar 25, 2024 •

edited

tcharding commented Apr 25, 2024 •

edited