Skip to content

Surface bitcoind rpc error information #2057

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 2, 2023
Merged

Surface bitcoind rpc error information #2057

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions lightning-block-sync/src/rpc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,27 @@ use serde_json;

use std::convert::TryFrom;
use std::convert::TryInto;
use std::error::Error;
use std::fmt;
use std::sync::atomic::{AtomicUsize, Ordering};

/// An error returned by the RPC server.
#[derive(Debug)]
pub struct RpcError {
/// The error code.
pub code: i64,
/// The error message.
pub message: String,
}

impl fmt::Display for RpcError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "RPC error {}: {}", self.code, self.message)
}
}

impl Error for RpcError {}

/// A simple RPC client for calling methods using HTTP `POST`.
pub struct RpcClient {
basic_auth: String,
Expand Down Expand Up @@ -69,8 +88,11 @@ impl RpcClient {
let error = &response["error"];
if !error.is_null() {
// TODO: Examine error code for a more precise std::io::ErrorKind.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking into this a little more, I think the idea was that some errors may be considered transient while others are persistent. And this would affect the conversion done here:

rust-lightning/lightning-block-sync/src/convert.rs

Lines 21 to 29 in 8311581

impl From<std::io::Error> for BlockSourceError {
fn from(e: std::io::Error) -> BlockSourceError {
match e.kind() {
std::io::ErrorKind::InvalidData => BlockSourceError::persistent(e),
std::io::ErrorKind::InvalidInput => BlockSourceError::persistent(e),
_ => BlockSourceError::transient(e),
}
}
}

This let's the user know if they should give-up or retry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok, that makes sense. Well I can put the TODO back. Seems like to actually impl the TODO we would need to map the error rpc error codes into buckets for persistent vs transient.

let message = error["message"].as_str().unwrap_or("unknown error");
return Err(std::io::Error::new(std::io::ErrorKind::Other, message));
let rpc_error = RpcError {
code: error["code"].as_i64().unwrap_or(-1),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make RpcError::code an Option given -1 is RPC_MISC_ERROR? Possibly the same for message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I guess we could do that. I'm not sure what case (if ever) it won't have a code and message. I purposefully mapped it to RPC_MISC_ERROR since that's sort of what it is but happy to do it as Option's too if you think that makes more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe it'd be a little cleaner to make RpcError an enum with a struct variant and an "Unknown" variant when there's no code/message 🤷‍♂️

I guess you don't gain much, still need to do a match to find out error code

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it as is. Like you said, it probably shouldn't ever happen and the code is used as a catch-all for when the RPC handling code throws an exception in Core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, Matt had made a comment that we should probably just use a new RpcError enum that wraps the io Error. think it’s worth just doing that?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a much larger change and is orthogonal, IMO, if you're just trying to include an extra piece of data. Changing the error type here means would make it inconsistent with RestCleint, and you'd ultimately need to wrap it again at the BlockSourceError level, anyhow, unless you want to change that as well. But that already is an enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense. I was trying to avoid having to deal with a cascade of changes like that. If we're okay with this solution then it would work for what we needed (access to bitcoind error code)

message: error["message"].as_str().unwrap_or("unknown error").to_string()
};
return Err(std::io::Error::new(std::io::ErrorKind::Other, rpc_error));
}

let result = &mut response["result"];
Expand Down Expand Up @@ -163,7 +185,9 @@ mod tests {
match client.call_method::<u64>("getblock", &[invalid_block_hash]).await {
Err(e) => {
assert_eq!(e.kind(), std::io::ErrorKind::Other);
assert_eq!(e.get_ref().unwrap().to_string(), "invalid parameter");
let rpc_error: Box<RpcError> = e.into_inner().unwrap().downcast().unwrap();
assert_eq!(rpc_error.code, -8);
assert_eq!(rpc_error.message, "invalid parameter");
},
Ok(_) => panic!("Expected error"),
}
Expand Down