feat: optimise backup & restore for performance

[ovitrif]

This PR optimizes backup & restore performance while fixing several bugs.

Description

Performance Improvements

• Switch from BgDispatcher to IoDispatcher for backup operations (more appropriate for I/O-bound work)
• Add immediate running = true flag when scheduling backups to prevent false failure UI during debounce period

Bug Fixes

• Fix bug where restoring wallet B from seed resulted in restoring the backup data of wallet A, by reinitializing vss client during reset & restore
• Fix zero balance shown briefly after restore by loading balance from restored cache before node warms up
• Fix red backup status right after restore by setting both synced and required timestamps
• Fix backup status showing epoch dates by adding scheduleFullBackup() after wallet create
• Stop backup observers before wallet wipe to prevent race conditions and potential crashes
• Fix health check comparison to use >= instead of > for accurate backup status

Refactoring

• Encapsulate backup status logic with isRequired computed property
• Remove redundant @Retention annotations from dispatcher DI qualifiers
• Add nowMillis() global helper
• Consolidate backup status comparison logic throughout codebase
• Update bitkit-core to version 0.1.23

Preview

Roundtrip	Boosts
backupPerfOptimal.mp4	restoreBoost.mp4

QA Notes

1️⃣ Test backup status after restore

1. Create wallet with activity (transactions, tags, widgets, contacts)
2. Settings > Back up… > expect all categories sync
3. Reset & restore wallet
4. Expect: all backup categories immediately show green status, no red flash
5. Expect: no epoch dates (1970) in backup timestamps

2️⃣ Test backup during wallet wipe

1. With active wallet, ensure backups are running/scheduled
2. Settings > Back up… > Reset…
3. Expect: No more "scheduled backup" logs right after data stores were cleaned

3️⃣ Regression - Backup & Restore roundtrip

1. Create new wallet
2. Receive some funds
3. Boost received tx without mining a block
4. Remove suggestion cards, change widgets
5. Settings > Back up… > expect all categories complete successfully
6. Expect: All backup indicators green/success
7. Expect: Health check shows backups OK
8. Settings > Back up…
9. Reset & restore wallet
10. Expect balance instantly populated
11. Expect boost activity restored & accurate
12. Mine a block
13. Expect boost activity confirmed

4️⃣ Test reset wallet A & restore wallet B

1. Restore wallet A
2. Take note of some settings / data that will be backed up
3. Reset & Restore
4. Restore wallet B
5. Quickly continue after rocket screen & next one
6. Expect no brief moment showing previous wallet's balance
7. Expect Different settings & data than the one seen at step 2. (ie. wallet A)

[Copilot]

Pull Request Overview

This PR refactors backup management logic by introducing a computed isRequired property to the BackupItemStatus model and updates the backup infrastructure. The changes improve code maintainability by replacing repetitive synced < required comparisons with a clearer semantic property.

Key changes:

• Added isRequired computed property to BackupItemStatus for cleaner backup status checks
• Refactored BackupRepo to use IoDispatcher for I/O operations and enhanced backup scheduling logic
• Updated WalletViewModel to trigger backup scheduling after wallet creation and simplified backup restore flow

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
gradle/libs.versions.toml	Bumped bitkit-core-android version from 0.1.22 to 0.1.23
app/src/main/java/to/bitkit/models/BackupCategory.kt	Added isRequired computed property to BackupItemStatus and changed default values from 0L to 0
app/src/main/java/to/bitkit/repositories/BackupRepo.kt	Changed from BgDispatcher to IoDispatcher, replaced direct comparisons with isRequired property, enhanced backup scheduling with immediate running flag, added scheduleFullBackup function, extracted nowMillis utility
app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt	Refactored restoreFromBackup to be suspend function, added backup scheduling after wallet creation
app/src/main/java/to/bitkit/viewmodels/BackupsViewModel.kt	Moved disableRetry logic from ViewModel to toUiState extension function
app/src/main/java/to/bitkit/ui/settings/BackupSettingsScreen.kt	Updated all backup status checks to use isRequired property instead of direct comparison
app/src/main/java/to/bitkit/repositories/HealthRepo.kt	Changed sync comparison from > to >= for consistency
app/src/main/java/to/bitkit/repositories/WalletRepo.kt	Added BackupRepo dependency and call to stop observing backups during wallet wipe
app/src/main/java/to/bitkit/data/backup/VssBackupClient.kt	Changed from BgDispatcher to IoDispatcher, simplified storeId retrieval in else branch
app/src/main/java/to/bitkit/di/DispatchersModule.kt	Removed @retention annotations from dispatcher qualifiers
app/src/main/java/to/bitkit/ext/DateTime.kt	Added nowMillis utility function that wraps Clock usage

[Copilot]

Pull Request Overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code Review - PR #458: Optimize backup & restore for performance

Summary

This PR makes significant improvements to the backup and restore system with a focus on performance optimization and bug fixes. Overall, the changes are well-structured and address real issues. Below are my findings organized by category.

---

✅ Strengths

1. Dispatcher Optimization - Switching from BgDispatcher to IoDispatcher for I/O-bound backup operations is the correct choice and should improve performance.

2. Improved User Experience - Setting running = true immediately when scheduling backups (line 257 in BackupRepo.kt) prevents false failure UI during the debounce period - excellent UX improvement.

3. Critical Bug Fixes:

   • VSS client reset during wallet wipe prevents cross-wallet backup contamination
   • Reordering restore to load cache first ensures balance displays correctly
   • Setting both synced and required timestamps after restore prevents red status flash

4. Code Quality - The isRequired computed property encapsulates backup status logic well, making the code more readable and maintainable.

---

🔴 Critical Issues

1. Race Condition in VssBackupClient.reset() (VssBackupClient.kt:65-69)

Severity: HIGH

The reset() method has a critical thread safety issue:

fun reset() {
    isSetup = CompletableDeferred()  // Not thread-safe
    vssStoreIdProvider.clearCache()
    Logger.debug("VSS client reset", context = TAG)
}

Problem: reset() is not synchronized, but isSetup can be accessed concurrently by:

• setup() (line 29) - writes to isSetup
• putObject() and getObject() (lines 75, 90) - call isSetup.await()
• Multiple backup operations running in parallel

Impact: If reset() is called while backup operations are in flight, those operations may await on the old CompletableDeferred that will never complete, causing them to hang indefinitely.

Recommendation:

fun reset() {
    synchronized(this) {
        val oldSetup = isSetup
        isSetup = CompletableDeferred()
        oldSetup.completeExceptionally(CancellationException("VSS client reset"))
    }
    vssStoreIdProvider.clearCache()
    Logger.debug("VSS client reset", context = TAG)
}

2. Missing Parameterless clearCache() in VssStoreIdProvider (VssBackupClient.kt:67)

Severity: MEDIUM

fun reset() {
    isSetup = CompletableDeferred()
    vssStoreIdProvider.clearCache()  // ⚠️ This doesn't exist\!
    Logger.debug("VSS client reset", context = TAG)
}

Problem: VssStoreIdProvider only has clearCache(walletIndex: Int = 0), not a parameterless clearCache(). The call at line 67 will clear only wallet index 0.

Impact: In a multi-wallet scenario, resetting wallet 1 won't clear its VSS cache properly, potentially causing the bug this PR aims to fix.

Recommendation: Either:

1. Add fun clearCache() { cacheMap.clear() } to VssStoreIdProvider, OR
2. Change the call to vssStoreIdProvider.clearCache(0) and document the limitation

---

⚠️ Medium Priority Issues

3. Order of Operations in WalletRepo.wipeWallet() (WalletRepo.kt:240-261)

Severity: MEDIUM

suspend fun wipeWallet(walletIndex: Int = 0): Result<Unit> = withContext(bgDispatcher) {
    backupRepo.stopObservingBackups()  // Good\!
    
    try {
        _walletState.update { WalletState() }      // Clears UI state
        _balanceState.update { BalanceState() }    // Clears UI state
        
        keychain.wipe()         // ⚠️ Destroys mnemonic
        backupRepo.reset()      // Needs mnemonic to clear VSS cache\!
        // ...

Problem: keychain.wipe() is called before backupRepo.reset(). If reset() or VssStoreIdProvider.clearCache() needs the mnemonic (which getVssStoreId() does), this will fail.

Current Behavior: Looking at the code, clearCache() only removes from the in-memory map, so it doesn't actually need the mnemonic. However, this is fragile and could break if the implementation changes.

Recommendation: Move backupRepo.reset() before keychain.wipe() to make the code more robust:

backupRepo.reset()      // Clear backup state first
keychain.wipe()         // Then wipe sensitive data

4. Missing Error Propagation in scheduleFullBackup() (BackupRepo.kt:458-465)

fun scheduleFullBackup() {
    Logger.debug("Scheduling backups for all categories", context = TAG)
    BackupCategory.entries
        .filter { it \!= BackupCategory.LIGHTNING_CONNECTIONS }
        .forEach {
            scheduleBackup(it)  // Launches async jobs, no error handling
        }
}

Problem: If backup scheduling fails, errors are silently lost. The caller in WalletViewModel.createWallet() has no way to know if backups were successfully scheduled.

Recommendation: Consider returning a result or at least logging if any category fails to schedule.

---

💡 Code Quality Suggestions

5. Inconsistent Removal of @retention Annotations (DispatchersModule.kt)

The PR removes @Retention(AnnotationRetention.BINARY) from dispatcher qualifiers. While this is technically correct (BINARY is the default), the change seems unrelated to the PR's goals.

Recommendation: Document why these were removed in the PR description, or consider keeping them for explicit clarity.

6. Health Check Comparison Change (HealthRepo.kt:129)

-fun isSyncOk(synced: Long, required: Long) = synced > required || ...
+fun isSyncOk(synced: Long, required: Long) = synced >= required || ...

Good catch! This fixes a subtle off-by-one error. However, consider if this could cause issues where synced == required == 0 (initial state) is now considered "OK". This might be intentional, but worth validating.

7. Test Coverage Gap

The test file changes only add loadFromCache() calls to existing tests but don't add new tests for:

• The new reset() method in VssBackupClient
• The scheduleFullBackup() method
• The race condition scenarios during wallet wipe
• The backup restore ordering changes

Recommendation: Add integration tests covering the critical bug fixes, especially the cross-wallet restoration scenario.

---

🔒 Security Considerations

✅ No security issues found. The PR properly:

• Resets PIN data before backup (resetPin())
• Cleans up sensitive data during wallet wipe
• Maintains proper isolation between wallets (once issues Android - Project setup #1 and POC wake2pay from notification #2 are fixed)

---

🚀 Performance Considerations

✅ Performance improvements are well-implemented:

1. IoDispatcher is appropriate for I/O-bound work
2. Debouncing prevents excessive backups (5s delay)
3. Immediate running = true prevents unnecessary UI updates
4. Cache restoration before node warmup improves perceived performance

Minor concern: The nowMillis() helper adds an extra parameter with a default value. This is fine for testing but adds a tiny bit of overhead. Consider if direct Clock.System.now().toEpochMilliseconds() calls would be simpler in production code.

---

📋 Testing Recommendations

Based on the QA notes, ensure testing covers:

1. ✅ Backup status after restore (covered)
2. ✅ No logs during wipe (covered)
3. ✅ Roundtrip restore (covered)
4. ⚠️ Missing: Reset wallet A, restore wallet B with rapid switching - test for race conditions
5. ⚠️ Missing: Multiple rapid backup triggers during debounce period
6. ⚠️ Missing: Backup failure during wallet wipe

---

Summary & Recommendation

Overall Assessment: This is a valuable PR that fixes real bugs and improves performance. However, the critical race condition in VssBackupClient.reset() and the missing clearCache() overload must be addressed before merging.

Recommended Actions:

1. MUST FIX: Issue Android - Project setup #1 (race condition in reset())
2. MUST FIX: Issue POC wake2pay from notification #2 (clearCache() method signature)
3. SHOULD FIX: Issue Android - Migrate to ldk-node #3 (operation order in wipeWallet())
4. CONSIDER: Issues Migrate to ldk-node #4-7 (code quality improvements)

Once the critical issues are resolved, this PR will be ready to merge. Great work on identifying and fixing these backup/restore issues!

---

Review generated with Claude Code

[ovitrif]

Fixed all relevant points from Claude's review 🚀

[claude]

Code Review - PR #458: Optimize backup & restore for performance

Summary

This PR delivers solid performance improvements and critical bug fixes for backup and restore operations. The changes are well-structured with good attention to race conditions and edge cases. Overall quality is high with a few areas for consideration.

---

✅ Strengths

1. Excellent Bug Fixes

   • Fixed critical cross-wallet restore bug by reinitializing VSS client (VssBackupClient.kt:65-72)
   • Addressed race condition by stopping backup observers before wallet wipe (WalletRepo.kt:242)
   • Fixed backup status comparison logic throughout (> → >= in HealthRepo.kt:129)

2. Smart Performance Optimizations

   • Switching from BgDispatcher to IoDispatcher for I/O-bound backup operations is the correct choice
   • Setting running = true immediately during scheduling prevents false failure UI (BackupRepo.kt:259-262)
   • Restoring cache first before node warmup improves perceived performance (BackupRepo.kt:413-420)

3. Clean Refactoring

   • isRequired computed property nicely encapsulates backup logic (BackupCategory.kt:63)
   • nowMillis() global helper promotes consistency
   • Removed redundant @Retention annotations (DI qualifiers default to RUNTIME)

---

🔍 Potential Issues

1. Thread Safety Concern - VssBackupClient (Medium Priority)

Location: VssBackupClient.kt:65-72

fun reset() {
    synchronized(this) {
        isSetup.cancel()
        isSetup = CompletableDeferred()
    }
    vssStoreIdProvider.clearCache()  // ⚠️ Outside synchronized block
    Logger.debug("VSS client reset", context = TAG)
}

Issue: clearCache() is called outside the synchronized block. If another thread calls setup() between when the synchronized block exits and clearCache() completes, the cache could be in an inconsistent state.

Recommendation: Move clearCache() inside the synchronized block, or verify that VssStoreIdProvider.clearCache() is thread-safe.

---

2. Race Condition in Backup Scheduling (Low Priority)

Location: BackupRepo.kt:258-276

The double-check pattern has a small race window:

1. Thread A: Sets running = true (line 260-262)
2. Thread B: Data changes, markBackupRequired() updates required timestamp
3. Thread A: Delay completes, checks status (line 267)
4. Thread A: Sees isRequired = true, calls triggerBackup()
5. triggerBackup() sets running = true again (line 320) ← redundant

Impact: Minor - just redundant state updates.

Recommendation: Consider checking status.running in the double-check to avoid redundant triggerBackup calls:

if (status.isRequired && !status.running && !isRestoring) {
    triggerBackup(category)
}

---

3. Inconsistent State Updates Order (Low Priority)

Location: WalletRepo.kt:240-260

suspend fun wipeWallet(walletIndex: Int = 0): Result<Unit> = withContext(bgDispatcher) {
    try {
        backupRepo.reset()  // Stops observers
        
        _walletState.update { WalletState() }  // Clears state
        _balanceState.update { BalanceState() }
        
        keychain.wipe()  // Then wipes sensitive data
        db.clearAllTables()
        // ...

Observation: State is cleared before keychain/DB. If someone observes walletState between state clear and keychain wipe, they might see an inconsistent view.

Recommendation: Consider clearing state after wiping persistent storage, or document why this order is intentional.

---

🚀 Performance Considerations

1. Dispatcher Usage ✅

   • IoDispatcher for backup I/O operations: Correct
   • BgDispatcher for WalletRepo: Appropriate for CPU-bound work

2. Backup Debounce ✅

   • 5-second debounce prevents excessive backup calls during rapid changes
   • Immediate running = true prevents UI flicker

3. Cache-First Restore ✅

   • Loading balance from cache before node warmup (onCacheRestored callback) is a smart UX optimization
   • Prevents "zero balance flash"

Minor Suggestion: Consider adding metrics/logging for backup performance to track improvements in production.

---

🔒 Security Considerations

1. VSS Client Reset ✅

   • Properly clears cache and reinitializes on reset
   • Prevents cross-wallet data leakage

2. Mnemonic Handling ✅

   • Mnemonic loaded from keychain only when needed
   • Not stored in VSS client beyond setup

3. PIN Reset During Restore ✅

   • resetPin() called on settings restore (BackupRepo.kt:423) prevents PIN conflicts

Note: The synchronized(this) in VssBackupClient.reset() protects against concurrent reset/setup calls, but ensure vssStoreIdProvider.clearCache() doesn't introduce timing issues (see issue #1).

---

🧪 Test Coverage

Updated Tests:

• WalletRepoTest.kt updated with loadFromCache() calls: Good coverage of the change
• 7 tests modified to account for explicit cache loading

Missing Tests (Recommendations):

1. Test BackupRepo.reset() → VssBackupClient.reset() flow
2. Test concurrent scheduleBackup() calls for same category
3. Test scheduleFullBackup() called after wallet creation
4. Test restore with onCacheRestored callback execution order
5. Test backup status after restore (verifying synced = required fix)

Test Quality: Existing tests properly updated. Consider adding integration tests for the backup/restore round-trip scenarios described in QA notes.

---

📝 Code Quality

1. Documentation ✅

   • BackupItemStatus properties well-documented with KDoc
   • Computed property isRequired is self-documenting

2. Error Handling ✅

   • Proper use of Result types
   • Errors logged with context

3. Kotlin Idioms ✅

   • Good use of scope functions, flows, and coroutines
   • Computed property over function for isRequired

4. Constants ✅

   • MINUTE_IN_MS constant added for clarity (BackupRepo.kt:498)

Minor Suggestion: Consider extracting magic numbers to named constants:

• Line 77 in VssBackupClient.kt: isSetup.await() timeout (implicit 30s from line 31)

---

🎯 Recommendations

High Priority

1. ✅ Verify thread safety of VssStoreIdProvider.clearCache() or move inside synchronized block

Medium Priority

2. ✅ Add integration tests for backup/restore round-trip
3. ✅ Document the intentional state-clearing order in wipeWallet() if that's by design

Low Priority

4. Add backup performance metrics
5. Consider the redundant running = true in the double-check pattern

---

📊 Overall Assessment

Rating: Approve with minor suggestions ⭐⭐⭐⭐½

This is a high-quality PR that addresses real bugs and delivers meaningful performance improvements. The code is well-structured, the dispatcher change is correct, and the race condition fixes are important. The few concerns raised are mostly minor and could be addressed in follow-up improvements.

The QA test plan is thorough and covers the critical scenarios. Great work on this optimization!

---

Files Reviewed

• ✅ VssBackupClient.kt - Good reset logic, minor thread safety question
• ✅ VssStoreIdProvider.kt - Clean API simplification
• ✅ BackupRepo.kt - Excellent refactoring, smart optimizations
• ✅ WalletRepo.kt - Proper integration with backup reset
• ✅ HealthRepo.kt - Critical comparison fix
• ✅ WalletViewModel.kt - Clean integration of cache restore callback
• ✅ BackupCategory.kt - Nice computed property
• ✅ Test updates - Properly updated for new behavior

[ben-kaufman]

utACK, code looks good to me

About tags, is android not using bitkit-core for saving tags?

[ovitrif]

utACK, code looks good to me

About tags, is android not using bitkit-core for saving tags?

Can repeat here what we discussed so it's out in the open:

Android uses bitkit-core tags, but we also have tags for receive, which need to be persisted standalone and attached to the activity when someone pays that bip21.

Receive tags are persisted until a tx actually comes in, then they're removed.