Introduce new error handling convention #635

tcharding · 2023-07-26T04:07:19Z

Please do not consider this PR for merge until after the upcoming v0.28.0 is released.

This PR is a proof of concept based on ideas discussed:

The aim is to come up with a new error handling convention that can be applied across the whole organisation.

Patch 1: preparatory cleanup
Implement specific errors

Now includes a general error enum that has a variant for each other "specific error" type in the lib as well as a macro that users can use to implement From<err> for CustomError for their own CustomError where err done for every error type in the lib (see example for usage).

tcharding · 2023-07-26T04:08:24Z

CC @Kixunil for when he comes back because this will be of interest to him.

tcharding · 2023-07-26T06:06:26Z

Got some issues with Rust 1.48, will wait for concept ack before continuing.

apoelstra · 2023-07-26T14:03:05Z

Innnnteresting. I guess, if we accept this, the new macros should probably be generalized a bit and moved into bitcoin-internals. (This is enough stuff that I'd accept a bitcoin-internals dependency.)

I continue to be skeptical of macros but here they're really pulling a lot of weight, and this is mostly write-only code anyway.

Not sure how I feel about it.

sanket1729 · 2023-07-28T06:53:09Z

strong concept ACK. This will be a major breaking change, we also need to carefully audit the places we will not need non-exhuastive stuff.

Not sure about the macros vs manual typing. I don't mind having a lot of boilerplate code vs macros. The error code is nicely hidden in a error module and does not really hinder the main code.

For secp code, I feel comfortable with macros as they are nicely self contained. But as soon we start hacking more stuff into it to support weird nested error structures etc, it starts to be unreadable. I also don't mind just typing the from impl, it's not that much boilerplate as serde stuff.

tcharding · 2023-08-06T23:38:53Z

I found a bunch more error improvements that can be done, converting to draft while I hack them up.

tcharding · 2023-08-07T00:15:05Z

I forget what was the policy on feature gating enum variants within error types? Is that frowned upon? (Currently includes an variant feature gated on "recovery" feature.)

tcharding · 2023-08-07T06:24:22Z

I do not know what is causing the CI fail, @apoelstra can you put me out of my misery?

apoelstra · 2023-08-07T15:05:33Z

Well, the 1.48 one is because serde_test apparently broke MSRV.

The other one is because the latest nightly changed the format of the panic message to add a newline, so our grep is no longer working. We should change it to grep for the exact text [libsecp256k1] illegal argument. !rustsecp256k1_v0_8_1_fe_is_zero(&ge->x).

tcharding · 2023-08-07T22:06:30Z

Thanks man.

, the 1.48 one is because serde_test apparently broke MSRV.

More pinning, awesome.

The other one is because the latest nightly changed the format of the panic message to add a newline, so our grep is no longer working. We should change it to grep for the exact text [libsecp256k1] illegal argument. !rustsecp256k1_v0_8_1_fe_is_zero(&ge->x).

Bother, that is what I thought happened (if you saw the closed PR) but I guess I fell over at the "grep for the right thing" stage. Will try again, cheers.

tcharding · 2023-08-08T02:57:19Z

Seems there are multiple problems with the nightly job. Attempting to fix them in #641

tcharding · 2023-08-08T02:57:51Z

Note please, this PR is not currently on top of the latest version of #641

tcharding · 2023-08-08T23:59:00Z

On ice until after v.0.28.0 release is done because using these new errors in rust-bitcoin is non-trivial.

Kixunil · 2023-09-27T11:02:35Z

My thoughts:

This is close to what I wanted originally and the only downside is complexity/boilerplate which I'm willing to accept. Not sure about others.

There are no #[non_exhaustive] attributes which I find questionable. On structs the only advantage of the attribute being absent is ability to create the errors externally - I don't find this very useful and the disadvantage - being unable to add or change error information is quite large. So I wish to add non_exhaustive to all structs.

For enums, #[non_exhaustive] affects whether matching is exhaustive. I don't think we can make blanket statements about viability here. Both exhaustive matching and future-proofing are valuable but they are at odds with each-other. Thus we need to carefully think about each and figure out if it's likely that a new error type will be added.

Finally, IIUC this change doesn't add error information, only rearranges it (didn't check fully). That's an OK step, just let's not forget about adding some important things.

tcharding · 2023-09-27T23:52:03Z

Thanks for the review @Kixunil! Added non_exhaustive to all structs. Added it to enums by default and excluded only those I thought warranted it.

If we return `Self, Self::Err` then the implementations of `FromStr` are easier to maintain.

As is customary put the associated type at the start of the `FromStr` impl block.

In preparation for adding separate errors move the hex code to its own private module.

Kixunil

Crap, I had these comments pending and I forgot about it. I will review the rest later.

Kixunil · 2023-10-03T06:00:34Z

examples/sign_verify_recovery.rs

@@ -15,7 +17,7 @@ fn recover<C: Verification>(
    let id = ecdsa::RecoveryId::from_i32(recovery_id as i32)?;
    let sig = ecdsa::RecoverableSignature::from_compact(&sig, id)?;

-    secp.recover_ecdsa(&msg, &sig)
+    Ok(secp.recover_ecdsa(&msg, &sig)?)


Do we prefer this over secp.recover_ecdsa(&msg, &sig).map(Into::into)? I personally tend to use the latter but I don't really care much.

I like yours. Will use.

I couldn't get it to work, used

let pk = secp.recover_ecdsa(&msg, &sig)?; Ok(pk)

instead

I meant map_err, sorry.

Kixunil · 2023-10-03T06:03:43Z

src/context.rs

+
+impl fmt::Display for NotEnoughMemoryError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        f.write_str("not enough preallocated memory for the requested buffer size")


Just realized what this type is used for and it could help having required and available fields in the error.

Sure thing, thanks.

Kixunil · 2023-10-03T06:10:14Z

src/context.rs

-    pub fn preallocated_gen_new(buf: &'buf mut [AlignedType]) -> Result<Secp256k1<C>, Error> {
+    pub fn preallocated_gen_new(
+        buf: &'buf mut [AlignedType],
+    ) -> Result<Secp256k1<C>, NotEnoughMemoryError> {


I think it'd be nicer to accept a newtype rather than slice to avoid the error entirely.

Added a patch to do this, as suggested. Thanks

Kixunil · 2023-10-03T06:14:21Z

src/ecdh.rs

        match bytes.len() {
            SHARED_SECRET_SIZE => {
                let mut ret = [0u8; SHARED_SECRET_SIZE];
                ret[..].copy_from_slice(bytes);
                Ok(SharedSecret(ret))
            }
-            _ => Err(Error::InvalidSharedSecret),
+            _ => Err(SharedSecretError),
        }
    }
 }


This effectively duplicates the functionality of <[u8; SHARED_SECRET_SIZE] as TryFrom<[u8]>>::try_from. Maybe we should just use the same error type? Although our own could carry more information. Anyway, we use this error type in many places, so I think we should have a shared one. This applies to bitcoin_hashes as well.

I did a refactor patch to use TryFrom, also I removed the SharedSecretError and added an InvalidSliceLengthError. Does not include the core::array::TryFromSliceError because that type does not implement PartialEq and Eq.

Kixunil · 2023-10-03T06:18:05Z

src/ecdsa/mod.rs

@@ -36,22 +37,21 @@ impl fmt::Display for Signature {
 }

 impl str::FromStr for Signature {
-    type Err = Error;
-    fn from_str(s: &str) -> Result<Signature, Error> {
+    type Err = SignatureFromStrError;


SignatureParseError would be shorter.

Used as suggested.

Add a wrapper type `PreallocatedBuffer` that maintains the invariant that the inner buffer is big enough. This allows us to take this as a parameter for preallocated context constructors and remove the error path.

Using `TryFrom` is more terse with no loss of clarity. Refactor only, no logic changes.

Currently we have a large general error type. Create specific error types for each function as needed so that a function returns ever variant available in its return type.

Kixunil · 2023-11-10T05:30:59Z

src/context.rs

@@ -320,30 +321,51 @@ unsafe impl<'buf> PreallocatedContext<'buf> for AllPreallocated<'buf> {}
 unsafe impl<'buf> PreallocatedContext<'buf> for SignOnlyPreallocated<'buf> {}
 unsafe impl<'buf> PreallocatedContext<'buf> for VerifyOnlyPreallocated<'buf> {}

+/// A preallocated buffer, enforces the invariant that the buffer is big enough.
+#[allow(missing_debug_implementations)]
+pub struct PreallocatedBuffer<'buf>(&'buf [AlignedType]);


This needs to be mut as it's unsound otherwise.

Also FYI I probably meant something else but can't remember what it was. I guess I meant &mut AlignedType which is unsound. We could also just use unsized type pub struct PreallocatedBuffer([AligneType]); which is more idiomatic.

In ideal world we would use an extern type but those aren't even stable.

We should also probably make AlignedType contain MaybeUninit or something like that, as the C code might write padding bytes to it.

I think it wasn't stable (enough) back when I implemented this logic

Yeah, it wasn't stable back then. Definitely should. I promoted this to #707 because I expect this old PR to get closed.

Kixunil · 2023-11-10T05:33:00Z

src/context.rs

+            return Err(NotEnoughMemoryError);
+        }
+        Ok(PreallocatedBuffer(buf))
+    }


This also should have an unsafe constructor to initialize it from pointer and size.

See also #665

tcharding force-pushed the 07-26-new-error-handling-convention branch 2 times, most recently from 609a514 to cd28e09 Compare July 26, 2023 05:51

tcharding mentioned this pull request Jul 26, 2023

WIP: Use the new error stuff from secp rust-bitcoin/rust-bitcoin#1956

Closed

tcharding force-pushed the 07-26-new-error-handling-convention branch from cd28e09 to 50cf4f8 Compare July 26, 2023 05:59

tcharding force-pushed the 07-26-new-error-handling-convention branch 2 times, most recently from d4cd52a to eced889 Compare August 2, 2023 04:01

tcharding mentioned this pull request Aug 2, 2023

Use new error convention introduced in secp rust-bitcoin/rust-bitcoin#1971

Closed

tcharding force-pushed the 07-26-new-error-handling-convention branch from eced889 to 384ad31 Compare August 4, 2023 04:19

tcharding marked this pull request as draft August 6, 2023 23:38

tcharding force-pushed the 07-26-new-error-handling-convention branch 2 times, most recently from b18475c to 67e8c7e Compare August 7, 2023 00:14

tcharding mentioned this pull request Aug 7, 2023

CI: Modify regex #638

Closed

tcharding force-pushed the 07-26-new-error-handling-convention branch 3 times, most recently from f626506 to 17d7734 Compare August 7, 2023 06:21

tcharding marked this pull request as ready for review August 7, 2023 06:24

tcharding force-pushed the 07-26-new-error-handling-convention branch 2 times, most recently from b981b7c to 148bc48 Compare August 7, 2023 07:01

tcharding force-pushed the 07-26-new-error-handling-convention branch from 148bc48 to 462cbd6 Compare August 8, 2023 02:01

tcharding mentioned this pull request Aug 8, 2023

Improve errors #601

Closed

tcharding marked this pull request as draft August 8, 2023 23:58

tcharding force-pushed the 07-26-new-error-handling-convention branch from 462cbd6 to 5e0839c Compare August 11, 2023 01:16

tcharding mentioned this pull request Aug 15, 2023

Add segwit API rust-bitcoin/rust-bech32#120

Merged

tcharding mentioned this pull request Sep 22, 2023

Polishing/stabilization roadmap? rust-bitcoin/rust-bech32#60

Closed

tcharding force-pushed the 07-26-new-error-handling-convention branch from 5e0839c to 708da6e Compare September 27, 2023 23:50

tcharding mentioned this pull request Nov 6, 2023

Hide all error internals rust-bitcoin/hex-conservative#44

Merged

tcharding added 2 commits November 6, 2023 16:23

Return Result<Self, Self::Err> in FromStr impls

c5b6226

If we return `Self, Self::Err` then the implementations of `FromStr` are easier to maintain.

Move associated type to top of impl

ebfb3e2

As is customary put the associated type at the start of the `FromStr` impl block.

tcharding force-pushed the 07-26-new-error-handling-convention branch 2 times, most recently from 0a8f8c8 to 34e0f0e Compare November 6, 2023 07:04

Move hex code to new submodule

ec02f3f

In preparation for adding separate errors move the hex code to its own private module.

tcharding force-pushed the 07-26-new-error-handling-convention branch 2 times, most recently from 9fad4af to f71b678 Compare November 6, 2023 07:17

Kixunil reviewed Nov 6, 2023

View reviewed changes

tcharding added 2 commits November 8, 2023 15:23

Add PreallocatedBuffer

08c330f

Add a wrapper type `PreallocatedBuffer` that maintains the invariant that the inner buffer is big enough. This allows us to take this as a parameter for preallocated context constructors and remove the error path.

Use TryFrom in SharedSecret::from_slice

7bd28e1

Using `TryFrom` is more terse with no loss of clarity. Refactor only, no logic changes.

tcharding force-pushed the 07-26-new-error-handling-convention branch 3 times, most recently from 65a955b to ddd3176 Compare November 8, 2023 05:21

Split general error into specific

4e72db6

Currently we have a large general error type. Create specific error types for each function as needed so that a function returns ever variant available in its return type.

tcharding force-pushed the 07-26-new-error-handling-convention branch from ddd3176 to 4e72db6 Compare November 8, 2023 05:33

Kixunil reviewed Nov 10, 2023

View reviewed changes

apoelstra mentioned this pull request Jul 6, 2024

We should also probably make AlignedType contain MaybeUninit or something like that, as the C code might write padding bytes to it. #707

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new error handling convention #635

Introduce new error handling convention #635

tcharding commented Jul 26, 2023 •

edited

Loading

tcharding commented Jul 26, 2023

tcharding commented Jul 26, 2023

apoelstra commented Jul 26, 2023

sanket1729 commented Jul 28, 2023

tcharding commented Aug 6, 2023

tcharding commented Aug 7, 2023

tcharding commented Aug 7, 2023

apoelstra commented Aug 7, 2023

tcharding commented Aug 7, 2023

tcharding commented Aug 8, 2023

tcharding commented Aug 8, 2023

tcharding commented Aug 8, 2023

Kixunil commented Sep 27, 2023

tcharding commented Sep 27, 2023

Kixunil left a comment

Kixunil Oct 3, 2023

tcharding Nov 6, 2023 •

edited

Loading

tcharding Nov 8, 2023 •

edited

Loading

Kixunil Jul 6, 2024

Kixunil Oct 3, 2023

tcharding Nov 6, 2023

Kixunil Oct 3, 2023

tcharding Nov 8, 2023

Kixunil Oct 3, 2023

tcharding Nov 8, 2023

Kixunil Oct 3, 2023

tcharding Nov 8, 2023

Kixunil Nov 10, 2023

elichai Jul 6, 2024

apoelstra Jul 6, 2024

Kixunil Nov 10, 2023 •

edited

Loading

Introduce new error handling convention #635

Are you sure you want to change the base?

Introduce new error handling convention #635

Conversation

tcharding commented Jul 26, 2023 • edited Loading

tcharding commented Jul 26, 2023

tcharding commented Jul 26, 2023

apoelstra commented Jul 26, 2023

sanket1729 commented Jul 28, 2023

tcharding commented Aug 6, 2023

tcharding commented Aug 7, 2023

tcharding commented Aug 7, 2023

apoelstra commented Aug 7, 2023

tcharding commented Aug 7, 2023

tcharding commented Aug 8, 2023

tcharding commented Aug 8, 2023

tcharding commented Aug 8, 2023

Kixunil commented Sep 27, 2023

tcharding commented Sep 27, 2023

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

tcharding Nov 8, 2023 • edited Loading

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

Kixunil Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

tcharding commented Jul 26, 2023 •

edited

Loading

tcharding Nov 6, 2023 •

edited

Loading

tcharding Nov 8, 2023 •

edited

Loading

Kixunil Nov 10, 2023 •

edited

Loading