Enforce maximum length #142

tcharding · 2023-10-17T03:52:33Z

Enforce maximum string length when encoding.

apoelstra · 2023-10-17T13:53:20Z

The maximum length needs to be a parameter of the checksum. 93 is only for bech32.

And IMO we should only enforce the maximum length of 93 for segwit ... for the non-segwit API we should use 1023 since that's the actual length of the code.

tcharding · 2023-10-17T20:26:16Z

Patch separation for the win! I had a feeling there was going to be something to this that I was missing :) Thanks, will re-spin.

tcharding · 2023-10-18T01:32:23Z

The maximum length needs to be a parameter of the checksum. 93 is only for bech32.

And IMO we should only enforce the maximum length of 93 for segwit ... for the non-segwit API we should use 1023 since that's the actual length of the code.

Why do you write 93 here? In BIP-173 it says 90, or am I missing something?

A Bech32[2] string is at most 90 characters long and consists of:

https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki

apoelstra · 2023-10-18T17:20:12Z

Sorry, I was confusing 93 (which is 1023 / 11, a fact which is relevant when searching for BCH codes) with 90 (an empirically derived limit in BIP 173).

tcharding · 2023-10-18T21:22:44Z

Deleted that long message to save anyone having to read it. I found my problem

MAY exceed the 90-character limit specified in BIP-0173.

ref: https://github.com/lightning/bolts/blob/master/11-payment-encoding.md

clarkmoody · 2023-10-18T22:42:40Z

src/segwit.rs

@@ -364,6 +378,8 @@ pub enum EncodeError {
    WitnessVersion(InvalidWitnessVersionError),
    /// Invalid witness length.
    WitnessLength(WitnessLengthError),
+    /// Encoding HRP, witver, and program into a bech32 string exceeds limit of 90 characters.
+    TooLong(EncodedLengthError),


Why not EncodedLengh here?

You were reviewing while I coded so this comment is stale now since I removed the error type all together. FTR the policy we have come up with is to always prefix error types with Error.

Thanks for the review, please see the new version for better handling of the maximum string length.

tcharding · 2023-10-18T23:54:41Z

Converted to draft while I push up the changes in the first three patches.

tcharding · 2023-10-22T17:14:20Z

Rebased. Also I pulled a random column width fix out into its own patch because I just mentioned it to @realeinherjar in another PR/repo :)

apoelstra · 2023-10-22T19:27:53Z

OK, I think this is fine. TBH I think we should just enforce the 1023-character maximum and say "screw LN" because if they're actually using an error-correcting code with no length limit then the resulting code is unable to correct even a single error and cannot detect more than one.

apoelstra

ACK 65790bf

tcharding · 2023-10-22T19:46:19Z

I can do that, can you point me to some docs so I can (a) understand the 1023 limit and (b) reference it in the code?

tcharding · 2023-10-22T19:48:08Z

A real limit is bettor for #138 too, saves looping.

apoelstra · 2023-10-22T20:35:55Z

I don't know where it's documented. But it's a fact of the bech32 and bech32m checksums that they have length 1023. sipa's find_bch.py can be used to regenerate the code and it outputs some extra data.

codex32 has length 93; the "long codex32" checksum has length 1023.

I'm not sure about the descriptor checksum, I'll have to check.

tcharding · 2023-10-23T21:57:45Z

I don't know where it's documented.

I found this at the end of bip-173 (in the "Checksum design" section):

Even though the chosen code performs reasonably well up to 1023 characters, other designs are preferable for lengths above 89 characters (excluding the separator).

I'll add that as a reference for the numbers.

apoelstra · 2023-10-24T01:19:12Z

We can't unconditionally limit strings to 1023. The constant needs to be a property of the checksum.

tcharding · 2023-10-24T03:16:10Z

Here is your previous comment:

And IMO we should only enforce the maximum length of 93 for segwit ... for the non-segwit API we should use 1023 since that's the actual length of the code.

I went searching for what "since that's the actual length of the code" meant, and all I could find was the docs in bip-173 "Checksum design" section. I went through a bunch of wiki pages on codes and couldn't find any mention of an inherent "length". Its just to do with how many errors can be detected with a certain length checksum for a certain amount of input data, right?

apoelstra · 2023-10-24T13:44:06Z

https://en.wikipedia.org/wiki/BCH_code#General_BCH_codes defines length, though pretty obtusely. Here q is 32 for every code this library supports, m is always 2 (though this only affects error correction so maybe some users could define checksums with other values..). So q^m - 1 is 1023, and the length of a BCH code, being the order of an element in a multiplicative group of size 1023, must be 1023 or one of its factors.

A more useful definition is probably the one in https://secretcodex32.com/docs/2023-08-23--math.pdf on page 18:

The length of the code is how long a coded message can be (including the checksum!)
for the code to retain its error-correcting properties.

@tcharding

I don't see how (abstractly not technically). The segwit stuff wants to enforce 90 char limit for Bech32m but lib.rs does not? And the limit is a bip-173 thing not a BCH code thing, right?

Yes, the limit is defined by bip 173 based on empirical error-correcting properties. It is not related to the design length of the code and non-segwit users can (probably) get away without caring about it. Though they will get somewhat worse error correcting properties. It is a requirement of segwit but for non-segwit bech32 uses we shouldn't enforce it, because the code does continue to work up to length 1023.

Also, if its on Checksum folk that implement the trait would assume it is checked in lib.rs functions?

Yes, if we add a LENGTH field to the Checksum trait, people would assume that it's checked in lib.rs. And if people want to override this they can make their own copy of Bech32 where they change the field to usize::MAX or whatever.

tcharding · 2023-10-25T02:24:32Z

I had an epic rebase fail, not totally sure I didn't loose stuff ...

8426386 Use string not address in lib.rs (Tobin C. Harding) c9aa166 Use NoData instead of MissingWitnessVersion (Tobin C. Harding) 2eac8e6 Fix colum width typo in rustdoc (Tobin C. Harding) Pull request description: This is the first 4 patches from #142, pushing up separately because that PR is quite complicated to review well and these cleanups are unrelated. ACKs for top commit: apoelstra: ACK 8426386 Tree-SHA512: 154bce12df068e80f17aa3fa5096390ce43eafd5efb9ed2319e0e26767ebb178b729f268bdb2a76c8a709c867d226d67522999da31dbb6f0bc30eb7106c0be9e

Currently we do not enforce the BIP-173 maximum character length for bech32 endoded strings. Add two public functions, both called `encoded_length` that calculate the length of an encoded bech32 string. Do not call the functions yet.

tcharding · 2023-10-26T00:19:33Z

Force push is fix a couple of things from yesterday's rebase fail.

tcharding · 2023-10-26T00:20:19Z

This one turned out to be pretty complicated, thanks for your patience reviewing.

apoelstra · 2023-10-26T13:18:49Z

src/primitives/segwit.rs

+/// The maximum enforced string length of a segwit address.
+///
+/// The maximum length as specified in BIP-173, this is less than the 1023 character code length.
+/// This limit is based on empirical error-correcting properties, see ["Checksum design"] section.


In 6ca06b1:

I'd grep for , see. Almost all of the instances should . See :).

Ha! I literally was looking at someone else's code yesterday thinking the opposite. I'm going to go ahead and trust you, your English tends to be better than mine :)

apoelstra · 2023-10-26T13:20:37Z

6ca06b1 looks good other than the doc nit.

In a followup PR I think 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.)

But this should be a followup PR because it's a debatable thing and I'd rather it have its own reference for future readers.

BIP-173 states that a bech32 string must not exceed 90 characters but this is a bip specification, the bech32/bech32m checksum algos can actually cover more characters than that. To keep the `segwit` stuff bip compliant and also keep the library general add an associated `CODE_LENGTH` const to the `Checksum` trait. Then in the `segwit` module and types enforce the 90 character limit but in the general API and modules enforce the `Ck::CODE_LENGTH` limit. FTR in `bech32 v0.9.0` no lengths were not enforced.

tcharding · 2023-10-26T23:21:52Z

Force push is just the docs thing, no other changes.

tcharding · 2023-10-26T23:28:38Z

Please see #156, I think this PR may be buggy. Perhaps we should just drop the length stuff all together from the Checksum trait and lib.rs? That would still be an improvement over v0.9 (because it enforces 90 chars) but not a regression because it allows any length strings with lib.rs?

apoelstra

ACK 12d7b38

apoelstra · 2023-10-27T00:00:15Z

Perhaps we should just drop the length stuff all together from the Checksum trait and lib.rs? That would still be an improvement

It would still be an improvement, yes, but it would really be better if you couldn't (easily) use checksums for strings for which the checksums' properties didn't apply.

apoelstra · 2023-10-27T00:01:06Z

In any case I would like to move the checksum length discussion into a separate PR, since this one also contains doccomment improvements and segwit stuff and adds the CODE_LENGTH field to the Checksum trait, all of which we definitely want to do.

tcharding mentioned this pull request Oct 17, 2023

Add buffering during encode #138

Merged

tcharding force-pushed the 10-17-enforce-maximum-length branch 2 times, most recently from c41081c to 44e605e Compare October 17, 2023 04:08

clarkmoody reviewed Oct 18, 2023

View reviewed changes

tcharding force-pushed the 10-17-enforce-maximum-length branch 3 times, most recently from b45c4f7 to b728eab Compare October 18, 2023 23:52

tcharding marked this pull request as draft October 18, 2023 23:54

tcharding force-pushed the 10-17-enforce-maximum-length branch 4 times, most recently from d1ead65 to 64310d5 Compare October 22, 2023 17:08

tcharding force-pushed the 10-17-enforce-maximum-length branch from 64310d5 to 65790bf Compare October 22, 2023 17:15

tcharding marked this pull request as ready for review October 22, 2023 17:15

apoelstra approved these changes Oct 22, 2023

View reviewed changes

tcharding force-pushed the 10-17-enforce-maximum-length branch 2 times, most recently from d244386 to 24f1496 Compare October 24, 2023 00:06

tcharding force-pushed the 10-17-enforce-maximum-length branch 3 times, most recently from 0a8b2ca to 6885ece Compare October 25, 2023 02:11

tcharding mentioned this pull request Oct 25, 2023

Do a bunch of cleanups #154

Merged

tcharding force-pushed the 10-17-enforce-maximum-length branch 2 times, most recently from 2a2a703 to 78180aa Compare October 25, 2023 02:24

Add encoded_length functions

f5bd09b

Currently we do not enforce the BIP-173 maximum character length for bech32 endoded strings. Add two public functions, both called `encoded_length` that calculate the length of an encoded bech32 string. Do not call the functions yet.

tcharding force-pushed the 10-17-enforce-maximum-length branch 4 times, most recently from 801febf to 1235674 Compare October 26, 2023 00:19

tcharding force-pushed the 10-17-enforce-maximum-length branch from 1235674 to 6ca06b1 Compare October 26, 2023 00:49

apoelstra reviewed Oct 26, 2023

View reviewed changes

tcharding force-pushed the 10-17-enforce-maximum-length branch from 6ca06b1 to 12d7b38 Compare October 26, 2023 23:21

tcharding mentioned this pull request Oct 26, 2023

Checksum length #156

Open

apoelstra approved these changes Oct 26, 2023

View reviewed changes

apoelstra merged commit ac74415 into rust-bitcoin:master Oct 27, 2023
12 checks passed

tcharding deleted the 10-17-enforce-maximum-length branch October 27, 2023 01:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce maximum length #142

Enforce maximum length #142

tcharding commented Oct 17, 2023 •

edited

Loading

apoelstra commented Oct 17, 2023

tcharding commented Oct 17, 2023

tcharding commented Oct 18, 2023 •

edited

Loading

apoelstra commented Oct 18, 2023

tcharding commented Oct 18, 2023

clarkmoody Oct 18, 2023

tcharding Oct 18, 2023 •

edited

Loading

tcharding commented Oct 18, 2023

tcharding commented Oct 22, 2023

apoelstra commented Oct 22, 2023

apoelstra left a comment

tcharding commented Oct 22, 2023

tcharding commented Oct 22, 2023

apoelstra commented Oct 22, 2023

tcharding commented Oct 23, 2023

apoelstra commented Oct 24, 2023

tcharding commented Oct 24, 2023 •

edited

Loading

apoelstra commented Oct 24, 2023

tcharding commented Oct 25, 2023

tcharding commented Oct 26, 2023

tcharding commented Oct 26, 2023

apoelstra Oct 26, 2023

tcharding Oct 26, 2023

apoelstra commented Oct 26, 2023

tcharding commented Oct 26, 2023

tcharding commented Oct 26, 2023

apoelstra left a comment

apoelstra commented Oct 27, 2023

apoelstra commented Oct 27, 2023

Enforce maximum length #142

Enforce maximum length #142

Conversation

tcharding commented Oct 17, 2023 • edited Loading

apoelstra commented Oct 17, 2023

tcharding commented Oct 17, 2023

tcharding commented Oct 18, 2023 • edited Loading

apoelstra commented Oct 18, 2023

tcharding commented Oct 18, 2023

clarkmoody Oct 18, 2023

Choose a reason for hiding this comment

tcharding Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

tcharding commented Oct 18, 2023

tcharding commented Oct 22, 2023

apoelstra commented Oct 22, 2023

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Oct 22, 2023

tcharding commented Oct 22, 2023

apoelstra commented Oct 22, 2023

tcharding commented Oct 23, 2023

apoelstra commented Oct 24, 2023

tcharding commented Oct 24, 2023 • edited Loading

apoelstra commented Oct 24, 2023

tcharding commented Oct 25, 2023

tcharding commented Oct 26, 2023

tcharding commented Oct 26, 2023

apoelstra Oct 26, 2023

Choose a reason for hiding this comment

tcharding Oct 26, 2023

Choose a reason for hiding this comment

apoelstra commented Oct 26, 2023

tcharding commented Oct 26, 2023

tcharding commented Oct 26, 2023

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Oct 27, 2023

apoelstra commented Oct 27, 2023

tcharding commented Oct 17, 2023 •

edited

Loading

tcharding commented Oct 18, 2023 •

edited

Loading

tcharding Oct 18, 2023 •

edited

Loading

tcharding commented Oct 24, 2023 •

edited

Loading