Skip to content
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

Bech32m Support #50

Merged
merged 5 commits into from Feb 14, 2021
Merged

Bech32m Support #50

merged 5 commits into from Feb 14, 2021

Conversation

clarkmoody
Copy link
Member

Adds support for BIP-0350, Bech32m encoding. Bumps version to 0.8.0.

Breaking Changes

  • decode returns the variant as third value upon success. This will be used downstream when validating SegWit addresses against the allowed encoding variant.
  • encode, encode_to_fmt, and Bech32Writer::new require variant parameters. When creating SegWit addresses, the proper variant will be selected based on the script version.

Closes #48

This breaks the API by requiring a `variant` parameter in a few places,
which instructs the library whether to use Bech32 or Bech32m.

Test cases from BIP-0350
Copy link
Contributor

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not reviewed against the spec; few suggestion after the first code read

#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
pub enum Variant {
/// The original Bech32 described in [BIP-0173](https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki)
Bech32,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we pls have the actual constants assigned as the values here? When we will not need BECH32_CONST, BECH32M_CONST, and can simply use Variant::Bech32 as u32 instead of constant() function call (and avoid one call)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with @clarkmoody's version, it's more "standard" imo, do you feel strongly about this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure what you mean @dr-orlovsky. Could you post a code example?

Performance-wise, the compiler will inline these simple methods for sure.

@@ -400,22 +410,52 @@ pub fn encode_to_fmt<T: AsRef<[u5]>>(
}
}

/// Used for encode/decode operations for the two variants of Bech32
#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
pub enum Variant {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest also to add repr(u32) here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do for us?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's necessary to implement the Variant::Bech32 as u32 API afaik (making the enum castable to its internal representation). I don't really like it because it isn't too well-known this is possible and also seems a bit like abusing this feature here for probably non-existent performance gains.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I'll leave this out for now, and we can circle back later if the need arises.

Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, very elegant 😃

ACK 411d8c4

#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
pub enum Variant {
/// The original Bech32 described in [BIP-0173](https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki)
Bech32,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with @clarkmoody's version, it's more "standard" imo, do you feel strongly about this?

@sgeisler
Copy link
Contributor

Btw, do you know what's wrong with CI? It just doesn't run that one job, maybe we should switch to GH actions too …

@clarkmoody
Copy link
Member Author

Btw, do you know what's wrong with CI? It just doesn't run that one job, maybe we should switch to GH actions too …

I think it's a modern version of Clippy failing on our 1.22-compatible code.

I agree that we should switch to GH actions. I'll work on that. Do you think it's also time to raise our version to 1.29 to match the rust-bitcoin project?

@sgeisler
Copy link
Contributor

Do you think it's also time to raise our version to 1.29 to match the rust-bitcoin project?

Oh lol, I was totally unaware that we didn't do that yet, otoh no surprise given this crate's stability. If it helps sure! Idk how much better clippy can cope with 1.29 though, it's also ancient.

Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new tests are a bit redundant (why check when you are going to build and test anyway?) and I'd prefer to only test if there's not a good argument to be made for checking. Later we will probably expand the matrix to test features like no-std and this could cost us time when we hit the maximum parallel worker limit.

@clarkmoody
Copy link
Member Author

The new tests are a bit redundant (why check when you are going to build and test anyway?) and I'd prefer to only test if there's not a good argument to be made for checking. Later we will probably expand the matrix to test features like no-std and this could cost us time when we hit the maximum parallel worker limit.

Agree. Removed the check job, as that functionality is duplicated by test.

Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 335f6bb

@clarkmoody clarkmoody merged commit 6c87d18 into master Feb 14, 2021
@clarkmoody clarkmoody deleted the bech32m branch February 14, 2021 21:38
@dr-orlovsky dr-orlovsky added this to Done in Taproot Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Support bech32m
3 participants