Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce new error handling convention #635

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Jul 26, 2023

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
Copy link
Member Author

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

@tcharding 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 tcharding force-pushed the 07-26-new-error-handling-convention branch from cd28e09 to 50cf4f8 Compare July 26, 2023 05:59
@tcharding
Copy link
Member Author

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

@apoelstra
Copy link
Member

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
Copy link
Member

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 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 tcharding force-pushed the 07-26-new-error-handling-convention branch from eced889 to 384ad31 Compare August 4, 2023 04:19
@tcharding
Copy link
Member Author

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

@tcharding tcharding marked this pull request as draft August 6, 2023 23:38
@tcharding 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
Copy link
Member Author

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 tcharding mentioned this pull request Aug 7, 2023
@tcharding 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
Copy link
Member Author

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

@tcharding tcharding marked this pull request as ready for review August 7, 2023 06:24
@tcharding 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
@apoelstra
Copy link
Member

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
Copy link
Member Author

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 tcharding force-pushed the 07-26-new-error-handling-convention branch from 148bc48 to 462cbd6 Compare August 8, 2023 02:01
@tcharding
Copy link
Member Author

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

@tcharding
Copy link
Member Author

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

@tcharding tcharding mentioned this pull request Aug 8, 2023
@tcharding tcharding marked this pull request as draft August 8, 2023 23:58
@tcharding
Copy link
Member Author

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

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 27, 2023

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 tcharding force-pushed the 07-26-new-error-handling-convention branch from 5e0839c to 708da6e Compare September 27, 2023 23:50
@tcharding
Copy link
Member Author

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.
@tcharding 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
In preparation for adding separate errors move the hex code to its own
private module.
@tcharding 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
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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)?)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@tcharding tcharding Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like yours. Will use.

Copy link
Member Author

@tcharding tcharding Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get it to work, used

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

instead

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant map_err, sorry.

src/context.rs Outdated

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")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing, thanks.

src/context.rs Outdated
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> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a patch to do this, as suggested. Thanks

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),
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/ecdsa/mod.rs Outdated
@@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SignatureParseError would be shorter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@tcharding 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
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 tcharding force-pushed the 07-26-new-error-handling-convention branch from ddd3176 to 4e72db6 Compare November 8, 2023 05:33
@@ -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]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return Err(NotEnoughMemoryError);
}
Ok(PreallocatedBuffer(buf))
}
Copy link
Collaborator

@Kixunil Kixunil Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

See also #665

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants