Introduce `FieldVec` and improve `Polynomial` and `InvalidResidueError` #204

apoelstra · 2024-09-19T14:09:56Z

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

tcharding

My review does not cover the math.

tcharding · 2024-09-19T22:43:21Z

src/primitives/polynomial.rs

+    /// Panics if [`Self::has_data`] is false, with an informative panic message.
+    /// Provide access to the underlying [`FieldVec`].
+    pub fn into_inner(self) -> FieldVec<F> { self.inner }


Docs and code don't match, this doesn't panic, did you mean to call assert_has_data()

No, I think the comment is just out of date. I moved all the panics into functions on FieldVec so there's no need for anything in Polynomial to directly panic.

tcharding · 2024-09-19T22:44:22Z

src/primitives/polynomial.rs

+    /// Provide access to the underlying [`FieldVec`].
+    pub fn into_inner(self) -> FieldVec<F> { self.inner }
+
+    /// Provide access to the underlying [`FieldVec`].


Doing super picky review because we should be very close to 1.0'ing this crate.

Suggested change

/// Provide access to the underlying [`FieldVec`].

/// Provides access to the underlying [`FieldVec`].

src/primitives/polynomial.rs

src/lib.rs

tcharding · 2024-09-19T23:00:18Z

src/primitives/polynomial.rs

+
+impl<F: Field> ops::Sub<&Polynomial<F>> for Polynomial<F> {
+    type Output = Polynomial<F>;
+    fn sub(self, other: &Polynomial<F>) -> Polynomial<F> { self + other }


This is finite field math so add and sub are the same, right? Perhaps add a comment for less cryptographically trained devs.

In this case I probably should actually pass through to the sub impl for F (which is the same as the add impl in all cases, because I have a binary field, but this stuff should all still work even with non-binary fields).

apoelstra · 2024-09-21T14:05:46Z

Clicked "resolve" on the conversations we agreed were not actionable.

Force-pushed to address the other comments.

apoelstra

Successfully ran local tests on 7a28bac.

apoelstra · 2024-09-21T15:29:58Z

Second force-push was to fix a bug converting residues to polynomials which I noticed when working on the next PR.

apoelstra

Successfully ran local tests on 8ee88fc.

apoelstra

Successfully ran local tests on 8ee88fc.

apoelstra · 2024-09-21T20:38:39Z

Ok, I have working error correction code now based on this PR :). I will stop iterating on it until somebody reviews it.

apoelstra

Successfully ran local tests on 8ee88fc.

tcharding · 2024-09-21T20:41:05Z

Is a further non-math review useful by me, I can do it if so. I don't want to bother the PR with petty concerns if not.

apoelstra · 2024-09-21T20:57:20Z

I think it'd be valuable, yeah, if you're willing to take the time.

tcharding · 2024-09-21T21:11:07Z

I'll make a coffee :)

tcharding

There were a couple of other super trivial docs things I saw but we'll likely need to go over the docs with a fine tooth comb before we try to do 1.0 so I didn't bother mentioning them.

tcharding · 2024-09-21T21:30:30Z

src/primitives/fieldvec.rs

+/// Parameterized by the field type `F` which can be anything, but for most methods
+/// to be enabled needs `Default` and `Clone`. (Both are implied by `Field`.)
+#[derive(PartialEq, Eq, Clone, Debug, Hash)]
+pub struct FieldVec<F> {


Why go to trouble of mentioning that this should not be exposed in the public API and make this type pub? Code seems to build with pub(crate). Is it because of upcoming work?

pub(crate) would make this visible to the entire crate. pub makes it visible only to the parent module (who in turn re-exports it privately to make it visible to other submodules).

I could do pub(super) if it makes you happier.

Oh with private field module, I've never thought to do that before. Thanks for the explanation.

tcharding · 2024-09-21T21:36:54Z

src/primitives/fieldvec.rs

+        iter::successors(Some(F::ONE), |gen| Some(elem.clone() * gen)).take(n + 1).collect()
+    }
+
+    /// Multiply the elements of two vectors together, pointwise.


Needs panics docs.

Sure, I'll add them. Though all these docs are private of course.

Should have had two coffees, didn't notice the whole field module being private yesterday.

tcharding · 2024-09-21T21:37:11Z

src/primitives/fieldvec.rs

+        }
+    }
+
+    /// Multiply the elements of two vectors together, pointwise.


And panic docs here I suppose as well.

tcharding · 2024-09-21T21:41:34Z

src/primitives/fieldvec.rs

+impl<F: Default> FieldVec<F> {
+    /// Pushes an item onto the end of the vector.
+    pub fn push(&mut self, item: F) {
+        self.len += 1;


Perhaps its slightly cleaner to put this increment at the end of the function, saves all the + 1s below and saves reader from having to check there are no return paths where the increment was done and an item not added.

Oh, did you write this to be uniform with pop?

Yeah, agreed, it'd be cleaner to move the increment. Will do.

@tcharding actually, the increment is before the assert_has_data assertion so that the panic message will be informative if you try to push too much data. Otherwise there'll just be an "index out of bounds" panic in the no-alloc case.

Interesting, I missed that. Thanks.

tcharding · 2024-09-21T21:43:21Z

src/primitives/fieldvec.rs

+        #[cfg(not(feature = "alloc"))]
+        {
+            self.inner_a[self.len - 1] = item;
+        }


Suggested change

#[cfg(not(feature = "alloc"))]

{

self.inner_a[self.len - 1] = item;

}

#[cfg(not(feature = "alloc"))]

self.inner_a[self.len - 1] = item;

Or self.inner_a[self.len] = item if you use my other suggestion.

Did you try compiling this suggestion?

error[E0658]: attributes on expressions are experimental

Worst review suggestion ever :)

src/primitives/fieldvec.rs

apoelstra · 2024-09-22T14:54:32Z

Improved doccomments and added a bunch of unit tests. Also added missing Index<Range<usize>> impl on fieldvec.

This will allow `Polynomial` to work without an allocator (at least for "small" checksums. Here a "small" checksum is one of length at most 6 (which covers bech32 and bech32m). The descriptor checksum (8 characters), codex32 (13 characters) and "long codex32" (15 characters) will not work with no-alloc. I would like to fix this but it results in large types, especially a large InvalidResidueError type (see last commit of this PR), so probably it will need to be feature-gated or something. For now we just punt on it. This PR introduces the `primitives::correction` module but only puts a single constant into it for now.

We can now use `PrintImpl` even without an allocator. This is of limited use of course, but it paves the ground for use to do error correction without an allocator, which is interesting.

We are going to want to extend the ChecksumError::InvalidResidue error variant to allow error correction, and in order to do so, we need to know the actual residue computed from a string, not just that the residue failed to match the target. Uses the `Polynomial` type; see the previous commits for some caveats about this type when alloc is disabled. (Prior to this PR, you just couldn't use Polynomial at all without alloc.) As an initial application of the new error, avoid re-validating a bech32 address to handle the distinction between bech32m and bech32. Instead, if bech32m validation fails, check if the "bad" residue actually matches the bech32 target. If so, accept. On my system this reduces the `bech32_parse_address` benchmark from 610ns to 455ns, more than a 33% speedup.

This is just a helper trait to describe all the errors in the library from which we can get an "invalid residue". The residues contain all the information needed for error correction.

Two polynomials are equal even if they have a different number of leading zeros. Putting this into its own commit rather than folding it into one of the FieldVec commits, because this bug was present even before the FieldVec stuff. Also remove the `Hash` impl because it's unneeded and we would be expected to keep it in sync with Eq.

apoelstra

Successfully ran local tests on c209ce8.

tcharding

ACK c209ce8

clarkmoody

ACK c209ce8

Tests running. No further code comments.

apoelstra force-pushed the 2024-09--fieldvec branch from 3ad8929 to 74a80d3 Compare September 19, 2024 14:10

tcharding reviewed Sep 19, 2024

View reviewed changes

apoelstra force-pushed the 2024-09--fieldvec branch from 74a80d3 to 7a28bac Compare September 21, 2024 14:05

apoelstra commented Sep 21, 2024

View reviewed changes

apoelstra force-pushed the 2024-09--fieldvec branch from 7a28bac to 8ee88fc Compare September 21, 2024 14:27

apoelstra commented Sep 21, 2024

View reviewed changes

apoelstra force-pushed the 2024-09--fieldvec branch 5 times, most recently from 8d1b8c8 to 3b74657 Compare September 21, 2024 18:17

apoelstra commented Sep 21, 2024

View reviewed changes

apoelstra force-pushed the 2024-09--fieldvec branch from 3b74657 to 03d265d Compare September 21, 2024 20:03

apoelstra commented Sep 21, 2024

View reviewed changes

tcharding reviewed Sep 21, 2024

View reviewed changes

clarkmoody reviewed Sep 22, 2024

View reviewed changes

src/primitives/fieldvec.rs Show resolved Hide resolved

apoelstra force-pushed the 2024-09--fieldvec branch 2 times, most recently from 61f0810 to 28c97e6 Compare September 22, 2024 14:54

apoelstra added 5 commits September 22, 2024 14:56

polynomial: use FieldVec in Polynomial

690fc7d

We can now use `PrintImpl` even without an allocator. This is of limited use of course, but it paves the ground for use to do error correction without an allocator, which is interesting.

polynomial: add some extra functionality enabled by FieldVec

b76adbe

correction: introduce CorrectableError trait

d08da46

This is just a helper trait to describe all the errors in the library from which we can get an "invalid residue". The residues contain all the information needed for error correction.

apoelstra added 3 commits September 22, 2024 14:56

fieldvec: add a bunch of unit tests

dfc29ad

polynomial: expand unit tests

c209ce8

apoelstra force-pushed the 2024-09--fieldvec branch from 28c97e6 to c209ce8 Compare September 22, 2024 14:56

apoelstra commented Sep 22, 2024

View reviewed changes

tcharding approved these changes Sep 22, 2024

View reviewed changes

clarkmoody approved these changes Sep 30, 2024

View reviewed changes

apoelstra merged commit 3f98190 into rust-bitcoin:master Sep 30, 2024
12 checks passed

apoelstra deleted the 2024-09--fieldvec branch September 30, 2024 18:07

	/// Provide access to the underlying [`FieldVec`].
	/// Provides access to the underlying [`FieldVec`].

Introduce FieldVec and improve Polynomial and InvalidResidueError #204

Introduce FieldVec and improve Polynomial and InvalidResidueError #204

Conversation

apoelstra commented Sep 19, 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

apoelstra commented Sep 21, 2024

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Sep 21, 2024

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Sep 21, 2024

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Sep 21, 2024

apoelstra commented Sep 21, 2024

tcharding commented Sep 21, 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

apoelstra commented Sep 22, 2024

apoelstra left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

clarkmoody left a comment

Choose a reason for hiding this comment

Introduce `FieldVec` and improve `Polynomial` and `InvalidResidueError` #204

Introduce `FieldVec` and improve `Polynomial` and `InvalidResidueError` #204