[#766] Delete Blocks After Scanning

* [#766] Delete blocks after scanning

- Initial commit

* Minor formatting changes

* Add automated test

* Update manual test

* Move delete before enhance

* Improve terminology about deletion

docs/testing/manual_testing/Switch cache to store blocks on disk.md

@@ -47,3 +47,6 @@ Observed result of this manual test should be:
    `wal`) are present. The file names can vary, depending on the current build variant.
 1. Inspect older legacy database folder `/data/data/cash.z.ecc.android.sdk.demoapp.mainnet/databases/`, which 
    also should not contain `cache.sqlite3` or rollback files.
+1. Once the whole sync process is done, verify that the temporary `blocks` directory is removed from the device
+   storage with all its block files, or is empty, as it's automatically created by the new blocks polling mechanism.
+1. Verify also that the `blockmeta.sqlite` still preserves

sdk-lib/src/androidTest/java/cash/z/ecc/android/sdk/internal/storage/block/FileCompactBlockRepositoryTest.kt

@@ -2,6 +2,7 @@ package cash.z.ecc.android.sdk.internal.storage.block
 
 import cash.z.ecc.android.sdk.internal.ext.deleteRecursivelySuspend
 import cash.z.ecc.android.sdk.internal.ext.existsSuspend
+import cash.z.ecc.android.sdk.internal.ext.listSuspend
 import cash.z.ecc.android.sdk.internal.ext.mkdirsSuspend
 import cash.z.ecc.android.sdk.jni.RustBackendWelding
 import cash.z.ecc.android.sdk.model.BlockHeight
@@ -27,13 +28,12 @@ class FileCompactBlockRepositoryTest {
     @OptIn(ExperimentalCoroutinesApi::class)
     @Before
     fun setup() = runTest {
-        val rootDirectory = FilePathFixture.newBlocksDir()
+        val blocksDirectory = FilePathFixture.newBlocksDir()
-        if (rootDirectory.existsSuspend()) {
+        if (blocksDirectory.existsSuspend()) {
-            rootDirectory.deleteRecursivelySuspend()
+            blocksDirectory.deleteRecursivelySuspend()
         }
 
-        val blocksDir = FilePathFixture.newBlocksDir()
+        blocksDirectory.mkdirsSuspend()
-        blocksDir.mkdirsSuspend()
     }
 
     @OptIn(ExperimentalCoroutinesApi::class)
@@ -148,6 +148,40 @@ class FileCompactBlockRepositoryTest {
         assertEquals(blocks.count(), rootBlocksDirectory.list()!!.size)
     }
 
+    @OptIn(ExperimentalCoroutinesApi::class)
+    @Test
+    fun deleteCompactBlockFilesTest() = runTest {
+        val rustBackend = FakeRustBackendFixture().new()
+        val blocksDirectory = FilePathFixture.newBlocksDir()
+        val parentDirectory = blocksDirectory.parentFile!!
+
+        val blockRepository = getMockedFileCompactBlockRepository(rustBackend, blocksDirectory)
+
+        val testedBlocksRange = ListOfCompactBlocksFixture.DEFAULT_FILE_BLOCK_RANGE
+        val blocks = ListOfCompactBlocksFixture.new(testedBlocksRange)
+
+        val persistedBlocksCount = blockRepository.write(blocks)
+
+        parentDirectory.also {
+            assertTrue(it.existsSuspend())
+            assertTrue(it.listSuspend()!!.contains(FilePathFixture.DEFAULT_BLOCKS_DIR_NAME))
+        }
+        blocksDirectory.also {
+            assertTrue(it.existsSuspend())
+            assertEquals(blocks.count(), persistedBlocksCount)
+        }
+
+        blockRepository.deleteCompactBlockFiles()
+
+        parentDirectory.also {
+            assertTrue(it.existsSuspend())
+            assertFalse(it.listSuspend()!!.contains(FilePathFixture.DEFAULT_BLOCKS_DIR_NAME))
+        }
+        blocksDirectory.also { blocksDir ->
+            assertFalse(blocksDir.existsSuspend())
+        }
+    }
+
     @OptIn(ExperimentalCoroutinesApi::class)
     @Test
     fun rewindToTest() = runTest {

sdk-lib/src/androidTest/java/cash/z/ecc/fixture/FilePathFixture.kt

@@ -5,7 +5,7 @@ import java.io.File
 
 object FilePathFixture {
     private val DEFAULT_ROOT_DIR_PATH = DatabasePathFixture.new()
-    private const val DEFAULT_BLOCKS_DIR_NAME = FileCompactBlockRepository.BLOCKS_DOWNLOAD_DIRECTORY
+    internal const val DEFAULT_BLOCKS_DIR_NAME = FileCompactBlockRepository.BLOCKS_DOWNLOAD_DIRECTORY
 
     internal fun newBlocksDir(
         rootDirectoryPath: String = DEFAULT_ROOT_DIR_PATH,

sdk-lib/src/main/java/cash/z/ecc/android/sdk/block/CompactBlockProcessor.kt

@@ -46,7 +46,6 @@ import co.electriccoin.lightwallet.client.fixture.BenchmarkingBlockRangeFixture
 import co.electriccoin.lightwallet.client.model.BlockHeightUnsafe
 import co.electriccoin.lightwallet.client.model.LightWalletEndpointInfoUnsafe
 import co.electriccoin.lightwallet.client.model.Response
-import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.Dispatchers.IO
 import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.MutableStateFlow
@@ -238,6 +237,13 @@ class CompactBlockProcessor internal constructor(
                         }
                         delay(napTime)
                     }
+                    is BlockProcessingResult.FailedDeleteBlocks -> {
+                        Twig.debug {
+                            "Failed to delete temporary blocks files from the device disk. It will be retried on the" +
+                                " next time, while downloading new blocks."
+                        }
+                        // Do nothing. The other phases went correctly.
+                    }
                     is BlockProcessingResult.Error -> {
                         if (consecutiveChainErrors.get() >= RETRIES) {
                             val errorMessage = "ERROR: unable to resolve reorg at height $result after " +
@@ -289,40 +295,74 @@ class CompactBlockProcessor internal constructor(
             setState(State.Scanned(currentInfo.lastScanRange))
             BlockProcessingResult.NoBlocksToProcess
         } else {
-            if (BenchmarkingExt.isBenchmarking()) {
+            val processRanges = ProcessBlocksRanges().apply {
-                // We inject a benchmark test blocks range at this point to process only a restricted range of blocks
+                if (BenchmarkingExt.isBenchmarking()) {
-                // for a more reliable benchmark results.
+                    // We inject a benchmark test blocks range at this point to process only a restricted range of
-                val benchmarkBlockRange = BenchmarkingBlockRangeFixture.new().let {
+                    // blocks for a more reliable benchmark results.
-                    // Convert range of Longs to range of BlockHeights
+                    val benchmarkBlockRange = BenchmarkingBlockRangeFixture.new().let {
-                    BlockHeight.new(ZcashNetwork.Mainnet, it.start)..(
+                        // Convert range of Longs to range of BlockHeights
-                        BlockHeight.new(ZcashNetwork.Mainnet, it.endInclusive)
+                        BlockHeight.new(ZcashNetwork.Mainnet, it.start)..(
-                        )
+                            BlockHeight.new(ZcashNetwork.Mainnet, it.endInclusive)
-                }
+                            )
-                downloadNewBlocks(benchmarkBlockRange)
+                    }
-                val error = validateAndScanNewBlocks(benchmarkBlockRange)
+                    downloadRange = benchmarkBlockRange
-                if (error != BlockProcessingResult.Success) {
+                    scanRange = benchmarkBlockRange
-                    error
                 } else {
-                    enhanceTransactionDetails(benchmarkBlockRange)
+                    downloadRange = currentInfo.lastDownloadRange
-                }
+                    scanRange = currentInfo.lastScanRange
-            } else {
-                downloadNewBlocks(currentInfo.lastDownloadRange)
-                val error = validateAndScanNewBlocks(currentInfo.lastScanRange)
-                if (error != BlockProcessingResult.Success) {
-                    error
-                } else {
-                    currentInfo.lastScanRange?.let { enhanceTransactionDetails(it) }
-                        ?: BlockProcessingResult.NoBlocksToProcess
                 }
             }
+
+            downloadNewBlocks(processRanges.downloadRange)
+
+            val validationResult = validateAndScanNewBlocks(processRanges.scanRange)
+            if (validationResult != BlockProcessingResult.Success) {
+                deleteAllBlockFiles()
+                return@withContext validationResult
+            }
+
+            val deleteBlocksResult = deleteAllBlockFiles()
+
+            val enhanceResult = enhanceTransactionDetails(processRanges.scanRange)
+
+            // Return enhance or delete operations result in case of any failure happened
+            if (enhanceResult != BlockProcessingResult.Success ||
+                enhanceResult != BlockProcessingResult.NoBlocksToProcess
+            ) {
+                return@withContext enhanceResult
+            } else if (deleteBlocksResult != BlockProcessingResult.Success) {
+                return@withContext BlockProcessingResult.FailedDeleteBlocks
+            }
+
+            return@withContext BlockProcessingResult.Success
         }
     }
 
+    private suspend fun deleteAllBlockFiles(): BlockProcessingResult {
+        Twig.debug { "Deleting all temporary blocks files now." }
+        // Now we approach to delete all the temporary blocks files from the device disk. Note that we'd
+        // like to ideally do this continuously while syncing a next group of blocks in the future.
+        return if (downloader.compactBlockRepository.deleteCompactBlockFiles()) {
+            BlockProcessingResult.Success
+        } else {
+            BlockProcessingResult.FailedDeleteBlocks
+        }
+    }
+
+    /**
+     * Helper wrapping class to provide more clarity about ranges used in the processing new blocks mechanism.
+     */
+    private data class ProcessBlocksRanges(
+        var downloadRange: ClosedRange<BlockHeight>? = null,
+        var scanRange: ClosedRange<BlockHeight>? = null
+    )
+
     sealed class BlockProcessingResult {
         object NoBlocksToProcess : BlockProcessingResult()
         object Success : BlockProcessingResult()
         object Reconnecting : BlockProcessingResult()
         object FailedEnhance : BlockProcessingResult()
+        object FailedDeleteBlocks : BlockProcessingResult()
         data class Error(val failedAtHeight: BlockHeight) : BlockProcessingResult()
     }
 
@@ -421,7 +461,11 @@ class CompactBlockProcessor internal constructor(
             result
         }
 
-    private suspend fun enhanceTransactionDetails(lastScanRange: ClosedRange<BlockHeight>): BlockProcessingResult {
+    private suspend fun enhanceTransactionDetails(lastScanRange: ClosedRange<BlockHeight>?): BlockProcessingResult {
+        if (lastScanRange == null) {
+            return BlockProcessingResult.NoBlocksToProcess
+        }
+
         Twig.debug { "Enhancing transaction details for blocks $lastScanRange" }
         setState(Enhancing)
         @Suppress("TooGenericExceptionCaught")
@@ -448,10 +492,10 @@ class CompactBlockProcessor internal constructor(
             BlockProcessingResult.FailedEnhance
         }
     }
-// TODO [#683]: we still need a way to identify those transactions that failed to be enhanced
+    // TODO [#683]: we still need a way to identify those transactions that failed to be enhanced
-// TODO [#683]: https://github.com/zcash/zcash-android-wallet-sdk/issues/683
+    // TODO [#683]: https://github.com/zcash/zcash-android-wallet-sdk/issues/683
 
-    private suspend fun enhance(transaction: TransactionOverview) = withContext(Dispatchers.IO) {
+    private suspend fun enhance(transaction: TransactionOverview) = withContext(IO) {
         transaction.minedHeight?.let { minedHeight ->
             enhanceHelper(transaction.id, transaction.rawId.byteArray, minedHeight)
         }
@@ -480,7 +524,7 @@ class CompactBlockProcessor internal constructor(
     /**
      * Confirm that the wallet data is properly setup for use.
      */
-// Need to refactor this to be less ugly and more testable
+    // Need to refactor this to be less ugly and more testable
     @Suppress("NestedBlockDepth")
     private suspend fun verifySetup() {
         // verify that the data is initialized
@@ -642,7 +686,7 @@ class CompactBlockProcessor internal constructor(
      * @param range the range of blocks to download.
      */
     @VisibleForTesting
-// allow mocks to verify how this is called, rather than the downloader, which is more complex
+    // allow mocks to verify how this is called, rather than the downloader, which is more complex
     @Suppress("MagicNumber")
     internal suspend fun downloadNewBlocks(range: ClosedRange<BlockHeight>?) =
         withContext<Unit>(IO) {
@@ -1048,7 +1092,7 @@ class CompactBlockProcessor internal constructor(
         repository.lastScannedHeight()
 
     // CompactBlockProcessor is the wrong place for this, but it's where all the other APIs that need
-//  access to the RustBackend live. This should be refactored.
+    //  access to the RustBackend live. This should be refactored.
     internal suspend fun createAccount(seed: ByteArray): UnifiedSpendingKey =
         rustBackend.createAccount(seed)
 
@@ -1266,9 +1310,9 @@ class CompactBlockProcessor internal constructor(
         val hash: String?
     )
 
-//
+    //
-// Helper Extensions
+    // Helper Extensions
-//
+    //
 
     /**
      * Log the mutex in great detail just in case we need it for troubleshooting deadlock.

sdk-lib/src/main/java/cash/z/ecc/android/sdk/internal/db/DatabaseCoordinator.kt

@@ -104,7 +104,7 @@ internal class DatabaseCoordinator private constructor(context: Context) {
      * Returns the file of the Data database that would correspond to the given alias
      * and network attributes.
      *
-     * @param network the network associated with the data in the database.
+     * @param network the network associated with the data in the database
      * @param alias the alias to convert into a database path
      *
      * @return the Data database file
@@ -135,7 +135,7 @@ internal class DatabaseCoordinator private constructor(context: Context) {
      * PendingTransactions.db, we choose slightly different approach, but it also leads to
      * original database files migration with additional renaming too.
      *
-     * @param network the network associated with the data in the database.
+     * @param network the network associated with the data in the database
      * @param alias the alias to convert into a database path
      *
      * @return the PendingTransaction database file
@@ -169,7 +169,7 @@ internal class DatabaseCoordinator private constructor(context: Context) {
      * Function for common deletion of Data and Cache database files. It also checks and deletes
      * additional journal and wal files, if they exist.
      *
-     * @param network the network associated with the data in the database.
+     * @param network the network associated with the data in the database
      * @param alias the alias to convert into a database path
      *
      * @return true only if any database deleted, false otherwise
@@ -229,7 +229,7 @@ internal class DatabaseCoordinator private constructor(context: Context) {
      * as the preferred (i.e. newly created) file for subsequent use (and eventually move).
      *
      * @param appContext the application context
-     * @param network the network associated with the data in the database.
+     * @param network the network associated with the data in the database
      * @param alias the alias to convert into a database path
      * @param databaseName the name of the new database file
      */
@@ -425,8 +425,8 @@ internal class DatabaseCoordinator private constructor(context: Context) {
  * instantiation.
  *
  * @param context
- * @param klass The database class.
+ * @param klass the database class
- * @param databaseFile  The database file.
+ * @param databaseFile the database file
  * @return A {@code RoomDatabaseBuilder<T>} which you can use to create the database.
  */
 internal fun <T : RoomDatabase> commonDatabaseBuilder(

sdk-lib/src/main/java/cash/z/ecc/android/sdk/internal/ext/FileExt.kt

@@ -23,6 +23,8 @@ suspend fun File.deleteRecursivelySuspend() = withContext(Dispatchers.IO) { dele
 
 suspend fun File.listFilesSuspend(): Array<File>? = withContext(Dispatchers.IO) { listFiles() }
 
+suspend fun File.listSuspend(): Array<String>? = withContext(Dispatchers.IO) { list() }
+
 suspend fun File.inputStreamSuspend(): FileInputStream = withContext(Dispatchers.IO) { inputStream() }
 
 suspend fun File.createNewFileSuspend() = withContext(Dispatchers.IO) { createNewFile() }

sdk-lib/src/main/java/cash/z/ecc/android/sdk/internal/repository/CompactBlockRepository.kt

@@ -22,6 +22,14 @@ interface CompactBlockRepository {
      */
     suspend fun findCompactBlock(height: BlockHeight): JniBlockMeta?
 
+    /**
+     * This function is supposed to be used once the whole blocks sync process done. It removes all the temporary
+     * block files from the device disk together with theirs parent directory.
+     *
+     * @return true when all block files are deleted, false only if the deletion fails
+     */
+    suspend fun deleteCompactBlockFiles(): Boolean
+
     /**
      * Write the given blocks to this store, which may be anything from an in-memory cache to a DB.
      *

sdk-lib/src/main/java/cash/z/ecc/android/sdk/internal/storage/block/FileCompactBlockRepository.kt

@@ -3,6 +3,7 @@ package cash.z.ecc.android.sdk.internal.storage.block
 import androidx.annotation.VisibleForTesting
 import cash.z.ecc.android.sdk.internal.Twig
 import cash.z.ecc.android.sdk.internal.ext.createNewFileSuspend
+import cash.z.ecc.android.sdk.internal.ext.deleteRecursivelySuspend
 import cash.z.ecc.android.sdk.internal.ext.deleteSuspend
 import cash.z.ecc.android.sdk.internal.ext.existsSuspend
 import cash.z.ecc.android.sdk.internal.ext.mkdirsSuspend
@@ -63,6 +64,16 @@ internal class FileCompactBlockRepository(
 
     override suspend fun rewindTo(height: BlockHeight) = rustBackend.rewindBlockMetadataToHeight(height)
 
+    override suspend fun deleteCompactBlockFiles(): Boolean {
+        Twig.debug { "Removing blocks directory ${blocksDirectory.path} with all its children." }
+
+        if (blocksDirectory.existsSuspend()) {
+            return blocksDirectory.deleteRecursivelySuspend()
+        }
+
+        return true
+    }
+
     companion object {
         /**
          * The name of the directory for downloading blocks