-
Notifications
You must be signed in to change notification settings - Fork 113
Fix bitcoind shutdown hang #682
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
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
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.
So, the changes to bitcoind behavior here make partially sense to me, as we want to respect the shutdown signal when retrying the initial sync.
However, for the other changes I'm not so sure: a) we generally should attempt for chain syncing to finish gracefully, and picked corresponding timeouts to at least try. b) For Electrum we use a bunch of spawn_blocking internally, which can't be aborted. This means that even though we'd abort polling them, Electrum would still make changes to our LDK objects, which could lead to unexpected behavior if we had already shut down our background processor and hence won't re-persist.
Could you check whether the issue you were seeing would be accounted for by just the changes for the initial bitcoind sync? If not, we'll have to further debug why the timeouts aren't respected.
6d91eab to
2c15475
Compare
|
Yeah did not need the first commit. Fixed up based on your comments for the second commit. |
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.
LGTM
Please rewrite the commit message to better state why this change is necessary and what it does (preferably in 1-2 paragraphs max, nobody will want to read a book of ~inaccurate AI slop in git log).
Previously, the shutdown process could hang when using the bitcoind backend because the syncing process was not always checking if we had received the shutdown signal. Now with any future we call during the sync process we will use a `tokio::select!` to make sure we abort early if we receive a stop signal.
2c15475 to
faebb30
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.
Landing, CI failure should be fixed by #689.
Uh oh!
There was an error while loading. Please reload this page.