Update our state upon broadcasting a transaction#1010
Merged
darosior merged 7 commits intowizardsardine:masterfrom Mar 22, 2024
Merged
Update our state upon broadcasting a transaction#1010darosior merged 7 commits intowizardsardine:masterfrom
darosior merged 7 commits intowizardsardine:masterfrom
Conversation
darosior
force-pushed
the
2403_poll_on_broadcast
branch
from
b699d7c to
0963c0a
Compare
| // Finally, update our state with the changes from this transaction. | ||
| let (tx, rx) = mpsc::channel(); | ||
| if let Err(e) = self.poller_sender.send(PollerMessage::PollNow(tx)) { | ||
| log::error!("Error requesting update from poller: {}", e); |
Member
There was a problem hiding this comment.
I think it is not useful to make the broadcast command wait for the return of the poller, the broadcast did happen since the bitcoind received the tx to broadcast, and the notification to poll now makes the lapse of time of "unsync" db very very short, but If you prefer it, I am ok with it
Member
Author
There was a problem hiding this comment.
I'm not sure to follow. Not waiting for the completion of the poll introduces a significant race condition (we'll probably return before the poll is complete), which makes the whole point of doing this moot?
darosior
force-pushed
the
2403_poll_on_broadcast
branch
from
8eead79 to
c9b4e3d
Compare
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).
We now provide a way for a user of the daemon to poll for errors in the threads, so aborting the process on a thread panic shouldn't be necessary anymore.
darosior
force-pushed
the
2403_poll_on_broadcast
branch
from
c9b4e3d to
38ff395
Compare
darosior
force-pushed
the
2403_poll_on_broadcast
branch
from
0ffcad8 to
4836dd0
Compare
We'll need to ask the poller thread another thing besides to shut down, so it's cleaner to start using proper messages. The mpsc channel in the std lib was buggy for awhile but since they merged crossbeam and are using this behind the hood now it should be fine starting with Rust 1.67. That's (slightly) higher than our MSRV but it's what we use for releases so that's reasonable. See rust-lang/rust#39364 for details.
This is a temporary hack. We should improve this API.
darosior
force-pushed
the
2403_poll_on_broadcast
branch
from
4836dd0 to
58c71c7
Compare
Member
Author
|
ACK 58c71c7 -- did another pass and Edouard tested this in the GUI. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
DaemonHandleand 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.