Use `bech32` to calculate descriptor checksums #608

tcharding · 2023-10-04T22:50:29Z

This is just the last patch, the rest are in #609 now.

The checksum::poly_mod function implements BCH codes to calculate a checksum (appended to descriptors). We recently released a version of BCH codes in bech32. We can implement the bech32::Checksum trait for BIP-380 and use the primitives::checksum module, removing the custom poly_mod function.

apoelstra · 2023-10-05T12:51:59Z

Up to 5018673 is great and could be PR'd separately if you want to move more quickly.

apoelstra · 2023-10-05T13:02:53Z

As for 9ca3380:

We should pull apart the Engine and replace the cls/clscount stuff with an iterator adaptor from chars to Fes.
Osd can be DescriptorChecksum or something. It doesn't have to be short; it shouldn't ever be used by users.
I believe your expects are all correct; you are construting a 3-digit base 3 number, which can be at most 222 (or 26 in decimal) which fits into a Fe32.
Your "comment why we do this" TODO is because you're manually computing a checksum and manually futzing with the target checksum. Just use Engine::input_target_residue and then you can drop the code that inputs 0s and basically all the rest of this function. Also you can just replace the whole checksum_chars function with the Checksummed iterator adaptor from rust-bech32.

tcharding · 2023-10-05T21:10:48Z

Sick! Thanks man.

tcharding · 2023-10-05T21:15:38Z

I learned a new expression 'futzing with', not the first one I've learned from you @apoelstra, I love it.

apoelstra · 2023-10-06T14:09:54Z

src/descriptor/bare.rs

+            for ch in Checksummed::new_unchecked(&s) {
+                f.write_char(ch)?;
+            }
+        }


In 22ee854:

We definitely want to keep the wrapped_f stuff. This new code is both longer and now allocates.

Yeah I balked at this before I pushed it. Will add it back in.

apoelstra · 2023-10-06T14:12:57Z

src/descriptor/checksum.rs

+    ///
+    /// [BIP-380]: <https://github.com/bitcoin/bips/blob/master/bip-0380.mediawiki>
+    #[inline]
+    pub fn new(descriptor: &'a str) -> Result<Checksummed<'a>, Error> {


In 22ee854:

Rather than taking a string this should take a D: Display and a fmt::Formatter.

apoelstra · 2023-10-06T14:17:19Z

src/descriptor/checksum.rs

+        let checksummed = Checksummed::new(desc_str)?;
+        if !checksummed.eq(s.chars()) {
+            return Err(Error::BadDescriptor("invalid checksum".to_owned()));
+        }


In 22ee854:

~~It would be faster to feed the checksum into the checksum iterator and compare the residue against the target. Then in the happy case we only need to iterate once.~~ Actually, the speedup here would be minimal, since you are only checksumming once, and s.chars() is extremely fast to iterate on.

In the unhappy case, we should probably return a better error variant that has the expected and received checksum in it. It's fine if this is slow and allocating.

Sure thing.

tcharding · 2023-10-09T03:10:38Z

Alright, I think I got a little trigger happy before to use my new found iterator skills from the recent bech32 work. Instead this PR now just does the minimum required to remove the poly_mod function and use bech32::primitives::checksum instead.

tcharding · 2023-10-09T03:12:21Z

I was futzing with the whole descriptor module before ;)

apoelstra · 2023-10-09T13:59:37Z

Ok, ACK this. It's a good start.

5018673 Remove magic number (Tobin C. Harding) 535cd17 Improve docs in the checksum module (Tobin C. Harding) fec700a Add bip 380 checksum test vectors (Tobin C. Harding) 73f4892 Add output descriptor bip referenece (Tobin C. Harding) Pull request description: The first 4 patches from #608, no changes. Clean up and add some test vector unit tests. ACKs for top commit: apoelstra: ACK 5018673 Tree-SHA512: f50f64bf9a1fc28eebca809379e02580cab96e7e41228aab6045441eb71702bef1b1979e497a6dcb1e1bce082965e5c93e78dba6e8fbd78c7a0ae2e3c8035660

tcharding · 2023-10-09T18:10:28Z

Rebased to pick up changes in #609 and use newly release bech32 v0.10.0-beta, no other changes.

apoelstra

ACK c5c08fe

apoelstra · 2023-10-09T19:09:19Z

@sanket1729 I'll let you take a look at this before merging. It basically gets rid of some of the magic constants and uses rust-bech32 facilities to do the checksum. But this doesn't represent a "real" use of the new rust-bech32 API, which would pretty-much reduce the checksum.rs module to just encoding/decoding strings as u5s and then handing off to rust-bech32 APIs.

tcharding · 2023-10-09T19:45:36Z

I was going to attack it in steps:

Move all the Formatter stuff in descriptor/*.rs to use a macro write_desciptor!.
Then in the macro work out how to convert the input string to a stream of Fe32s like you suggest.

The reason to do two steps is that I don't think we can have a general "input multiple strings" to the checksum engine because cls/clscount is state that has to be kept between successive input strings. But i noticed that we never do more than input a single descriptor.

apoelstra · 2023-10-09T21:06:19Z

I don't like the idea of moving so much logic into a macro.

If we need to retain state then we can keep the existing Engine structure with cls/clscount. If we don't, we should change this to be an iterator adaptor.

But either way, once we have a stream of Fe32s, we should be able to just put them into a rust-bech32 checksum::Engine and everything should be easy.

tcharding · 2023-10-09T21:34:46Z

I don't like the idea of moving so much logic into a macro.

The logic is just the more or less duplicate lines

        use fmt::Write;
        let mut wrapped_f = checksum::Formatter::new(f);
        write!(wrapped_f, "{}", self.ms)?;
        wrapped_f.write_checksum_if_not_alt()

Other descriptors have a match statement when calling write but they are essentially the same. Its totally just an abstraction thing, as it is it appears sane to call write multiple times, and the current implementation supports this. But its an unnecessary generalisation I believe because we only ever call write once, to write a single descriptor. I want to replace those 4 lines with write_descriptor!(f, "{}", self.ms)?;

If we need to retain state then we can keep the existing Engine structure with cls/clscount. If we don't, we should change this to be an iterator adaptor.

But either way, once we have a stream of Fe32s, we should be able to just put them into a rust-bech32 checksum::Engine and everything should be easy.

Another complication is we cannot use bech32 iterators to encode because of the # symbol. That is what led me to create the Checksummed type. Anyways, I'll have another play with it.

apoelstra · 2023-10-09T21:45:22Z

The logic is just the more or less duplicate lines

Oh, yeah, this seems fine to me.

Another complication is we cannot use bech32 iterators to encode because of the # symbol.

Yeah, that sucks. But I think your solution is good.

tcharding · 2023-10-10T00:02:02Z

I had another look at this, I'm not sure we can do much better even using some sort of iterator because of how we use checksum::Formatter. I'm pretty sure that our usage of fmt::Write::write_str is doing an allocation under the hood. If I'm right then changing our usage is then an optimization problem not a "use bech32" problem.

tcharding · 2023-10-10T00:27:13Z

I think this could be considered for merge as is, I wrote up #610 to discuss potential improvements.

apoelstra · 2023-10-10T00:49:19Z

I'm not sure we can do much better even using some sort of iterator

Yeah, you may be right.

apoelstra

ACK c44ce81

sanket1729

utACK c44ce81. Did not review the use of bech32 carefully. Any breakage there should have caused existing test cases to fail.

sanket1729 · 2023-10-11T20:56:48Z

We can also safely merge this despite the red crosses as CI is fixed in master.

The `checksum::poly_mod` function implements BCH codes to calculate a checksum (appended to descriptors). We recently released a version of BCH codes in `bech32`. We can implement the `bech32::Checksum` trait for BIP-380 and use the `primitives::checksum` module, removing the custom `poly_mod` function.

Currently we have a bunch of places in the code where we create a checksum formatter wrapper then write a descriptor to it then write the checksum. This can all be wrapped up in a macro, has to be a macro because the number of args is variable. This reduces the line count with no real loss of clarity `write_descriptor!` is pretty self explanatory.

tcharding · 2023-10-11T22:52:24Z

You were correct @sanket1729, I rebased it for good measure, no changes.

apoelstra

ACK fe1a6be

tcharding force-pushed the 10-04-use-new-bech32 branch from 7865784 to 9ca3380 Compare October 4, 2023 22:52

tcharding mentioned this pull request Oct 5, 2023

Do some checksum module improvements #609

Merged

tcharding force-pushed the 10-04-use-new-bech32 branch from 9ca3380 to 22ee854 Compare October 6, 2023 01:36

apoelstra reviewed Oct 6, 2023

View reviewed changes

tcharding force-pushed the 10-04-use-new-bech32 branch from 22ee854 to 7335471 Compare October 9, 2023 03:09

tcharding force-pushed the 10-04-use-new-bech32 branch 2 times, most recently from 045d645 to ad7d5dc Compare October 9, 2023 03:24

tcharding force-pushed the 10-04-use-new-bech32 branch from ad7d5dc to c5c08fe Compare October 9, 2023 18:09

tcharding marked this pull request as ready for review October 9, 2023 18:10

apoelstra previously approved these changes Oct 9, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via c44ce81 October 9, 2023 23:58

tcharding mentioned this pull request Oct 10, 2023

checksum module needs some love #610

Open

apoelstra approved these changes Oct 10, 2023

View reviewed changes

sanket1729 approved these changes Oct 11, 2023

View reviewed changes

tcharding added 2 commits October 12, 2023 08:25

tcharding force-pushed the 10-04-use-new-bech32 branch from c44ce81 to fe1a6be Compare October 11, 2023 21:25

apoelstra approved these changes Oct 11, 2023

View reviewed changes

apoelstra merged commit 29b612c into rust-bitcoin:master Oct 12, 2023
16 checks passed

tcharding deleted the 10-04-use-new-bech32 branch October 12, 2023 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `bech32` to calculate descriptor checksums #608

Use `bech32` to calculate descriptor checksums #608

tcharding commented Oct 4, 2023 •

edited

Loading

apoelstra commented Oct 5, 2023

apoelstra commented Oct 5, 2023

tcharding commented Oct 5, 2023

tcharding commented Oct 5, 2023

apoelstra Oct 6, 2023

tcharding Oct 6, 2023

apoelstra Oct 6, 2023

apoelstra Oct 6, 2023 •

edited

Loading

tcharding Oct 6, 2023

tcharding commented Oct 9, 2023

tcharding commented Oct 9, 2023

apoelstra commented Oct 9, 2023

tcharding commented Oct 9, 2023

apoelstra left a comment

apoelstra commented Oct 9, 2023

tcharding commented Oct 9, 2023

apoelstra commented Oct 9, 2023

tcharding commented Oct 9, 2023 •

edited

Loading

apoelstra commented Oct 9, 2023

tcharding commented Oct 10, 2023 •

edited

Loading

tcharding commented Oct 10, 2023 •

edited

Loading

apoelstra commented Oct 10, 2023

apoelstra left a comment

sanket1729 left a comment

sanket1729 commented Oct 11, 2023

tcharding commented Oct 11, 2023 •

edited

Loading

apoelstra left a comment

Use bech32 to calculate descriptor checksums #608

Use bech32 to calculate descriptor checksums #608

Conversation

tcharding commented Oct 4, 2023 • edited Loading

apoelstra commented Oct 5, 2023

apoelstra commented Oct 5, 2023

tcharding commented Oct 5, 2023

tcharding commented Oct 5, 2023

apoelstra Oct 6, 2023

Choose a reason for hiding this comment

tcharding Oct 6, 2023

Choose a reason for hiding this comment

apoelstra Oct 6, 2023

Choose a reason for hiding this comment

apoelstra Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

tcharding Oct 6, 2023

Choose a reason for hiding this comment

tcharding commented Oct 9, 2023

tcharding commented Oct 9, 2023

apoelstra commented Oct 9, 2023

tcharding commented Oct 9, 2023

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Oct 9, 2023

tcharding commented Oct 9, 2023

apoelstra commented Oct 9, 2023

tcharding commented Oct 9, 2023 • edited Loading

apoelstra commented Oct 9, 2023

tcharding commented Oct 10, 2023 • edited Loading

tcharding commented Oct 10, 2023 • edited Loading

apoelstra commented Oct 10, 2023

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

sanket1729 commented Oct 11, 2023

tcharding commented Oct 11, 2023 • edited Loading

apoelstra left a comment

Choose a reason for hiding this comment

Use `bech32` to calculate descriptor checksums #608

Use `bech32` to calculate descriptor checksums #608

tcharding commented Oct 4, 2023 •

edited

Loading

apoelstra Oct 6, 2023 •

edited

Loading

tcharding commented Oct 9, 2023 •

edited

Loading

tcharding commented Oct 10, 2023 •

edited

Loading

tcharding commented Oct 10, 2023 •

edited

Loading

tcharding commented Oct 11, 2023 •

edited

Loading