search

ACINQ / phoenix

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

Upgrade to Kotlin 2.0.10 #628

Closed dpad85 closed 1 week ago

dpad85 commented 2 months ago

This PR upgrades Kotlin as well as some other libraries to Kotlin 2.0.10. This upgrade is required due to side effects of iOS 18 and Xcode 16.

Issues found

iOS: SKIE does not support kotlin 2.0.20 yet

We have to use Kotlin 2.0.10 instead, although lightning-kmp can keep using 2.0.20 without issues.

kotlinx.serialisation v1.7.2 is not compatible

This is a side effect of SKIE. kotlinx.serialisation v1.7.2 requires Kotlin 2.0.20 (because they use the new kotlin.uuid.Uuid object). In the meantime, we must use kotlinx.serialisation v1.7.1.

iOS: Framework script removed

In the build phases of phoenix-ios-framework we have a script that manually copied the phoenix-shared framework (and DSYM) generated by gradle in phoenix-shared/build into the Xcode DerivedData folder for the phoenix-ios application.

This script now fails with Kotlin 2.0 because the files are already copied during the build process. @robbiehanson It should be investigated further but it seems this script is not needed anymore, and thus must be removed.

iOS: Odd cache error

⚠️ needs investigation

An odd build error occurs in :phoenix-shared:linkDebugFrameworkIosArm64:

/Users/xxx/.gradle/caches/modules-2/files-2.1/app.cash.sqldelight/coroutines-extensions-iosarm64/2.0.2/51f8ddc76a4aa3d45402f46017e7b8405a5f6761/coroutines-extensions is cached (in /Users/xxx/.konan/kotlin-native-prebuilt-macos-x86_64-2.0.10/klib/cache/ios_arm64-gSTATIC-pl/app.cash.sqldelight:sqldelight-coroutines-extensions/unspecified/1v8rzgt0jt9l5.30bg0yuy4wun9/app.cash.sqldelight:sqldelight-coroutines-extensions-cache/bin/libapp.cash.sqldelight:sqldelight-coroutines-extensions-cache.a), but its dependency isn't: /Users/xxx/.konan/kotlin-native-prebuilt-macos-x86_64-2.0.10/klib/common/stdlib

Although it's mentioned in the log, it does not seem linked to the sqldelight coroutine extension. This error is "fixed" by removing the kotlinOptions.freeCompilerArgs += "-Xallocator=std" in phoenix-shared/build.gradle.kts.

A similar report can be found here.

robbiehanson commented 2 months ago

it seems this script is not needed anymore, and thus must be removed.

I can confirm. It seems the bug was fixed in Kotlin, so the workaround is no longer needed. However I did discover a new problem:

Screenshot 2024-09-21 at 3 43 30 PM

It seems the Framework is being copied within itself. So duplicated. Which means 2 copies of a the ~70MB binary. So I added another build script to remove the duplicate (if found).

robbiehanson commented 2 months ago

This error is "fixed" by removing the kotlinOptions.freeCompilerArgs += "-Xallocator=std" in phoenix-shared/build.gradle.kts.

Unfortunately this breaks background payments on iOS: https://github.com/ACINQ/phoenix/pull/324

On iOS, the notification-service-extension (which handles background payments) is limited to 24MB. Any allocation attempt that exceeds that hard limit causes a crash.

The mimalloc algorithm may be theoretically faster, but it does so by over-allocating (in batches) which causes the app's maximum memory to balloon. In testing, it looks better than it was before. However, I can easily hit 22MB on a routine incoming payment. And a second payment will cause a crash.

It's a little better if we set kotlin.native.binary.mimallocUseCompaction=true in gradle.properties. However the memory is hitting 23.XMB, and is way too close for comfort.

When I switch back to the standard memory allocator, we're using less than half the memory of mimalloc. Not only that, but the app is noticeably faster.

The solution (for now) seems to be to disable caches on the native build.

dpad85 commented 1 month ago

Notes:

Also: Commit 736d164 triggers a new build error (in :phoenix-shared:linkDebugFrameworkIosArm64):

e: java.lang.IllegalStateException: IrClassPublicSymbolImpl for kotlin/Function6|null[0] is already bound: 
CLASS IR_EXTERNAL_DECLARATION_STUB INTERFACE name:Function6 modality:ABSTRACT visibility:public superTypes:[kotlin.Function<R of kotlin.Function6>]
...

That error is due to the kotlin.native.cacheKind=none configuration and is seemingly caused by the tor-mobile-kmp library.

Removing that cache configuration triggers another error, x is cached, but its dependency isn't, which is caused by -Xallocator=std.

dpad85 commented 1 week ago

Superseded by https://github.com/ACINQ/phoenix/pull/654.