Rework and simplify electrum client, miniwallet and watcher

[sstone]

CompletableDeferred<> is used to handle multiple concurrent calls and provide suspend methods which are much easier to use. For example, to retrieve a transaction you can simply call client.getTx(txid) and you can chain such calls in your message handling loops: this eliminate the need to send messages to yourself and create intermediate states that are just waiting for a response from the electrum client.

I've also remove the NotifyTxEvent event which I believe is not used anywhere.

[robbiehanson]

This looks like a nice cleanup!

I tested it in branch test-pr-411 and I ran into one issue.

The problem we're having on the Phoenix side is that ElectrumClient's connect()/disconnect() functions have been replaced with start()/stop(), but they don't do the same thing. From what I'm reading, start() is expected to be invoked once, and has it's own reconnection logic. And stop(), if invoked, prevents ElectrumClient from working again.

The issue is that, Phoenix has a better understanding of the network state than lightning-kmp. Specifically, it has a NetworkMonitor, which knows whether or not the device has an Intenet connection. This is obviously OS-specific (for example, here's the iOS version). And since Phoenix receives notifications from the OS about the network state, it can better determine when to attempt a re-connect.

And there are other complications. On iOS, when the app is sent into the background, the OS will kill any open network connections (after a brief delay). The Phoenix layer knows when the app is going to be sent to the background, and prefers to perform a clean disconnect on its own.

Unless of course an outgoing payment is in progress when the user background's the app. In which case Phoenix asks the OS for permission to perform a "long-lived background task". If granted, it then keeps the connection(s) open until the payment completes, or we run out of granted time.

There's also the WatchTower task, which runs every few days while the app is in the background...

Long story short, there are a bunch of complications surrounding connect/disconnect/reconnect. All this logic is implemented in AppConnectionsDaemon. (See AppConnectionsDaemon.TrafficControl) So I think it makes sense to remove the auto-reconnect logic from within ElectrumClient, and allow the external layer (e.g. Phoenix) to handle it instead. (This is also how Peer works - it outsources the reconnection logic.)

[sstone]

Sorry I missed that, I'll restore connect()/disconnect() to their original behaviour.

[sstone]

Rebased and updated to be 100% compatible with the current client and watcher (tested on Android but not on iOS).

[dpad85]

Tested on Phoenix Android and iOS, it works fine. There are issues with Tor in edge cases but I think they should be investigated separately.

[pm47]

I have added a few nits on branch electrum-client-rework-pm.

Why is this PR draft, what's left to work on?

[sstone]

I have added a few nits on branch electrum-client-rework-pm.

Thanks, I used these changes and also updated ElectrumClient's notification flow to a Flow<ElectrumSubscriptionResponse>

Why is this PR draft, what's left to work on?

The only thing that I'm unsure of is the handling of NotifyUpToDateEvent but otherwise there is nothing missing that I know of or was reported so far, I'll update the PR status

[pm47]

Code LGTM, but this requires an ACK from the Phoenix team.

[dpad85]

LGTM

[sstone]

LGTM

@dpad85 with or without ElectrumWatcher.openUpToDateFlow (see #411 (comment)) ? Or do you want to add that notification back in another PR ?

[dpad85]

So the removal of openUpToDateFlow should be reverted as it is needed by the background watcher in the iOS app. This is ad hoc code that should be cleaned up, but it's outside the scope of this PR.

[sstone]

I've restored openUpToDateFlow and simplified the watcher a bit (there is now one flow for watch events, one flow for up-to-date events)