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
chore: suppress tx already submitted error logs #3733
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3733 +/- ##
==========================================
- Coverage 57.74% 57.71% -0.03%
==========================================
Files 192 193 +1
Lines 41808 41976 +168
==========================================
+ Hits 24142 24228 +86
- Misses 17666 17748 +82 ☔ View full report in Codecov by Sentry. |
61739d9
to
fde641c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added logs prior to these changes as comments to the PR instead of comments in the codebase since that seemed too cluttered.
// This is considered a success case, so we don't surface the error log. | ||
// | ||
// https://github.com/bitcoin/bitcoin/blob/daa56f7f665183bcce3df146f143be37f33c123e/src/rpc/protocol.h#L48 | ||
Err(JsonRpc(Rpc(e))) if e.code == -27 => (), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2023-11-26T19:54:48.505711Z INFO fedimint_bitcoind::bitcoincore: Error broadcasting transaction error=JsonRpc(Rpc(RpcError { code: -27, message: "Transaction already in block chain", data: None }))
// TODO: Filter `electrs` errors using codes instead of string when available in | ||
// `electrum-client` | ||
// https://github.com/fedimint/fedimint/issues/3731 | ||
match error.get("message").and_then(|value| value.as_str()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2023-11-26T19:58:26.863259Z INFO fedimint_bitcoind::electrum: Error broadcasting transaction error=Protocol(Object {"code": Number(2), "message": String("Transaction already in block chain")})
// from detecting errors for transactions already submitted. | ||
// TODO: Suppress `esplora-client` already submitted errors when client is | ||
// updated | ||
// https://github.com/fedimint/fedimint/issues/3732 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2023-11-26T19:59:21.563877Z INFO fedimint_bitcoind::esplora: Error broadcasting transaction error=Reqwest(reqwest::Error { kind: Status(400), url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Ipv4(127.0.0.1)), port: Some(16009), path: "/tx", query: None, fragment: None } })
Closes #3676
These changes suppress error logs returned when submitting a transaction already in a block, since this is a success case (see issue thread for more details).
We are unable to address the issue with esplora (#3732) and aren't able to handle errors with electrs as gracefully as I'd like (#3731). The associated followup issues will allow us to handle these errors gracefully across all clients but requires upstream PR merges/releases and for us to update our clients.
Relevant discussion from previous PR #2016 (comment)