Order mixclient and syncer shutdown

[jrick]

During clean shutdown, signal the mixing client to shutdown first, waiting for it to finish running, before closing the RPC and SPV syncers.

Updates the mixing module to a version that supports continuing active mixes before terminating the mixing client.

[jrick]

there's another bug i'm seeing in the spv code now. if any of the other errgroup handlers (which take the gctx) error, they will all stop properly, except for the one running the mixing client because its context has not been canceled yet. I think some additional context wrapping is unavoidable here.

[davecgh]

This doesn't look right to me yet.

In the case an error is returned from Run unrelated to the context being canceled, that error would be returned which would cause the group context to be canceled so everything would be fine.

However, when wallet.Run returns nil, walletCtx will never be canceled afaict. Thus, it would just hang here.

[jrick]

It will be cancelled by the goroutine started above this.

[jrick]

https://go.dev/play/p/5VPuDP_Bbtw

[davecgh]

That is assuming there is an error in the goroutines in the group though. There won't necessarily be one.

[davecgh]

https://go.dev/play/p/sYabLXOyzoN

[jrick]

but they all return errors....

have you ran this and seen it hang?

[davecgh]

I haven't seen any issues, but I'm just considering the edge cases. So long as there is a never a case where the grouped goroutines all exit without an error, then the code is fine.

My worry is just basically what I described. Namely, wallet.Run returns nil and then it just sits and waits, effectively until either there is an error in the group or the outer context is canceled. That part is correct.

However, it implies all of the goroutines in the group will be sitting there doing nothing after wallet.Run returns until eventually the outer context is canceled even though they really should be exiting after wallet.Run returns unconditionally so that syncer.Run returns without error versus sitting around in a defunct state (aka wallet.Run is no longer running, but all of the goroutines in the group are).

EDIT: So long as you're fine with that outcome, I'll approve it.

[davecgh]

Ok, after looking through more of the surrounding code, I guess it's just the poor naming of things that misled me and those other goroutines are supposed to remain active until the outer context is canceled regardless of whether or not the "wallet" is running. It'll still be processing p2p messages, fetching filters, etc.

I was completely misled by the naming to believe it was running the entire wallet (as it's named) as opposed to some optional additional background tasks.