Improve base58 crate #2481

tcharding · 2024-02-15T05:44:53Z

Improve the error code in the new base58 crate.

coveralls · 2024-02-15T05:46:21Z

Pull Request Test Coverage Report for Build 8334091085

Details

14 of 113 (12.39%) changed or added relevant lines in 7 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.2%) to 83.599%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
base58/src/lib.rs	9	11	81.82%
bitcoin/src/address/mod.rs	0	3	0.0%
bitcoin/src/bip32.rs	0	10	0.0%
bitcoin/src/address/error.rs	0	24	0.0%
bitcoin/src/crypto/key.rs	0	25	0.0%
base58/src/error.rs	4	39	10.26%

Files with Coverage Reduction	New Missed Lines	%
bitcoin/src/crypto/key.rs	1	68.7%

Totals
Change from base Build 8318043190:	-0.2%
Covered Lines:	19757
Relevant Lines:	23633

💛 - Coveralls

Kixunil

Concept ACK, light review.

bitcoin/src/bip32.rs

Kixunil · 2024-02-15T06:44:01Z

bitcoin/src/address/error.rs

+    /// Legacy address is too long.
+    LegacyAddressTooLong(LegacyAddressTooLongError),
+    /// Invalid base58 payload data length for legacy address.
+    InvalidBase58PayloadLength(InvalidBase58PayloadLengthError),


Why not treat too long and InvalidBase58PayloadLenght the same? They are both invalid lengths just in different layers. It'd be nicer to merge them into a simple InvalidLength if we can compute one from the other (which I believe we can) and only present the outer one to the user. (The user is only concerned with number of chars, not decoded bytes.)

Warning, hand wavey response.

Without commenting too much on this specific case, I find complex errors annoying to work with. Error handling is proving to be hard enough as it is using simple types, every time we have more complex errors I end up having to spend ages working out if they conform to all the million nuanced rules we have, this is compounded when errors get used in more places - then when they break the rules it takes much longer to detangle them than if there are just many types. One of the reasons this PR adds multiple errors that are the same "shape" (same fields). I have the gut feeling that forward compatibility will be harder the more we reuse errors in different places also. I think the error code should all be brain dead simple and ridiculously uniform if we are to have any hope of limiting mistakes. Its a moving target still, unfortunately, I can do error stuff mechanically in the afternoon if its brain dead easy - if its complex it means I have to do it earlier in the day when I have more brain power and it also needs more thought when reviewing. Long wall of text, sorry, I've spent a lot of time writing error stuff lately :)

I tend to agree -- and to the extent possible, as a user I like when every error is only constructible from one place. Then I can trace backward from the rust-bitcoin source to figure out where the error actually came from (after grepping to reverse the error message into an error variant).

While conceptually "invalid length" always means that the string was somehow an invalid length, it's useful to know which layer of the decoding complained.

@tcharding is it possible that you're confusing this with an objection against same-shaped errors? Because I'm not objecting to that. Same-shaped errors with different semantics are fine. Differently-shaped errors with the same semantics are not really that great.

I know it can be annoying but really, programming is hard anyway. If we merge the errors in thee library we can save downstream users from verbose matching. I tend to prefer making libraries internally complex if it can make downstream crates less complex because there are multiple downstream projects.

@apoelstra by "users" here I mean the people who are not programmers. I see no reason why they should concern themselves with layers. If you need to debug things we could make the debug representation print out the source or we could even add optional backtrace.

@apoelstra by "users" here I mean the people who are not programmers

You've defined "user" in a way that can't possibly include me, which isn't fair. I also use software. In basically every software I use when I encounter an error of some sort I dig into the source code.

If there is no source code (e.g. the opaque "Contact Key Verification" error that you get when you try to enable the new iMessage fingerprint verification tool) then generally I behave like a non-technical person by giving up and doing something else that doesn't involve computers.

If you need to debug things we could make the debug representation print out the source or we could even add optional backtrace.

I really like this idea. We could compile out that stuff when debug_assertions is off.

The backtrace API isn't stable until 1.65 so something to revisit, not related to this PR or anything soon.

BTW this is an example of something we could add where @tcharding's constructors would be unaffected :). They would just need to have #[track_caller] on and they could conditionally internally generate backtraces.

You've defined "user" in a way that can't possibly include me, which isn't fair. I also use software. In basically every software I use when I encounter an error of some sort I dig into the source code.

Fair, I do this too. :D

Maybe the error messages can be slightly different such that it doesn't bother users but allows debugging.

We could compile out that stuff when debug_assertions is off.

I'd rather use a different feature flag if we want to provide a flag at all. The Backtrace type already handles not capturing when RUST_BACKTRACE is not set

The backtrace API isn't stable until 1.65 so something to revisit

We can use cfg and/or replace it with core::panic::Location in the meantime (and maybe we should do it anyway because panic location is no_std while backtrace isn't).

There's also external library for this but we can't make it conditional on Rust version so I'd rather not use it.

Is this thread resolved @Kixunil or do you still want to see changes to the introduced error types?

Kixunil · 2024-02-15T06:45:40Z

bitcoin/src/crypto/key.rs

    Base58(base58::Error),
+    /// Base58 decoded data was an invalid length.
+    InvalidBase58PayloadLength(InvalidBase58PayloadLengthError),


It'd be nicer to just call this InvalidLength and present the number of chars to the user.

I can get behind that change though!

I had a bit of a play with doing this but because one error uses expected length and one uses expected length I believe its more clear if left as is.

I don't understand, can you explain more?

Wow, that was bad English I wrote. Should have been - one uses expected length and one uses "should have been 33 or 34". WIP accepts data length of 33 or 34

We could distinguish those internally with an enum but maybe it's too much. I suggest we at least make the error messages more user-friendly.

I looked at the error messages and did not see exactly where you wanted them improved. FTR I'm not confident that all our nested errors stack error messages in a meaningful, non-duplicate way, I've kind of given up for now and am just putting something grep'able. I think we will need to come back pre-1.0 and do some deep investigation like I tried to do in rust-bitcoin/rust-bech32#151

base58/src/error.rs

Kixunil · 2024-02-15T06:49:55Z

base58/src/error.rs

+    /// Checksum was not correct.
+    BadChecksum(BadChecksumError),
+    /// Checked data was too short.
+    TooShort(TooShortError),


This is also invalid length so we should then just convert this one into InvalidLength downstream.

Conceptually you are correct but since I've argued above that we should not have the InvalidLength then I believe this comment is mute.

I'm not convinced. The parsed type has a set of valid lengths. Either the length is in the set or isn't. The only issue is how we represent the sets (although they are statically known for their types).

base58/src/error.rs

tcharding · 2024-02-15T19:52:11Z

Thanks for the review @Kixunil. ~~I'll leave the 'pub' constructors in there but note that the merit of constructors/getters is still under debate.~~

~~I've now used pub fields and non_exhaustive along with private constructors.~~

tcharding · 2024-02-21T06:28:43Z

Note, I just saw "incorrect" vs "invalid" comment, will change tomorrow.

tcharding · 2024-02-29T00:11:54Z

I do not claim that this PR makes the errors perfect but IMO it does get the crate into a state that it can be released as v0.1.0 - can we merge as is and release? We now need v.0.1.0 to be out before the next rust-bitcoin release.

Kixunil

Regarding invalid length, I'm fine with not doing it in this PR.

Kixunil · 2024-02-29T10:02:10Z

base58/src/error.rs

+            | TooShort(_) => None,
+        }
+    }
+}


What's the point of this change if the error is reexported anyway? I find this annoying to review.

I'm not sure exactly what is annoying you, did you mean commit: 4f6f0bf6 base58: Add error module?

@Kixunil I guess the point is just code organization. I do a very lax review of these things.

Check that number of variants in one file is the same of one in another file.

Check some random method if it is correctly copied or not.

Check that total lines are the same.

Check the total diff is small.

While reviewing move-only/formatting commits from contributors whose PRs I have been reviewing for years, I am looking for accidental mistakes not for deliberate/malicious errors. Sure, it is possible that @tcharding's or @apoelstra's account is hacked or they deliberately sneak in a change that causes issues. If the commit from a regular contributor cargo fmt something or move something, I tend to a spot sanity check and not use my brain trying to carefully review it.

If these guys wanted to break something, they just push a malicious crate to crates io that is unrelated to github code :) . I do rely on strongly on what commit messages and certainly it is possible that authors sneak in a code logic change along under a move/cargo fmt label under a PR that I ACKed.

base58/src/error.rs

bitcoin/src/address/error.rs

tcharding · 2024-03-13T22:41:38Z

Rebased and changed InvalidCharError to InvalidCharacterError as discussed previously with @Kixunil somewhere else. I also created rust-bitcoin/hex-conservative#85

apoelstra · 2024-03-15T22:59:20Z

I have a mild preference for using pub(super) rather than pub(crate) for error internals. This signals "this error can be created by the module that owns this errors.rs but not by random other parts of the codebase".

But this is just a nit.

apoelstra

ACK 1784b0d

tcharding · 2024-03-18T06:38:04Z

I have a mild preference for using pub(super) rather than pub(crate) for error internals.

Yeah, I like that. I'll fix and re-spin tomorrow.

tcharding · 2024-03-18T21:50:26Z

Just the pub(super) thing.

apoelstra

ACK 33347ac

tcharding · 2024-03-20T01:48:32Z

This whole PR is error handling, totally boring, and can be iterated upon later. However, we need to release this crate already so we can get to the RC release of rust-bitcoin. Can I get an ACK please @Kixunil or @sanket1729.

sanket1729

ACK 33347ac

sanket1729 · 2024-03-20T02:07:02Z

base58/src/error.rs

+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct IncorrectChecksumError {
+    /// The incorrect checksum.
+    pub(super) incorrect: u32,


nit: Most of errors I have seen got and expected format.

I'm happy to change all the errors across the board and just use got/expected, will leave it for another day (or month). Thanks man.

sanket1729 · 2024-03-20T02:12:24Z

I only reviewed this for correctness and I think the moves the code in forward direction. There are still some unresolved issues about which error variants to emit or whether to combine them or not.

I don't have a strong opinion on them and all of those decisions fall well within the acceptable good API boundary and I did not think about what would be the "perfect" solution.

tcharding · 2024-03-20T02:58:34Z

Awesome, thanks for the review man.

apoelstra · 2024-03-20T16:35:52Z

Frustratingly this needs a rebase now because of #2518, which was basically trivial.

In preparation for improving the `base58` error types crate an `error` module and move the single current error type there. Make the module public and reexport the type.

The `base58::decode` function can only return a single error type, add a `InvalidCharacterError` struct (leaf error) to use as the return type.

We are currently using the `base58::Error` type to create errors in `bitcoin`, these are bitcoin errors not `base58` errors. Note that we add what looks like duplicate `InvalidBase58PayloadLengthError` types but they are different because of the expected length. This could have been a field but I elected not to do so for two reasons: 1. We will need to do so anyways if we crate smash more 2. The `crypto::key` one can have one of two values 33 or 34. With this applied we can remove the now unused error variants from `base58::Error`.

As is convention here in `rust-bitcoin`, hide the `base58::Error` internals by adding struct error types.

tcharding · 2024-03-20T19:23:12Z

Done! No other changes.

tcharding · 2024-03-20T19:24:09Z

This is a good example of how something in our set up requires manual intervention when it shouldn't really, there were no conflicts, I just ran cargo rebase master and force pushed - the machines should be able to do that.

(Lazy comment, I did not spend time thinking why.)

apoelstra

ACK af49841

tcharding · 2024-03-22T04:29:32Z

Process idea: when there is an ack and then a force push with minor changes could a single ack from you @apoelstra mean "I ack the changes and I checked the diff with git range-diff since Dev A acked and its still reasonable to think that ack stands"? Especially when you previously acked the same hash so you are running git range-diff anyway. Just an idea.

apoelstra · 2024-03-22T13:32:54Z

Yeah, agreed. Though we should PR such a change separately to CONTRIBUTING.md as a new one-ack carveout.

sanket1729

ACK af49841

sanket1729 · 2024-03-22T19:48:03Z

base58/src/error.rs

+            | TooShort(_) => None,
+        }
+    }
+}


@Kixunil I guess the point is just code organization. I do a very lax review of these things.

Check that number of variants in one file is the same of one in another file.

Check some random method if it is correctly copied or not.

Check that total lines are the same.

Check the total diff is small.

While reviewing move-only/formatting commits from contributors whose PRs I have been reviewing for years, I am looking for accidental mistakes not for deliberate/malicious errors. Sure, it is possible that @tcharding's or @apoelstra's account is hacked or they deliberately sneak in a change that causes issues. If the commit from a regular contributor cargo fmt something or move something, I tend to a spot sanity check and not use my brain trying to carefully review it.

If these guys wanted to break something, they just push a malicious crate to crates io that is unrelated to github code :) . I do rely on strongly on what commit messages and certainly it is possible that authors sneak in a code logic change along under a move/cargo fmt label under a PR that I ACKed.

sanket1729 · 2024-03-22T19:55:21Z

bitcoin/src/address/mod.rs

@@ -732,11 +735,11 @@ impl FromStr for Address<NetworkUnchecked> {
        // If segwit decoding fails, assume its a legacy address.

        if s.len() > 50 {
-            return Err(ParseError::Base58(base58::Error::InvalidLength(s.len() * 11 / 15)));


nice catch!

github-actions bot added C-bitcoin PRs modifying the bitcoin crate doc C-base58 labels Feb 15, 2024

tcharding force-pushed the 02-15-improve-base58 branch from e9d0618 to 3cef6a5 Compare February 15, 2024 05:48

Kixunil reviewed Feb 15, 2024

View reviewed changes

tcharding force-pushed the 02-15-improve-base58 branch from 3cef6a5 to dcb771d Compare February 21, 2024 06:09

github-actions bot added C-hashes PRs modifying the hashes crate C-units PRs modifying the units crate C-io PRs modifying the io crate test labels Feb 21, 2024

tcharding force-pushed the 02-15-improve-base58 branch 2 times, most recently from 4463ffc to f6b86b3 Compare February 21, 2024 06:17

tcharding force-pushed the 02-15-improve-base58 branch 2 times, most recently from f4135a1 to 6221c76 Compare February 26, 2024 01:57

tcharding marked this pull request as ready for review February 26, 2024 02:46

tcharding force-pushed the 02-15-improve-base58 branch 4 times, most recently from dce2aab to d7ceb25 Compare February 29, 2024 00:03

Kixunil reviewed Feb 29, 2024

View reviewed changes

tcharding force-pushed the 02-15-improve-base58 branch from d7ceb25 to 6957218 Compare March 1, 2024 22:04

tcharding added this to the 0.32.0 milestone Mar 5, 2024

tcharding force-pushed the 02-15-improve-base58 branch from 6957218 to 9561468 Compare March 12, 2024 00:56

tcharding mentioned this pull request Mar 13, 2024

Remove InvalidLength from Base58::Error #2376

Open

tcharding force-pushed the 02-15-improve-base58 branch from 9561468 to 1784b0d Compare March 13, 2024 22:40

apoelstra previously approved these changes Mar 15, 2024

View reviewed changes

tcharding dismissed apoelstra’s stale review via 33347ac March 18, 2024 21:50

tcharding force-pushed the 02-15-improve-base58 branch from 1784b0d to 33347ac Compare March 18, 2024 21:50

tcharding added the P-high High priority label Mar 18, 2024

apoelstra previously approved these changes Mar 18, 2024

View reviewed changes

sanket1729 previously approved these changes Mar 20, 2024

View reviewed changes

apoelstra mentioned this pull request Mar 20, 2024

base58: Re-name crate to base58ck #2503

Merged

tcharding added 5 commits March 21, 2024 06:22

base58: Run the formatter

42fabba

base58: Add error module

ec86093

In preparation for improving the `base58` error types crate an `error` module and move the single current error type there. Make the module public and reexport the type.

base58: Add InvalidCharacterError for decoding

669d5e8

The `base58::decode` function can only return a single error type, add a `InvalidCharacterError` struct (leaf error) to use as the return type.

Hide base58::Error internals

af49841

As is convention here in `rust-bitcoin`, hide the `base58::Error` internals by adding struct error types.

tcharding dismissed stale reviews from sanket1729 and apoelstra via af49841 March 20, 2024 19:22

tcharding force-pushed the 02-15-improve-base58 branch from 33347ac to af49841 Compare March 20, 2024 19:22

apoelstra approved these changes Mar 20, 2024

View reviewed changes

sanket1729 approved these changes Mar 22, 2024

View reviewed changes

apoelstra merged commit bfd5255 into rust-bitcoin:master Mar 24, 2024
169 checks passed

apoelstra mentioned this pull request Mar 24, 2024

Add a validation variant to ParseError #2610

Merged

Improve base58 crate #2481

Improve base58 crate #2481

Conversation

tcharding commented Feb 15, 2024 • edited

coveralls commented Feb 15, 2024 • edited

Pull Request Test Coverage Report for Build 8334091085

Details

💛 - Coveralls

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

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

tcharding commented Feb 15, 2024 • edited

tcharding commented Feb 21, 2024

tcharding commented Feb 29, 2024 • edited

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Mar 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Mar 13, 2024 • edited

apoelstra commented Mar 15, 2024

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Mar 18, 2024

tcharding commented Mar 18, 2024

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Mar 20, 2024 • edited

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 commented Mar 20, 2024

tcharding commented Mar 20, 2024

apoelstra commented Mar 20, 2024

tcharding commented Mar 20, 2024

tcharding commented Mar 20, 2024 • edited

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Mar 22, 2024

apoelstra commented Mar 22, 2024

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Feb 15, 2024 •

edited

coveralls commented Feb 15, 2024 •

edited

tcharding commented Feb 15, 2024 •

edited

tcharding commented Feb 29, 2024 •

edited

tcharding Mar 11, 2024 •

edited

tcharding commented Mar 13, 2024 •

edited

tcharding commented Mar 20, 2024 •

edited

tcharding commented Mar 20, 2024 •

edited