feat: activity boosted tx parents by ovitrif · Pull Request #460 · synonymdev/bitkit-android

ovitrif · 2025-11-06T23:57:20Z

ROADMAP: Transaction Details

This PR adds boosted txids tracking and displays parent transaction IDs in the activity details, matching iOS PR #199.

Description

Added boostTxIds to PendingBoostActivity to store parent transaction IDs
CPFP: Append child transaction ID to parent's boostTxIds when boosting
RBF:
- Track parent chain (existing boostTxIds + current txId) in PendingBoostActivity.
- Store parent txIds when creating replacement
- retrieve and apply when replacement syncs
UI: Display boosted transaction IDs in ActivityExploreScreen

Preview

CPFP (received)	RBF (sent)
TODO	TODO

QA Notes

1️⃣ RBF Test

Send onchain
Boost transaction
Activity Item > Explore
Expect list containing txid of previously boosted tx under "Boosted Transaction…" header

1️⃣ CPFP Test

Same but start with RECEIVE onchain.

Copilot

Pull Request Overview

This PR enhances boost transaction tracking by properly recording boost transaction IDs for both RBF (Replace-By-Fee) and CPFP (Child Pays For Parent) scenarios. The implementation ensures that parent-child transaction relationships are maintained and properly displayed in the activity detail screen.

Added boostTxIds field to PendingBoostActivity data class
Updated boost logic to track and persist transaction IDs through the boost process
Added UI component to display boosted transaction IDs in activity details

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
`BoostTransactionViewModel.kt`	Updated CPFP and cachePendingBoostActivity logic to properly track boost transaction IDs
`ActivityDetailScreen.kt`	Added UI section to display boosted transaction IDs with appropriate labels for RBF and CPFP
`ActivityRepo.kt`	Updated pending boost activities handling to apply boostTxIds during sync
`PendingBoostActivity.kt`	Added boostTxIds field with default empty list for backward compatibility

… explore

claude · 2025-11-07T14:46:38Z

PR Review: Activity Boosted TX Parents

Summary

This PR adds tracking and display of parent transaction IDs for boosted transactions (both CPFP and RBF), aligning with iOS implementation. Overall, the implementation is solid with good documentation and careful handling of edge cases. Below are my findings across code quality, potential issues, performance, security, and test coverage.

✅ Strengths

Excellent Documentation: The code includes comprehensive comments explaining RBF vs CPFP behavior, particularly in BoostTransactionViewModel.kt:419-430 and ActivityRepo.kt:437-447.
Proper State Management: The use of PendingBoostActivity to cache boost data for retry on sync failure is a robust pattern.
Good Separation of Concerns: Clear distinction between immediate CPFP updates and deferred RBF replacements.
Consistent Code Style: Follows Kotlin best practices with trailing commas, proper nullability handling, and immutable data structures.

🔴 Critical Issues

1. Race Condition in BackupRepo (High Priority)

Location: BackupRepo.kt:256-274

backupJobs[category] = scope.launch {
// Set running immediately to prevent UI showing failure during debounce
cacheStore.updateBackupStatus(category) {
it.copy(running =true)
}
delay(BACKUP_DEBOUNCE)
// Double-check if backup is still neededval status = cacheStore.backupStatuses.first()[category] ?:BackupItemStatus()
if (status.isRequired &&!isRestoring) {
triggerBackup(category)
} else {
// Backup no longer needed, reset running flag
cacheStore.updateBackupStatus(category) {
it.copy(running =false)
}
}
}

Issue: Not thread-safe. Multiple calls to scheduleBackup() for the same category could overwrite backupJobs[category] without canceling the previous job, leading to:

Multiple concurrent backups for the same category
Orphaned coroutines that never reset running = false
UI showing incorrect backup status

Recommendation:

backupJobs[category]?.cancel() // Cancel existing job
backupJobs[category] = scope.launch {
// ... rest of implementation
}

2. Synchronization Issue in VssBackupClient.reset()

Location: VssBackupClient.kt:65-72

funreset() {
synchronized(this) {
isSetup.cancel()
isSetup =CompletableDeferred()
}
vssStoreIdProvider.clearCache()
Logger.debug("VSS client reset", context =TAG)
}

Issue: vssStoreIdProvider.clearCache() is called outside the synchronized block, which could lead to:

Cache being cleared while setup() is reading from it
Inconsistent state between isSetup and cache

Recommendation: Move vssStoreIdProvider.clearCache() inside the synchronized block.

🟡 Medium Priority Issues

3. Missing Null Safety Check

Location: BoostTransactionViewModel.kt:422-430

val currentActivity = activity?.v1
val boostTxIds =if (activityToDelete !=null&& currentActivity !=null) {
currentActivity.boostTxIds + currentActivity.txId
} else {
emptyList()
}

Issue: activityToDelete != null is checked but currentActivity could still be null for RBF scenarios, leading to an empty boostTxIds list when it should contain the parent chain.

Recommendation: Add explicit error handling or logging when activityToDelete is non-null but currentActivity is null, as this indicates a bug.

4. Backup Order Changed Without Migration

Location: BackupRepo.kt:402-458

The restore order was changed from:

// Old orderSETTINGS → WALLET → ACTIVITY → METADATA → BLOCKTANK

to:

// New order METADATA → SETTINGS → WALLET → ACTIVITY → BLOCKTANK

Issue: METADATA is restored first to populate caches early via onCacheRestored() callback, but:

No migration logic handles cases where users have existing backups in the old format
If METADATA restore fails, subsequent restores may operate with stale cache data
The callback onCacheRestored() is not used in this PR

Recommendation:

Document why METADATA must be restored first
Add error handling if METADATA restore fails (should it abort the entire restore?)
Consider if partial restore states are acceptable

5. Potential Memory Leak in WalletRepo

Location: WalletRepo.kt:240-260

suspendfunwipeWallet(walletIndex:Int = 0): Result<Unit> = withContext(bgDispatcher) {
try {
backupRepo.reset() // Cancels all coroutines in BackupRepo_walletState.update { WalletState() }
_balanceState.update { BalanceState() }
keychain.wipe()
// ... more cleanup
}
}

Issue: backupRepo.reset() is called before state is cleared, but if any backup observer coroutines are in the middle of processing state changes, they may attempt to read from already-wiped datastores.

Recommendation: Consider the order of operations - perhaps reset state first, then cancel observers.

🟢 Minor Issues / Suggestions

6. Inconsistent Use of `nowMillis()` vs `nowTimestamp()`

The PR introduces nowMillis() but both functions are used inconsistently:

BoostTransactionViewModel.kt: Uses nowMillis().toULong()
ActivityRepo.kt:316: Still uses nowTimestamp().toEpochMilli().toULong()

Recommendation: Standardize on one approach for consistency.

7. Annotation Retention Changes

Location: DispatchersModule.kt:11-20

@Qualifier
-@Retention(AnnotationRetention.BINARY)
annotationclassUiDispatcher

Issue: Removing @Retention(AnnotationRetention.BINARY) changes the default to RUNTIME, which:

Increases APK size slightly (annotations retained in bytecode)
Allows runtime reflection (usually not needed for Dagger qualifiers)

Recommendation: Clarify if this change is intentional or if it should use BINARY retention.

8. BackupItemStatus.isRequired Property

Location: BackupCategory.kt:63

data classBackupItemStatus(
valrunning:Boolean = false,
valsynced:Long = 0,
valrequired:Long = 0,
) {
val isRequired:Boolean get() = synced < required
}

Issue: The computed property name isRequired is slightly misleading. It actually means "backup is outdated/stale," not "backup is required by user."

Recommendation: Consider renaming to isStale or needsSync for clarity.

9. Health Check Comparison Changed

Location: HealthRepo.kt:129

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

Issue: Changing from > to >= means synced == required is now considered "OK", but this is technically still a pending backup (hasn't happened yet).

Recommendation: Verify this is the intended behavior. The 5-minute grace period should handle this, but the semantic change is subtle.

🔒 Security Considerations

10. No Sensitive Data Concerns

Status: ✅ Good

Transaction IDs are public blockchain data
No credentials or private keys exposed in new code
VSS backup client properly handles encryption (outside scope of this PR)

🧪 Test Coverage

Issues with Test Coverage:

No Tests for New Functionality: The only test change is adding the backupRepo mock to WalletRepoTest.kt:51-82, but there are:
- No tests for boostTxIds tracking logic
- No tests for the CPFP child appending behavior
- No tests for the RBF parent chain tracking
- No tests for BackupRepo.reset() or scheduleFullBackup()
Missing Edge Case Tests:
- What happens if boostTxIds list grows very large (multiple RBF chains)?
- What if a transaction is boosted multiple times in succession?
- What if the activity is not found when applying pending boost?
UI Tests: No mention of UI tests for the new "Boosted Transaction" section in ActivityExploreScreen.kt:266-281.

Recommendation: Add unit tests for:

// BoostTransactionViewModel
@Test
fun`CPFP should append child txId to parent boostTxIds`()
@Test fun`RBF should track full parent chain in boostTxIds`()
@Test
fun`cachePendingBoostActivity should store parent chain for RBF`()
// ActivityRepo
@Test
fun`boostPendingActivities should apply parent chain to RBF transactions`()
// BackupRepo
@Test
fun`scheduleBackup should cancel existing job for same category`()

📊 Performance Considerations

11. Potential N+1 Query in UI

Location: ActivityExploreScreen.kt:266-281

onchain.v1.boostTxIds.forEachIndexed { index, parent ->Section(
title = stringResource(...),
// ... rendering
)
}

Issue: Each boostTxIds item renders a full Section. If a transaction has been boosted many times, this could create a very long list.

Recommendation: Consider limiting display to last N transactions with a "Show more" option, or use a lazy loading approach.

12. Backup Debounce is Acceptable

Status: ✅ Good

5-second debounce (BACKUP_DEBOUNCE = 5000L) prevents excessive backup calls
Backup check interval of 1 minute is reasonable

🎯 Recommendations Summary

Must Fix Before Merge:

✅ Fix race condition in BackupRepo.scheduleBackup() by canceling existing jobs
✅ Move vssStoreIdProvider.clearCache() inside synchronized block in VssBackupClient.reset()

Should Fix Before Merge:

⚠️ Add null safety check in BoostTransactionViewModel.cachePendingBoostActivity()
⚠️ Document backup restore order changes and add error handling
⚠️ Add unit tests for new boostTxIds functionality

Nice to Have:

💡 Standardize on nowMillis() vs nowTimestamp()
💡 Clarify annotation retention changes in DispatchersModule
💡 Rename isRequired to isStale for clarity
💡 Add UI tests for new explore screen section
💡 Consider limiting displayed boost chain length

✅ Conclusion

This is a well-structured PR with clear intent and good documentation. The core functionality for tracking boosted transaction parents is sound. However, there are 2 critical thread-safety issues that must be addressed before merging, and test coverage is insufficient for the new functionality.

Once the critical issues are resolved and tests are added, this PR will be ready to merge.

Recommendation: Request changes for issues #1, #2, #3, #4, and #5.

ovitrif · 2025-11-18T00:36:38Z

Closed in favour of:

Save boosted txs #470

ovitrif · 2025-11-06T23:57:20Z

ROADMAP: Transaction Details

This PR adds boosted txids tracking and displays parent transaction IDs in the activity details, matching iOS PR #199.

Description

Added boostTxIds to PendingBoostActivity to store parent transaction IDs
CPFP: Append child transaction ID to parent's boostTxIds when boosting
RBF:
- Track parent chain (existing boostTxIds + current txId) in PendingBoostActivity.
- Store parent txIds when creating replacement
- retrieve and apply when replacement syncs
UI: Display boosted transaction IDs in ActivityExploreScreen

Preview

CPFP (received)	RBF (sent)
TODO	TODO

QA Notes

1️⃣ RBF Test

Send onchain
Boost transaction
Activity Item > Explore
Expect list containing txid of previously boosted tx under "Boosted Transaction…" header

1️⃣ CPFP Test

Same but start with RECEIVE onchain.

Copilot

Pull Request Overview

This PR enhances boost transaction tracking by properly recording boost transaction IDs for both RBF (Replace-By-Fee) and CPFP (Child Pays For Parent) scenarios. The implementation ensures that parent-child transaction relationships are maintained and properly displayed in the activity detail screen.

Added boostTxIds field to PendingBoostActivity data class
Updated boost logic to track and persist transaction IDs through the boost process
Added UI component to display boosted transaction IDs in activity details

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
`BoostTransactionViewModel.kt`	Updated CPFP and cachePendingBoostActivity logic to properly track boost transaction IDs
`ActivityDetailScreen.kt`	Added UI section to display boosted transaction IDs with appropriate labels for RBF and CPFP
`ActivityRepo.kt`	Updated pending boost activities handling to apply boostTxIds during sync
`PendingBoostActivity.kt`	Added boostTxIds field with default empty list for backward compatibility

… explore

claude · 2025-11-07T14:46:38Z

PR Review: Activity Boosted TX Parents

Summary

This PR adds tracking and display of parent transaction IDs for boosted transactions (both CPFP and RBF), aligning with iOS implementation. Overall, the implementation is solid with good documentation and careful handling of edge cases. Below are my findings across code quality, potential issues, performance, security, and test coverage.

✅ Strengths

Excellent Documentation: The code includes comprehensive comments explaining RBF vs CPFP behavior, particularly in BoostTransactionViewModel.kt:419-430 and ActivityRepo.kt:437-447.
Proper State Management: The use of PendingBoostActivity to cache boost data for retry on sync failure is a robust pattern.
Good Separation of Concerns: Clear distinction between immediate CPFP updates and deferred RBF replacements.
Consistent Code Style: Follows Kotlin best practices with trailing commas, proper nullability handling, and immutable data structures.

🔴 Critical Issues

1. Race Condition in BackupRepo (High Priority)

Location: BackupRepo.kt:256-274

backupJobs[category] = scope.launch {
// Set running immediately to prevent UI showing failure during debounce
cacheStore.updateBackupStatus(category) {
it.copy(running =true)
}
delay(BACKUP_DEBOUNCE)
// Double-check if backup is still neededval status = cacheStore.backupStatuses.first()[category] ?:BackupItemStatus()
if (status.isRequired &&!isRestoring) {
triggerBackup(category)
} else {
// Backup no longer needed, reset running flag
cacheStore.updateBackupStatus(category) {
it.copy(running =false)
}
}
}

Issue: Not thread-safe. Multiple calls to scheduleBackup() for the same category could overwrite backupJobs[category] without canceling the previous job, leading to:

Multiple concurrent backups for the same category
Orphaned coroutines that never reset running = false
UI showing incorrect backup status

Recommendation:

backupJobs[category]?.cancel() // Cancel existing job
backupJobs[category] = scope.launch {
// ... rest of implementation
}

2. Synchronization Issue in VssBackupClient.reset()

Location: VssBackupClient.kt:65-72

funreset() {
synchronized(this) {
isSetup.cancel()
isSetup =CompletableDeferred()
}
vssStoreIdProvider.clearCache()
Logger.debug("VSS client reset", context =TAG)
}

Issue: vssStoreIdProvider.clearCache() is called outside the synchronized block, which could lead to:

Cache being cleared while setup() is reading from it
Inconsistent state between isSetup and cache

Recommendation: Move vssStoreIdProvider.clearCache() inside the synchronized block.

🟡 Medium Priority Issues

3. Missing Null Safety Check

Location: BoostTransactionViewModel.kt:422-430

val currentActivity = activity?.v1
val boostTxIds =if (activityToDelete !=null&& currentActivity !=null) {
currentActivity.boostTxIds + currentActivity.txId
} else {
emptyList()
}

Issue: activityToDelete != null is checked but currentActivity could still be null for RBF scenarios, leading to an empty boostTxIds list when it should contain the parent chain.

Recommendation: Add explicit error handling or logging when activityToDelete is non-null but currentActivity is null, as this indicates a bug.

4. Backup Order Changed Without Migration

Location: BackupRepo.kt:402-458

The restore order was changed from:

// Old orderSETTINGS → WALLET → ACTIVITY → METADATA → BLOCKTANK

to:

// New order METADATA → SETTINGS → WALLET → ACTIVITY → BLOCKTANK

Issue: METADATA is restored first to populate caches early via onCacheRestored() callback, but:

No migration logic handles cases where users have existing backups in the old format
If METADATA restore fails, subsequent restores may operate with stale cache data
The callback onCacheRestored() is not used in this PR

Recommendation:

Document why METADATA must be restored first
Add error handling if METADATA restore fails (should it abort the entire restore?)
Consider if partial restore states are acceptable

5. Potential Memory Leak in WalletRepo

Location: WalletRepo.kt:240-260

suspendfunwipeWallet(walletIndex:Int = 0): Result<Unit> = withContext(bgDispatcher) {
try {
backupRepo.reset() // Cancels all coroutines in BackupRepo_walletState.update { WalletState() }
_balanceState.update { BalanceState() }
keychain.wipe()
// ... more cleanup
}
}

Issue: backupRepo.reset() is called before state is cleared, but if any backup observer coroutines are in the middle of processing state changes, they may attempt to read from already-wiped datastores.

Recommendation: Consider the order of operations - perhaps reset state first, then cancel observers.

🟢 Minor Issues / Suggestions

6. Inconsistent Use of `nowMillis()` vs `nowTimestamp()`

The PR introduces nowMillis() but both functions are used inconsistently:

BoostTransactionViewModel.kt: Uses nowMillis().toULong()
ActivityRepo.kt:316: Still uses nowTimestamp().toEpochMilli().toULong()

Recommendation: Standardize on one approach for consistency.

7. Annotation Retention Changes

Location: DispatchersModule.kt:11-20

@Qualifier
-@Retention(AnnotationRetention.BINARY)
annotationclassUiDispatcher

Issue: Removing @Retention(AnnotationRetention.BINARY) changes the default to RUNTIME, which:

Increases APK size slightly (annotations retained in bytecode)
Allows runtime reflection (usually not needed for Dagger qualifiers)

Recommendation: Clarify if this change is intentional or if it should use BINARY retention.

8. BackupItemStatus.isRequired Property

Location: BackupCategory.kt:63

data classBackupItemStatus(
valrunning:Boolean = false,
valsynced:Long = 0,
valrequired:Long = 0,
) {
val isRequired:Boolean get() = synced < required
}

Issue: The computed property name isRequired is slightly misleading. It actually means "backup is outdated/stale," not "backup is required by user."

Recommendation: Consider renaming to isStale or needsSync for clarity.

9. Health Check Comparison Changed

Location: HealthRepo.kt:129

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

Issue: Changing from > to >= means synced == required is now considered "OK", but this is technically still a pending backup (hasn't happened yet).

Recommendation: Verify this is the intended behavior. The 5-minute grace period should handle this, but the semantic change is subtle.

🔒 Security Considerations

10. No Sensitive Data Concerns

Status: ✅ Good

Transaction IDs are public blockchain data
No credentials or private keys exposed in new code
VSS backup client properly handles encryption (outside scope of this PR)

🧪 Test Coverage

Issues with Test Coverage:

No Tests for New Functionality: The only test change is adding the backupRepo mock to WalletRepoTest.kt:51-82, but there are:
- No tests for boostTxIds tracking logic
- No tests for the CPFP child appending behavior
- No tests for the RBF parent chain tracking
- No tests for BackupRepo.reset() or scheduleFullBackup()
Missing Edge Case Tests:
- What happens if boostTxIds list grows very large (multiple RBF chains)?
- What if a transaction is boosted multiple times in succession?
- What if the activity is not found when applying pending boost?
UI Tests: No mention of UI tests for the new "Boosted Transaction" section in ActivityExploreScreen.kt:266-281.

Recommendation: Add unit tests for:

// BoostTransactionViewModel
@Test
fun`CPFP should append child txId to parent boostTxIds`()
@Test fun`RBF should track full parent chain in boostTxIds`()
@Test
fun`cachePendingBoostActivity should store parent chain for RBF`()
// ActivityRepo
@Test
fun`boostPendingActivities should apply parent chain to RBF transactions`()
// BackupRepo
@Test
fun`scheduleBackup should cancel existing job for same category`()

📊 Performance Considerations

11. Potential N+1 Query in UI

Location: ActivityExploreScreen.kt:266-281

onchain.v1.boostTxIds.forEachIndexed { index, parent ->Section(
title = stringResource(...),
// ... rendering
)
}

Issue: Each boostTxIds item renders a full Section. If a transaction has been boosted many times, this could create a very long list.

Recommendation: Consider limiting display to last N transactions with a "Show more" option, or use a lazy loading approach.

12. Backup Debounce is Acceptable

Status: ✅ Good

5-second debounce (BACKUP_DEBOUNCE = 5000L) prevents excessive backup calls
Backup check interval of 1 minute is reasonable

🎯 Recommendations Summary

Must Fix Before Merge:

✅ Fix race condition in BackupRepo.scheduleBackup() by canceling existing jobs
✅ Move vssStoreIdProvider.clearCache() inside synchronized block in VssBackupClient.reset()

Should Fix Before Merge:

⚠️ Add null safety check in BoostTransactionViewModel.cachePendingBoostActivity()
⚠️ Document backup restore order changes and add error handling
⚠️ Add unit tests for new boostTxIds functionality

Nice to Have:

💡 Standardize on nowMillis() vs nowTimestamp()
💡 Clarify annotation retention changes in DispatchersModule
💡 Rename isRequired to isStale for clarity
💡 Add UI tests for new explore screen section
💡 Consider limiting displayed boost chain length

✅ Conclusion

This is a well-structured PR with clear intent and good documentation. The core functionality for tracking boosted transaction parents is sound. However, there are 2 critical thread-safety issues that must be addressed before merging, and test coverage is insufficient for the new functionality.

Once the critical issues are resolved and tests are added, this PR will be ready to merge.

Recommendation: Request changes for issues #1, #2, #3, #4, and #5.

ovitrif · 2025-11-18T00:36:38Z

Closed in favour of:

Save boosted txs #470

ovitrif · 2025-11-06T23:57:20Z

ROADMAP: Transaction Details

This PR adds boosted txids tracking and displays parent transaction IDs in the activity details, matching iOS PR #199.

Description

Added boostTxIds to PendingBoostActivity to store parent transaction IDs
CPFP: Append child transaction ID to parent's boostTxIds when boosting
RBF:
- Track parent chain (existing boostTxIds + current txId) in PendingBoostActivity.
- Store parent txIds when creating replacement
- retrieve and apply when replacement syncs
UI: Display boosted transaction IDs in ActivityExploreScreen

Preview

CPFP (received)	RBF (sent)
TODO	TODO

QA Notes

1️⃣ RBF Test

Send onchain
Boost transaction
Activity Item > Explore
Expect list containing txid of previously boosted tx under "Boosted Transaction…" header

1️⃣ CPFP Test

Same but start with RECEIVE onchain.

Copilot

Pull Request Overview

This PR enhances boost transaction tracking by properly recording boost transaction IDs for both RBF (Replace-By-Fee) and CPFP (Child Pays For Parent) scenarios. The implementation ensures that parent-child transaction relationships are maintained and properly displayed in the activity detail screen.

Added boostTxIds field to PendingBoostActivity data class
Updated boost logic to track and persist transaction IDs through the boost process
Added UI component to display boosted transaction IDs in activity details

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
`BoostTransactionViewModel.kt`	Updated CPFP and cachePendingBoostActivity logic to properly track boost transaction IDs
`ActivityDetailScreen.kt`	Added UI section to display boosted transaction IDs with appropriate labels for RBF and CPFP
`ActivityRepo.kt`	Updated pending boost activities handling to apply boostTxIds during sync
`PendingBoostActivity.kt`	Added boostTxIds field with default empty list for backward compatibility

… explore

claude · 2025-11-07T14:46:38Z

PR Review: Activity Boosted TX Parents

Summary

This PR adds tracking and display of parent transaction IDs for boosted transactions (both CPFP and RBF), aligning with iOS implementation. Overall, the implementation is solid with good documentation and careful handling of edge cases. Below are my findings across code quality, potential issues, performance, security, and test coverage.

✅ Strengths

Excellent Documentation: The code includes comprehensive comments explaining RBF vs CPFP behavior, particularly in BoostTransactionViewModel.kt:419-430 and ActivityRepo.kt:437-447.
Proper State Management: The use of PendingBoostActivity to cache boost data for retry on sync failure is a robust pattern.
Good Separation of Concerns: Clear distinction between immediate CPFP updates and deferred RBF replacements.
Consistent Code Style: Follows Kotlin best practices with trailing commas, proper nullability handling, and immutable data structures.

🔴 Critical Issues

1. Race Condition in BackupRepo (High Priority)

Location: BackupRepo.kt:256-274

backupJobs[category] = scope.launch {
// Set running immediately to prevent UI showing failure during debounce
cacheStore.updateBackupStatus(category) {
it.copy(running =true)
}
delay(BACKUP_DEBOUNCE)
// Double-check if backup is still neededval status = cacheStore.backupStatuses.first()[category] ?:BackupItemStatus()
if (status.isRequired &&!isRestoring) {
triggerBackup(category)
} else {
// Backup no longer needed, reset running flag
cacheStore.updateBackupStatus(category) {
it.copy(running =false)
}
}
}

Issue: Not thread-safe. Multiple calls to scheduleBackup() for the same category could overwrite backupJobs[category] without canceling the previous job, leading to:

Multiple concurrent backups for the same category
Orphaned coroutines that never reset running = false
UI showing incorrect backup status

Recommendation:

backupJobs[category]?.cancel() // Cancel existing job
backupJobs[category] = scope.launch {
// ... rest of implementation
}

2. Synchronization Issue in VssBackupClient.reset()

Location: VssBackupClient.kt:65-72

funreset() {
synchronized(this) {
isSetup.cancel()
isSetup =CompletableDeferred()
}
vssStoreIdProvider.clearCache()
Logger.debug("VSS client reset", context =TAG)
}

Issue: vssStoreIdProvider.clearCache() is called outside the synchronized block, which could lead to:

Cache being cleared while setup() is reading from it
Inconsistent state between isSetup and cache

Recommendation: Move vssStoreIdProvider.clearCache() inside the synchronized block.

🟡 Medium Priority Issues

3. Missing Null Safety Check

Location: BoostTransactionViewModel.kt:422-430

val currentActivity = activity?.v1
val boostTxIds =if (activityToDelete !=null&& currentActivity !=null) {
currentActivity.boostTxIds + currentActivity.txId
} else {
emptyList()
}

Issue: activityToDelete != null is checked but currentActivity could still be null for RBF scenarios, leading to an empty boostTxIds list when it should contain the parent chain.

Recommendation: Add explicit error handling or logging when activityToDelete is non-null but currentActivity is null, as this indicates a bug.

4. Backup Order Changed Without Migration

Location: BackupRepo.kt:402-458

The restore order was changed from:

// Old orderSETTINGS → WALLET → ACTIVITY → METADATA → BLOCKTANK

to:

// New order METADATA → SETTINGS → WALLET → ACTIVITY → BLOCKTANK

Issue: METADATA is restored first to populate caches early via onCacheRestored() callback, but:

No migration logic handles cases where users have existing backups in the old format
If METADATA restore fails, subsequent restores may operate with stale cache data
The callback onCacheRestored() is not used in this PR

Recommendation:

Document why METADATA must be restored first
Add error handling if METADATA restore fails (should it abort the entire restore?)
Consider if partial restore states are acceptable

5. Potential Memory Leak in WalletRepo

Location: WalletRepo.kt:240-260

suspendfunwipeWallet(walletIndex:Int = 0): Result<Unit> = withContext(bgDispatcher) {
try {
backupRepo.reset() // Cancels all coroutines in BackupRepo_walletState.update { WalletState() }
_balanceState.update { BalanceState() }
keychain.wipe()
// ... more cleanup
}
}

Issue: backupRepo.reset() is called before state is cleared, but if any backup observer coroutines are in the middle of processing state changes, they may attempt to read from already-wiped datastores.

Recommendation: Consider the order of operations - perhaps reset state first, then cancel observers.

🟢 Minor Issues / Suggestions

6. Inconsistent Use of `nowMillis()` vs `nowTimestamp()`

The PR introduces nowMillis() but both functions are used inconsistently:

BoostTransactionViewModel.kt: Uses nowMillis().toULong()
ActivityRepo.kt:316: Still uses nowTimestamp().toEpochMilli().toULong()

Recommendation: Standardize on one approach for consistency.

7. Annotation Retention Changes

Location: DispatchersModule.kt:11-20

@Qualifier
-@Retention(AnnotationRetention.BINARY)
annotationclassUiDispatcher

Issue: Removing @Retention(AnnotationRetention.BINARY) changes the default to RUNTIME, which:

Increases APK size slightly (annotations retained in bytecode)
Allows runtime reflection (usually not needed for Dagger qualifiers)

Recommendation: Clarify if this change is intentional or if it should use BINARY retention.

8. BackupItemStatus.isRequired Property

Location: BackupCategory.kt:63

data classBackupItemStatus(
valrunning:Boolean = false,
valsynced:Long = 0,
valrequired:Long = 0,
) {
val isRequired:Boolean get() = synced < required
}

Issue: The computed property name isRequired is slightly misleading. It actually means "backup is outdated/stale," not "backup is required by user."

Recommendation: Consider renaming to isStale or needsSync for clarity.

9. Health Check Comparison Changed

Location: HealthRepo.kt:129

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

Issue: Changing from > to >= means synced == required is now considered "OK", but this is technically still a pending backup (hasn't happened yet).

Recommendation: Verify this is the intended behavior. The 5-minute grace period should handle this, but the semantic change is subtle.

🔒 Security Considerations

10. No Sensitive Data Concerns

Status: ✅ Good

Transaction IDs are public blockchain data
No credentials or private keys exposed in new code
VSS backup client properly handles encryption (outside scope of this PR)

🧪 Test Coverage

Issues with Test Coverage:

No Tests for New Functionality: The only test change is adding the backupRepo mock to WalletRepoTest.kt:51-82, but there are:
- No tests for boostTxIds tracking logic
- No tests for the CPFP child appending behavior
- No tests for the RBF parent chain tracking
- No tests for BackupRepo.reset() or scheduleFullBackup()
Missing Edge Case Tests:
- What happens if boostTxIds list grows very large (multiple RBF chains)?
- What if a transaction is boosted multiple times in succession?
- What if the activity is not found when applying pending boost?
UI Tests: No mention of UI tests for the new "Boosted Transaction" section in ActivityExploreScreen.kt:266-281.

Recommendation: Add unit tests for:

// BoostTransactionViewModel
@Test
fun`CPFP should append child txId to parent boostTxIds`()
@Test fun`RBF should track full parent chain in boostTxIds`()
@Test
fun`cachePendingBoostActivity should store parent chain for RBF`()
// ActivityRepo
@Test
fun`boostPendingActivities should apply parent chain to RBF transactions`()
// BackupRepo
@Test
fun`scheduleBackup should cancel existing job for same category`()

📊 Performance Considerations

11. Potential N+1 Query in UI

Location: ActivityExploreScreen.kt:266-281

onchain.v1.boostTxIds.forEachIndexed { index, parent ->Section(
title = stringResource(...),
// ... rendering
)
}

Issue: Each boostTxIds item renders a full Section. If a transaction has been boosted many times, this could create a very long list.

Recommendation: Consider limiting display to last N transactions with a "Show more" option, or use a lazy loading approach.

12. Backup Debounce is Acceptable

Status: ✅ Good

5-second debounce (BACKUP_DEBOUNCE = 5000L) prevents excessive backup calls
Backup check interval of 1 minute is reasonable

🎯 Recommendations Summary

Must Fix Before Merge:

✅ Fix race condition in BackupRepo.scheduleBackup() by canceling existing jobs
✅ Move vssStoreIdProvider.clearCache() inside synchronized block in VssBackupClient.reset()

Should Fix Before Merge:

⚠️ Add null safety check in BoostTransactionViewModel.cachePendingBoostActivity()
⚠️ Document backup restore order changes and add error handling
⚠️ Add unit tests for new boostTxIds functionality

Nice to Have:

💡 Standardize on nowMillis() vs nowTimestamp()
💡 Clarify annotation retention changes in DispatchersModule
💡 Rename isRequired to isStale for clarity
💡 Add UI tests for new explore screen section
💡 Consider limiting displayed boost chain length

✅ Conclusion

This is a well-structured PR with clear intent and good documentation. The core functionality for tracking boosted transaction parents is sound. However, there are 2 critical thread-safety issues that must be addressed before merging, and test coverage is insufficient for the new functionality.

Once the critical issues are resolved and tests are added, this PR will be ready to merge.

Recommendation: Request changes for issues #1, #2, #3, #4, and #5.

ovitrif · 2025-11-18T00:36:38Z

Closed in favour of:

Save boosted txs #470

ovitrif · 2025-11-06T23:57:20Z

ROADMAP: Transaction Details

This PR adds boosted txids tracking and displays parent transaction IDs in the activity details, matching iOS PR #199.

Description

Added boostTxIds to PendingBoostActivity to store parent transaction IDs
CPFP: Append child transaction ID to parent's boostTxIds when boosting
RBF:
- Track parent chain (existing boostTxIds + current txId) in PendingBoostActivity.
- Store parent txIds when creating replacement
- retrieve and apply when replacement syncs
UI: Display boosted transaction IDs in ActivityExploreScreen

Preview

CPFP (received)	RBF (sent)
TODO	TODO

QA Notes

1️⃣ RBF Test

Send onchain
Boost transaction
Activity Item > Explore
Expect list containing txid of previously boosted tx under "Boosted Transaction…" header

1️⃣ CPFP Test

Same but start with RECEIVE onchain.

Copilot

Pull Request Overview

This PR enhances boost transaction tracking by properly recording boost transaction IDs for both RBF (Replace-By-Fee) and CPFP (Child Pays For Parent) scenarios. The implementation ensures that parent-child transaction relationships are maintained and properly displayed in the activity detail screen.

Added boostTxIds field to PendingBoostActivity data class
Updated boost logic to track and persist transaction IDs through the boost process
Added UI component to display boosted transaction IDs in activity details

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
`BoostTransactionViewModel.kt`	Updated CPFP and cachePendingBoostActivity logic to properly track boost transaction IDs
`ActivityDetailScreen.kt`	Added UI section to display boosted transaction IDs with appropriate labels for RBF and CPFP
`ActivityRepo.kt`	Updated pending boost activities handling to apply boostTxIds during sync
`PendingBoostActivity.kt`	Added boostTxIds field with default empty list for backward compatibility

… explore

claude · 2025-11-07T14:46:38Z

PR Review: Activity Boosted TX Parents

Summary

This PR adds tracking and display of parent transaction IDs for boosted transactions (both CPFP and RBF), aligning with iOS implementation. Overall, the implementation is solid with good documentation and careful handling of edge cases. Below are my findings across code quality, potential issues, performance, security, and test coverage.

✅ Strengths

Excellent Documentation: The code includes comprehensive comments explaining RBF vs CPFP behavior, particularly in BoostTransactionViewModel.kt:419-430 and ActivityRepo.kt:437-447.
Proper State Management: The use of PendingBoostActivity to cache boost data for retry on sync failure is a robust pattern.
Good Separation of Concerns: Clear distinction between immediate CPFP updates and deferred RBF replacements.
Consistent Code Style: Follows Kotlin best practices with trailing commas, proper nullability handling, and immutable data structures.

🔴 Critical Issues

1. Race Condition in BackupRepo (High Priority)

Location: BackupRepo.kt:256-274

backupJobs[category] = scope.launch {
// Set running immediately to prevent UI showing failure during debounce
cacheStore.updateBackupStatus(category) {
it.copy(running =true)
}
delay(BACKUP_DEBOUNCE)
// Double-check if backup is still neededval status = cacheStore.backupStatuses.first()[category] ?:BackupItemStatus()
if (status.isRequired &&!isRestoring) {
triggerBackup(category)
} else {
// Backup no longer needed, reset running flag
cacheStore.updateBackupStatus(category) {
it.copy(running =false)
}
}
}

Issue: Not thread-safe. Multiple calls to scheduleBackup() for the same category could overwrite backupJobs[category] without canceling the previous job, leading to:

Multiple concurrent backups for the same category
Orphaned coroutines that never reset running = false
UI showing incorrect backup status

Recommendation:

backupJobs[category]?.cancel() // Cancel existing job
backupJobs[category] = scope.launch {
// ... rest of implementation
}

2. Synchronization Issue in VssBackupClient.reset()

Location: VssBackupClient.kt:65-72

funreset() {
synchronized(this) {
isSetup.cancel()
isSetup =CompletableDeferred()
}
vssStoreIdProvider.clearCache()
Logger.debug("VSS client reset", context =TAG)
}

Issue: vssStoreIdProvider.clearCache() is called outside the synchronized block, which could lead to:

Cache being cleared while setup() is reading from it
Inconsistent state between isSetup and cache

Recommendation: Move vssStoreIdProvider.clearCache() inside the synchronized block.

🟡 Medium Priority Issues

3. Missing Null Safety Check

Location: BoostTransactionViewModel.kt:422-430

val currentActivity = activity?.v1
val boostTxIds =if (activityToDelete !=null&& currentActivity !=null) {
currentActivity.boostTxIds + currentActivity.txId
} else {
emptyList()
}

Issue: activityToDelete != null is checked but currentActivity could still be null for RBF scenarios, leading to an empty boostTxIds list when it should contain the parent chain.

Recommendation: Add explicit error handling or logging when activityToDelete is non-null but currentActivity is null, as this indicates a bug.

4. Backup Order Changed Without Migration

Location: BackupRepo.kt:402-458

The restore order was changed from:

// Old orderSETTINGS → WALLET → ACTIVITY → METADATA → BLOCKTANK

to:

// New order METADATA → SETTINGS → WALLET → ACTIVITY → BLOCKTANK

Issue: METADATA is restored first to populate caches early via onCacheRestored() callback, but:

No migration logic handles cases where users have existing backups in the old format
If METADATA restore fails, subsequent restores may operate with stale cache data
The callback onCacheRestored() is not used in this PR

Recommendation:

Document why METADATA must be restored first
Add error handling if METADATA restore fails (should it abort the entire restore?)
Consider if partial restore states are acceptable

5. Potential Memory Leak in WalletRepo

Location: WalletRepo.kt:240-260

suspendfunwipeWallet(walletIndex:Int = 0): Result<Unit> = withContext(bgDispatcher) {
try {
backupRepo.reset() // Cancels all coroutines in BackupRepo_walletState.update { WalletState() }
_balanceState.update { BalanceState() }
keychain.wipe()
// ... more cleanup
}
}

Issue: backupRepo.reset() is called before state is cleared, but if any backup observer coroutines are in the middle of processing state changes, they may attempt to read from already-wiped datastores.

Recommendation: Consider the order of operations - perhaps reset state first, then cancel observers.

🟢 Minor Issues / Suggestions

6. Inconsistent Use of `nowMillis()` vs `nowTimestamp()`

The PR introduces nowMillis() but both functions are used inconsistently:

BoostTransactionViewModel.kt: Uses nowMillis().toULong()
ActivityRepo.kt:316: Still uses nowTimestamp().toEpochMilli().toULong()

Recommendation: Standardize on one approach for consistency.

7. Annotation Retention Changes

Location: DispatchersModule.kt:11-20

@Qualifier
-@Retention(AnnotationRetention.BINARY)
annotationclassUiDispatcher

Issue: Removing @Retention(AnnotationRetention.BINARY) changes the default to RUNTIME, which:

Increases APK size slightly (annotations retained in bytecode)
Allows runtime reflection (usually not needed for Dagger qualifiers)

Recommendation: Clarify if this change is intentional or if it should use BINARY retention.

8. BackupItemStatus.isRequired Property

Location: BackupCategory.kt:63

data classBackupItemStatus(
valrunning:Boolean = false,
valsynced:Long = 0,
valrequired:Long = 0,
) {
val isRequired:Boolean get() = synced < required
}

Issue: The computed property name isRequired is slightly misleading. It actually means "backup is outdated/stale," not "backup is required by user."

Recommendation: Consider renaming to isStale or needsSync for clarity.

9. Health Check Comparison Changed

Location: HealthRepo.kt:129

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

Issue: Changing from > to >= means synced == required is now considered "OK", but this is technically still a pending backup (hasn't happened yet).

Recommendation: Verify this is the intended behavior. The 5-minute grace period should handle this, but the semantic change is subtle.

🔒 Security Considerations

10. No Sensitive Data Concerns

Status: ✅ Good

Transaction IDs are public blockchain data
No credentials or private keys exposed in new code
VSS backup client properly handles encryption (outside scope of this PR)

🧪 Test Coverage

Issues with Test Coverage:

No Tests for New Functionality: The only test change is adding the backupRepo mock to WalletRepoTest.kt:51-82, but there are:
- No tests for boostTxIds tracking logic
- No tests for the CPFP child appending behavior
- No tests for the RBF parent chain tracking
- No tests for BackupRepo.reset() or scheduleFullBackup()
Missing Edge Case Tests:
- What happens if boostTxIds list grows very large (multiple RBF chains)?
- What if a transaction is boosted multiple times in succession?
- What if the activity is not found when applying pending boost?
UI Tests: No mention of UI tests for the new "Boosted Transaction" section in ActivityExploreScreen.kt:266-281.

Recommendation: Add unit tests for:

// BoostTransactionViewModel
@Test
fun`CPFP should append child txId to parent boostTxIds`()
@Test fun`RBF should track full parent chain in boostTxIds`()
@Test
fun`cachePendingBoostActivity should store parent chain for RBF`()
// ActivityRepo
@Test
fun`boostPendingActivities should apply parent chain to RBF transactions`()
// BackupRepo
@Test
fun`scheduleBackup should cancel existing job for same category`()

📊 Performance Considerations

11. Potential N+1 Query in UI

Location: ActivityExploreScreen.kt:266-281

onchain.v1.boostTxIds.forEachIndexed { index, parent ->Section(
title = stringResource(...),
// ... rendering
)
}

Issue: Each boostTxIds item renders a full Section. If a transaction has been boosted many times, this could create a very long list.

Recommendation: Consider limiting display to last N transactions with a "Show more" option, or use a lazy loading approach.

12. Backup Debounce is Acceptable

Status: ✅ Good

5-second debounce (BACKUP_DEBOUNCE = 5000L) prevents excessive backup calls
Backup check interval of 1 minute is reasonable

🎯 Recommendations Summary

Must Fix Before Merge:

✅ Fix race condition in BackupRepo.scheduleBackup() by canceling existing jobs
✅ Move vssStoreIdProvider.clearCache() inside synchronized block in VssBackupClient.reset()

Should Fix Before Merge:

⚠️ Add null safety check in BoostTransactionViewModel.cachePendingBoostActivity()
⚠️ Document backup restore order changes and add error handling
⚠️ Add unit tests for new boostTxIds functionality

Nice to Have:

💡 Standardize on nowMillis() vs nowTimestamp()
💡 Clarify annotation retention changes in DispatchersModule
💡 Rename isRequired to isStale for clarity
💡 Add UI tests for new explore screen section
💡 Consider limiting displayed boost chain length

✅ Conclusion

This is a well-structured PR with clear intent and good documentation. The core functionality for tracking boosted transaction parents is sound. However, there are 2 critical thread-safety issues that must be addressed before merging, and test coverage is insufficient for the new functionality.

Once the critical issues are resolved and tests are added, this PR will be ready to merge.

Recommendation: Request changes for issues #1, #2, #3, #4, and #5.

ovitrif · 2025-11-18T00:36:38Z

Closed in favour of:

Save boosted txs #470

ovitrif · 2025-11-06T23:57:20Z

ROADMAP: Transaction Details

This PR adds boosted txids tracking and displays parent transaction IDs in the activity details, matching iOS PR #199.

Description

Added boostTxIds to PendingBoostActivity to store parent transaction IDs
CPFP: Append child transaction ID to parent's boostTxIds when boosting
RBF:
- Track parent chain (existing boostTxIds + current txId) in PendingBoostActivity.
- Store parent txIds when creating replacement
- retrieve and apply when replacement syncs
UI: Display boosted transaction IDs in ActivityExploreScreen

Preview

CPFP (received)	RBF (sent)
TODO	TODO

QA Notes

1️⃣ RBF Test

Send onchain
Boost transaction
Activity Item > Explore
Expect list containing txid of previously boosted tx under "Boosted Transaction…" header

1️⃣ CPFP Test

Same but start with RECEIVE onchain.

Copilot

Pull Request Overview

This PR enhances boost transaction tracking by properly recording boost transaction IDs for both RBF (Replace-By-Fee) and CPFP (Child Pays For Parent) scenarios. The implementation ensures that parent-child transaction relationships are maintained and properly displayed in the activity detail screen.

Added boostTxIds field to PendingBoostActivity data class
Updated boost logic to track and persist transaction IDs through the boost process
Added UI component to display boosted transaction IDs in activity details

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
`BoostTransactionViewModel.kt`	Updated CPFP and cachePendingBoostActivity logic to properly track boost transaction IDs
`ActivityDetailScreen.kt`	Added UI section to display boosted transaction IDs with appropriate labels for RBF and CPFP
`ActivityRepo.kt`	Updated pending boost activities handling to apply boostTxIds during sync
`PendingBoostActivity.kt`	Added boostTxIds field with default empty list for backward compatibility

… explore

claude · 2025-11-07T14:46:38Z

PR Review: Activity Boosted TX Parents

Summary

This PR adds tracking and display of parent transaction IDs for boosted transactions (both CPFP and RBF), aligning with iOS implementation. Overall, the implementation is solid with good documentation and careful handling of edge cases. Below are my findings across code quality, potential issues, performance, security, and test coverage.

✅ Strengths

Excellent Documentation: The code includes comprehensive comments explaining RBF vs CPFP behavior, particularly in BoostTransactionViewModel.kt:419-430 and ActivityRepo.kt:437-447.
Proper State Management: The use of PendingBoostActivity to cache boost data for retry on sync failure is a robust pattern.
Good Separation of Concerns: Clear distinction between immediate CPFP updates and deferred RBF replacements.
Consistent Code Style: Follows Kotlin best practices with trailing commas, proper nullability handling, and immutable data structures.

🔴 Critical Issues

1. Race Condition in BackupRepo (High Priority)

Location: BackupRepo.kt:256-274

backupJobs[category] = scope.launch {
// Set running immediately to prevent UI showing failure during debounce
cacheStore.updateBackupStatus(category) {
it.copy(running =true)
}
delay(BACKUP_DEBOUNCE)
// Double-check if backup is still neededval status = cacheStore.backupStatuses.first()[category] ?:BackupItemStatus()
if (status.isRequired &&!isRestoring) {
triggerBackup(category)
} else {
// Backup no longer needed, reset running flag
cacheStore.updateBackupStatus(category) {
it.copy(running =false)
}
}
}

Issue: Not thread-safe. Multiple calls to scheduleBackup() for the same category could overwrite backupJobs[category] without canceling the previous job, leading to:

Multiple concurrent backups for the same category
Orphaned coroutines that never reset running = false
UI showing incorrect backup status

Recommendation:

backupJobs[category]?.cancel() // Cancel existing job
backupJobs[category] = scope.launch {
// ... rest of implementation
}

2. Synchronization Issue in VssBackupClient.reset()

Location: VssBackupClient.kt:65-72

funreset() {
synchronized(this) {
isSetup.cancel()
isSetup =CompletableDeferred()
}
vssStoreIdProvider.clearCache()
Logger.debug("VSS client reset", context =TAG)
}

Issue: vssStoreIdProvider.clearCache() is called outside the synchronized block, which could lead to:

Cache being cleared while setup() is reading from it
Inconsistent state between isSetup and cache

Recommendation: Move vssStoreIdProvider.clearCache() inside the synchronized block.

🟡 Medium Priority Issues

3. Missing Null Safety Check

Location: BoostTransactionViewModel.kt:422-430

val currentActivity = activity?.v1
val boostTxIds =if (activityToDelete !=null&& currentActivity !=null) {
currentActivity.boostTxIds + currentActivity.txId
} else {
emptyList()
}

Issue: activityToDelete != null is checked but currentActivity could still be null for RBF scenarios, leading to an empty boostTxIds list when it should contain the parent chain.

Recommendation: Add explicit error handling or logging when activityToDelete is non-null but currentActivity is null, as this indicates a bug.

4. Backup Order Changed Without Migration

Location: BackupRepo.kt:402-458

The restore order was changed from:

// Old orderSETTINGS → WALLET → ACTIVITY → METADATA → BLOCKTANK

to:

// New order METADATA → SETTINGS → WALLET → ACTIVITY → BLOCKTANK

Issue: METADATA is restored first to populate caches early via onCacheRestored() callback, but:

No migration logic handles cases where users have existing backups in the old format
If METADATA restore fails, subsequent restores may operate with stale cache data
The callback onCacheRestored() is not used in this PR

Recommendation:

Document why METADATA must be restored first
Add error handling if METADATA restore fails (should it abort the entire restore?)
Consider if partial restore states are acceptable

5. Potential Memory Leak in WalletRepo

Location: WalletRepo.kt:240-260

suspendfunwipeWallet(walletIndex:Int = 0): Result<Unit> = withContext(bgDispatcher) {
try {
backupRepo.reset() // Cancels all coroutines in BackupRepo_walletState.update { WalletState() }
_balanceState.update { BalanceState() }
keychain.wipe()
// ... more cleanup
}
}

Issue: backupRepo.reset() is called before state is cleared, but if any backup observer coroutines are in the middle of processing state changes, they may attempt to read from already-wiped datastores.

Recommendation: Consider the order of operations - perhaps reset state first, then cancel observers.

🟢 Minor Issues / Suggestions

6. Inconsistent Use of `nowMillis()` vs `nowTimestamp()`

The PR introduces nowMillis() but both functions are used inconsistently:

BoostTransactionViewModel.kt: Uses nowMillis().toULong()
ActivityRepo.kt:316: Still uses nowTimestamp().toEpochMilli().toULong()

Recommendation: Standardize on one approach for consistency.

7. Annotation Retention Changes

Location: DispatchersModule.kt:11-20

@Qualifier
-@Retention(AnnotationRetention.BINARY)
annotationclassUiDispatcher

Issue: Removing @Retention(AnnotationRetention.BINARY) changes the default to RUNTIME, which:

Increases APK size slightly (annotations retained in bytecode)
Allows runtime reflection (usually not needed for Dagger qualifiers)

Recommendation: Clarify if this change is intentional or if it should use BINARY retention.

8. BackupItemStatus.isRequired Property

Location: BackupCategory.kt:63

data classBackupItemStatus(
valrunning:Boolean = false,
valsynced:Long = 0,
valrequired:Long = 0,
) {
val isRequired:Boolean get() = synced < required
}

Issue: The computed property name isRequired is slightly misleading. It actually means "backup is outdated/stale," not "backup is required by user."

Recommendation: Consider renaming to isStale or needsSync for clarity.

9. Health Check Comparison Changed

Location: HealthRepo.kt:129

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

Issue: Changing from > to >= means synced == required is now considered "OK", but this is technically still a pending backup (hasn't happened yet).

Recommendation: Verify this is the intended behavior. The 5-minute grace period should handle this, but the semantic change is subtle.

🔒 Security Considerations

10. No Sensitive Data Concerns

Status: ✅ Good

Transaction IDs are public blockchain data
No credentials or private keys exposed in new code
VSS backup client properly handles encryption (outside scope of this PR)

🧪 Test Coverage

Issues with Test Coverage:

No Tests for New Functionality: The only test change is adding the backupRepo mock to WalletRepoTest.kt:51-82, but there are:
- No tests for boostTxIds tracking logic
- No tests for the CPFP child appending behavior
- No tests for the RBF parent chain tracking
- No tests for BackupRepo.reset() or scheduleFullBackup()
Missing Edge Case Tests:
- What happens if boostTxIds list grows very large (multiple RBF chains)?
- What if a transaction is boosted multiple times in succession?
- What if the activity is not found when applying pending boost?
UI Tests: No mention of UI tests for the new "Boosted Transaction" section in ActivityExploreScreen.kt:266-281.

Recommendation: Add unit tests for:

// BoostTransactionViewModel
@Test
fun`CPFP should append child txId to parent boostTxIds`()
@Test fun`RBF should track full parent chain in boostTxIds`()
@Test
fun`cachePendingBoostActivity should store parent chain for RBF`()
// ActivityRepo
@Test
fun`boostPendingActivities should apply parent chain to RBF transactions`()
// BackupRepo
@Test
fun`scheduleBackup should cancel existing job for same category`()

📊 Performance Considerations

11. Potential N+1 Query in UI

Location: ActivityExploreScreen.kt:266-281

onchain.v1.boostTxIds.forEachIndexed { index, parent ->Section(
title = stringResource(...),
// ... rendering
)
}

Issue: Each boostTxIds item renders a full Section. If a transaction has been boosted many times, this could create a very long list.

Recommendation: Consider limiting display to last N transactions with a "Show more" option, or use a lazy loading approach.

12. Backup Debounce is Acceptable

Status: ✅ Good

5-second debounce (BACKUP_DEBOUNCE = 5000L) prevents excessive backup calls
Backup check interval of 1 minute is reasonable

🎯 Recommendations Summary

Must Fix Before Merge:

✅ Fix race condition in BackupRepo.scheduleBackup() by canceling existing jobs
✅ Move vssStoreIdProvider.clearCache() inside synchronized block in VssBackupClient.reset()

Should Fix Before Merge:

⚠️ Add null safety check in BoostTransactionViewModel.cachePendingBoostActivity()
⚠️ Document backup restore order changes and add error handling
⚠️ Add unit tests for new boostTxIds functionality

Nice to Have:

💡 Standardize on nowMillis() vs nowTimestamp()
💡 Clarify annotation retention changes in DispatchersModule
💡 Rename isRequired to isStale for clarity
💡 Add UI tests for new explore screen section
💡 Consider limiting displayed boost chain length

✅ Conclusion

This is a well-structured PR with clear intent and good documentation. The core functionality for tracking boosted transaction parents is sound. However, there are 2 critical thread-safety issues that must be addressed before merging, and test coverage is insufficient for the new functionality.

Once the critical issues are resolved and tests are added, this PR will be ready to merge.

Recommendation: Request changes for issues #1, #2, #3, #4, and #5.

ovitrif · 2025-11-18T00:36:38Z

Closed in favour of:

Save boosted txs #470

ovitrif · 2025-11-06T23:57:20Z

ROADMAP: Transaction Details

This PR adds boosted txids tracking and displays parent transaction IDs in the activity details, matching iOS PR #199.

Description

Added boostTxIds to PendingBoostActivity to store parent transaction IDs
CPFP: Append child transaction ID to parent's boostTxIds when boosting
RBF:
- Track parent chain (existing boostTxIds + current txId) in PendingBoostActivity.
- Store parent txIds when creating replacement
- retrieve and apply when replacement syncs
UI: Display boosted transaction IDs in ActivityExploreScreen

Preview

CPFP (received)	RBF (sent)
TODO	TODO

QA Notes

1️⃣ RBF Test

Send onchain
Boost transaction
Activity Item > Explore
Expect list containing txid of previously boosted tx under "Boosted Transaction…" header

1️⃣ CPFP Test

Same but start with RECEIVE onchain.

Copilot

Pull Request Overview

This PR enhances boost transaction tracking by properly recording boost transaction IDs for both RBF (Replace-By-Fee) and CPFP (Child Pays For Parent) scenarios. The implementation ensures that parent-child transaction relationships are maintained and properly displayed in the activity detail screen.

Added boostTxIds field to PendingBoostActivity data class
Updated boost logic to track and persist transaction IDs through the boost process
Added UI component to display boosted transaction IDs in activity details

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
`BoostTransactionViewModel.kt`	Updated CPFP and cachePendingBoostActivity logic to properly track boost transaction IDs
`ActivityDetailScreen.kt`	Added UI section to display boosted transaction IDs with appropriate labels for RBF and CPFP
`ActivityRepo.kt`	Updated pending boost activities handling to apply boostTxIds during sync
`PendingBoostActivity.kt`	Added boostTxIds field with default empty list for backward compatibility

… explore

claude · 2025-11-07T14:46:38Z

PR Review: Activity Boosted TX Parents

Summary

This PR adds tracking and display of parent transaction IDs for boosted transactions (both CPFP and RBF), aligning with iOS implementation. Overall, the implementation is solid with good documentation and careful handling of edge cases. Below are my findings across code quality, potential issues, performance, security, and test coverage.

✅ Strengths

Excellent Documentation: The code includes comprehensive comments explaining RBF vs CPFP behavior, particularly in BoostTransactionViewModel.kt:419-430 and ActivityRepo.kt:437-447.
Proper State Management: The use of PendingBoostActivity to cache boost data for retry on sync failure is a robust pattern.
Good Separation of Concerns: Clear distinction between immediate CPFP updates and deferred RBF replacements.
Consistent Code Style: Follows Kotlin best practices with trailing commas, proper nullability handling, and immutable data structures.

🔴 Critical Issues

1. Race Condition in BackupRepo (High Priority)

Location: BackupRepo.kt:256-274

backupJobs[category] = scope.launch {
// Set running immediately to prevent UI showing failure during debounce
cacheStore.updateBackupStatus(category) {
it.copy(running =true)
}
delay(BACKUP_DEBOUNCE)
// Double-check if backup is still neededval status = cacheStore.backupStatuses.first()[category] ?:BackupItemStatus()
if (status.isRequired &&!isRestoring) {
triggerBackup(category)
} else {
// Backup no longer needed, reset running flag
cacheStore.updateBackupStatus(category) {
it.copy(running =false)
}
}
}

Issue: Not thread-safe. Multiple calls to scheduleBackup() for the same category could overwrite backupJobs[category] without canceling the previous job, leading to:

Multiple concurrent backups for the same category
Orphaned coroutines that never reset running = false
UI showing incorrect backup status

Recommendation:

backupJobs[category]?.cancel() // Cancel existing job
backupJobs[category] = scope.launch {
// ... rest of implementation
}

2. Synchronization Issue in VssBackupClient.reset()

Location: VssBackupClient.kt:65-72

funreset() {
synchronized(this) {
isSetup.cancel()
isSetup =CompletableDeferred()
}
vssStoreIdProvider.clearCache()
Logger.debug("VSS client reset", context =TAG)
}

Issue: vssStoreIdProvider.clearCache() is called outside the synchronized block, which could lead to:

Cache being cleared while setup() is reading from it
Inconsistent state between isSetup and cache

Recommendation: Move vssStoreIdProvider.clearCache() inside the synchronized block.

🟡 Medium Priority Issues

3. Missing Null Safety Check

Location: BoostTransactionViewModel.kt:422-430

val currentActivity = activity?.v1
val boostTxIds =if (activityToDelete !=null&& currentActivity !=null) {
currentActivity.boostTxIds + currentActivity.txId
} else {
emptyList()
}

Issue: activityToDelete != null is checked but currentActivity could still be null for RBF scenarios, leading to an empty boostTxIds list when it should contain the parent chain.

Recommendation: Add explicit error handling or logging when activityToDelete is non-null but currentActivity is null, as this indicates a bug.

4. Backup Order Changed Without Migration

Location: BackupRepo.kt:402-458

The restore order was changed from:

// Old orderSETTINGS → WALLET → ACTIVITY → METADATA → BLOCKTANK

to:

// New order METADATA → SETTINGS → WALLET → ACTIVITY → BLOCKTANK

Issue: METADATA is restored first to populate caches early via onCacheRestored() callback, but:

No migration logic handles cases where users have existing backups in the old format
If METADATA restore fails, subsequent restores may operate with stale cache data
The callback onCacheRestored() is not used in this PR

Recommendation:

Document why METADATA must be restored first
Add error handling if METADATA restore fails (should it abort the entire restore?)
Consider if partial restore states are acceptable

5. Potential Memory Leak in WalletRepo

Location: WalletRepo.kt:240-260

suspendfunwipeWallet(walletIndex:Int = 0): Result<Unit> = withContext(bgDispatcher) {
try {
backupRepo.reset() // Cancels all coroutines in BackupRepo_walletState.update { WalletState() }
_balanceState.update { BalanceState() }
keychain.wipe()
// ... more cleanup
}
}

Issue: backupRepo.reset() is called before state is cleared, but if any backup observer coroutines are in the middle of processing state changes, they may attempt to read from already-wiped datastores.

Recommendation: Consider the order of operations - perhaps reset state first, then cancel observers.

🟢 Minor Issues / Suggestions

6. Inconsistent Use of `nowMillis()` vs `nowTimestamp()`

The PR introduces nowMillis() but both functions are used inconsistently:

BoostTransactionViewModel.kt: Uses nowMillis().toULong()
ActivityRepo.kt:316: Still uses nowTimestamp().toEpochMilli().toULong()

Recommendation: Standardize on one approach for consistency.

7. Annotation Retention Changes

Location: DispatchersModule.kt:11-20

@Qualifier
-@Retention(AnnotationRetention.BINARY)
annotationclassUiDispatcher

Issue: Removing @Retention(AnnotationRetention.BINARY) changes the default to RUNTIME, which:

Increases APK size slightly (annotations retained in bytecode)
Allows runtime reflection (usually not needed for Dagger qualifiers)

Recommendation: Clarify if this change is intentional or if it should use BINARY retention.

8. BackupItemStatus.isRequired Property

Location: BackupCategory.kt:63

data classBackupItemStatus(
valrunning:Boolean = false,
valsynced:Long = 0,
valrequired:Long = 0,
) {
val isRequired:Boolean get() = synced < required
}

Issue: The computed property name isRequired is slightly misleading. It actually means "backup is outdated/stale," not "backup is required by user."

Recommendation: Consider renaming to isStale or needsSync for clarity.

9. Health Check Comparison Changed

Location: HealthRepo.kt:129

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

Issue: Changing from > to >= means synced == required is now considered "OK", but this is technically still a pending backup (hasn't happened yet).

Recommendation: Verify this is the intended behavior. The 5-minute grace period should handle this, but the semantic change is subtle.

🔒 Security Considerations

10. No Sensitive Data Concerns

Status: ✅ Good

Transaction IDs are public blockchain data
No credentials or private keys exposed in new code
VSS backup client properly handles encryption (outside scope of this PR)

🧪 Test Coverage

Issues with Test Coverage:

No Tests for New Functionality: The only test change is adding the backupRepo mock to WalletRepoTest.kt:51-82, but there are:
- No tests for boostTxIds tracking logic
- No tests for the CPFP child appending behavior
- No tests for the RBF parent chain tracking
- No tests for BackupRepo.reset() or scheduleFullBackup()
Missing Edge Case Tests:
- What happens if boostTxIds list grows very large (multiple RBF chains)?
- What if a transaction is boosted multiple times in succession?
- What if the activity is not found when applying pending boost?
UI Tests: No mention of UI tests for the new "Boosted Transaction" section in ActivityExploreScreen.kt:266-281.

Recommendation: Add unit tests for:

// BoostTransactionViewModel
@Test
fun`CPFP should append child txId to parent boostTxIds`()
@Test fun`RBF should track full parent chain in boostTxIds`()
@Test
fun`cachePendingBoostActivity should store parent chain for RBF`()
// ActivityRepo
@Test
fun`boostPendingActivities should apply parent chain to RBF transactions`()
// BackupRepo
@Test
fun`scheduleBackup should cancel existing job for same category`()

📊 Performance Considerations

11. Potential N+1 Query in UI

Location: ActivityExploreScreen.kt:266-281

onchain.v1.boostTxIds.forEachIndexed { index, parent ->Section(
title = stringResource(...),
// ... rendering
)
}

Issue: Each boostTxIds item renders a full Section. If a transaction has been boosted many times, this could create a very long list.

Recommendation: Consider limiting display to last N transactions with a "Show more" option, or use a lazy loading approach.

12. Backup Debounce is Acceptable

Status: ✅ Good

5-second debounce (BACKUP_DEBOUNCE = 5000L) prevents excessive backup calls
Backup check interval of 1 minute is reasonable

🎯 Recommendations Summary

Must Fix Before Merge:

✅ Fix race condition in BackupRepo.scheduleBackup() by canceling existing jobs
✅ Move vssStoreIdProvider.clearCache() inside synchronized block in VssBackupClient.reset()

Should Fix Before Merge:

⚠️ Add null safety check in BoostTransactionViewModel.cachePendingBoostActivity()
⚠️ Document backup restore order changes and add error handling
⚠️ Add unit tests for new boostTxIds functionality

Nice to Have:

💡 Standardize on nowMillis() vs nowTimestamp()
💡 Clarify annotation retention changes in DispatchersModule
💡 Rename isRequired to isStale for clarity
💡 Add UI tests for new explore screen section
💡 Consider limiting displayed boost chain length

✅ Conclusion

This is a well-structured PR with clear intent and good documentation. The core functionality for tracking boosted transaction parents is sound. However, there are 2 critical thread-safety issues that must be addressed before merging, and test coverage is insufficient for the new functionality.

Once the critical issues are resolved and tests are added, this PR will be ready to merge.

Recommendation: Request changes for issues #1, #2, #3, #4, and #5.

ovitrif · 2025-11-18T00:36:38Z

Closed in favour of:

Save boosted txs #470

ovitrif · 2025-11-06T23:57:20Z

ROADMAP: Transaction Details

This PR adds boosted txids tracking and displays parent transaction IDs in the activity details, matching iOS PR #199.

Description

Added boostTxIds to PendingBoostActivity to store parent transaction IDs
CPFP: Append child transaction ID to parent's boostTxIds when boosting
RBF:
- Track parent chain (existing boostTxIds + current txId) in PendingBoostActivity.
- Store parent txIds when creating replacement
- retrieve and apply when replacement syncs
UI: Display boosted transaction IDs in ActivityExploreScreen

Preview

CPFP (received)	RBF (sent)
TODO	TODO

QA Notes

1️⃣ RBF Test

Send onchain
Boost transaction
Activity Item > Explore
Expect list containing txid of previously boosted tx under "Boosted Transaction…" header

1️⃣ CPFP Test

Same but start with RECEIVE onchain.

Copilot

Pull Request Overview

This PR enhances boost transaction tracking by properly recording boost transaction IDs for both RBF (Replace-By-Fee) and CPFP (Child Pays For Parent) scenarios. The implementation ensures that parent-child transaction relationships are maintained and properly displayed in the activity detail screen.

Added boostTxIds field to PendingBoostActivity data class
Updated boost logic to track and persist transaction IDs through the boost process
Added UI component to display boosted transaction IDs in activity details

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
`BoostTransactionViewModel.kt`	Updated CPFP and cachePendingBoostActivity logic to properly track boost transaction IDs
`ActivityDetailScreen.kt`	Added UI section to display boosted transaction IDs with appropriate labels for RBF and CPFP
`ActivityRepo.kt`	Updated pending boost activities handling to apply boostTxIds during sync
`PendingBoostActivity.kt`	Added boostTxIds field with default empty list for backward compatibility

… explore

claude · 2025-11-07T14:46:38Z

PR Review: Activity Boosted TX Parents

Summary

This PR adds tracking and display of parent transaction IDs for boosted transactions (both CPFP and RBF), aligning with iOS implementation. Overall, the implementation is solid with good documentation and careful handling of edge cases. Below are my findings across code quality, potential issues, performance, security, and test coverage.

✅ Strengths

Excellent Documentation: The code includes comprehensive comments explaining RBF vs CPFP behavior, particularly in BoostTransactionViewModel.kt:419-430 and ActivityRepo.kt:437-447.
Proper State Management: The use of PendingBoostActivity to cache boost data for retry on sync failure is a robust pattern.
Good Separation of Concerns: Clear distinction between immediate CPFP updates and deferred RBF replacements.
Consistent Code Style: Follows Kotlin best practices with trailing commas, proper nullability handling, and immutable data structures.

🔴 Critical Issues

1. Race Condition in BackupRepo (High Priority)

Location: BackupRepo.kt:256-274

backupJobs[category] = scope.launch {
// Set running immediately to prevent UI showing failure during debounce
cacheStore.updateBackupStatus(category) {
it.copy(running =true)
}
delay(BACKUP_DEBOUNCE)
// Double-check if backup is still neededval status = cacheStore.backupStatuses.first()[category] ?:BackupItemStatus()
if (status.isRequired &&!isRestoring) {
triggerBackup(category)
} else {
// Backup no longer needed, reset running flag
cacheStore.updateBackupStatus(category) {
it.copy(running =false)
}
}
}

Issue: Not thread-safe. Multiple calls to scheduleBackup() for the same category could overwrite backupJobs[category] without canceling the previous job, leading to:

Multiple concurrent backups for the same category
Orphaned coroutines that never reset running = false
UI showing incorrect backup status

Recommendation:

backupJobs[category]?.cancel() // Cancel existing job
backupJobs[category] = scope.launch {
// ... rest of implementation
}

2. Synchronization Issue in VssBackupClient.reset()

Location: VssBackupClient.kt:65-72

funreset() {
synchronized(this) {
isSetup.cancel()
isSetup =CompletableDeferred()
}
vssStoreIdProvider.clearCache()
Logger.debug("VSS client reset", context =TAG)
}

Issue: vssStoreIdProvider.clearCache() is called outside the synchronized block, which could lead to:

Cache being cleared while setup() is reading from it
Inconsistent state between isSetup and cache

Recommendation: Move vssStoreIdProvider.clearCache() inside the synchronized block.

🟡 Medium Priority Issues

3. Missing Null Safety Check

Location: BoostTransactionViewModel.kt:422-430

val currentActivity = activity?.v1
val boostTxIds =if (activityToDelete !=null&& currentActivity !=null) {
currentActivity.boostTxIds + currentActivity.txId
} else {
emptyList()
}

Issue: activityToDelete != null is checked but currentActivity could still be null for RBF scenarios, leading to an empty boostTxIds list when it should contain the parent chain.

Recommendation: Add explicit error handling or logging when activityToDelete is non-null but currentActivity is null, as this indicates a bug.

4. Backup Order Changed Without Migration

Location: BackupRepo.kt:402-458

The restore order was changed from:

// Old orderSETTINGS → WALLET → ACTIVITY → METADATA → BLOCKTANK

to:

// New order METADATA → SETTINGS → WALLET → ACTIVITY → BLOCKTANK

Issue: METADATA is restored first to populate caches early via onCacheRestored() callback, but:

No migration logic handles cases where users have existing backups in the old format
If METADATA restore fails, subsequent restores may operate with stale cache data
The callback onCacheRestored() is not used in this PR

Recommendation:

Document why METADATA must be restored first
Add error handling if METADATA restore fails (should it abort the entire restore?)
Consider if partial restore states are acceptable

5. Potential Memory Leak in WalletRepo

Location: WalletRepo.kt:240-260

suspendfunwipeWallet(walletIndex:Int = 0): Result<Unit> = withContext(bgDispatcher) {
try {
backupRepo.reset() // Cancels all coroutines in BackupRepo_walletState.update { WalletState() }
_balanceState.update { BalanceState() }
keychain.wipe()
// ... more cleanup
}
}

Issue: backupRepo.reset() is called before state is cleared, but if any backup observer coroutines are in the middle of processing state changes, they may attempt to read from already-wiped datastores.

Recommendation: Consider the order of operations - perhaps reset state first, then cancel observers.

🟢 Minor Issues / Suggestions

6. Inconsistent Use of `nowMillis()` vs `nowTimestamp()`

The PR introduces nowMillis() but both functions are used inconsistently:

BoostTransactionViewModel.kt: Uses nowMillis().toULong()
ActivityRepo.kt:316: Still uses nowTimestamp().toEpochMilli().toULong()

Recommendation: Standardize on one approach for consistency.

7. Annotation Retention Changes

Location: DispatchersModule.kt:11-20

@Qualifier
-@Retention(AnnotationRetention.BINARY)
annotationclassUiDispatcher

Issue: Removing @Retention(AnnotationRetention.BINARY) changes the default to RUNTIME, which:

Increases APK size slightly (annotations retained in bytecode)
Allows runtime reflection (usually not needed for Dagger qualifiers)

Recommendation: Clarify if this change is intentional or if it should use BINARY retention.

8. BackupItemStatus.isRequired Property

Location: BackupCategory.kt:63

data classBackupItemStatus(
valrunning:Boolean = false,
valsynced:Long = 0,
valrequired:Long = 0,
) {
val isRequired:Boolean get() = synced < required
}

Issue: The computed property name isRequired is slightly misleading. It actually means "backup is outdated/stale," not "backup is required by user."

Recommendation: Consider renaming to isStale or needsSync for clarity.

9. Health Check Comparison Changed

Location: HealthRepo.kt:129

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

Issue: Changing from > to >= means synced == required is now considered "OK", but this is technically still a pending backup (hasn't happened yet).

Recommendation: Verify this is the intended behavior. The 5-minute grace period should handle this, but the semantic change is subtle.

🔒 Security Considerations

10. No Sensitive Data Concerns

Status: ✅ Good

Transaction IDs are public blockchain data
No credentials or private keys exposed in new code
VSS backup client properly handles encryption (outside scope of this PR)

🧪 Test Coverage

Issues with Test Coverage:

No Tests for New Functionality: The only test change is adding the backupRepo mock to WalletRepoTest.kt:51-82, but there are:
- No tests for boostTxIds tracking logic
- No tests for the CPFP child appending behavior
- No tests for the RBF parent chain tracking
- No tests for BackupRepo.reset() or scheduleFullBackup()
Missing Edge Case Tests:
- What happens if boostTxIds list grows very large (multiple RBF chains)?
- What if a transaction is boosted multiple times in succession?
- What if the activity is not found when applying pending boost?
UI Tests: No mention of UI tests for the new "Boosted Transaction" section in ActivityExploreScreen.kt:266-281.

Recommendation: Add unit tests for:

// BoostTransactionViewModel
@Test
fun`CPFP should append child txId to parent boostTxIds`()
@Test fun`RBF should track full parent chain in boostTxIds`()
@Test
fun`cachePendingBoostActivity should store parent chain for RBF`()
// ActivityRepo
@Test
fun`boostPendingActivities should apply parent chain to RBF transactions`()
// BackupRepo
@Test
fun`scheduleBackup should cancel existing job for same category`()

📊 Performance Considerations

11. Potential N+1 Query in UI

Location: ActivityExploreScreen.kt:266-281

onchain.v1.boostTxIds.forEachIndexed { index, parent ->Section(
title = stringResource(...),
// ... rendering
)
}

Issue: Each boostTxIds item renders a full Section. If a transaction has been boosted many times, this could create a very long list.

Recommendation: Consider limiting display to last N transactions with a "Show more" option, or use a lazy loading approach.

12. Backup Debounce is Acceptable

Status: ✅ Good

5-second debounce (BACKUP_DEBOUNCE = 5000L) prevents excessive backup calls
Backup check interval of 1 minute is reasonable

🎯 Recommendations Summary

Must Fix Before Merge:

✅ Fix race condition in BackupRepo.scheduleBackup() by canceling existing jobs
✅ Move vssStoreIdProvider.clearCache() inside synchronized block in VssBackupClient.reset()

Should Fix Before Merge:

⚠️ Add null safety check in BoostTransactionViewModel.cachePendingBoostActivity()
⚠️ Document backup restore order changes and add error handling
⚠️ Add unit tests for new boostTxIds functionality

Nice to Have:

💡 Standardize on nowMillis() vs nowTimestamp()
💡 Clarify annotation retention changes in DispatchersModule
💡 Rename isRequired to isStale for clarity
💡 Add UI tests for new explore screen section
💡 Consider limiting displayed boost chain length

✅ Conclusion

This is a well-structured PR with clear intent and good documentation. The core functionality for tracking boosted transaction parents is sound. However, there are 2 critical thread-safety issues that must be addressed before merging, and test coverage is insufficient for the new functionality.

Once the critical issues are resolved and tests are added, this PR will be ready to merge.

Recommendation: Request changes for issues #1, #2, #3, #4, and #5.

ovitrif · 2025-11-18T00:36:38Z

Closed in favour of:

Save boosted txs #470

ovitrif · 2025-11-06T23:57:20Z

ROADMAP: Transaction Details

This PR adds boosted txids tracking and displays parent transaction IDs in the activity details, matching iOS PR #199.

Description

Added boostTxIds to PendingBoostActivity to store parent transaction IDs
CPFP: Append child transaction ID to parent's boostTxIds when boosting
RBF:
- Track parent chain (existing boostTxIds + current txId) in PendingBoostActivity.
- Store parent txIds when creating replacement
- retrieve and apply when replacement syncs
UI: Display boosted transaction IDs in ActivityExploreScreen

Preview

CPFP (received)	RBF (sent)
TODO	TODO

QA Notes

1️⃣ RBF Test

Send onchain
Boost transaction
Activity Item > Explore
Expect list containing txid of previously boosted tx under "Boosted Transaction…" header

1️⃣ CPFP Test

Same but start with RECEIVE onchain.

Copilot

Pull Request Overview

This PR enhances boost transaction tracking by properly recording boost transaction IDs for both RBF (Replace-By-Fee) and CPFP (Child Pays For Parent) scenarios. The implementation ensures that parent-child transaction relationships are maintained and properly displayed in the activity detail screen.

Added boostTxIds field to PendingBoostActivity data class
Updated boost logic to track and persist transaction IDs through the boost process
Added UI component to display boosted transaction IDs in activity details

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
`BoostTransactionViewModel.kt`	Updated CPFP and cachePendingBoostActivity logic to properly track boost transaction IDs
`ActivityDetailScreen.kt`	Added UI section to display boosted transaction IDs with appropriate labels for RBF and CPFP
`ActivityRepo.kt`	Updated pending boost activities handling to apply boostTxIds during sync
`PendingBoostActivity.kt`	Added boostTxIds field with default empty list for backward compatibility

… explore

claude · 2025-11-07T14:46:38Z

PR Review: Activity Boosted TX Parents

Summary

This PR adds tracking and display of parent transaction IDs for boosted transactions (both CPFP and RBF), aligning with iOS implementation. Overall, the implementation is solid with good documentation and careful handling of edge cases. Below are my findings across code quality, potential issues, performance, security, and test coverage.

✅ Strengths

Excellent Documentation: The code includes comprehensive comments explaining RBF vs CPFP behavior, particularly in BoostTransactionViewModel.kt:419-430 and ActivityRepo.kt:437-447.
Proper State Management: The use of PendingBoostActivity to cache boost data for retry on sync failure is a robust pattern.
Good Separation of Concerns: Clear distinction between immediate CPFP updates and deferred RBF replacements.
Consistent Code Style: Follows Kotlin best practices with trailing commas, proper nullability handling, and immutable data structures.

🔴 Critical Issues

1. Race Condition in BackupRepo (High Priority)

Location: BackupRepo.kt:256-274

backupJobs[category] = scope.launch {
// Set running immediately to prevent UI showing failure during debounce
cacheStore.updateBackupStatus(category) {
it.copy(running =true)
}
delay(BACKUP_DEBOUNCE)
// Double-check if backup is still neededval status = cacheStore.backupStatuses.first()[category] ?:BackupItemStatus()
if (status.isRequired &&!isRestoring) {
triggerBackup(category)
} else {
// Backup no longer needed, reset running flag
cacheStore.updateBackupStatus(category) {
it.copy(running =false)
}
}
}

Issue: Not thread-safe. Multiple calls to scheduleBackup() for the same category could overwrite backupJobs[category] without canceling the previous job, leading to:

Multiple concurrent backups for the same category
Orphaned coroutines that never reset running = false
UI showing incorrect backup status

Recommendation:

backupJobs[category]?.cancel() // Cancel existing job
backupJobs[category] = scope.launch {
// ... rest of implementation
}

2. Synchronization Issue in VssBackupClient.reset()

Location: VssBackupClient.kt:65-72

funreset() {
synchronized(this) {
isSetup.cancel()
isSetup =CompletableDeferred()
}
vssStoreIdProvider.clearCache()
Logger.debug("VSS client reset", context =TAG)
}

Issue: vssStoreIdProvider.clearCache() is called outside the synchronized block, which could lead to:

Cache being cleared while setup() is reading from it
Inconsistent state between isSetup and cache

Recommendation: Move vssStoreIdProvider.clearCache() inside the synchronized block.

🟡 Medium Priority Issues

3. Missing Null Safety Check

Location: BoostTransactionViewModel.kt:422-430

val currentActivity = activity?.v1
val boostTxIds =if (activityToDelete !=null&& currentActivity !=null) {
currentActivity.boostTxIds + currentActivity.txId
} else {
emptyList()
}

Issue: activityToDelete != null is checked but currentActivity could still be null for RBF scenarios, leading to an empty boostTxIds list when it should contain the parent chain.

Recommendation: Add explicit error handling or logging when activityToDelete is non-null but currentActivity is null, as this indicates a bug.

4. Backup Order Changed Without Migration

Location: BackupRepo.kt:402-458

The restore order was changed from:

// Old orderSETTINGS → WALLET → ACTIVITY → METADATA → BLOCKTANK

to:

// New order METADATA → SETTINGS → WALLET → ACTIVITY → BLOCKTANK

Issue: METADATA is restored first to populate caches early via onCacheRestored() callback, but:

No migration logic handles cases where users have existing backups in the old format
If METADATA restore fails, subsequent restores may operate with stale cache data
The callback onCacheRestored() is not used in this PR

Recommendation:

Document why METADATA must be restored first
Add error handling if METADATA restore fails (should it abort the entire restore?)
Consider if partial restore states are acceptable

5. Potential Memory Leak in WalletRepo

Location: WalletRepo.kt:240-260

suspendfunwipeWallet(walletIndex:Int = 0): Result<Unit> = withContext(bgDispatcher) {
try {
backupRepo.reset() // Cancels all coroutines in BackupRepo_walletState.update { WalletState() }
_balanceState.update { BalanceState() }
keychain.wipe()
// ... more cleanup
}
}

Issue: backupRepo.reset() is called before state is cleared, but if any backup observer coroutines are in the middle of processing state changes, they may attempt to read from already-wiped datastores.

Recommendation: Consider the order of operations - perhaps reset state first, then cancel observers.

🟢 Minor Issues / Suggestions

6. Inconsistent Use of `nowMillis()` vs `nowTimestamp()`

The PR introduces nowMillis() but both functions are used inconsistently:

BoostTransactionViewModel.kt: Uses nowMillis().toULong()
ActivityRepo.kt:316: Still uses nowTimestamp().toEpochMilli().toULong()

Recommendation: Standardize on one approach for consistency.

7. Annotation Retention Changes

Location: DispatchersModule.kt:11-20

@Qualifier
-@Retention(AnnotationRetention.BINARY)
annotationclassUiDispatcher

Issue: Removing @Retention(AnnotationRetention.BINARY) changes the default to RUNTIME, which:

Increases APK size slightly (annotations retained in bytecode)
Allows runtime reflection (usually not needed for Dagger qualifiers)

Recommendation: Clarify if this change is intentional or if it should use BINARY retention.

8. BackupItemStatus.isRequired Property

Location: BackupCategory.kt:63

data classBackupItemStatus(
valrunning:Boolean = false,
valsynced:Long = 0,
valrequired:Long = 0,
) {
val isRequired:Boolean get() = synced < required
}

Issue: The computed property name isRequired is slightly misleading. It actually means "backup is outdated/stale," not "backup is required by user."

Recommendation: Consider renaming to isStale or needsSync for clarity.

9. Health Check Comparison Changed

Location: HealthRepo.kt:129

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

Issue: Changing from > to >= means synced == required is now considered "OK", but this is technically still a pending backup (hasn't happened yet).

Recommendation: Verify this is the intended behavior. The 5-minute grace period should handle this, but the semantic change is subtle.

🔒 Security Considerations

10. No Sensitive Data Concerns

Status: ✅ Good

Transaction IDs are public blockchain data
No credentials or private keys exposed in new code
VSS backup client properly handles encryption (outside scope of this PR)

🧪 Test Coverage

Issues with Test Coverage:

No Tests for New Functionality: The only test change is adding the backupRepo mock to WalletRepoTest.kt:51-82, but there are:
- No tests for boostTxIds tracking logic
- No tests for the CPFP child appending behavior
- No tests for the RBF parent chain tracking
- No tests for BackupRepo.reset() or scheduleFullBackup()
Missing Edge Case Tests:
- What happens if boostTxIds list grows very large (multiple RBF chains)?
- What if a transaction is boosted multiple times in succession?
- What if the activity is not found when applying pending boost?
UI Tests: No mention of UI tests for the new "Boosted Transaction" section in ActivityExploreScreen.kt:266-281.

Recommendation: Add unit tests for:

// BoostTransactionViewModel
@Test
fun`CPFP should append child txId to parent boostTxIds`()
@Test fun`RBF should track full parent chain in boostTxIds`()
@Test
fun`cachePendingBoostActivity should store parent chain for RBF`()
// ActivityRepo
@Test
fun`boostPendingActivities should apply parent chain to RBF transactions`()
// BackupRepo
@Test
fun`scheduleBackup should cancel existing job for same category`()

📊 Performance Considerations

11. Potential N+1 Query in UI

Location: ActivityExploreScreen.kt:266-281

onchain.v1.boostTxIds.forEachIndexed { index, parent ->Section(
title = stringResource(...),
// ... rendering
)
}

Issue: Each boostTxIds item renders a full Section. If a transaction has been boosted many times, this could create a very long list.

Recommendation: Consider limiting display to last N transactions with a "Show more" option, or use a lazy loading approach.

12. Backup Debounce is Acceptable

Status: ✅ Good

5-second debounce (BACKUP_DEBOUNCE = 5000L) prevents excessive backup calls
Backup check interval of 1 minute is reasonable

🎯 Recommendations Summary

Must Fix Before Merge:

✅ Fix race condition in BackupRepo.scheduleBackup() by canceling existing jobs
✅ Move vssStoreIdProvider.clearCache() inside synchronized block in VssBackupClient.reset()

Should Fix Before Merge:

⚠️ Add null safety check in BoostTransactionViewModel.cachePendingBoostActivity()
⚠️ Document backup restore order changes and add error handling
⚠️ Add unit tests for new boostTxIds functionality

Nice to Have:

💡 Standardize on nowMillis() vs nowTimestamp()
💡 Clarify annotation retention changes in DispatchersModule
💡 Rename isRequired to isStale for clarity
💡 Add UI tests for new explore screen section
💡 Consider limiting displayed boost chain length

✅ Conclusion

This is a well-structured PR with clear intent and good documentation. The core functionality for tracking boosted transaction parents is sound. However, there are 2 critical thread-safety issues that must be addressed before merging, and test coverage is insufficient for the new functionality.

Once the critical issues are resolved and tests are added, this PR will be ready to merge.

Recommendation: Request changes for issues #1, #2, #3, #4, and #5.

ovitrif · 2025-11-18T00:36:38Z

Closed in favour of:

Save boosted txs #470

ovitrif requested a review from Copilot November 6, 2025 23:57

github-advanced-securityAI found potential problems Nov 6, 2025
View reviewed changes

CopilotAI reviewed Nov 6, 2025
View reviewed changes

feat: add boosted txids tracking and display parent txids in activity…
17b44e5
… explore

ovitrif force-pushed the feat/boosted-tx-parents branch from a96c04d to 17b44e5Compare November 7, 2025 13:20

feat: copy boosted parent txid on click
cb4a77a

ovitrif mentioned this pull request Nov 18, 2025
Save boosted txs #470
Merged

ovitrif changed the base branch from master to feat/backup-polishNovember 18, 2025 00:35

ovitrif closed this Nov 18, 2025

ovitrif requested a review from Copilot November 6, 2025 23:57

github-advanced-securityAI found potential problems Nov 6, 2025
View reviewed changes

CopilotAI reviewed Nov 6, 2025
View reviewed changes

feat: add boosted txids tracking and display parent txids in activity…
17b44e5
… explore

ovitrif force-pushed the feat/boosted-tx-parents branch from a96c04d to 17b44e5Compare November 7, 2025 13:20

feat: copy boosted parent txid on click
cb4a77a

ovitrif mentioned this pull request Nov 18, 2025
Save boosted txs #470
Merged

ovitrif changed the base branch from master to feat/backup-polishNovember 18, 2025 00:35

ovitrif closed this Nov 18, 2025

ovitrif requested a review from Copilot November 6, 2025 23:57

github-advanced-securityAI found potential problems Nov 6, 2025
View reviewed changes

CopilotAI reviewed Nov 6, 2025
View reviewed changes

feat: add boosted txids tracking and display parent txids in activity…
17b44e5
… explore

ovitrif force-pushed the feat/boosted-tx-parents branch from a96c04d to 17b44e5Compare November 7, 2025 13:20

feat: copy boosted parent txid on click
cb4a77a

ovitrif mentioned this pull request Nov 18, 2025
Save boosted txs #470
Merged

ovitrif changed the base branch from master to feat/backup-polishNovember 18, 2025 00:35

ovitrif closed this Nov 18, 2025

ovitrif requested a review from Copilot November 6, 2025 23:57

github-advanced-securityAI found potential problems Nov 6, 2025
View reviewed changes

CopilotAI reviewed Nov 6, 2025
View reviewed changes

feat: add boosted txids tracking and display parent txids in activity…
17b44e5
… explore

ovitrif force-pushed the feat/boosted-tx-parents branch from a96c04d to 17b44e5Compare November 7, 2025 13:20

feat: copy boosted parent txid on click
cb4a77a

ovitrif mentioned this pull request Nov 18, 2025
Save boosted txs #470
Merged

ovitrif changed the base branch from master to feat/backup-polishNovember 18, 2025 00:35

ovitrif closed this Nov 18, 2025

ovitrif requested a review from Copilot November 6, 2025 23:57

github-advanced-securityAI found potential problems Nov 6, 2025
View reviewed changes

CopilotAI reviewed Nov 6, 2025
View reviewed changes

feat: add boosted txids tracking and display parent txids in activity…
17b44e5
… explore

ovitrif force-pushed the feat/boosted-tx-parents branch from a96c04d to 17b44e5Compare November 7, 2025 13:20

feat: copy boosted parent txid on click
cb4a77a

ovitrif mentioned this pull request Nov 18, 2025
Save boosted txs #470
Merged

ovitrif changed the base branch from master to feat/backup-polishNovember 18, 2025 00:35

ovitrif closed this Nov 18, 2025

ovitrif requested a review from Copilot November 6, 2025 23:57

github-advanced-securityAI found potential problems Nov 6, 2025
View reviewed changes

CopilotAI reviewed Nov 6, 2025
View reviewed changes

feat: add boosted txids tracking and display parent txids in activity…
17b44e5
… explore

ovitrif force-pushed the feat/boosted-tx-parents branch from a96c04d to 17b44e5Compare November 7, 2025 13:20

feat: copy boosted parent txid on click
cb4a77a

ovitrif mentioned this pull request Nov 18, 2025
Save boosted txs #470
Merged

ovitrif changed the base branch from master to feat/backup-polishNovember 18, 2025 00:35

ovitrif closed this Nov 18, 2025

ovitrif requested a review from Copilot November 6, 2025 23:57

github-advanced-securityAI found potential problems Nov 6, 2025
View reviewed changes

CopilotAI reviewed Nov 6, 2025
View reviewed changes

feat: add boosted txids tracking and display parent txids in activity…
17b44e5
… explore

ovitrif force-pushed the feat/boosted-tx-parents branch from a96c04d to 17b44e5Compare November 7, 2025 13:20

feat: copy boosted parent txid on click
cb4a77a

ovitrif mentioned this pull request Nov 18, 2025
Save boosted txs #470
Merged

ovitrif changed the base branch from master to feat/backup-polishNovember 18, 2025 00:35

ovitrif closed this Nov 18, 2025

ovitrif requested a review from Copilot November 6, 2025 23:57

github-advanced-securityAI found potential problems Nov 6, 2025
View reviewed changes

CopilotAI reviewed Nov 6, 2025
View reviewed changes

feat: add boosted txids tracking and display parent txids in activity…
17b44e5
… explore

ovitrif force-pushed the feat/boosted-tx-parents branch from a96c04d to 17b44e5Compare November 7, 2025 13:20

feat: copy boosted parent txid on click
cb4a77a

ovitrif mentioned this pull request Nov 18, 2025
Save boosted txs #470
Merged

ovitrif changed the base branch from master to feat/backup-polishNovember 18, 2025 00:35

ovitrif closed this Nov 18, 2025

Uh oh!

Conversation

ovitrif commented Nov 6, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Preview

QA Notes

1️⃣ RBF Test

1️⃣ CPFP Test

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

claudeBot commented Nov 7, 2025

PR Review: Activity Boosted TX Parents

Summary

✅ Strengths

🔴 Critical Issues

1. Race Condition in BackupRepo (High Priority)

2. Synchronization Issue in VssBackupClient.reset()

🟡 Medium Priority Issues

3. Missing Null Safety Check

4. Backup Order Changed Without Migration

5. Potential Memory Leak in WalletRepo

🟢 Minor Issues / Suggestions

6. Inconsistent Use of nowMillis() vs nowTimestamp()

7. Annotation Retention Changes

8. BackupItemStatus.isRequired Property

9. Health Check Comparison Changed

🔒 Security Considerations

10. No Sensitive Data Concerns

🧪 Test Coverage

Issues with Test Coverage:

📊 Performance Considerations

11. Potential N+1 Query in UI

12. Backup Debounce is Acceptable

🎯 Recommendations Summary

Must Fix Before Merge:

Should Fix Before Merge:

Nice to Have:

✅ Conclusion

Uh oh!

ovitrif commented Nov 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

ovitrif commented Nov 6, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Preview

QA Notes

1️⃣ RBF Test

1️⃣ CPFP Test

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ovitrif commented Nov 6, 2025•
edited
Loading

6. Inconsistent Use of `nowMillis()` vs `nowTimestamp()`

ovitrif commented Nov 6, 2025•
edited
Loading

6. Inconsistent Use of `nowMillis()` vs `nowTimestamp()`

ovitrif commented Nov 6, 2025•
edited
Loading