Support bech32 encoding without a checksum #66

jkczyz · 2022-06-24T04:39:07Z

BOLT 12 Offers uses bech32 encoding without a checksum since QR codes already have a checksum. Add functions encode_without_checksum and decode_without_checksum to support this use case.

Also, remove overall length check in decode since it is unnecessary.

TheBlueMatt · 2022-06-27T20:29:40Z

src/lib.rs

@@ -134,6 +134,7 @@ pub struct Bech32Writer<'a> {
    formatter: &'a mut fmt::Write,
    chk: u32,
    variant: Variant,
+    checksum: Checksum,


I don't really understand the point of a struct that appears ~entirely dedicated to tracking the checksum having a Checksum::Disabled flag? Wouldn't it make more sense to use WriteBase32 directly or with a thinner wrapper?

Made a similar wrapper for without a checksum, though there's a small amount of code duplication that way.

jkczyz · 2022-08-04T15:14:47Z

@tcharding @apoelstra Any chance either of you could be a second reviewer on this?

tcharding · 2022-08-04T23:04:14Z

Sure thing. Can you squash the first two commits please because the second one is patching the changes in the first. Not super important but I'd put patch 3 at the front since it is a trivial improvement. Thanks.

tcharding · 2022-08-05T00:06:04Z

Hi @jkczyz, thanks for your contribution. This PR does the job but its not pretty. How urgent is this? Is it something you need to use today or is it just something you did for fun?

The reason its not pretty is that it adds functionality that is obviously just plastered on top. By that I mean the whole Bech32Writer design is there to track and add the checksum so then adding another type that mirrors it but does not add a checksum is kinda strange. I'm basically making the same observation @TheBlueMatt did above but on this iteration of the PR. If we want a good solution I think we need to dig a bit deeper.

jkczyz · 2022-08-05T18:39:59Z

Hi @jkczyz, thanks for your contribution. This PR does the job but its not pretty. How urgent is this? Is it something you need to use today or is it just something you did for fun?

Somewhat urgent as it will hold up our work on Offers (lightningdevkit/rust-lightning#1597). I have a draft that uses this through rust-bitcoin's re-export, so would need a new release for both.

The reason its not pretty is that it adds functionality that is obviously just plastered on top. By that I mean the whole Bech32Writer design is there to track and add the checksum so then adding another type that mirrors it but does not add a checksum is kinda strange. I'm basically making the same observation @TheBlueMatt did above but on this iteration of the PR. If we want a good solution I think we need to dig a bit deeper.

I don't actually need to use Bech32WithoutChecksumWriter directly, so I'm happy to inline the code. I mainly included for consistency and for code reuse, which is no longer the case it seems.

apoelstra · 2022-08-05T18:54:27Z

As a stop-gap you could just drop the last 6 characters/u8s from a normal bech32 encoding.

jkczyz · 2022-08-05T19:48:46Z

Sure thing. Can you squash the first two commits please because the second one is patching the changes in the first. Not super important but I'd put patch 3 at the front since it is a trivial improvement. Thanks.

All done.

jkczyz · 2022-08-05T19:50:06Z

As a stop-gap you could just drop the last 6 characters/u8s from a normal bech32 encoding.

For encoding that would work, but we would still fail to decode a bech32 offer since it doesn't have a checksum.

jkczyz · 2022-08-05T19:52:45Z

src/lib.rs

+        }
+        Ok(())
+    };
+    Ok(write())


I'm a little uncertain about this. Seems to maintain the same behavior, but it is odd that an fmt::Error is always wrapped with Ok.

Yeah, the existing API is really weird.

Why are you constructing a closure and then immediately calling it?

Mainly as a simple way to maintain the API behavior. i.e., wrapping a Err(fmt::Error) returned in each of the three places using ? with an Ok. Without the closure, it would be:

if let Err(e) = fmt.write_str(&hrp) { return Ok(Err(e)); } if let Err(e) = fmt.write_char(SEP) { return Ok(Err(e)); } for b in data.as_ref() { if let Err(e) = fmt.write_char(b.to_char()) { return Ok(Err(e)); } } Ok(Ok(()))

Happy to update the error type is your prefer. It probably should be an enum that is either bech32::Error or fmt::Error. Or maybe adding a variant to bech32::Error for fmt::Error.

This whole Result<Result> thing is weird to me -- I think we should add fmt::Error to the main Error enum and then flatten this return type.

I wouldn't worry about maintaining the API here. We want to overhaul this API anyway.

Flatted the Result. Let me know if you prefer a different name for the variant or if you want to drop the wrapped fmt::Error as it currently doesn't provide any additional information.

This simplifies the return type of encode_to_fmt, which used a nested Result type.

apoelstra · 2022-08-07T14:41:52Z

Loosk good to me! Though I guess CI wants you to add a rustfmt commit.

@Kixunil mind taking another look at this? I think now my remaining unease with the API is just my existing unease at the API, and if we want to clean it up we should just go over the whole library.

ACK 3e95975

apoelstra

ACK 573d7f4

Kixunil · 2022-08-07T21:42:49Z

Can't right now, give me a day or so.

tcharding

Thanks for sticking with this! Functionality looks correct to me. Perhaps add the unit test I used if you like it. I'm not super happy with the function naming but perhaps we can leave tat till another day, like others have said already this crate is about to get an overhaul anyways. If this solves your usecase I'm happy to ack it.

src/lib.rs

tcharding · 2022-08-08T00:13:00Z

tACK 573d7f4

jkczyz · 2022-08-08T18:58:22Z

Addressed all feedback. Can squash fixups whenever you're ready.

apoelstra

ACK f768f03

apoelstra · 2022-08-08T19:00:06Z

Looks good to me. Yes, would prefer squashing all the fixups.

BOLT 12 Offers uses bech32 encoding without a checksum since QR codes already have a checksum. Add functions encode_without_checksum and decode_without_checksum to support this use case. Also, remove overall length check in decode since it is unnecessary.

apoelstra

ACK 86edca9

Kixunil

Finally had the time to review it. The API has problems but it was the case before, so not a problem with this particular PR. It just needs to be cleaned up before 1.0.

Kixunil · 2022-08-09T09:27:05Z

src/lib.rs

@@ -213,7 +216,7 @@ impl<'a> WriteBase32 for Bech32Writer<'a> {

 impl<'a> Drop for Bech32Writer<'a> {
    fn drop(&mut self) {
-        self.inner_finalize()
+        self.write_checksum()
            .expect("Unhandled error writing the checksum on drop.")


FTR std ignores errors instead of panicking in drop and it is considered the right thing to do because panicking in drop is a footgun.

Lol, oops! I tried to look for double-panics but got a bit confused and somehow missed this completely obvious one.

But I think we should remove this Drop business entirely so it's fine for now. I've never heard of a wrapper struct that takes a &mut ref and then has finalization logic on Drop...I suspect this was an API experiment that never failed enough to be removed.

IDK, it's not too different from BufWriter behavior. But I guess compiler warning would be better than drop. I was thinking of abusing must_use but then we couldn't implement any methods taking &mut self and it'd be fragile anyway.

Kixunil · 2022-08-09T09:33:27Z

src/lib.rs

+    let hrp = match check_hrp(hrp)? {
+        Case::Upper => Cow::Owned(hrp.to_lowercase()),
+        Case::Lower | Case::None => Cow::Borrowed(hrp),
+    };


It is possible to o this without Cow which should be more efficient:

let hrp_owned; // yes, this is allowed in Rust! let hrp = match check_hrp(hrp)? { Case::Upper => { hrp_owned = hrp.to_lowercase(); hrp_owned.as_str() }, Case::Lower | Case::None => hrp, };

That's a neat lifetime-organizing trick!

Kixunil · 2022-08-09T09:35:12Z

src/lib.rs

+/// * No length limits are enforced for the data part
+pub fn encode_without_checksum_to_fmt<T: AsRef<[u5]>>(
+    fmt: &mut fmt::Write,
+    hrp: &str,


It'd be better to have Hrp newtype to make the reurn type fmt::Result allowing use in Display implementations.

Kixunil · 2022-08-09T09:51:02Z

src/lib.rs

@@ -622,6 +668,14 @@ pub enum Error {
    InvalidPadding,
    /// The whole string must be of one case
    MixedCase,
+    /// Writing UTF-8 data failed
+    WriteFailure(fmt::Error),


I don't think this is the right way of using fmt::Error. It's just a marker that something failed and you're supposed to retrieve the error fro underlying stream. In case of std::io it's done for you and in case of String there's no error so unwrapping is the correct thing to do.

This library abuses the fmt::Write API to signal display failure but Display must not fail for any other reason than formatter failing.

From the docs:

Formatting implementations should ensure that they propagate errors from the Formatter (e.g., when calling write!). However, they should never return errors spuriously. That is, a formatting implementation must and may only return an error if the passed-in Formatter returns an error. This is because, contrary to what the function signature might suggest, string formatting is an infallible operation. This function only returns a result because writing to the underlying stream might fail and it must provide a way to propagate the fact that an error has occurred back up the stack.

Good find! This will definitely inform our API overhaul.

jkczyz · 2022-08-11T19:20:32Z

As a stop-gap you could just drop the last 6 characters/u8s from a normal bech32 encoding.

For encoding that would work, but we would still fail to decode a bech32 offer since it doesn't have a checksum.

For sake of planning, what timeframe should I expect for seeing this in a release? Do let me know if I can help in review if other PRs need to be merged first. Just want to set appropriate expectations given this will need to get into a rust-bitcoin release, too, before we can use it in rust-lightning.

tcharding · 2022-08-11T20:18:30Z

We just dropped a rust-bitcoin release so the next one could be six months away. This poses an interesting dilemma, we don't want to be doing major releases of rust-bitcoin ever week but if a user is waiting on a merged feature in a dependency that we control it seems rude to hold it up too long.

If we are going to release then #68 and #72 would likely be nice to consider first.

cc @apoelstra

apoelstra · 2022-08-11T20:37:32Z

We can do a much faster release of this than six months. It's still fine if rust-bitcoin depends on an old version -- bech32 is pretty self-contained so it should be easy to work with multiple versions of it at once.

apoelstra · 2022-08-11T20:37:56Z

But having said that yes, I'd like to do a bunch more work on this crate before releasing .. so it could be a month or more.

jkczyz · 2022-08-11T21:29:12Z

Gotcha. I think had trouble with multiple versions but can't recall the exact error. Will try again once there's a new bech32 release. Otherwise, I can unblock the current work by holding off on offer parsing. I made a builder framework in the interim, so we can move forward with dependent work using that.

Kixunil · 2022-08-11T21:48:34Z

Could the code be changed to be non-breaking? Minor version bump wouldn't be so painful and we could do API overhaul later.

jkczyz · 2022-08-12T01:17:06Z

Could the code be changed to be non-breaking? Minor version bump wouldn't be so painful and we could do API overhaul later.

Are you suggesting doing that in rust-bech32 and do a similar minor version bump in rust-bitcoin?

Kixunil · 2022-08-12T08:22:38Z

We wouldn't need to bump anything in bitcoin unless we started using the newly added feature which I don't think is planned.

Edit: and yes, release minor version of bech32, of course.

jkczyz · 2022-08-12T13:10:26Z

Ah, so since rust-bitcoin specifies version = "0.9.0" for bech32, any version >=0.9.0, <0.10.0 cab be used. Sure, I can put together a PR.

jkczyz · 2022-08-12T13:37:24Z

Opened #77.

TheBlueMatt reviewed Jun 27, 2022

View reviewed changes

jkczyz force-pushed the 2022-06-without-checksum branch from e0494d7 to 2cfe657 Compare June 28, 2022 14:04

jkczyz mentioned this pull request Jul 6, 2022

BOLT 12 offer encoding lightningdevkit/rust-lightning#1597

Closed

8 tasks

Use a constant for checksum length

1735cb3

jkczyz force-pushed the 2022-06-without-checksum branch from 2cfe657 to 665fac0 Compare August 5, 2022 19:47

jkczyz commented Aug 5, 2022

View reviewed changes

jkczyz force-pushed the 2022-06-without-checksum branch from 665fac0 to 9dc60b4 Compare August 6, 2022 23:08

Add an Error variant for fmt::Error

0ef5bb9

This simplifies the return type of encode_to_fmt, which used a nested Result type.

jkczyz force-pushed the 2022-06-without-checksum branch from 9dc60b4 to 3e95975 Compare August 6, 2022 23:13

apoelstra approved these changes Aug 7, 2022

View reviewed changes

tcharding approved these changes Aug 8, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

apoelstra approved these changes Aug 8, 2022

View reviewed changes

jkczyz force-pushed the 2022-06-without-checksum branch from f768f03 to 86edca9 Compare August 8, 2022 19:00

apoelstra approved these changes Aug 8, 2022

View reviewed changes

apoelstra merged commit 6df4a67 into rust-bitcoin:master Aug 9, 2022

Kixunil reviewed Aug 9, 2022

View reviewed changes

Support bech32 encoding without a checksum #66

Support bech32 encoding without a checksum #66

Conversation

jkczyz commented Jun 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Aug 4, 2022

tcharding commented Aug 4, 2022

tcharding commented Aug 5, 2022

jkczyz commented Aug 5, 2022

apoelstra commented Aug 5, 2022

jkczyz commented Aug 5, 2022

jkczyz commented Aug 5, 2022

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 Aug 7, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil commented Aug 7, 2022

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Aug 8, 2022

jkczyz commented Aug 8, 2022

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Aug 8, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil 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

jkczyz commented Aug 11, 2022

tcharding commented Aug 11, 2022 • edited Loading

apoelstra commented Aug 11, 2022

apoelstra commented Aug 11, 2022

jkczyz commented Aug 11, 2022

Kixunil commented Aug 11, 2022

jkczyz commented Aug 12, 2022

Kixunil commented Aug 12, 2022 • edited Loading

jkczyz commented Aug 12, 2022

jkczyz commented Aug 12, 2022

tcharding commented Aug 11, 2022 •

edited

Loading

Kixunil commented Aug 12, 2022 •

edited

Loading