crypto: Overhaul the errors #1862

tcharding · 2023-05-18T06:48:22Z

EDIT: The commit hashes below are stale but the text is valid still.

In an effort to "perfect" our error handling, overhaul the error handling in the crypto module.

The aim is to do a small chunk so we can bikeshed on it then I can apply the learnings to the rest of the codebase.

Its all pretty trivial except:

commit 4c180277 Put NonStandardSighashTypeError inside ecdsa::Error variant
comimt 5a196535 Add InvalidSighashTypeError
commit 05772ade Use defensive documentation

Particularly the last one might be incorrect/controversial.

Also, please take the time to check the overall state of error code in the crypto module on this branch in case there is anything else we want to do.

Thanks

tcharding · 2023-05-18T22:29:46Z

Bother, I had changes, to the last patch, staged that I did not commit.

apoelstra · 2023-05-22T13:02:15Z

Looks good! Only 4a322fb I think might be controversial (adds #[doc(hidden)] to all the error conversions). IMHO this is the correct thing to do.

apoelstra

ACK 154cf74

stevenroose · 2023-06-05T04:57:59Z

bitcoin/src/crypto/sighash.rs

@@ -518,15 +518,15 @@ impl TapSighashType {
 /// This type is consensus valid but an input including it would prevent the transaction from
 /// being relayed on today's Bitcoin network.
 #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
-pub struct NonStandardSighashType(pub u32);
+pub struct NonStandardSighashTypeError(pub(crate) u32);


Wait why would you make the sighash type actually used private?

I mean I agree we should never expose newtype struct internals, but this value is useful, couldn't we make this a regular struct and name the field as sighash or something so that it's useful. Or if we insist on newtype format, have a method exposing it.

My bad, I missed actioning this when I first read it. Will fix, thanks.

stevenroose · 2023-06-05T04:59:35Z

bitcoin/src/crypto/ecdsa.rs

+            NonStandardSighashType(hash_ty) =>
+                write!(f, "non-standard signature hash type {}", hash_ty),
+            EmptySignature => write!(f, "empty ECDSA signature"),
+            Secp256k1(ref e) => write_err!(f, "invalid ECDSA signature"; e),


Is this somehow standard practice now or so? I know this trick but I have usually tried to steer clear of it somehow as I found it nonstandard and weird. Though I have recently started using Self:: more in general. Though for errors, I guess I wouldn't do it as it's only a single character more and the Error name will probably never change.

I'm not sure which part of the code you are referring to as the trick?

Do you mean how I always add use Error::* locally in code that matches on error types? If so that's my personal thing to make code more terse.

stevenroose · 2023-06-05T05:01:44Z

bitcoin/src/crypto/key.rs

@@ -22,64 +22,6 @@ use crate::prelude::*;
 use crate::taproot::{TapNodeHash, TapTweakHash};
 use crate::{base58, io};

-/// A key-related error.


random hint on this commit: for rebase hell, it's probably smarter to do the move commit first to simplify rebases :)

You mean to simplify rebases where I want to change the PR, right? (As opposed to rebasing to fix merge conflicts.) If so, good point, will keep in mind. Thanks

stevenroose · 2023-06-05T05:02:07Z

bitcoin/src/crypto/taproot.rs

@@ -30,15 +30,13 @@ impl Signature {
        match sl.len() {
            64 => {
                // default type
-                let sig =
-                    secp256k1::schnorr::Signature::from_slice(sl).map_err(Error::Secp256k1)?;
+                let sig = secp256k1::schnorr::Signature::from_slice(sl)?;


YES 🙏 🙏

stevenroose · 2023-06-05T05:06:14Z

bitcoin/src/crypto/ecdsa.rs

@@ -202,10 +202,10 @@ impl fmt::Display for Error {
        use Error::*;

        match *self {
-            HexEncoding(ref e) => write_err!(f, "signature hex encoding error"; e),


Heh, when did this macro get introduced. It doesn't print the inner error anymore in the error.. That's very non-standard and I really don't like that.. That means in a panic you won't see the underlying error anymore... The macro seems to assume the person dealing with the error has access to .source() which is almost never the case lol, that's why it's printed into the Display..

I'll defer to @Kixunil to debate the finer points of error handling :)

@stevenroose it's always the case for people using anyhow, which is the majority of actual applications that end-users see.

If we stick the outer error into our Display implementations then it's impossible for any downstream users to remove it so they're screwed if they want to provide a better error display with more context.

What we're doing has been standard practice for many years now. The only thing non-standard is that we re-introduce the prefix when std is disabled, since then we don't have source.

tcharding · 2023-06-05T06:41:24Z

Rebased on master to fix merge conflicts and removed the "remove hidden docs attribute" patch as per recent discussion.

yancyribbens · 2023-05-19T12:44:44Z

bitcoin/src/crypto/taproot.rs

-/// A taproot sig related error.
-#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
+/// A taproot signature-related error.
+#[derive(Clone, Debug, PartialEq, Eq)]


This will cause a breaking change for downstream crates that used theses traits? Ord, Hash, Debug

IE breaking API change?

Correct, it is a breaking change. But IMO ordering traits and Hash really don't make sense for an error type.

There are a ton of breaking changes to be done to clean up the errors. The next release, (and maybe the one after that too), are going to break everything ... again. The way I see it there isn't much we can do about this except let everyone know, apologise profusely and try help out with upgrade docs and things. This won't go on for ever, just hope folk know we take the breaks seriously and we are really trying to get to a place where we can not change things.

apoelstra · 2023-06-07T19:25:17Z

d8f6556 looks fine to me except for the pub thing

tcharding · 2023-06-08T05:27:57Z

I need more of my brain working to fix this. On ice till it starts functioning.

tcharding · 2023-06-09T05:36:44Z

Changes in force push:

Remove the pub(crate) change
Put the "reduce derives" change first to make later error addition patch easier to review
Added a final patch to use named fields - done separately so both errors are done in a single patch since the named field change is discreet and both errors are related

tcharding · 2023-06-09T05:37:13Z

Good reviewing @stevenroose, I'm much happier with this patch set now. Thanks

tcharding · 2023-06-29T01:33:29Z

Gentle bump please crew.

apoelstra · 2023-06-29T12:34:47Z

Compilation has broken on rust nightly and I need to fix that before I can test this. But utACK eabafe5

apoelstra · 2023-06-29T12:36:55Z

#1927

tcharding · 2023-07-08T01:24:50Z

Rebase on master, no other changes.

apoelstra

ACK b1904c5

tcharding · 2023-07-20T05:39:10Z

Rebased and removed the doc hidden patch since we decided not to use that attribute.

apoelstra · 2023-07-20T17:11:51Z

@tcharding looks like this rebase loses the pub(crate) change. That is, you're again using pub(crate) instead of pub for the inner u32 for a couple errors.

tcharding · 2023-07-20T23:19:22Z

Bother, my bad. I think I need to update my workflow to start running git status to check my local branch is up to date with the remote before hacking so I don't loose changes pushed from my laptop - thanks man.

tcharding · 2023-07-20T23:23:41Z

Good reviewing BTW, did you catch that with git range-diff?

tcharding · 2023-07-20T23:27:56Z

Made inner error ints public ... again.

apoelstra · 2023-07-21T13:25:14Z

Good reviewing BTW, did you catch that with git range-diff?

Yep :) it wasn't too hard since this was pretty-much the only real change. I also use delta as my diff viewer which has pretty good coloring for range-diff output.

In this case I also remembered the pub(crate) conversation and recognized what had probably happened.

apoelstra

ACK 8e2443d

Error types conventionally include `Error` as a suffix. Rename `NonStandardSighashType` to `NonStandardSighashTypeError`. While we are at it make the inner type private to the crate, there is no need to leak the inner values type.

Improve the `Error` `Display` impls by doing: - Be more terse by importing the error enum's variants. - Do not use capital letters for error messages.

As per convention; put the error type inside a variant and delegate to it instead of carrying an integer around.

As we do for `NonStandardSighashErrorType` add an error struct for invalid sighash type, used by the `taproot` module instead of returning a generic error enum with loads of unused variants.

As per convention; do not capitalize error messages.

Make the rustdoc arguable clearer, this is a sig error.

Move the From impl to be below the other code for the error. Refactor only, no logic changes.

Error code is boring, put it at the bottom of the file. Refactor only, no logic changes.

Only commit in the docs and error messages to what we _really_ know. In an attempt to reduce the likelyhood of the code going stale only commit to what is guaranteed - that we have an error from a module. This does arguably reduce the amount of context around the error.

55be538 policy: Add refactor carve out (Tobin C. Harding) Pull request description: I have managed to burn out or bore our reviewers/maintainers. Getting two acks is becoming increasingly difficult. I've pestered everyone to the limit that I feel socially comfortable doing so, and as such, am requesting a carve out to the 2-ACK before merge rule. The primary justification is that I feel we should have a bit more of BDFL and a bit less total consensus if we are to push forwards. ### Example PRs where this change would apply - #1925 - #1854 - #1862 ACKs for top commit: elichai: I agree this makes sense for refactors ACK 55be538 apoelstra: ACK 55be538 sanket1729: ACK 55be538. Same reasons as apoelstra. And this is only for re-factors that are not adding any new features. RCasatta: ACK 55be538 Tree-SHA512: a5e206252015f49245ed282a3be7a35760d16f94dc6e60f31edf589a41ef642eba52a3bd7d1375b6033f3cf0128f47beee4f03e59cad151c64eedd71ac98baac

apoelstra

ACK 3c0bb63

apoelstra · 2023-08-01T20:18:36Z

Rebase was mostly trivial. Merging based on 2-week policy in #1945

tcharding force-pushed the 05-18-sighash-errors branch from 05772ad to 154cf74 Compare May 18, 2023 22:29

tcharding mentioned this pull request May 19, 2023

Add check_pub_error macro #1743

Closed

apoelstra previously approved these changes May 22, 2023

View reviewed changes

stevenroose reviewed Jun 5, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via d8f6556 June 5, 2023 06:40

tcharding force-pushed the 05-18-sighash-errors branch from 154cf74 to d8f6556 Compare June 5, 2023 06:40

yancyribbens reviewed Jun 7, 2023

View reviewed changes

tcharding marked this pull request as draft June 8, 2023 05:28

tcharding marked this pull request as ready for review June 9, 2023 05:36

tcharding force-pushed the 05-18-sighash-errors branch 2 times, most recently from 3e086a8 to eabafe5 Compare June 9, 2023 15:51

tcharding force-pushed the 05-18-sighash-errors branch from eabafe5 to b1904c5 Compare July 8, 2023 01:24

apoelstra previously approved these changes Jul 10, 2023

View reviewed changes

tcharding mentioned this pull request Jul 18, 2023

policy: Add refactor carve out #1945

Merged

tcharding dismissed apoelstra’s stale review via a9c7c49 July 20, 2023 05:38

tcharding force-pushed the 05-18-sighash-errors branch from b1904c5 to a9c7c49 Compare July 20, 2023 05:38

tcharding force-pushed the 05-18-sighash-errors branch from a9c7c49 to 8e2443d Compare July 20, 2023 23:27

apoelstra previously approved these changes Jul 21, 2023

View reviewed changes

tcharding added 10 commits August 1, 2023 15:55

Rename non standard sighash error type

22c7aa8

Error types conventionally include `Error` as a suffix. Rename `NonStandardSighashType` to `NonStandardSighashTypeError`. While we are at it make the inner type private to the crate, there is no need to leak the inner values type.

Improve error display imlps

6c9d9d9

Improve the `Error` `Display` impls by doing: - Be more terse by importing the error enum's variants. - Do not use capital letters for error messages.

Put NonStandardSighashTypeError inside ecdsa::Error variant

baba0fd

As per convention; put the error type inside a variant and delegate to it instead of carrying an integer around.

Add InvalidSighashTypeError

ae07786

As we do for `NonStandardSighashErrorType` add an error struct for invalid sighash type, used by the `taproot` module instead of returning a generic error enum with loads of unused variants.

Do not capitalize error messages

f23155a

As per convention; do not capitalize error messages.

Fix docs on error type

5f8e0ad

Make the rustdoc arguable clearer, this is a sig error.

Move From for Error impl

fe3b1e1

Move the From impl to be below the other code for the error. Refactor only, no logic changes.

crypto: key: Move error code to the bottom of the file

80d5d66

Error code is boring, put it at the bottom of the file. Refactor only, no logic changes.

Do trivial rustdoc improvements

3c0bb63

tcharding dismissed apoelstra’s stale review via 3c0bb63 August 1, 2023 06:30

tcharding force-pushed the 05-18-sighash-errors branch from 8e2443d to 3c0bb63 Compare August 1, 2023 06:30

apoelstra approved these changes Aug 1, 2023

View reviewed changes

tcharding mentioned this pull request Aug 2, 2023

Use new error convention introduced in secp #1971

Draft

apoelstra merged commit 205544b into rust-bitcoin:master Aug 2, 2023
29 checks passed

crypto: Overhaul the errors #1862

crypto: Overhaul the errors #1862

Conversation

tcharding commented May 18, 2023 • edited

tcharding commented May 18, 2023

apoelstra commented May 22, 2023

apoelstra 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

tcharding commented Jun 5, 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

apoelstra commented Jun 7, 2023

tcharding commented Jun 8, 2023

tcharding commented Jun 9, 2023

tcharding commented Jun 9, 2023 • edited

tcharding commented Jun 29, 2023

apoelstra commented Jun 29, 2023

apoelstra commented Jun 29, 2023

tcharding commented Jul 8, 2023

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Jul 20, 2023

apoelstra commented Jul 20, 2023

tcharding commented Jul 20, 2023

tcharding commented Jul 20, 2023

tcharding commented Jul 20, 2023

apoelstra commented Jul 21, 2023 • edited

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Aug 1, 2023

tcharding commented May 18, 2023 •

edited

tcharding commented Jun 9, 2023 •

edited

apoelstra commented Jul 21, 2023 •

edited