Rewrite bech32 crate #102

tcharding · 2023-04-23T23:25:29Z

Draft because done on top of #101, also because @apoelstra suggested doing this in smaller parts but I think it might be easier to just pull the trigger and do it all at once. @clarkmoody please say if you find this approach too much and would prefer to see the PR broken up into multiple PRs.

Why the rewrite?

It was observed that the writer API could be generalized to use an iterator API.
It was observed that the checksum logic is an implementation of BCH codes, this is the only cryptography part of the crate, as such it is complex, isolating it helps reduce complexity of the code for non-cryptographers.
~~This is a bitcoin crate but currently it supports arbitrary HRP prefixes, by making the top level API more bitcoin specific we can simplify the API.~~

Aims

Make a segwit API that is ergonomic to use for Bitcoin users and that implements the specification laid out in BIP-173 And BIP-350
Isolate the cryptography to a submodule
Create a stable API that we can commit to maintaining

For example usage in rust-bitcoin see: rust-bitcoin/rust-bitcoin#1809

This requires dropping `Ord` impls from the error types, but IMHO these didn't really make sense anyway. This also keeps the `u5` name. We will rename this in a later PR which replaces the *Base32 traits with iterator adaptors, since at that point there will be a much smaller API surface to change.

tcharding · 2023-04-23T23:41:08Z

src/lib.rs

+/// The codex32 checksum algorithm, defined in BIP-93.
+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+enum Codex32 {}


Do we want to keep this in?

No, we'll drop it from the final PR.

tcharding · 2023-04-24T00:01:05Z

Notes:

The fuzzing needs fixing
I've managed to loose the benchmark code you wrote @apoelstra

apoelstra · 2023-04-24T01:23:07Z

src/primitives/hrpstring.rs

+    }
+
+    #[test]
+    #[allow(unused_variables)] // Triggered by matches macro.


This is triggered because the matches macro is used incorrectly.

Removed, and fixed tests to fully implement the test vectors from both bips.

apoelstra · 2023-04-24T01:23:22Z

src/primitives/gf1024.rs

+// Written by Clark Moody and the rust-bitcoin developers.
+// SPDX-License-Identifier: MIT
+
+//! GF1024 - Galois Field over 1024 elements.


I'm unclear on the purpose of including this in this PR.

Will drop, thanks.

apoelstra · 2023-04-24T01:25:14Z

src/lib.rs

+
+/// True if program length is either 20 bytes (p2wpkh) or 32 bytes (p2wsh).
+/// ref: [BIP-141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#user-content-Witness_program)
+#[cfg(feature = "alloc")]


what part of this function requires an allocator?

This is to prevent clippy warning when "alloc" is not enabled, I added a comment to it to say as such.

tcharding · 2023-04-26T04:02:52Z

Is it even worth fuzzing this crate? We accept random data and convert it to a string (filters out invalid utf-8) and then decode (filters out invalid bech32 characters). Won't the number of times this actually succeeds be so small that running the fuzzing is essentially doing nothing? Or is the fuzzing meaningfully helping to prove 1. below?

Am I correct in thinking we want to test that:

arbitrary &str doesn't cause us to panic
all valid bech32 strings are correctly parsed and roundtripped

apoelstra · 2023-04-26T13:34:48Z

You can test whether it's able to produce valid checksums. It would surprise me if it can't, given how simple the algorithm is and the fact that it only requires modiying the last 6 characters of the string (and modifying them independently, by simply xoring them with the correct value).

But regardless, it is valuable to fuzz that we don't panic.

clarkmoody · 2023-04-26T22:53:25Z

src/network.rs

+    /// ```
+    ///
+    /// [segwit address format]: <https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#user-content-Segwit_address_format>
+    pub const fn to_hrp(&self) -> &'static str {


Why not return an instance of the human-readable part type instead of a string slice?

I'm having a play with introducing the Hrp type as you suggested previously, cheers.

Aight, I hacked up a new version of this PR that adds an Hrp type, and also a KnownHrp type for segwit specific code. Could be mistakes because it was a fair bit of work and I should really go over it on another day before pushing but I'm pretty psyched for this so if you'll excuse me I'll push right away.

clarkmoody · 2023-04-27T02:48:14Z

src/primitives/hrpstring.rs

+///
+/// Parsing as an HRP string does not validate the checksum in any way.
+#[derive(Debug)]
+pub struct Parsed<'s> {


The names here don't make a ton of sense. The module is called hrpstring, but the primary data structure is Parsed and contains not only HRP data but also witness version and the data part.

The data structure is an HRP string which has been parsed into an HRP, witness version, and data part.

We should change this if it's not understandable but it seemed pretty straightforward to me.

tcharding · 2023-04-27T04:57:15Z

The major change here is that the PR no longer removes the current API. Instead it adds a segwit module that implements bip-173 and 350 using the new primitives stuff. Also:

Adds Hrp type
Adds KnownHrp type
Uses new proposed rust-bitcoin-primitives crate to get the WitnessVersion

Thanks for your patience @clarkmoody and keeping on prodding me about the Hrp type, this worked out really nicely.

tcharding · 2023-07-09T23:15:52Z

Closing since this is stale.

tcharding changed the title ~~04 24 rewrite~~ Rewrite bech32 crate Apr 23, 2023

tcharding mentioned this pull request Apr 23, 2023

WIP: Use new bech32 crate rewrite rust-bitcoin/rust-bitcoin#1809

Closed

tcharding force-pushed the 04-24-rewrite branch 2 times, most recently from b77ef94 to 13827d3 Compare April 23, 2023 23:37

tcharding commented Apr 23, 2023

View reviewed changes

tcharding force-pushed the 04-24-rewrite branch from 13827d3 to 61437ed Compare April 23, 2023 23:42

apoelstra reviewed Apr 24, 2023

View reviewed changes

tcharding force-pushed the 04-24-rewrite branch 2 times, most recently from a07a1b1 to 035f026 Compare April 26, 2023 03:55

clarkmoody reviewed Apr 26, 2023

View reviewed changes

clarkmoody reviewed Apr 27, 2023

View reviewed changes

tcharding force-pushed the 04-24-rewrite branch 2 times, most recently from 67c92ec to 4b841ad Compare April 27, 2023 04:55

tcharding force-pushed the 04-24-rewrite branch 4 times, most recently from c518a2c to 33cd60a Compare April 27, 2023 07:05

Add segwit API

1f9f8b5

tcharding force-pushed the 04-24-rewrite branch from 33cd60a to 1f9f8b5 Compare April 27, 2023 23:32

tcharding closed this Jul 9, 2023

tcharding deleted the 04-24-rewrite branch September 18, 2023 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite bech32 crate #102

Rewrite bech32 crate #102

tcharding commented Apr 23, 2023 •

edited

Loading

tcharding Apr 23, 2023

apoelstra Apr 26, 2023

tcharding commented Apr 24, 2023

apoelstra Apr 24, 2023

tcharding Apr 26, 2023

apoelstra Apr 24, 2023

tcharding Apr 26, 2023

apoelstra Apr 24, 2023

tcharding Apr 26, 2023

tcharding commented Apr 26, 2023

apoelstra commented Apr 26, 2023

clarkmoody Apr 26, 2023

tcharding Apr 27, 2023

tcharding Apr 27, 2023

clarkmoody Apr 27, 2023

apoelstra Apr 27, 2023

tcharding commented Apr 27, 2023

tcharding commented Jul 9, 2023

Rewrite bech32 crate #102

Rewrite bech32 crate #102

Conversation

tcharding commented Apr 23, 2023 • edited Loading

Why the rewrite?

Aims

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Apr 24, 2023

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

tcharding commented Apr 26, 2023

apoelstra commented Apr 26, 2023

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

tcharding commented Apr 27, 2023

tcharding commented Jul 9, 2023

tcharding commented Apr 23, 2023 •

edited

Loading