-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend Field
and Checksum
for error correction
#203
Extend Field
and Checksum
for error correction
#203
Conversation
This is purely a code refactor to get rid of a `use` statement (which will trigger an "unused code" warning if the macro is ever called somewhere where the Field trait is already in scope). As a side effect it reduces the size of the macro by roughly 75% in terms of line count.
Several methods (and we are going to add more) don't really belong in a general-purpose field trait, which really is just "a type that can be multiplied and added and has some associated constants". This isn't a general-purpose math library, but fields are a pretty common thing for people to want. And it's pretty easy for us to expose this trait in a nice way, so we might as well do it.
This will make it easier for PrintImpl to output error correction parameters. Since this is an implementation detail of the library, stick it into the Bech32Field trait rather than the Field one.
In the next commits we'll introduce a generic no-alloc container called FieldVec whose implementation will be much nicer if we have a Default bound on all of our field elements. This way we can implement the "FieldVec", which despite the name really has nothing to do with fields, purely in terms of core traits, rather than confusing readers of the code by having Field bounds everywhere without ever doing algebra. Since fields all have a ZERO member, which is a sensible output for Default, this bound is reasonable to require.
This adds some new methods to Field, which have default impls and are therefore non-breaking. (And they are general-purpose "field" things.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Successfully ran local tests on f47ec0b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f47ec0b
Tests running locally. The math is above my pay-grade.
/// Sealing trait. | ||
pub trait Sealed {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clever 👌
There are two parameterizations of the bech32 checksum (see the "roots" unit test in src/primitives/polynomial.rs for what they are). In rust-bitcoin#203 we mixed them up, using the generator from one but the exponents from the other. We made the same mistake with codex32 apparently. When we implement error correction this will cause failures. Fix it.
There are two parameterizations of the bech32 checksum (see the "roots" unit test in src/primitives/polynomial.rs for what they are). In rust-bitcoin#203 we mixed them up, using the generator from one but the exponents from the other. We made the same mistake with codex32 apparently. When we implement error correction this will cause failures. Fix it.
The first couple commits of this PR split the
Field
trait into two -- a generalField
trait which has the algebraic functionality of a field, and a sealedBech32Field
trait which has the functionality that is specific to this crate.The rest add new functionality to the traits which describe extra properties of our checksums needed for error correction.
The next PRs will:
FieldVec
type to backPolynomial
to make it work in a no-alloc setting (at least for small checksums, for a configurable notion of "small")