checksum: introduce `PrintImpl` object that can compute various code properties by apoelstra · Pull Request #180 · rust-bitcoin/rust-bech32

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).

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 :)

.github/workflows/rust.yml

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

    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.

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.

tcharding · 2024-07-04T22:01:08Z

Hi @clarkmoody, is there anything we can do to make reviewing this easier for you? I got a message out of band from FROST devs chasing error correction. I realize this is a massive PR and not at all easy to review.

cc @nickfarrow

src/primitives/field.rs

src/primitives/gf32.rs

src/primitives/polynomial.rs

clarkmoody · 2024-07-05T15:53:00Z

src/primitives/checksum.rs

+        // any sort of `eval`, but you can manually check the output works.
+        let _s = PrintImpl::<Fe1024>::new(
+            "Bech32",
+            &[Fe32::A, Fe32::K, Fe32::_5, Fe32::_4, Fe32::A, Fe32::J],


Can we pass &unpacked_poly here directly, to avoid duplication/confusion?

I'll try but IIRC there was some type confusion that prevented me from doing so

Oh, I see, you mean to take the unpacked_poly computation from the unit test and put it in the doctest, so that the only magic constants that appear clearly came from the BIP. Sure.

src/primitives/checksum.rs

clarkmoody · 2024-07-05T16:31:23Z

This is some real Big Brain stuff! I've pointed out some minor things here and there.

I apologize for going AWOL for so long.

apoelstra · 2024-07-05T19:09:12Z

Let me open a PR to get CI working then I'll rebase this and address your comments. In the last couple months there has been a bunch of breakage in check-api and even rustc itself.

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.

apoelstra · 2024-07-05T20:56:24Z

Rebased and addressed all nits.

src/primitives/polynomial.rs

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.

clarkmoody

ACK caefc44

tcharding · 2024-07-05T23:05:45Z

Thanks @clarkmoody, way to go.

tcharding · 2024-07-05T23:48:24Z

I'm not going to re-review this, I don't think a shallow re-ack from me adds much value.

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

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 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

Merged

clarkmoody reviewed Jul 5, 2024

View reviewed changes

src/primitives/field.rs Outdated Show resolved Hide resolved

clarkmoody reviewed Jul 5, 2024

View reviewed changes

src/primitives/gf32.rs Outdated Show resolved Hide resolved

clarkmoody reviewed Jul 5, 2024

View reviewed changes

src/primitives/gf32.rs Outdated Show resolved Hide resolved

clarkmoody reviewed Jul 5, 2024

View reviewed changes

src/primitives/polynomial.rs Outdated Show resolved Hide resolved

clarkmoody reviewed Jul 5, 2024

View reviewed changes

src/primitives/polynomial.rs Outdated Show resolved Hide resolved

clarkmoody reviewed Jul 5, 2024

View reviewed changes

src/primitives/checksum.rs Outdated Show resolved Hide resolved

clarkmoody mentioned this pull request Jul 5, 2024

fixes for last couple months of CI changes #183

Merged

apoelstra added 3 commits July 5, 2024 20:23

bump MSRV to 1.56.1.

bd6f2f6

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

cf545e4

Again, strictly adding API surface, not removing anything.

apoelstra force-pushed the 2024-03--error-correction branch from 02282fa to b977a7e Compare July 5, 2024 20:53

clarkmoody reviewed Jul 5, 2024

View reviewed changes

src/primitives/polynomial.rs Outdated Show resolved Hide resolved

apoelstra added 5 commits July 5, 2024 22:11

PackedFe32: introduce ability to re-pack elements

4c06f72

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

65aefd2

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

caefc44

apoelstra force-pushed the 2024-03--error-correction branch from b977a7e to caefc44 Compare July 5, 2024 22:12

clarkmoody approved these changes Jul 5, 2024

View reviewed changes

apoelstra merged commit b03c2cc into rust-bitcoin:master Jul 6, 2024

apoelstra deleted the 2024-03--error-correction branch September 11, 2024 13:10

apoelstra mentioned this pull request Sep 11, 2024

Source code for error correction? BlockstreamResearch/codex32#54

Open

	//! 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.

Conversation

apoelstra commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Mar 25, 2024

Uh oh!

apoelstra commented Mar 26, 2024

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Mar 27, 2024

Uh oh!

apoelstra commented Mar 27, 2024

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Mar 30, 2024

Uh oh!

tcharding commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Apr 25, 2024

Uh oh!

tcharding commented Apr 25, 2024

Uh oh!

tcharding commented Jul 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clarkmoody commented Jul 5, 2024

Uh oh!

apoelstra commented Jul 5, 2024

Uh oh!

apoelstra commented Jul 5, 2024

Uh oh!

Uh oh!

clarkmoody left a comment

apoelstra commented Mar 25, 2024 •

edited

Loading

tcharding commented Apr 25, 2024 •

edited

Loading