Bubble up errors from the Bitcoin interface instead of panic'ing there #909
Conversation
I personally believe Result is not cluttering the API. Rust has great ways to write error handling code in contrary of GO where you have if err != nil everywhere, and even with GO I was told to never use
We agree about the user UX/UI, better to show him the issue. I see three kinds of errors:
Yes, especially in the long term. If we have multiple backend support we may have errors that we want to propagate instead of panicking.
I like that we have a final error handling in the
I believe it is fine for now, we can always rework on top if we need fined grained error matching. |
darosior
force-pushed
the
2401_remove_hard_dep_bitcoind
branch
from
a557a2b
to
fbeb693
Compare
If we are making a bad request i don't see how restarting could make a difference. We should stop as soon as possible. (Except maybe in rare mishandled edge cases where the delay might solve the issue.)
I'm not sure. It seems to be very specific while the error could be due to many other things than the connection with Maybe if the poller crash, a modal with the error should be displayed. We could let the user change their |
The caller is now responsible for running the poller in a thread.
… the Bitcoin poller We won't do it here to avoid further increasing a large diff.
For now this just moves the expect()s one layer up.
Same as previous commit but for wallet requests
It's less straightforward for calls which return a JSONRPC error on not found
darosior
force-pushed
the
2401_remove_hard_dep_bitcoind
branch
from
fbeb693
to
72920b2
Compare
Then not a banner, but a full page error (not in ugly orange) with a warning icon and all the details.
In both case, it is good to have an error message to the user and maybe a button "retrieve the logs", that open the file system management window and let user register the logs of the day in a file he chose the location. And I agree that we shouldn't make it feel as if it's necessarily going to fix the issue. |
|
Yes, that's all good points. |
| @@ -208,13 +208,11 @@ fn new_tip(bit: &impl BitcoinInterface, current_tip: &BlockChainTip) -> TipUpdat | |||
| } | |||
|
|
|||
| fn updates( | |||
| db_conn: &mut Box<dyn DatabaseConnection>, | |||
There was a problem hiding this comment.
Commit a8ca850 "poller: hold the same database lock across one update round" is confused. This is not a lock, it's a connection.
This is inspired from the work in wizardsardine#909 (specifically wizardsardine@d8c59e3) to externalize the management of the poller thread. However, there may be only one poller thread. Starting more than one can lead to a crash or potentially to data corruption. Therefore it feels safer to manage it internally. Instead of exposing the management of the poller to the user of the library, we manage both threads inside the `DaemonHandle` data structure and expose a way for a user to check for errors which may have occured in any of the threads. This makes it possible to: 1. Eventually propagate errors from the threads to the user of the daemon (wizardsardine#909); 2. Communicate internally with the poller thread, for instance to trigger a poll immediately (following commits).
This is inspired from the work in wizardsardine#909 (specifically wizardsardine@d8c59e3) to externalize the management of the poller thread. However, there may be only one poller thread. Starting more than one can lead to a crash or potentially to data corruption. Therefore it feels safer to manage it internally. Instead of exposing the management of the poller to the user of the library, we manage both threads inside the `DaemonHandle` data structure and expose a way for a user to check for errors which may have occured in any of the threads. This makes it possible to: 1. Eventually propagate errors from the threads to the user of the daemon (wizardsardine#909); 2. Communicate internally with the poller thread, for instance to trigger a poll immediately (following commits).
58c71c7 lib: gate the RPC server availability on the 'daemon' feature (Antoine Poinsot) b7fde6a commands: update our state immediately after broadcasting a tx (Antoine Poinsot) 1cf42d9 poller: introduce a communication channel with the poller thread (Antoine Poinsot) f6ce85c lib: remove the panic hook. (Antoine Poinsot) b4fe963 lib: encapsulate the handling of both threads (poller and RPC server) (Antoine Poinsot) fd5387f poller: use the same database connection across one update round (Antoine Poinsot) ea6923e poller: make the updating process into its own function. (Antoine Poinsot) Pull request description: Fixes #887. This takes a couple commits from #909 but takes the approach from there in another direction: we don't externalize the poller, since only a single instance must be ran. Instead we properly keep track of the (up to) two threads we manage in the `DaemonHandle` and provide a way for a user of the library to check for errors in any of the threads. This approach allows us to 1) communicate with the poller thread from inside the Liana library/daemon (here we leverage this to tell it to poll) 2) eventually (#909) expose all internal errors from the library to the user instead of panic'ing internally. See the commit messages for details. ACKs for top commit: darosior: ACK 58c71c7 -- did another pass and Edouard tested this in the GUI. Tree-SHA512: 0ab436b2a187f9d124ed8861a47f03bb1e9252cdc4f3b5c4308db07be738c78b2ea3f07dc0a9586e3d5bd34f071a1e2a2569cad30676c9cc004e39260ebb94ca
nondiremanuel
added
the
Nice to have
We assume a
bitcoindis always running beneath Liana. As such, we make a reasonable attempt to stay connected on spurious errors but we panic if we can't reach it for more than a minute. We also panic on unexpected API breaks (if an expected field isn't present or of the right type).The rationale for this is pretty simple:
bitcoind. On abitcoindfailure there is nothing else to than stopping the program.bitcoindmodule (or any other implementation of the Bitcion interface) avoids cluttering the whole API withResult's.That said, we manifested some interest recently in actually bearing the cost of cumbersome
Result's in the API in order to:bitcoind. But it doesn't mean we should straight away crash from under the user who would probably be using the GUI and have no clue what's going on.bitcoindbeing restarted from under us #566? If so there is nicer way to achieve this..)I open this as a draft as it's based on #906 and i still have open questions:
Result<T, Box<dyn std::error::Error>>for the Bitcoin interface trait. I haven't looked too hard into it but if you've got a better suggestion i'd love to hear it.