[KMP] Memory leak found in AnyAddress

[10gic]

Describe the bug
I found a memory leak in AnyAddress within the Kotlin package of Wallet Core.

dependencies {
    implementation "com.trustwallet:wallet-core-kotlin-android:4.1.7"
}

To Reproduce
The following code can trigger a memory leak:

    println("Test started")
    val publicKeyHex = "044ba28b11af1561042b03b9d0f940446315af11358aa12d798050b3cf76265dab0f48b22ea1fc1f9560c969b966221f2821b746c4e56efaeaeec8689caf5843c9"
    var i = 1
    while (i <= 1_000_000) {
        val publicKey = PublicKey(publicKeyHex.hexToByteArray(), PublicKeyType.SECP256k1Extended)
        val anyAddress = AnyAddress(publicKey, Ethereum, Derivation.Default)   // This leaks memory!
        if (i % 10_000 == 0) {
            println("Test case $i run")
        }
        i++
    }
    println("Test completed")

Expected behavior
No memory leak.

Screenshots
Memory Profiler Screenshot:

Additional context
The above test was conducted on Android. Based on the generated code in directory wallet-core-kotlin, this issue should also exist on iOS.

[satoshiotomakan]

Hi @10gic, good catch! I'll investigate what can be the reason for the memory leak.
Could you please advise if you caught the memory leak while using other WC bindings? For example, PrivateKey or HDWallet?

[10gic]

Hi @satoshiotomakan, I found that this is a common issue. It exists in HDWallet, PrivateKey, PublicKey, and others.

For swift binding (swift/Sources/Generated/AnyAddress.swift), we have deinit code to release memory:

    deinit {
        TWAnyAddressDelete(rawValue)
    }

For wasm binding (wasm/src/generated/AnyAddress.h), we have destructor to release memory:

    ~WasmAnyAddress() {
        TWAnyAddressDelete(instance);
    }

Kotlin don't support destructor, we need to think of other solutions to release memory.

[satoshiotomakan]

Hi @10gic, I see, thank you for the huge amount of work to find the root causes and propose the fix #4031, I'm checking it carefully right now.

[10gic]

Hi @satoshiotomakan, the pr #4031 only try to fix the issue #4030.

Issue #4030 and issue #4021 are two different types of memory leaks.

Issue #4021 hasn't been fixed yet.

[10gic]

Hi @satoshiotomakan, do you have any ideas on how to fix this issue (#4021)?

[satoshiotomakan]

@10gic
I scheduled a call this Friday with Kotlin engineers from another team. One of the ideas is to use native Swift, Java, JS wrappers to free the memory.
Unfortunately, I can't fully dedicate to the memory leaks as I'm currently focused on BTC PSBT, but will do the research and PR review end of this week

[satoshiotomakan]

Hi @10gic, we're trying to investigate, how it can be fixed. Will let you know once I have an update

[10gic]

Hi @10gic, we're trying to investigate, how it can be fixed. Will let you know once I have an update

I'm glad to hear this. Hope there is good news soon.

[10gic]

I performed the same test using two different versions (4.1.7/4.2.0-dev-rc2) and found that the new version(4.2.0-dev-rc2) is much better. However, the memory consumption keeps increasing with the number of PublicKey/AnyAddress.

Test case:

            println("Test started")
            val publicKeyHex =
                @Suppress("ktlint:standard:max-line-length")
                "044ba28b11af1561042b03b9d0f940446315af11358aa12d798050b3cf76265dab0f48b22ea1fc1f9560c969b966221f2821b746c4e56efaeaeec8689caf5843c9"
            var i = 1
            while (i <= 2_000_000) {
                val publicKey = PublicKey(publicKeyHex.hexToByteArray(), PublicKeyType.SECP256k1Extended)
                val anyAddress = AnyAddress(publicKey, Ethereum, Derivation.Default) // This leaks memory
                if (i % 10_000 == 0) {
                    println("Test case $i run")
                }
                i++
            }
            println("Test completed")

Result of version 4.1.7 (The native memory usage keeps increasing and has reached up to 759.8MB):

Result of version 4.2.0-dev-rc2 (The native memory usage keeps increasing and has reached up to 197.4MB):

So, I suspect that the problem may not be completely solved.

[10gic]

Hi @vcoolish, do you have any idea why the memory consumption is still increasing in the test case above after the fix?

[satoshiotomakan]

Hi @10gic, could you please clarify which WalletCore package do you use?
com.trustwallet.wallet-core (maven) or com.trustwallet.wallet-core-kotlin-jvm (maven) or com.trustwallet.wallet-core-kotlin-android (maven)?

[10gic]

Hi @10gic, could you please clarify which WalletCore package do you use? com.trustwallet.wallet-core (maven) or com.trustwallet.wallet-core-kotlin-jvm (maven) or com.trustwallet.wallet-core-kotlin-android (maven)?

Hi @satoshiotomakan, I used wallet-core-kotlin-android in previous tests.

[vcoolish]

Hi @vcoolish, do you have any idea why the memory consumption is still increasing in the test case above after the fix?

could you try adding delay(1) to your while loop?

[10gic]

Hi @vcoolish, do you have any idea why the memory consumption is still increasing in the test case above after the fix?

could you try adding delay(1) to your while loop?

I think adding a delay in the 'while' loop won't affect the test results. Could you please elaborate on your intention?

[vcoolish]

Hi @vcoolish, do you have any idea why the memory consumption is still increasing in the test case above after the fix?

could you try adding delay(1) to your while loop?

I think adding a delay in the 'while' loop won't affect the test results. Could you please elaborate on your intention?

GC doesn't clean the memory instantly and without 1 ms delay the test will throw outofmemory. Having 1ms delay simulates more real usage and allows GC to do the job

[10gic]

Hi @vcoolish, do you have any idea why the memory consumption is still increasing in the test case above after the fix?

could you try adding delay(1) to your while loop?

I think adding a delay in the 'while' loop won't affect the test results. Could you please elaborate on your intention?

GC doesn't clean the memory instantly and without 1 ms delay the test will throw outofmemory. Having 1ms delay simulates more real usage and allows GC to do the job

Do you mean that the GC has no chance to clean up memory without a 1ms delay? After I finish the while loop, I think the GC has enough time to clean up the memory. However, I don't see the memory consumption drop after the while loop finishes. Anyway, I will test it with a 1ms delay in each iteration of the while loop.

[10gic]

Hi @vcoolish, I have retested wallet-core-kotlin-android:4.2.0-dev-rc2 with a 1ms delay in each iteration. Here are the results:

The native memory usage keeps increasing and has reached up to 200.1MB (almost the same as the previous result of 197.4MB). It seems that adding a delay doesn't work.

[vcoolish]

Hi @vcoolish, I have retested wallet-core-kotlin-android:4.2.0-dev-rc2 with a 1ms delay in each iteration. Here are the results:

The native memory usage keeps increasing and has reached up to 200.1MB (almost the same as the previous result of 197.4MB). It seems that adding a delay doesn't work.

ok, thank you for checking. Sergei has an idea what else can be done between C++ and rust we will investigate

[10gic]

Hi @vcoolish, I noticed that the code changes in PR #4070 are not included in version wallet-core-kotlin-android:4.2.0-dev-rc2. Could PR #4070 have resolved this memory leak issue?

[satoshiotomakan]

Hi @10gic, #4070 doesn't resolve the memory leak issue for wallet-core-kotlin-android KMP package, but for wallet-core-android Android native package only.
We'll merge the PR into dev shortly

[satoshiotomakan]

@10gic I debugged the test you provided above, and it turned out that the fix works fine in general. However PhantomReference is acting strange in some stress tests.

For example, in this case ReferenceQueue<PhantomReference> is triggered rare and with small butch of objects to be released. This leads to the fact that we don't release all objects allocated via WalletCore, but only a part of them.
Note that delay for 1ms or even 10ms doesn't help the queue to handle all objects.

        var i = 1
        while (i <= 200_000) {
            val publicKey = PublicKey(pk, PublicKeyType.SECP256k1Extended)
            val anyAddress = AnyAddress(publicKey, Ethereum, Derivation.Default)

            i++
            delay(1)
        }

However, if we move PublicKey generation out of the loop and leave AnyAddress only, all objects will be released without exceptions.

        var i = 1
        val publicKey = PublicKey(pk, PublicKeyType.SECP256k1Extended)
        while (i <= 200_000) {
            val anyAddress = AnyAddress(publicKey, Ethereum, Derivation.Default)

            i++
            // delay(1) // the memory clean up happening even without the delay
        }

[10gic]

@satoshiotomakan Thanks for your debugging. May I ask if there's a plan for when the dev branch can be merged into the master branch?

[satoshiotomakan]

Hi @10gic, we don't want to merge dev into master until we are sure memory leaks fixed.
Unfortunately, our KMP engineers are busy right now to debug and improve GC experience. I'll try to prioritise the work on this issue this week, thank you for the reminder