bitcoindevkit / rust-esplora-client

Bitcoin Esplora API client library. Supports plaintext, TLS and Onion servers. Blocking or async.
MIT License
28 stars 44 forks source link

RPC error message is being swallowed #47

Open luckysori opened 1 year ago

luckysori commented 1 year ago

When calling something like AsyncClient::broadcast, if we get an RPC error back from the electrum server, we immediately convert the response into a generic one based on the status. This means that we lose the context of the error message, which makes debugging harder as one has to check the logs of the electrum server to figure out exactly what went wrong.

For instance, electrum might report

WARN - HttpError(400, "sendrawtransaction RPC error: {\"code\":-26,\"message\":\"non-mandatory-script-verify-flag (Signature must be zero for failed CHECK(MULTI)SIG operation)\"}")

whilst the client gives us

Esplora client error: Reqwest(reqwest::Error { kind: Status(400), url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(3000), path: "/tx", query: None, fragment: None } })

which is much less useful.

LLFourn commented 1 year ago

I solved this problem for myself once but never managed to upstream it. See: https://github.com/LLFourn/bdk/blob/9e6150717d392538ea8b514de7c6af9c3ea69e1f/src/blockchain/mod.rs#L285

This parsing of RPC error messages could be rolled into its own crate I think.

remix7531 commented 1 year ago

I am going to work on this

remix7531 commented 1 year ago

That's nonsense:

rust-esplora-client is basically a wrapper around the Esplora HTTP API. This is different from an Electrum RPC interface. While an Electrum server provides detailed information about the reasons for an invalid transaction, Esplora merely returns a status code 400, leaving the rust-esplora-client unaware of the specific cause. Only rust-electrum-client is capable of delivering detailed error messages.

luckysori commented 1 year ago

Isn't it the case that we can parse the error response returned by the HTTP API instead of solely considering the response status? I thought the response included a message field describing the error.

remix7531 commented 1 year ago

You are right.

remix7531 commented 1 year ago

I created an example on how to solve this. Using the bitcoin_rpc_errorr crate I created and my fork of rust-esplora-client one can do something link this:

 let builder = esplora_client::Builder::new("https://blockstream.info/testnet/api");
 let client_esplora = builder.build_blocking().unwrap();

 let txid: Txid = Txid::from_str("0a7c8109b0e31bad63be1c1561837b44cb40575d48b03e66a33abd91b085629f").unwrap();
 let tx: Transaction = client_esplora.get_tx_no_opt(&txid).unwrap();

 let result = client_esplora.broadcast(&tx);
 println!("{result:?}");

which prints Err(RPCErrorCode(RPC_VERIFY_ALREADY_IN_CHAIN))

vladimirfomene commented 1 year ago

Hi @remix75 ! Good progress on the error crate work. I will suggest we go for a quick solution for now. Can we have our esplora errors have a cause field with a string of the error coming from electrum server. Can you implement this in a quick PR?

vladimirfomene commented 11 months ago

@luckysori we have merged a quick fix to this issue where we expose the errors we get from the Esplora backend rather than swallowing them.

luckysori commented 11 months ago

@luckysori we have merge a quick fix to this issue where we expose the errors we get from the Esplora backend rather than swallowing them.

Sounds great, thank you!

luckysori commented 6 months ago

@vladimirfomene I finally remembered to update to 269360f and it works great. Thanks!