Hi all,

I guess, in this line

impl fmt::Display for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        use Error::*;

        match *self {
            TooLong(len) => write!(f, "hrp is too long, found {} characters, must be <= 126", len),  // <--- HERE
            Empty => write!(f, "hrp is empty, must have at least 1 character"),
            NonAsciiChar(c) => write!(f, "found non-ASCII character: {}", c),
            InvalidAsciiByte(b) => write!(f, "byte value is not valid US-ASCII: \'{:x}\'", b),
            MixedCase => write!(f, "hrp cannot mix upper and lower case"),
        }
    }
}

a previously defined constant should be used, like:

impl fmt::Display for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        use Error::*;

        match *self {
            TooLong(len) => write!(f, "hrp is too long, found {} characters, must be <= {}", len, MAX_HRP_LEN),
            Empty => write!(f, "hrp is empty, must have at least 1 character"),
            NonAsciiChar(c) => write!(f, "found non-ASCII character: {}", c),
            InvalidAsciiByte(b) => write!(f, "byte value is not valid US-ASCII: \'{:x}\'", b),
            MixedCase => write!(f, "hrp cannot mix upper and lower case"),
        }
    }
}

Currently, such an error is reported when the prefix is too long:
hrp is too long, found 85 characters, must be <= 126
which is somewhat confusing ;-).

Looks like a typo in our rust.yml introduced by #71. To fix this we'll have to open a PR and keep checking the "Actions" tab (which I should've done on 71 itself, sorry) until it passes. I think.

https://github.com/rust-bitcoin/rust-bech32/actions/runs/4298692799

Hi

I am using this library, and saw that an update is available from the 0.8.1 release I am currently using. It is good practice to keep a changelog so users know what changed in every release.

I checked the commit history, so no need to add one for past releases, but consider adding one for future releases.

All the while working on this crate I have mistook the u5 type to be a general purpose integer type. To improve the API and help the next non-mathematician reading the code we could re-work the u5 type to make its purpose more explicit.

I think of u5 as a field element in GF32, definitely not as a standalone integer type. We could rename it if this is confusing.

Do we have Add and Mul implemented on this type, and do they respect field operations or "mod 32" operatons?

To be clearer: this crate has no use for integers mod 32, and I can't think of any crate that does.

ref: #89 (comment)

Consider fixing #80 at the same time.

It'd be great to polish this crate and make it stable with idiomatic API and good documentation. Since this crate appears stale I'm willing to help maintain it to make the progress a bit faster.

I'm still unsure what all those traits do, they seem to be meant as extension traits?
Thinking about ideal API, I identified these basic building blocks:

• u5 type
• converting stream of u8 to stream of u5 and back
• calculating checksum
• converting stream of u8 to ASCII and back

I think each deserves its own module, maybe even crate. The commonly used things can still be flattened for consumers.

u5 type

I think this is good as is. Could add impl TryFrom<*> for u5 and arithmetic functions for completeness.

u8-u5 conversion

The API should enable to perform this conversion without allocation by having wrappers around iterators.
Convenience functions for slices and Vecs can be added but not sure how important given .iter() and .collect() are easy to call.
This should also provide free-standing functions to calculate encoded/decoded lengths.

checksum calculation

The API should be similar to hashers with possible convenience to compute from iterators.
It should also provide a wrapper for the case when checksum is calculated together with conversions.

u8-ASCII conversion

Should be similar to u8-u5 conversion. Might be useful to provide wrappers returning both ASCII u8 and char to avoid overhead when writing to writer. Also maybe provide a method to construct String without unnecessary UTF-8 check (using unsafe internally).

Facade

Convenience functions for converting stream of u8 to strings (with/without checksum) and back should be provided.

Also for consideration: exploit the invariant of u5 to skip bounds checks when converting to char.

After #30, #31, and #32, let's bump the minor version, since we're expanding the API.

Hello!

Thank you for your hard work.

Would it be possible to maintain a changelog.md file to track changes between versions?

None of our encoding functions enforce the 90 maximum length specified in BIP-173.

We should be enforcing hrp.len() + 1 + bytes_len_to_fes_len(data.len()) <= 90

Found while implementing buffered encoding in: #138

Fuzz tests in rust-bitcoin/rust-bitcoin#100 uncovered the bug that if the bech32 data part contains an uppercase 'O' the parser panics. This is due to insufficient checks that only check the lowercase variants of the forbidden characters (before normalizing them to their lowercase variant):

if b == b'1' || b == b'b' || b == b'i' || b == b'o' {
    return Err(Error::InvalidChar(b))
}

To avoid such issues in the future I will have a look at fuzz testing this crate (#21).

The current implementation leads to confusing type errors and needlessly limits the usable types.

@clarkmoody what do you think about adding me (or @TheBlueMatt or someone else involved in rust-bitcoin/rust-lightning) as a manitainer?

Any reason why the u5 type does not implement Display?

We have an epic on rust-bitcoin to overhaul/audit errors. Would it be useful to do it here first as a test run?

FTR I do not know exactly what is the optimal error handling strategy so need to learn.

cc @Kixunil

Using From (or into()) is easier to read than using casts because it stops devs having to wonder if an data being lost.

We have a few places where we use casts to cast u8 to usize

            let log1 = LOG[self.0 as usize];
            let log2 = LOG[other.0 as usize];

Could be written as:

            let log1 = LOG[usize::from(self.0)];
            let log2 = LOG[usize::from(other.0)];

When writing out an object as a bech32 string via fmt it seems we currently have to first encode the [u8] into a Vec<u5> and only then we can call encode_without_checksum_to_fmt. This is a bit wasteful, ISTM it wouldn't take a whole log of change to build a version of convert_bits that returns an iterator rather than a Vec, and allows avoiding the intermediate allocation.

Tracking issue for what is needed for us to be able to stabalize the bech32 crate.

Issues we want resolved

• #95

Issues that might apply

I would like to only verify that the hrp part is valid.
The crate has a method; check_hrp but this is private.

I would prefer not to copy the code :)

Thanks 🙏

This release includes a major API change due to #35. Are there any changes that we want to bundle with it in 0.7?

• merge version bump #36
• tag version 0.7.0
• release to crates.io

I saw at least a lack of doc(cfg(...)) on std::error::Error impl there's likely more.

Hi,

Lets say I have a base32 string 'nbswy3dpo5xxe3de', I was expecting this library to support it as input or at least provide with an helper function to do so, but it seems I need a pre-step to convert the string to a special vec with integers ranging from 0 to 31.

Does it makes sense to add this helper function ?

It has a scary enough name and is useful outside of the crate.

Didn't look deeply but at least position of the character and unexpected length could be stored.

To stabilize the crate more quickly it may be better to hide the error representation:

enum ErrorInner {
 // ...
}

pub struct Error {
    inner: ErrorInner,
    // maybe more stuff here
}

This structure is a POD pair of a HRP/data with some utility functions for choosing a checksum and stripping it. To be useful you need to be able to access the underlying data.

Probably same with CheckedHrpstring.

Consider adding buffered encoding as we do in hex-conservative

We need to update the fuzztests to use --no-default-features to avoid the broken arbitrary dependency of honggfuzz. Also we should run the fuzztests in CI so that we notice this.

Noticed while trying to test #43.

Found this while working on fuzz testing (#27 ).

Cause: we don't check for forbidden characters in the hrp when constructing a Bech32 struct.

Fuzz testing in rust-bitcoin found an error in rust-bech32 (rust-bitcoin/rust-bitcoin#100 ). This crate should have its own fuzz testing.

Link to the official BIP for Bech32, both in README and in doc strings

Hello, thanks for the recent updates that allowed us to start using your crate.

Is there a specific reason why there is no direct access to the Hrp::buf bytes through something like AsRef<[u8]> or a method?
Looks like our only solution at the moment is to use byte_iter and collect it when all we need is just a reference.
I'm happy doing a PR myself, let me know.

Thanks in advance!

#17 introduces the CheckBase32 trait which allows to check if all values of a [u8] are in the range 0..32 and returns the values of the slice as T: AsRef<[u5]>. Currently this can only be implemented for T=Vec<u5> because of uncertainties of the memory layout of u5 (see discussion). Hopefully rust-lang/rust#43036 will resolve this uncertainty.

If [u8] and [u5] would be guaranteed to have the same memory layout we could do a simple cast from &'f [u8] to &'f [u5] after checking that all values are in range and save an allocation.

Ok, that's interesting, we got a build error after merging #36 :

1. Nightly seems to have shifted under us (since #35 was last tested)
2. But this should have gotten caught by travis. Unfortunately my travis account got flagged for trying to spin up bitcoind for an integrationtest in the RPC repo 🤦‍♂️, so travis seems to block my PR build too and GitHub doesn't disable merging if the tests aren't being run at all.

In rust-lightning-invoice I pass around slices of the data part as &[u8], but each byte is guaranteed to be in the range 0..32. I'd like to encode this guarantee in the type by defining pub struct u5(u8).

The traits FromBase32 and IntoBase32 could be introduced and implemented for &[u8] and &[u5] respectively and replace convert_bits (From<&[u8]> for &[u5] and From<&[u5]> for &[u8] can't be implemented since type and trait are foreign and it would be quite ambiguous). Furthermore FromBase32 could be implemented for data structures that get parsed from bech32 data (like lightning invoice fields).

The best case UX would be that you can now implement FromBase32 for your types and don't have to worry about out-of-range errors anymore. The worst case UX would probably be that every use of an u5 (formerly u8) will now require an .into::<u8>().

What do you think about this proposal @clarkmoody and @apoelstra (I know I brought this up already in #5 but it got lost when we released the new version)?

You can see the documentation for 0.10 at docs.rs: https://docs.rs/bech32/0.10.0-alpha/bech32/index.html

A few comments:

• We have FromBase32, ToBase32, WriteBase32, WriteBase256, CheckBase32 traits. None of these are used anywhere. I think every one of these traits should be dropped.
• If you have an allocator, the encode and decode functions (and their variants) are available. We should draw attention to these in the main docs. If you want segwit, we should point the user to the segwit module.
• If you do not have an allocator, we currently have the messy arrayvec-based API with functions like decode_lowercase which really aren't useful in the absense of arrayvec.

I propose:

• We drop all the existing traits.
• We expose primitives::decode::CheckedHrpString at the top level for non-alloc decoding
• We expose a wrapper for the data.iter().copied().bytes_to_fes().with_checksum::<Bech32>(&hrp).chars() iterator chain, and maybe another for the with_witness_version variant, at the top level for non-alloc encoding. These wrappers should just take a byte slice. We can document that if you want to use an arbitrary iterator, the primitives::iter module will give you the pieces to do so.
• We rewrite the top-level docs to give examples of various things:
  • A normal Bitcoin user probably wants to use the segwit module; see examples there.
  • A non-Bitcoin user probably wants to use encode/decode functions (add example).
  • A non-Bitcoin no-alloc user probably wants to use the CheckedHrpString and non-alloc decoding method.
  • If you want to define your own checksum you should use the Checksum trait. We should provide an example, maybe of the BIP-93 checksum.
• We should also change all the re-exports to use #[doc(inline)] so that they appear as top-level structures rather than re-exports.

Decoding bc10rmfwl8nxdweeyc4sf89t0tn9fv9w6qpyzsnl2r4k48vjqh03qas9asdje0rlr0phru0wqw0p didn't fail.

 let res = bech32::decode("bc10rmfwl8nxdweeyc4sf89t0tn9fv9w6qpyzsnl2r4k48vjqh03qas9asdje0rlr0phru0wqw0p");

This returned:

Ok(("bc", [u5(15), u5(3), u5(27), u5(9), u5(14), u5(31), u5(7), u5(19), u5(6), u5(13), u5(14), u5(25), u5(25), u5(4), u5(24), u5(21), u5(16), u5(9), u5(7), u5(5), u5(11), u5(15), u5(11), u5(19), u5(5), u5(9), u5(12), u5(5), u5(14), u5(26), u5(0), u5(1), u5(4), u5(2), u5(16), u5(19), u5(31), u5(10), u5(3), u5(21), u5(22), u5(21), u5(7), u5(12), u5(18), u5(0), u5(23), u5(15), u5(17), u5(0), u5(29), u5(16), u5(5), u5(29), u5(16), u5(13), u5(18), u5(25), u5(15), u5(3), u5(31), u5(3), u5(15), u5(1), u5(23), u5(3), u5(28)], Bech32m))

I was expecting this to fail, as seen in:

• https://slowli.github.io/bech32-buffer/ and
• https://bitcoin.sipa.be/bech32/demo/demo.html

Right now if we have an invalid witness version we print the field element directly, which will print it in the bech32 alphabet. This is confusing because, for example, the number 17 will be printed as "3".

We should convert it to a number by adding to_u8 before printing.

Create example code documentation for Bech32.to_string and Bech32::from_string.

#4 implemented FromStr for Bech32. Since it's a API-breaking change I want to collect more API cleanup ideas that can be batched in one release.

• implement FromStr #4
• implement Display, which automatically implements ToString. This requires the members ob Bech32 to become private and a constructor that checks the validity of the data to encode (so to_string can never return Err(…)). #6
• implement convert_bits add-convert-bits
• final cleanup #10

The checksum algorithm has error detection that can identify which characters were actually wrong (with some probability). It'd be great to support this in the library.

we should change the checksum length check so that we enforce it only on the data part, not the HRP or separator. (It appears that for segwit, the BIP says the 90 limit applies to the whole string.)

ref: #142 (comment)

BIP 173 states:

Uppercase/lowercase

The lowercase form is used when determining a character's value for checksum purposes.

Encoders MUST always output an all lower-case Bech32 string.

bech32::encode verifies the provided HRP is internally consistent, but then ignores the returned case type and uses the HRP directly:

This causes two issues:

• The encoded string is in mixed-case, which is likely what the caller intended but is invalid per the spec. Either the HRP should be auto-lower-cased, or an error should be returned.
• The encoded string is invalid and cannot be decoded, because the decoder (correctly) lower-cases the HRP before verifying the checksum.