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

Improve error message #942

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Eunovo
Copy link

@Eunovo Eunovo commented Oct 17, 2023

This PR improves the error message returned for blockchain.transaction.get when the bitcoin daemon cannot find the transaction.
See #936

@antonilol
Copy link
Contributor

might be worth to define all error codes (in a new enum) to allow for easy future mapping of them to a different or more verbose message (like this pr)

@Eunovo
Copy link
Author

Eunovo commented Oct 18, 2023

might be worth to define all error codes (in a new enum) to allow for easy future mapping of them to a different or more verbose message (like this pr)

I have added all the rpc error codes defined in bitcoin's protocol.h file

@antonilol
Copy link
Contributor

The enum members don't need the i32 fields, they will only every be set to one value
define them like this:
RpcInvalidRequest = -32600,

and the enum definition and parse_code function can be macro generated, to not have 2 places where these codes are mapped to the enum member (but 1) to avoid inconsistencies/desyncs

@Eunovo
Copy link
Author

Eunovo commented Oct 22, 2023

The enum members don't need the i32 fields, they will only every be set to one value define them like this: RpcInvalidRequest = -32600,

and the enum definition and parse_code function can be macro generated, to not have 2 places where these codes are mapped to the enum member (but 1) to avoid inconsistencies/desyncs

@antonilol All done

src/daemon.rs Outdated
Comment on lines 304 to 305

RpcForbiddenBySafeMode = -2,
Copy link
Contributor

Choose a reason for hiding this comment

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

can be removed as this was reserved in a version lower than the minimum supported bitcoind version of electrs

@antonilol
Copy link
Contributor

the documentation of the error codes can also be copied from the header file of bitcoind source
i meant something different with macro generated (macro_rules! ...), but maybe thats just me that i dont like introducing new dependencies. going from an enum variant to a number is as easy as the as keyword (as i16 in this case), proc macros also add (sometimes significant) extra compile time
dont get me wrong, this works like i intended but adds a dependency

@Eunovo
Copy link
Author

Eunovo commented Oct 29, 2023

the documentation of the error codes can also be copied from the header file of bitcoind source

Are you referring to the comments in the protocol.h file?

i meant something different with macro generated (macro_rules! ...), but maybe thats just me that i dont like introducing new dependencies. going from an enum variant to a number is as easy as the as keyword (as i16 in this case), proc macros also add (sometimes significant) extra compile time dont get me wrong, this works like i intended but adds a dependency

I'm not sure I understand what you mean here. Can I generate a from_i16 implementation on the enum with just declarative macros (macro_rules!)? Even if I remove the num_traits dependency, it looks like I'll still have to create a proc_macro and that will require that I add the syn crate to process the inputs to the macro.

@antonilol
Copy link
Contributor

Are you referring to the comments in the protocol.h file?

yes

Can I generate a from_i16 implementation on the enum with just declarative macros (macro_rules!)?

yes

with something along the lines of

macro_rules! define_error_codes {
    ($($name:ident = $value:expr,)*) => {
        /// some doc comments
        // some derives
        #[derive(Debug, Clone, Copy)]
        #[repr(i16)]
        pub enum CoreRPCErrorCode {
            $(
                $name = $value,
            )*
        }

        impl CoreRPCErrorCode {
            pub fn from_error_code(code: i16) -> Option<Self> {
                Some(match code {
                    $(
                        $value => Self::$name,
                    )*
                    _ => return None,
                })
            }
        }
    };
}

define_error_codes! {
    RpcInvalidRequest = -32600,
    RpcMethodNotFound = -32601,
    RpcInvalidParams = -32602,
    RpcInternalError = -32603,
    RpcParseError = -32700,

    ... etc
}

impl CoreRPCErrorCode {
    pub fn to_error_code(self) -> i16 {
        self as i16
    }
}

it can also be made more flexible but i dont think that is needed (at least yet), but you could also take the name and visibility of the enum as metavariables.

@Eunovo Eunovo force-pushed the improve_error_message branch 2 times, most recently from c5dece3 to 2f4e29d Compare October 31, 2023 15:48
@Eunovo
Copy link
Author

Eunovo commented Oct 31, 2023

@antonilol Thanks for your help with the macro_rules! definition. I've changed the implementation to use that and I also added doc comments. Please take a look and let me know if there's anything I need to change.

@antonilol
Copy link
Contributor

@antonilol Thanks for your help with the macro_rules! definition. I've changed the implementation to use that and I also added doc comments. Please take a look and let me know if there's anything I need to change.

this is great!

(test fails on a type mismatch, change the input type of from_error_code from i16 to i32 and should be fine)

@Eunovo Eunovo force-pushed the improve_error_message branch 2 times, most recently from 6f0674f to 5da08b4 Compare October 31, 2023 20:48
@Eunovo
Copy link
Author

Eunovo commented Oct 31, 2023

@antonilol I had to remove the to_error_code fn because it was never used and it caused the tests to fail

@Eunovo
Copy link
Author

Eunovo commented Jan 30, 2024

@romanz Is there anything else I need to address before this can be merged?

@@ -653,7 +664,10 @@ impl Call {
.downcast_ref::<bitcoincore_rpc::Error>()
.and_then(extract_bitcoind_error)
{
Some(e) => error_msg(&self.id, RpcError::DaemonError(e.clone())),
Some(e) => match self.params.parse_rpc_error_code(e.code) {
Some(err) => error_msg(&self.id, err),
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC this is sending the underlying error message to the caller. And while it's not recommended to connect untrusted devices to electrs some people may do that and it'd be better to just log the message and send back "internal server error" to avoid leaking information.

Copy link
Author

Choose a reason for hiding this comment

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

IIUC this is sending the underlying error message to the caller.

It is not. I added self.params.parse_rpc_error_code to allow certain errors to be intercepted and replaced but the underlying error message is not sent to the caller

Eunovo added 2 commits May 7, 2024 09:21
Defines all the rpc error codes specified in bitcoin's protocol.h file
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.

3 participants