Improve error message

[Eunovo]

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

[antonilol]

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]

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]

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]

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

[antonilol]

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

[antonilol]

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]

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]

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]

@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]

@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]

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

[Eunovo]

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

[Kixunil]

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.

[Eunovo]

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