`firstUnenhancedHeight` can become stuck and cause enhancement to skip transactions

[str4d]

_processorInfo.value.firstUnenhancedHeight is passed into processNewBlocksInSbSOrder, and then through to runSyncingAndEnhancingOnRange as enhanceStartHeight:

https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/5bca00a1a4092d35201a2fb7eff14374e1839b88/sdk-lib/src/main/java/cash/z/ecc/android/sdk/block/processor/CompactBlockProcessor.kt#L274-L281

https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/5bca00a1a4092d35201a2fb7eff14374e1839b88/sdk-lib/src/main/java/cash/z/ecc/android/sdk/block/processor/CompactBlockProcessor.kt#L467-L474

_processorInfo.value.firstUnenhancedHeight is set as a side-effect of processing the list of suggested scan ranges. Oddly this happens just inside processNewBlocksInSbSOrder, after we've already used _processorInfo.value.firstUnenhancedHeight; I have not figured out where it is initially set.

https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/5bca00a1a4092d35201a2fb7eff14374e1839b88/sdk-lib/src/main/java/cash/z/ecc/android/sdk/block/processor/CompactBlockProcessor.kt#L685-L695

https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/5bca00a1a4092d35201a2fb7eff14374e1839b88/sdk-lib/src/main/java/cash/z/ecc/android/sdk/block/processor/CompactBlockProcessor.kt#L796-L802

In any case, the actual value used for firstUnenhancedHeight is sourced from the SQL database:

https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/5bca00a1a4092d35201a2fb7eff14374e1839b88/sdk-lib/src/main/java/cash/z/ecc/android/sdk/internal/db/derived/AllTransactionView.kt#L163-L179

This query orders v_transactions by mined_height and then selects the first row for which raw IS NULL, which is intended to be the earliest-mined transaction that is unenhanced.

This query has a bug: mined_height is null for unmined transactions, and in SQLite ORDER BY will order nulls first in the natural ASC order, meaning that we get unmined transactions first. Normally this only occurs for transactions created by the wallet, that have raw IS NOT NULL and thus are ignored (even if these transactions expire and are never mined). However, if a tx detected by scanning gets unmined before it is enhanced (due to a race condition), we will select it first and get a null mined_height, and thus a null firstUnenhancedHeight.

As a result, this causes enhancingRange to be initialized to the first block of the first batch being scanned (because these are inclusive-end ranges), such that after the batch is scanned, only the scanned batch gets enhanced:

https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/5bca00a1a4092d35201a2fb7eff14374e1839b88/sdk-lib/src/main/java/cash/z/ecc/android/sdk/block/processor/CompactBlockProcessor.kt#L1449-L1456

https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/5bca00a1a4092d35201a2fb7eff14374e1839b88/sdk-lib/src/main/java/cash/z/ecc/android/sdk/block/processor/CompactBlockProcessor.kt#L1550-L1551

If the unmined transaction is later re-mined, then the condition undoes itself, and firstUnenhancedHeight will in a later sync pick up the correct start position for enhancement. But if it never gets re-mined and expires, then the issue becomes permanent: there will always be a transaction in the view with both mined_height and raw being null, and thus we will only ever enhance transactions that have just been scanned (and will therefore miss enhancing transactions if the app gets killed by the OS between scanning and enhancement).

We should add a test for this behaviour (add two transactions via scanning, reorg to unmine one of them, and then check that firstUnenhancedHeight is set to the mined height of the other transaction), and then fix the view.

[HonzaR]

Comment on:

_processorInfo.value.firstUnenhancedHeight is set as a side-effect of processing the list of suggested scan ranges. Oddly this happens just inside processNewBlocksInSbSOrder, after we've already used _processorInfo.value.firstUnenhancedHeight; I have not figured out where it is initially set.

firstUnenhancedHeight is set in the updateRange function, that is called in runSbSSyncingPreparation. So, basically from the beginning of the sync cycle (i.e. also after the sync is restarted after 10 minutes). And it's continuously updated (in the passed value) during the sync cycle

[HonzaR]

We should add a test for this behaviour (add two transactions via scanning, reorg to unmine one of them, and then check that firstUnenhancedHeight is set to the mined height of the other transaction), and then fix the view.

I agree. Unfortunately, darkside tests were abandoned a long time ago, and we have this tech dept filed. We'd like to prioritize it. https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/issues/1224, https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/issues/361