ACINQ / phoenix

Phoenix is a self-custodial Bitcoin wallet using Lightning to send/receive payments.
https://phoenix.acinq.co
Apache License 2.0
619 stars 93 forks source link

(ios) Use fresh lightning-kmp instance for every background operation #506

Closed robbiehanson closed 5 months ago

robbiehanson commented 5 months ago

A recent ticket has reminded us that IncomingPaymentHandler keeps an in-memory list of pending PaymentParts, and that this can cause issues when the background & foreground processes (on iOS) have a different view of the current state.

Long term, the proper solution is probably to address the in-memory pending variable. However, the exact details are still unknown. And to do it right will take some time.

In the meantime, the following partial-solution has been proposed:


A quick refresher on how the foreground/background stuff works on iOS:

To ensure that an iPhone's battery lasts "all day", Apple places many restrictions on an app's ability to use the CPU when the app is in the background. And the hoop they make us jump through is to use something called a "notification service extension". Here's how it works:

What normally happens is:

However, the last step is not a guarantee. If multiple push notifications arrive, then instead of killing the mini-process, iOS will invoke the didReceive(notification:) method again. It also seems to be the case that iOS may keep the mini-process alive if there's no memory pressure.

And for this reason, it was possible that the lightning-kmp instance was "stale" when processing a push notification. This PR deallocates the lightning-kmp instance after processing each push notification. My primary concern was that this would either leak memory, or cause the mini-process memory footprint to exceed the 24 MB limit. However, in testing, the app maintained a footprint of around 16 MB, even after cycling thru multiple lightning-kmp instances.


Note that this is only a partial solution. In the linked ticket, there was a specific example of the problem with a foreground & background process. This PR fixes that example. But if you simply switched roles (foreground <=> background), the same problem would occur.

t-bast commented 5 months ago

Nice, that makes a lot of sense! FWIW I've spent time studying the IncomingPaymentHandler, and thanks to our encrypted channel backup that is automatically restored whenever a new instance connects, I believe the only change we need to ensure we don't end up partially fulfilling payments because of state inconsistencies is to clean up the pending map on disconnections, which I implemented in https://github.com/ACINQ/lightning-kmp/pull/583

It's worth also trying our best to kill the mini-process at the end of its task, because there may be other in-memory state that could cause issues (even though they shouldn't be as severe, as nothing comes to mind that generated support tickets).

pm47 commented 5 months ago

I cannot really review the swift code, but the description LGTM. I guess we will see if we are not missing anything in PhoenixBusiness.stop()! That's quite a list of things to cancel 😁