Surface bitcoind rpc error information

[johncantrell97]

Currently the RpcClient only surfaces bitcoind's rpc error message but not the error code. The code is useful downstream to more reliably understand what went wrong.

This creates a new struct RpcError that holds the code and message and then provides that as the inner payload to std::io::Error that is already being returned.

[jkczyz]

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

[johncantrell97]

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.

[johncantrell97]

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

[jkczyz]

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.

[johncantrell97]

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?

[jkczyz]

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.

[johncantrell97]

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)

[jkczyz]

Oh, could you update the commit message with context? We should make sure it's there independent of Github.

[jkczyz]

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.

[johncantrell97]

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.

[dunxen]

LGTM with further specifying ErrorKind in a future PR.

[jkczyz]

Note that we currently check that the error contains some message strings in the sample client: https://github.com/lightningdevkit/ldk-sample/blob/e1a87a7e8533bb666ed8002032b124e628da8e12/src/bitcoind_client.rs#L261-L267

This should still work, but we may want to check the error code instead assuming that makes sense.

[TheBlueMatt]

I'm not really sure how we expect users to use this - in one specific case we happen to return a field inside the std::io::Error, so in theory they could downcast and do type-checking, but they have to know that its (maybe) there, which isn't documented anywhere afaict, and its not clear to me that std::io::ErrorKind::Other implies its there, so you can't just use that to check either.