Synchronizer and Verifier continues to run on a wallet that's been closed.

[cculianu]

To reproduce this, you will need 2 wallets.

1. Run electron-cash with -v option to see all console output.
2. Open your regular wallet which is already up-to-date
3. Create a new wallet. Make it watching-only and use this address: bitcoincash:qzcmxq23k9hx0edxjmt0navdukxhd4r23vcdrk893y, which has over 4500 tx's, so it takes a while to sync.
4. Observe your console output -- it's a lot of verifier/synchronizer spam.
5. Close the new wallet you just created.

Expected Result: Syncrhonizer and verifier die, along with the wallet.

Actual Result: The verifier continues to run. The wallet isn't really "closed" and instead the Wallet object still lives in memory because it's being held by the verifier and synchronizer objects. Also, the verifier and synchronizer are still attached to the network object and still continue to run.

Caveats: The current behavior has the plus side benefit of wallets actually continuing to update even when closed, which might be a pleasant surprise to users that re-open the same wallet later. I am not sure what happens if you re-open the wallet though -- do MULTIPLE sets of Synchronizer and Verifiers get started up?! Do they step on each others' toes? This needs to be investigated. (UPDATE: Thankfully, no, it appears the old zombie wallet object is resuscitated from the dead in Daemon.load_wallet()).

Proposed Solution: Object lifetime of the synchronizer and verifier should be clearly defined as existing for as long as the Wallet's main_window is open. Closing the wallet's window should lead to a deallocation of the wallet objects. As an implementation detail: Perhaps the parent wallet object should be held with a weak reference (python weakref) when referred-to by their child verifier and synchronizer objects. And the verifier and synchronizer should "know" to die once the wallet is closed (the weakref goes dead). That's one approach. Either that or the wallet should explicitly end the verifier/synchronizers when the wallet's window is closed by telling them to die.

Interestingly, deleting the wallet forces a "harder" type of wallet close by calling wallet.stop_threads() before killing the wallet, which DOES kill the verifier and synchronizer. Perhaps all wallet closes should do such a hard stop.

Possibly Related: There's currently no address unsubscribe command and I'm not sure how that impacts this. Ideally also the server would be told to unsubscribe from a set of addresses on wallet close. Apparently @kyuupichan is working on adding such a protocol command.

Anyway, I am posting this here because it's a pet peeve and it deserves some attention eventually.

[markblundeberg]

wallet.stop_threads() is supposed to take care of this, I suppose -- does it not get called, then?

[cculianu]

That is correct, it does indeed do more towards that end. And no, it's not called.

I tried naively calling it (or, rather, Daemon.stop_wallet() which calls it) as part of main_window's closeEvent (in the cleanup code) -- and it ends up still leading to the verifier/synchronizer continuing to run. So I suspect more needs to be done to make sure it actually stops -- hence my "we need to figure out what's going on with object lifecycles " tone of this issue.

Perhaps I should have clarified that.

On iOS I do call Daemon.stop_wallet() on wallet close and my synch/verifier still run. They run up until the point the daemon is killed (which happens when the app backgrounds for over 2 minutes).

So it's disconcerting. I can't stop a wallet reliably that's verifying -- we (I?) should figure out if/what the deal is and if object lifecycles are well defined (I suspect they aren't just from quick perusal of the code).

[cculianu]

I'm guessing this "feature" is intentional to keep wallets synched in the background. Closing