wallet: call SyncWithValidationInterfaceQueue after disconnecting chain notifications#34642
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
563ec53 to
c010fe8
Compare
There was a problem hiding this comment.
The current approach has a downside during IBD, any wallet unload during IBD could stall for hours because notification will keep coming. This wouldn't be much a deal for an RPC thread but for the GUI, this would totally freeze the app.
What we can do instead, is to unregister the wallet from the validation interface and submit a blocking task to the interface, so that we only wait up until the pending notifications gets cleared, and nothing else. Then we continue shutdown etc.
I implemented it here furszy@bf1abcd. All yours (haven't tested it).
If we want more concurrency in the future, instead of waiting for the queue to drain, we can submit the remaining unload steps into the validation interface so they are executed only after the pending tasks get processed. I didn't do it because it would require further changes that don't think they are acceptable for the release freeze period.
Isn't that what |
ha yeah.. nvm. Bad brain, dunno why I was completely convinced it was waiting until the queue was empty instead 🤷🏼♂️. |
stickies-v
left a comment
There was a problem hiding this comment.
crACK c010fe8
PR description needs to be updated after the changed approach.
Waiting for the chain notifications to be processed makes the wallet shutdown more correct, but also slower. In most cases this seems harmless, but I did not test if there are edge cases (e.g. IBD, multiple wallets, a somehow clogged up/slow validation queue) where this can be problematic.
…rfaceQueue() Co-Authored-By: stickies-v <stickies-v@protonmail.com>
When unloading a wallet, there may be unexecuted callbacks in the validation interface queue that can still execute after we have completed all of the other wallet shutdown tasks. Instead of letting these run in the background, once the notifications are disconnected, wait for the queue to drain before continuing with wallet shutdown.
c010fe8 to
98e8af4
Compare
|
utACK 98e8af4 |
There was a problem hiding this comment.
crACK 98e8af4
This makes the wallet unloading / shutdown operation more robust albeit slower (specially in case of multiple wallets loaded), which I don't mind. I'd prefer consistent & slow(er) operations over brittle & fast(er) ones.
I too feel that the waiting portion can be encapsulated within the disconnect function, but can be addressed later.
When the wallet disconnects chain notifications, it is expecting no further notifications to execute, but this is not the case. This results in test failures such as in #34354. Instead of disconnecting the notifications and continuing shutdown, we should wait for the validation interface queue to be drained before the rest of wallet shutdown. This is achieved by adding an
interfaces::Chain::waitForNotifications()function which callsSyncWithValidationInterfaceQueue().Fixes #34354