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

RPC error message is being swallowed #47

Open
luckysori opened this issue May 5, 2023 · 10 comments
Open

RPC error message is being swallowed #47

luckysori opened this issue May 5, 2023 · 10 comments
Labels
enhancement New feature or request

Comments

@luckysori
Copy link

luckysori commented May 5, 2023

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.

@luckysori luckysori changed the title RPC errors message is being swallowed RPC error message is being swallowed May 5, 2023
@LLFourn
Copy link
Contributor

LLFourn commented May 8, 2023

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.

@vladimirfomene vladimirfomene added the enhancement New feature or request label May 9, 2023
@remix7531
Copy link
Contributor

I am going to work on this

@remix7531
Copy link
Contributor

remix7531 commented May 16, 2023

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
Copy link
Author

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
Copy link
Contributor

You are right.

@remix7531
Copy link
Contributor

remix7531 commented Jun 14, 2023

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
Copy link
Contributor

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 added a commit that referenced this issue Oct 2, 2023
76dc3ae doc: add note for MSRV to README (Vladimir Fomene)
48ee09f test: propagate errors to users (Vladimir Fomene)
a1fadb0 chore: remove ampersand on pinned dependencies line (Vladimir Fomene)
fef4a77 Expose error message for Async client (Vladimir Fomene)
8d974ff Expose error message in addition to status in Blocking client (remix7531)

Pull request description:

  Quick fix for #47

Top commit has no ACKs.

Tree-SHA512: eeea5bebcb1d835015ef735e489eb85280e3cf36fa7e576dbe2d85970fffa5ec11c4065279ac5ba841dbb2f7cb29d64f8b12c6fa8eb2a1a8586b45bdbb2587e5
@vladimirfomene
Copy link
Contributor

vladimirfomene commented Oct 2, 2023

@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
Copy link
Author

@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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants