Improve `address` error handling #1950

tcharding · 2023-07-21T03:22:15Z

Improve the address::Error using ideas from @sanket1729 here: rust-bitcoin/rust-secp256k1#633

EDIT: This proof-of-concept is now done by: #1971

This PR is now a proof of concept for a new way of providing error types to users of the library. Specifically

Functions return specific error types that are either structs or enums where every variant is used
Modules provide a general error type that includes a variant for each of the specific error types and From impls so users of the lib can call multiple functions from a module and use ? if they do not wish to handle then specific errors.

The address module is a good module to use to test this new idea out because it is in flux anyway and is complicated enough to need multiple errors. Note also that existence of the new PayloadError should make upcoming work to hide the Payload type easier.

Notes

Since this PR introduces a new convention for error code please be picky about the small things, describe in the individual commit logs (eg, using error submodule, the import/export style).

apoelstra

ACK 5f161be

sanket1729

ACK 5f161be. This does create minor issue when we don't have a common unifying type for users who just want to bubble up the error.

let addr = Address::from_str()?; // Error type = Parse
let _ = addr.some_other_api()?; // Error type = Error

Now the downstream user who wants to return a Result for this function has to create a common unifying type for this.

ACK the current changes, just noting the issue as mentioned. If we deem this important. We can add another variant to general Address API, that captures this and make a from/into impl

apoelstra · 2023-07-23T15:13:07Z

Oh, yeah, that's a good point. I think that Error should have a new Parse variant that holds this, so that users can unify the types.

sanket1729 · 2023-07-23T19:27:12Z

Motivated me to write this: rust-bitcoin/rust-secp256k1#633

In preparation for improving error handling in the `address` module move `address.rs` to `address/mod.rs`, add an `address/error.rs` file, and move the public error type to the new module. Note: - Make there module private, this is a code organization thing only. - Use wildcard re-export of `self::error::*` in `address/mod.rs` - Use `super` to import things from `address` into `error` Refactor only, no logic changes.

In order to make the code slightly more terse with no loss of meaning import all the error variants locally in the `Display` impl for `address::Error`. Refactor only, no logic changes.

In error `source` function be explicit about what is a reference, match on `*self` and use `ref` when pattern matching.

The `address` currently has a single large error enum that is returned by all functions irrespective of whether the function can fail with all variants or not. Improve the address error handling by doing: - Create a general purpose error type `address::Error` that has a variant for each of the other error variants in the module. This type is not currently returned by any of our functions but is provided for users of the library who wish to call various `address` functions and do not care to match on the specific error type. - Create specific error types for functions so that each function returns an error that is either a struct or an enum in which all variants are used.

apoelstra · 2023-07-24T13:36:58Z

bitcoin/src/address/mod.rs

-    fn from(e: witness_program::Error) -> Error { Error::WitnessProgram(e) }
-}
+mod error;
+pub use self::error::*;


In a91b00d:

Thanks for highlighting this in the commit message. But I think we should drop it and just directly import Error. If there are other types that need importing we'll notice it when we try to use them. And right now there is only one type in the module.

I see, in a later commit you add more types. But I still think they should be listed explicitly.

Can do, in hindsight I should have realised you'd hold this position :) Is there, in your opinion, ever a valid use of wildcard imports? I'm not being facetious, legitimate question.

In my opinion, no. I don't mind use Error::* in functions that match on error variants, since it's extremely clear what's going on, but in basically every other case, they just break the ability to grep for stuff.

apoelstra · 2023-07-24T13:39:40Z

bitcoin/src/address/error.rs

+
+impl From<base58::Error> for ParseError {
+    fn from(e: base58::Error) -> Self { Self::Base58(e) }
+}


In bd8cdc1:

You also need a From<base58::Error> for Error (and same for all the other for ParseError impls).

If that is the case then I am misunderstanding the point of the Error type. As I understood it the type was for users of the lib to use as a return value for functions they write that call multiple functions from the address module/type. There is no use of it in the codebase on this branch. Or am I missing something?

I agree with @apoelstra. For some reason I thought that .into() would do a transitive call from base58::Error -> ParseError -> Error, but it does not. (and for good reasons!)

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4ee5670908a74cd6fdf709799f8cf2be

I am reconsidering this approach as a whole now. It is not true that base58::Error must go via address::Error. Consider the following code where this leads to surprising behaviour because of our from impl.

pub fn user_fn() -> Result<(), address::Error> { // This will convert to address::Error variant, but this has nothing to do with address. let base58_parsed = bytes::from_base58()?; }

I guess we should only do it for obvious cases where we are clearly defined error heararchy. (which is not the case with base58).

As I understood it the type was for users of the lib to use as a return value for functions they write that call multiple functions from the address module/type.

Right, but say they call multiple functions from the module and also call a function that returns a base58::Error. Now they need to write ?? (which won't compile) or .map_err(Error::from)? (which is ugly).

I guess we should only do it for obvious cases where we are clearly defined error heararchy. (which is not the case with base58).

Ah, yeah, I do agree with this. I had the same thought/fear when working this out in my head.

Right, but say they call multiple functions from the module and also call a function that returns a base58::Error.

I would think in this case the user should be defining a new error type or just using anyhow::Result. Actually now I write this only libs create error types anyway and library writers using rust-bitcoin will be used to creating error types, they do not need this to be in rust-bitcoin. Application devs just use anyhow so they don't need it either.

sanket1729

I am starting to see the issue with this approach. Suddenly, we will have an error type for each API and that can blow up. But I still think that this is the correct thing to do.

Given the extra amount of code that has to be written for this, I completely understand if we don't want to do it this way.

sanket1729 · 2023-07-25T02:52:35Z

bitcoin/src/address/mod.rs

        }
        Ok(Payload::ScriptHash(script.script_hash()))
    }

    /// Create a witness pay to public key payload from a public key
-    pub fn p2wpkh(pk: &PublicKey) -> Result<Payload, Error> {
+    pub fn p2wpkh(pk: &PublicKey) -> Result<Payload, PayloadError> {


I was expecting this to return only UncompressedPubKeyError, not the entire PayloadError.

Wow, you are totally correct and this might not be a good module to test run this new error convention on because loads of the Payload functions need changing. I'm going to convert this to draft and go find an easier module to proof this now convention with.

apoelstra · 2023-07-25T17:44:51Z

Suddenly, we will have an error type for each API and that can blow up. But I still think that this is the correct thing to do.

It won't actually be that much more code vs consolidating, since every error variant has to go somewhere. It'll result in more match self { and } lines mostly.

If we also implement From::from more agressively it will also add log(n) times as many From impls but that's fine, they're short.

I would think in this case the user should be defining a new error type or just using anyhow::Result. Actually now I write this only libs create error types anyway

This is definitely not true. People writing applications use real error types in their utility functons, and being forced to convert to anyhow is super annoying when rust-bitcoin already has a general enough error type to cover your use-case, the authors just didn't implement From so you can't actually use it without contortions.

tcharding · 2023-07-25T21:57:41Z

Righto, I haven't fully internalised the reasoning but I'll run with it for a bit and see if it sinks in. I'll just make a macro for the from impls.

tcharding · 2024-02-05T05:34:20Z

stale

tcharding mentioned this pull request Jul 21, 2023

Use newly released bech32 API #1951

Merged

apoelstra previously approved these changes Jul 21, 2023

View reviewed changes

sanket1729 previously approved these changes Jul 23, 2023

View reviewed changes

tcharding added 4 commits July 24, 2023 08:46

Import error variants locally in Display impl

d6a4edc

In order to make the code slightly more terse with no loss of meaning import all the error variants locally in the `Display` impl for `address::Error`. Refactor only, no logic changes.

Match on *self in error source function

2bbb763

In error `source` function be explicit about what is a reference, match on `*self` and use `ref` when pattern matching.

tcharding dismissed stale reviews from sanket1729 and apoelstra via bd8cdc1 July 23, 2023 23:00

tcharding force-pushed the 07-21-cleanup-address branch from 5f161be to bd8cdc1 Compare July 23, 2023 23:00

tcharding mentioned this pull request Jul 23, 2023

Improvements to error handling rust-bitcoin/rust-secp256k1#633

Open

apoelstra reviewed Jul 24, 2023

View reviewed changes

sanket1729 reviewed Jul 25, 2023

View reviewed changes

tcharding mentioned this pull request Jul 25, 2023

Implement From<Foo> for Descriptor<Pk> using macro rust-bitcoin/rust-miniscript#574

Closed

tcharding marked this pull request as draft July 26, 2023 00:53

tcharding mentioned this pull request Jul 26, 2023

Introduce new error handling convention rust-bitcoin/rust-secp256k1#635

Draft

sanket1729 mentioned this pull request Aug 2, 2023

Sighash method signatures could return no error or io::Error if it wasn't for input index argument #1965

Closed

tcharding mentioned this pull request Oct 3, 2023

Make Payload and AddressEncoding private #1979

Merged

tcharding mentioned this pull request Nov 17, 2023

Create and use NetworkKind #2201

Closed

tcharding closed this Feb 5, 2024

tcharding deleted the 07-21-cleanup-address branch May 22, 2024 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `address` error handling #1950

Improve `address` error handling #1950

tcharding commented Jul 21, 2023 •

edited

Loading

apoelstra left a comment

sanket1729 left a comment

apoelstra commented Jul 23, 2023

sanket1729 commented Jul 23, 2023

apoelstra Jul 24, 2023

apoelstra Jul 24, 2023

tcharding Jul 24, 2023

apoelstra Jul 24, 2023

apoelstra Jul 24, 2023

tcharding Jul 24, 2023

sanket1729 Jul 24, 2023

sanket1729 Jul 24, 2023

apoelstra Jul 24, 2023

apoelstra Jul 24, 2023

tcharding Jul 25, 2023

sanket1729 left a comment

sanket1729 Jul 25, 2023

tcharding Jul 26, 2023

apoelstra commented Jul 25, 2023

tcharding commented Jul 25, 2023

tcharding commented Feb 5, 2024

Improve address error handling #1950

Improve address error handling #1950

Conversation

tcharding commented Jul 21, 2023 • edited Loading

Notes

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

apoelstra commented Jul 23, 2023

sanket1729 commented Jul 23, 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

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

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

apoelstra commented Jul 25, 2023

tcharding commented Jul 25, 2023

tcharding commented Feb 5, 2024

Improve `address` error handling #1950

Improve `address` error handling #1950

tcharding commented Jul 21, 2023 •

edited

Loading