Page MenuHomePhabricator

rpc: Make unloadwallet wait for complete wallet unload
ClosedPublic

Authored by Fabien on Jan 27 2020, 16:39.

Details

Summary
Currently the unloadwallet RPC is asynchronous, it only signals the
intent to unload the wallet and then returns the response to the client.
The actual unload can happen later and the client has no way to be
notified of that.

This PR makes the unloadwallet RPC synchronous, meaning that it blocks
until the wallet is fully unloaded.

This is expected to fix wallet_multiwallet.py failure with TSAN.
Also include an undefined behavior issue, where the wallet pointer
is used after freed (see https://github.com/bitcoin/bitcoin/issues/16668).

Backport of PR14941 and PR16716.

Depends on D5079.

Test Plan
ninja check check-functional

Run the CI build-tsan configuration several times, check that the
wallet_multiwallet failure has gone.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Jan 27 2020, 16:39
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 27 2020, 16:39
teamcity edited the summary of this revision. (Show Details)Jan 27 2020, 16:39

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those Bitcoin Core PRs have been inserted into the summary for reference.

jasonbcox accepted this revision.Jan 27 2020, 22:44
This revision is now accepted and ready to land.Jan 27 2020, 22:44
deadalnix requested changes to this revision.Jan 27 2020, 22:45
deadalnix added a subscriber: deadalnix.

This introduce several bugs, cf the discussion on the PR.

For instance https://github.com/bitcoin/bitcoin/issues/15310

You need to have an attack plan here.

This revision now requires changes to proceed.Jan 27 2020, 22:45
Fabien planned changes to this revision.Jan 28 2020, 07:33

Thanks for pointing this out, I'll see how to mitigate the issues.

Fabien edited the summary of this revision. (Show Details)Jan 28 2020, 09:31
Fabien updated this revision to Diff 15831.Jan 28 2020, 09:33
Fabien edited the summary of this revision. (Show Details)

Include bugfix from PR16716.
This should be landed after D5085 and D5086 to avoid exposing bugs.

Fabien edited the summary of this revision. (Show Details)Jan 28 2020, 09:36
deadalnix accepted this revision.Jan 30 2020, 01:49
This revision is now accepted and ready to land.Jan 30 2020, 01:49