wallet: fix UnloadWallet thread safety assumptions #30659
Conversation
Releases wallet shared pointers prior to doing the final settings update and prevent GUI races trying to access a wallet that is no longer loaded.
And update function's documentation.
Multiple threads could try to delete the wallet at the same time.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
ismaelsadeeq
left a comment
There was a problem hiding this comment.
Code review ACK 7a73b0b7d5e690235bcefca9b82f01442b37ad5e
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 7a73b0b7d5e690235bcefca9b82f01442b37ad5e. This is a much cleaner bugfix than I suggested in #29073. Moving the unload notification makes the shutdown sequence easier to understand, more efficient, and safe.
7a73b0b to
e37e415
Compare
To better describe the function's behavior. And add wallet name to logprint.
e37e415 to
f550a8e
Compare
|
ACK f550a8e |
Coming from #29073.
Applied ryanofsky suggested changes on #29073 (comment) with few modifications coming from #18338 (comment).
The only point I did not tackle from #18338 (comment) is:
Because it would mean every
CWalletobject would flush data to disk during destruction. Which is not necessary for wallet tool utilities and unit tests.