Save boosted txs

[ben-kaufman]

This PR saves the boosted tx ids for an activity and shows them in activity details.
It also replaces the deleting of an RBFed tx with marking it as removed from mempool, and adds a check to mark boosted tx as removed if the original tx got confirmed.

Same as: synonymdev/bitkit-ios#199 and synonymdev/bitkit-ios#230

[github-advanced-security]

detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

[ovitrif]

@ben-kaufman guess I can close #470 in favour of this one?!

EDIT: #460.

[ben-kaufman]

@ben-kaufman guess I can close #470 in favour of this one?!

@ovitrif ideally yes

[ovitrif]

@ben-kaufman guess I can close #470 in favour of this one?!

@ovitrif ideally yes

Thanks! I meant #460, but I think you understood me 🙏🏻

[ovitrif]

sorry for removing last Claude code review, was not my intention.
another push should trigger it again.

[Copilot]

Pull Request Overview

This PR implements functionality to track and display boosted transaction IDs for both RBF (Replace-By-Fee) and CPFP (Child-Pays-For-Parent) boost operations. Instead of deleting replaced transactions, they are now marked as removed from mempool with a doesExist = false flag.

Key changes:

• Boost transaction IDs are now stored in the boostTxIds field for both RBF and CPFP operations
• RBFed transactions are marked as removed from mempool instead of being deleted
• Logic added to mark replacement transactions as removed when the original transaction confirms
• UI updated to display boost transaction IDs and show removed status

Reviewed Changes

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

Show a summary per file

File	Description
BoostTransactionViewModel.kt	Updated to save parent/child transaction IDs to boostTxIds for both RBF and CPFP operations
ActivityRow.kt	Added UI display for removed transactions
ActivityExploreScreen.kt	Shows boosted transaction IDs with copy-to-clipboard functionality; removed unused navigation callback
ActivityDetailScreen.kt	Added test tag for removed transaction status
CoreService.kt	Added logic to mark replacement transactions as removed when original transaction confirms
ActivityRepo.kt	Changed deletion to marking as removed from mempool; updated pending boost processing to include parentTxId
PendingBoostActivity.kt	Added optional parentTxId field for tracking RBF relationships

[ovitrif]

@ben-kaufman Can we delay merging until PR #461 is merged?!

TY

[piotr-iohk]

Please note, 'Removed from mempool' tx is showing a "Boosting icon" and I believe it shouldn't:

Also there's a discrepancy between iOS and Android. On iOS the label in the tx details reads "Transactions Removed from Mempool" and in Android "Removed from Mempool". I think it should be unified.

In addition I'd like to also ask as in synonymdev/bitkit-ios#230 (comment) to provide StatusRemoved test id for the removed tx.

[ben-kaufman]

Which version should we go with then for the UI?

[ovitrif]

Which version should we go with then for the UI?

Android seems more proper IMHO: both:

• icon because it's using an icon from Figma design system
• copy because it's cleaner and to the point

[ben-kaufman]

Updated the iOS PR so it should look the same

[piotr-iohk]

Updated the iOS PR so it should look the same

E2e tests updated and passing on both iOS and Android.

[claude]

Pull Request Review: Save Boosted TXs

Summary

This PR implements functionality to track and display boosted transaction relationships for both RBF and CPFP transaction boosting mechanisms. The changes properly handle the lifecycle of replaced transactions by marking them as "removed from mempool" rather than deleting them, and track the relationship between parent and replacement transactions.

Code Quality & Best Practices

✅ Strengths

1. Well-documented code: Comprehensive KDoc comments explain the purpose and behavior of new methods (e.g., markActivityAsRemovedFromMempool, markReplacementTransactionsAsRemoved)

2. Proper data modeling: The addition of parentTxId to PendingBoostActivity with a default null value maintains backward compatibility

3. Consistent error handling: All new methods follow the repository's error handling patterns with proper logging via Logger.error/warn/debug

4. Good separation of concerns: The logic is properly distributed between ActivityRepo, CoreService, and BoostTransactionViewModel

5. Existing test coverage: The codebase has good test coverage for ActivityRepo and BoostTransactionViewModel

⚠️ Areas for Improvement

1. Performance Concern in markReplacementTransactionsAsRemoved (CoreService.kt:485-527)

val allActivities = getActivities(
    filter = ActivityFilter.ONCHAIN,
    txType = null,
    tags = null,
    search = null,
    minDate = null,
    maxDate = null,
    limit = null,  // ← No limit!
    sortDirection = null
)

Issue: This fetches ALL onchain activities without a limit, which could be expensive for users with large transaction histories. The loop then iterates through all of them.

Recommendation:

• Add a limit parameter (e.g., last 100 unconfirmed transactions)
• Consider indexing or filtering at the database level to only fetch unconfirmed transactions with non-empty boostTxIds
• Add pagination if the transaction count grows large

2. Race Condition Risk (ActivityRepo.kt:469-489)

The markActivityAsRemovedFromMempool method reads and then updates without atomic transaction guarantees. If multiple threads call this simultaneously, there could be conflicts.

Recommendation: Consider if the underlying storage layer needs transaction isolation or if the coroutine dispatcher already provides sufficient serialization.

3. Inconsistent Timestamp Generation (CoreService.kt:515)

updatedAt = System.currentTimeMillis().toULong() / 1000u

Issue: Uses System.currentTimeMillis() directly while other places use nowTimestamp() helper.

Recommendation: Use the nowTimestamp() helper for consistency (as done in ActivityRepo.kt:478 and BoostTransactionViewModel.kt:311).

4. Missing Null Safety Check (ActivityRepo.kt:430-434)

val updatedBoostTxIds = if (pendingBoostActivity.parentTxId != null) {
    newOnChainActivity.v1.boostTxIds + pendingBoostActivity.parentTxId
} else {
    newOnChainActivity.v1.boostTxIds
}

Observation: This is correct, but the same pattern in BoostTransactionViewModel.kt:306 and 386 doesn't check for null because parentTxId is passed directly. Consider documenting that parentTxId should never be null in RBF scenarios.

5. UI/UX: Removed Navigation Feature (ActivityExploreScreen.kt:116-119)

The PR removes the onClickParent callback that was previously used to navigate to parent transaction details:

- onClickParent = { id ->
-     app.toast(
-         type = Toast.ToastType.WARNING,
-         title = "TODO",
-         description = "Navigate to Activity Detail for: $id",
-     )
- },

Issue: Now users can only copy the boosted TX ID (line 284) but cannot navigate to view the full details of that transaction. This is a regression in user experience.

Recommendation:

• Implement navigation to the activity detail screen for boosted transactions
• Or at least preserve the TODO toast to indicate this feature is planned

Potential Bugs

🐛 Bug 1: Logic Error in Confirmation Detection (CoreService.kt:441-443)

// If a removed transaction confirms, mark its replacement transactions as removed
if (wasRemoved && isConfirmed) {
    markReplacementTransactionsAsRemoved(originalTxId = kind.txid)
}

Issue: This logic seems inverted. If a transaction that was previously marked as removed (doesExist = false) actually gets confirmed, it means the "removed" transaction won the race and is the canonical one. In this case, you correctly want to mark replacements as removed.

However, the comment in markReplacementTransactionsAsRemoved says "This is called when a removed RBFed transaction gets confirmed" - but RBFed transactions that are removed should NOT confirm. If they do confirm, it means they weren't actually replaced.

Scenario:

1. User creates TX A
2. User RBF replaces with TX B (TX A marked as doesExist=false)
3. TX A somehow confirms (race condition, miner included old TX)
4. This code runs and marks TX B as removed (correct!)

This logic appears correct but the naming/comments are confusing. Consider renaming or clarifying that this handles the edge case where an "replaced" transaction actually wins confirmation.

🐛 Bug 2: Potential Double-Update Issue (ActivityRepo.kt:443-459)

if (pendingBoostActivity.activityToDelete != null) {
    replaceActivity(
        id = updatedActivity.v1.id,
        activity = updatedActivity,
        activityIdToDelete = pendingBoostActivity.activityToDelete
    ).onSuccess {
        cacheStore.removeActivityFromPendingBoost(pendingBoostActivity)
    }
} else {
    updateActivity(
        id = updatedActivity.v1.id,
        activity = updatedActivity
    ).onSuccess {
        cacheStore.removeActivityFromPendingBoost(pendingBoostActivity)
    }
}

Issue: Both branches call updateActivity (directly or via replaceActivity), but if replaceActivity fails during the mark-as-removed step (line 267), it adds the activity back to pending delete cache (line 273). This could cause the boost metadata to be applied multiple times on retry.

Risk Level: Low - the timestamp check on line 425-428 should prevent duplicate application, but worth verifying.

Security Concerns

✅ No Critical Issues Found

1. Input validation: The code properly validates inputs (e.g., checking for null activities, empty strings)
2. No SQL injection risk: Uses parameterized queries through the ORM layer
3. Proper error handling: Errors are caught and logged without exposing sensitive information
4. No authentication/authorization issues: This operates within the existing security context

💡 Minor Consideration

The markReplacementTransactionsAsRemoved method catches all exceptions (line 520), which is good for robustness but could hide unexpected issues. Consider more specific exception handling or alerting for unexpected errors in production.

Test Coverage

✅ Existing Coverage

• BoostTransactionViewModelTest.kt: Good coverage of boost flow, includes CPFP test (line 196-238)
• ActivityRepoTest.kt: Comprehensive coverage of activity operations, includes addActivityToPendingBoost test (line 439-450)

❌ Missing Test Coverage

1. No tests for markActivityAsRemovedFromMempool - This new critical method lacks unit tests
2. No tests for markReplacementTransactionsAsRemoved - This complex logic needs test coverage for:
   • Successful marking of replacement transactions
   • Edge case: What if there are multiple replacement chains (A → B → C)?
   • Edge case: What if boostTxIds is empty?
3. No tests for boostPendingActivities with new parentTxId - Should verify that parentTxId is properly added to boostTxIds
4. No integration tests for the full RBF flow with parent TX ID tracking

Recommendation: Add unit tests for the new markActivityAsRemovedFromMempool method and the updated boostPendingActivities logic that includes parentTxId.

Alignment with iOS Implementation

✅ The PR description notes this aligns with iOS PRs #199 and #230, which is good for cross-platform consistency.

Summary of Recommendations

High Priority

1. ⚠️ Add limit/pagination to markReplacementTransactionsAsRemoved to prevent performance issues
2. 🐛 Verify the edge case handling when a "removed" transaction confirms
3. 🧪 Add unit tests for markActivityAsRemovedFromMempool and the parentTxId logic

Medium Priority

4. 🔄 Use nowTimestamp() consistently instead of System.currentTimeMillis()
5. 📝 Clarify comments around the "removed transaction confirms" scenario
6. 🖱️ Restore navigation capability for boosted transaction IDs (removed TODO at line 116)

Low Priority

7. 🔍 Review error handling specificity in markReplacementTransactionsAsRemoved
8. 📊 Consider adding telemetry/metrics for RBF edge cases

Conclusion

This is a solid PR that implements an important feature for transaction management. The code quality is good with proper documentation and error handling. The main concerns are:

• Performance optimization needed for large transaction histories
• Missing test coverage for critical new methods
• UI regression where users lost the ability to navigate to boosted transactions

The logic appears sound, but the edge case where a "removed" transaction confirms deserves extra attention and testing to ensure it behaves correctly in production.

Recommendation: Request changes to address performance concerns and add test coverage before merging.

---

Review completed with attention to code quality, security, performance, and maintainability. No blocking security issues found.

[claude]

Pull Request Review: Save boosted txs

I've reviewed PR #470 which implements saving boosted transaction IDs for activities and changes the behavior from deleting RBFed transactions to marking them as removed from mempool.

✅ Strengths

Well-tested implementation:

• Comprehensive test coverage with 417 new test lines covering edge cases
• Tests include scenarios for RBF, CPFP, pending boosts, and activity removal
• Good use of helper functions like createOnchainActivity() for test maintainability

Clear semantic changes:

• Replacing deletion with doesExist = false is semantically correct for RBF transactions
• The markActivityAsRemovedFromMempool() naming clearly communicates intent
• Proper handling of the dual boost scenarios (RBF vs CPFP)

Good UI integration:

• Added visual indicators for removed transactions in ActivityRow.kt:104
• Proper test tags for UI testing
• Changed boosted transaction display from placeholder ("todo_first_parent_txid") to real data

🔍 Issues & Concerns

1. Performance Issue in markReplacementTransactionsAsRemoved() (CoreService.kt:485-527)

The function fetches ALL onchain activities without a limit:

val allActivities = getActivities(
    filter = ActivityFilter.ONCHAIN,
    txType = null,
    tags = null,
    search = null,
    minDate = null,
    maxDate = null,
    limit = null,  // ⚠️ No limit!
    sortDirection = null
)

Then iterates through all of them in a linear search. This could be slow for users with many transactions.

Recommendation: Consider adding a limit or using a more targeted query if the activity data model supports searching by boostTxIds.

2. Potential Race Condition in replaceActivity() (ActivityRepo.kt:248-287)

The function performs these operations sequentially:

1. Update new activity
2. Copy tags from old to new
3. Mark old activity as removed

If the app crashes between steps 1-2 or 2-3, the old activity won't be marked as removed. While this is mitigated by the pending delete cache, there's still a window where the data is inconsistent.

Recommendation: Consider whether these operations should be wrapped in a transaction or if the current retry mechanism via cacheStore.addActivityToPendingDelete() is sufficient for your use case.

3. Inconsistent Return Types (ActivityRepo.kt:275)

markActivityAsRemovedFromMempool(activityIdToDelete).onFailure { e ->
    // ...
    cacheStore.addActivityToPendingDelete(activityId = activityIdToDelete)
}
Result.success(Unit)  // Always returns success even if marking failed

The function returns Result.success(Unit) even when markActivityAsRemovedFromMempool() fails. This means callers can't distinguish between full success and partial success.

Recommendation: Consider returning the result from markActivityAsRemovedFromMempool() or documenting that partial failures are expected and handled via cache.

4. Missing Null Safety in Tag Transfer (ActivityRepo.kt:264)

val tags = coreService.activity.tags(activityIdToDelete)
addTagsToActivity(activityId = id, tags = tags)

If activityIdToDelete doesn't exist, what does tags() return? The code assumes it won't throw, but this should be verified or wrapped in error handling.

5. UI Navigation Removed Without Alternative (ActivityExploreScreen.kt:116-123)

The TODO for navigating to Activity Detail was removed:

- onClickParent = { id ->
-     app.toast(
-         type = Toast.ToastType.WARNING,
-         title = "TODO",
-         description = "Navigate to Activity Detail for: $id",
-     )
- },

Now the boosted transaction IDs are only copyable (line 277-280). Users might expect to tap to view details.

Recommendation: Consider whether click navigation to the transaction details would improve UX, even if it's just for copying the ID.

6. Timestamp Precision Inconsistency (CoreService.kt:515)

updatedAt = System.currentTimeMillis().toULong() / 1000u

vs elsewhere using:

updatedAt = nowTimestamp().toEpochMilli().toULong()

Recommendation: Use nowTimestamp() consistently for better testability and consistency.

🔒 Security Considerations

No major security concerns identified. The changes are primarily data model updates with proper validation.

📊 Test Coverage

Excellent test coverage with specific tests for:

• ✅ Marking activities as removed from mempool
• ✅ Adding parentTxId to boostTxIds
• ✅ Preserving existing boostTxIds
• ✅ Handling null parentTxId
• ✅ RBF and CPFP scenarios
• ✅ Skipping updates when activity is newer

📝 Minor Code Quality Items

1. ActivityExploreScreen.kt:261 - The comment could be clearer:

   // Display boosted transaction IDs from boostTxIds
   // For CPFP (RECEIVED): shows child transaction IDs that boosted this parent
   // For RBF (SENT): shows parent transaction IDs that this replacement replaced

   The "shows parent transaction IDs that this replacement replaced" is a bit confusing (should probably be "shows original transaction IDs that were replaced by this transaction").

2. BoostTransactionViewModel.kt:318-324 - Good documentation on the RBF handling explaining why parentTxId is needed.

Summary

This is a solid PR with good test coverage and clear intent. The main concerns are:

1. Performance - Fetching all activities in markReplacementTransactionsAsRemoved() could be slow
2. Consistency - Timestamp generation and error handling could be more consistent
3. UX - Consider adding navigation for boosted transactions instead of just copy

The changes correctly implement the desired behavior of tracking boosted transactions and preserving RBFed transaction history rather than deleting them.

Recommendation: Address the performance concern before merging, the others are nice-to-haves.

[ovitrif]

@ben-kaufman please merge master into this, fix conflicts and push, I will review afterwards 🙏🏻

[ben-kaufman]

@ovitrif I updated the branch (had no conflicts)

[ovitrif]

@ovitrif I updated the branch (had no conflicts)

Thanks!
Not sure you're using the lint, but note that you can run ./gradlew detekt before push, to check & fix lint issues.

Feel free to ignore some of the issues, we don't have to respect them blindly.

[claude]

PR Review: Save boosted txs

Overview

This PR implements tracking of boosted transaction IDs for activities and changes the behavior from deleting RBFed transactions to marking them as removed from mempool. The implementation is well-structured with comprehensive test coverage.

---

✅ Strengths

1. Excellent Test Coverage: Added 417 lines of tests covering various scenarios including edge cases for RBF and CPFP flows
2. Good Documentation: Clear comments explaining the purpose of changes, especially around RBF vs CPFP behavior
3. Backward Compatibility: The parentTxId field has a default value of null, ensuring compatibility with existing data
4. Comprehensive Logic: The markReplacementTransactionsAsRemoved function properly handles the case where an original transaction confirms after being replaced

---

🔍 Code Quality & Best Practices

ActivityRepo.kt (lines 248-296)

• ✅ Good: Changed from deleting to marking as removed preserves transaction history
• ✅ Good: Proper error handling with Result types
• ⚠️ Note: The replaceActivity function now returns Result.success(Unit) explicitly, which is good for clarity

CoreService.kt (lines 512-553)

• ✅ Good: markReplacementTransactionsAsRemoved has proper error handling
• ⚠️ Potential Issue: This function fetches ALL onchain activities without pagination (limit = null). This could be a performance concern if there are many activities.
  • Recommendation: Consider adding pagination or filtering by status/timestamp to reduce the query size

BoostTransactionViewModel.kt

• ✅ Good: Clear separation of RBF and CPFP handling logic
• ✅ Good: Proper propagation of parentTxId through the entire flow
• ✅ Good: Comments explain the rationale for storing parent transaction IDs

ActivityExploreScreen.kt (lines 261-291)

• ✅ Good: Removed the TODO placeholder and implemented actual functionality
• ✅ Good: Changed from navigation action to copy-to-clipboard, which makes sense for transaction IDs
• ✅ Good: The display logic correctly differentiates between RBF and CPFP boost types

---

🐛 Potential Bugs & Edge Cases

1. ActivityRepo.kt:437-446 - Race condition consideration:

   val updatedBoostTxIds = if (pendingBoostActivity.parentTxId != null) {
       newOnChainActivity.v1.boostTxIds + pendingBoostActivity.parentTxId
   } else {
       newOnChainActivity.v1.boostTxIds
   }

   • ✅ This correctly preserves existing boostTxIds when adding a parent
   • ✅ The logic handles null parentTxId properly

2. CoreService.kt:529 - Logical correctness:

   val isReplacement = onchainActivity.boostTxIds.contains(originalTxId) &&
       onchainActivity.doesExist &&
       !onchainActivity.confirmed

   • ✅ Good: Only marks unconfirmed existing transactions as removed
   • ✅ This prevents incorrectly marking already-confirmed transactions

3. CoreService.kt:458-470 - Restoration logic:

   val wasRemoved = !existingActivity.v1.doesExist
   val shouldRestore = wasRemoved && isConfirmed

   • ✅ Good: If a previously removed transaction confirms, it's restored
   • ✅ This handles the edge case where blockchain reorganizations might confirm an "old" transaction

---

⚡ Performance Considerations

1. Concern: markReplacementTransactionsAsRemoved in CoreService.kt:512-553

   • Fetches ALL onchain activities without limits
   • Iterates through all activities to find replacements
   • Impact: Could be slow with large transaction histories
   • Recommendation: Consider:
     • Adding a filter for unconfirmed transactions only
     • Adding an index on boostTxIds if the underlying storage supports it
     • Or maintaining a separate mapping of parent->child relationships

2. Minor: Multiple sequential activity updates in boostPendingActivities

   • Current implementation uses async/awaitAll which is good
   • ✅ Parallelization is properly implemented

---

🔒 Security Concerns

• ✅ No security issues identified
• ✅ Input validation is appropriate (transaction IDs are strings)
• ✅ No injection vulnerabilities
• ✅ Proper use of safe navigation operators

---

🧪 Test Coverage

Excellent test coverage with 417 new test lines!

Tests cover:

• ✅ markActivityAsRemovedFromMempool functionality
• ✅ boostPendingActivities with and without parentTxId
• ✅ Preserving existing boostTxIds when adding new ones
• ✅ replaceActivity marking old activity as removed instead of deleting
• ✅ Edge cases with non-existent activities

Suggestions for additional tests:

1. Test the markReplacementTransactionsAsRemoved function directly (currently only tested indirectly)
2. Test behavior when boostTxIds list is very large
3. Test concurrent boost operations (if applicable)

---

📝 Minor Suggestions

1. ActivityDetailScreen.kt:586 - New test tag added:

   statusTestTag = "StatusRemoved"

   • ✅ Good for E2E testing
   • Consider documenting this in docs/e2e-test-ids.md if not already done

2. ActivityRow.kt:102-106 - Display logic:

   • ✅ Good: Shows "Removed" status before other statuses
   • This is the correct priority

3. Consider adding logging for when markReplacementTransactionsAsRemoved marks activities:

   • Already present! ✅ (line 534-538 in CoreService.kt)

---

🎯 Summary

This is a well-implemented PR with:

• ✅ Clear purpose and good architectural decisions
• ✅ Excellent test coverage
• ✅ Proper error handling
• ✅ Good code documentation
• ⚠️ One minor performance concern with the unfiltered activity query

Recommendation: Approve with one minor optimization suggestion

The performance concern is minor and only affects edge cases with large transaction histories. Consider optimizing the markReplacementTransactionsAsRemoved query in a follow-up if needed.

---

Review generated by Claude Code

[claude]

Pull Request Review: Save Boosted Transactions

Overview

This PR implements tracking of boosted transaction IDs and changes the handling of RBF transactions from deletion to marking as removed from mempool. Overall, the implementation is solid with comprehensive test coverage. Below are my detailed findings.

---

✅ Strengths

1. Excellent Test Coverage

• 418 new lines of test code added to ActivityRepoTest.kt
• Comprehensive test cases covering:
  • RBF transaction replacement scenarios
  • CPFP boost tracking
  • Edge cases like confirming removed transactions
  • Pending boost activity handling
• Great use of helper functions (createOnchainActivity) for test data setup

2. Proper Data Model Evolution

• Added parentTxId field to PendingBoostActivity with default value for backward compatibility
• Added boostTxIds tracking to OnchainActivity
• Maintains data integrity through the boost lifecycle

3. Improved UX

• Better semantics: "removed from mempool" vs "deleted"
• Users can now see the relationship between original and boosted transactions
• Activity details show clickable boosted transaction IDs

---

🐛 Potential Issues

1. Race Condition in markReplacementTransactionsAsRemoved (CoreService.kt:508)

Location: CoreService.kt:508-553

Issue: The function iterates through all activities and marks replacements as removed when an original transaction confirms. However, there's no transaction/locking mechanism, which could lead to:

• Concurrent modifications if multiple confirmations happen simultaneously
• Inconsistent state if the function is called multiple times in parallel

Recommendation:

// Consider adding synchronization or using a mutex
private val replacementMarkingMutex = Mutex()

private suspend fun markReplacementTransactionsAsRemoved(originalTxId: String) {
    replacementMarkingMutex.withLock {
        // existing implementation
    }
}

2. Silent Failure in Error Handling (CoreService.kt:546)

Location: CoreService.kt:546-551

Issue: Errors are logged but not propagated or handled. The system continues as if nothing happened, potentially leaving data in an inconsistent state.

} catch (e: Exception) {
    Logger.error(
        "Error marking replacement transactions as removed for originalTxId: $originalTxId",
        e,
        context = TAG
    )
}

Recommendation: Consider whether this should:

• Retry the operation
• Cache the failed operation for retry during next sync
• At minimum, track these failures in metrics/monitoring

3. Unbounded Query in markReplacementTransactionsAsRemoved (CoreService.kt:511)

Location: CoreService.kt:511-519

Issue: getActivities() with limit = null retrieves ALL activities. For users with thousands of transactions, this could cause performance issues.

val allActivities = getActivities(
    filter = ActivityFilter.ONCHAIN,
    // ... other params
    limit = null,  // ⚠️ Could return thousands of activities
)

Recommendation:

• Query only unconfirmed activities with confirmed = false
• Add a date filter to limit the search window (e.g., last 30 days)
• Consider maintaining an index of pending boost relationships

4. Missing Null Safety Check (ActivityExploreScreen.kt:271)

Location: ActivityExploreScreen.kt:271-289

Issue: The code accesses onchain.v1.boostTxIds without checking if it's empty first in the conditional logic, though it does check in the outer if statement. While this works, the logic could be clearer.

Minor improvement suggestion:

val boostTxIds = onchain.v1.boostTxIds
if (boostTxIds.isNotEmpty()) {
    val isRbf = onchain.boostType() == BoostType.RBF
    boostTxIds.forEachIndexed { index, boostedTxId ->
        // existing implementation
    }
}

---

🔒 Security Considerations

✅ Good Practices:

1. No sensitive data exposure - Transaction IDs are safe to display
2. Input validation - Activity IDs are validated before operations
3. Proper error handling - Most failure cases are caught and logged

⚠️ Minor Concerns:

1. UI Change (ActivityExploreScreen.kt:264-289): Changed from navigating to activity detail to copying to clipboard. While safer, ensure this UX change was intentional and communicated to users.

---

⚡ Performance Considerations

1. Multiple Database Operations in Loop (CoreService.kt:525-541)

Each replacement transaction found triggers:

• updateActivity() call
• Separate database write

For multiple replacements, this could be optimized with batch updates.

2. Repeated Activity Queries

In ActivityRepo.kt:439, the code queries for activities that might have just been synced. Consider:

• Batch processing pending boost activities
• Using cached activity data when available

3. Test Performance

The test file now has 400+ lines. Consider splitting into multiple test files organized by functionality:

• ActivityRepoRBFTest.kt
• ActivityRepoCPFPTest.kt
• ActivityRepoSyncTest.kt

---

🎨 Code Quality & Best Practices

✅ Excellent:

• Clear documentation: Updated KDoc comments accurately describe behavior changes
• Consistent naming: markActivityAsRemovedFromMempool is descriptive and clear
• Follow Kotlin conventions: Proper use of coroutines, nullable types, and data classes
• Test organization: Good use of setup helpers and mock patterns

🔧 Suggestions:

1. Magic Numbers (BoostTransactionViewModel.kt:324):

updatedBoostTxIds = currentActivity.boostTxIds + childTxId

Consider validating the size of boostTxIds to prevent unbounded growth:

require(currentActivity.boostTxIds.size < MAX_BOOST_TX_IDS) { 
    "Maximum boost transactions exceeded" 
}

2. Consider Using Constants:

private const val MAX_ACTIVITIES_QUERY_LIMIT = 1000
private const val BOOST_SEARCH_WINDOW_DAYS = 30

3. Redundant Success Wrapper (ActivityRepo.kt:282):

Result.success(Unit)

This Result.success(Unit) is returned but the function signature is likely suspend fun replaceActivity(...): Result<Unit>, so this is correct. No issue here.

---

📊 Test Coverage Assessment

✅ Well-Covered:

• RBF transaction flow ✅
• CPFP transaction flow ✅
• Pending boost activity handling ✅
• Activity marking as removed ✅
• Error handling in replace activity ✅

⚠️ Missing Test Coverage:

1. markReplacementTransactionsAsRemoved function - No dedicated tests in this PR
2. Concurrent boost operations - What happens if two boosts occur simultaneously?
3. Edge case: What if boostTxIds already contains the parent? (Should it be deduplicated?)
4. UI tests for the changed ActivityExploreScreen behavior

Recommendation: Add tests for:

@Test
fun `markReplacementTransactionsAsRemoved marks correct transactions`() = test {
    // Test the CoreService functionality
}

@Test
fun `boostTxIds does not contain duplicates`() = test {
    // Verify deduplication
}

---

🎯 Recommendations Summary

High Priority:

1. Add synchronization to markReplacementTransactionsAsRemoved to prevent race conditions
2. Optimize the unbounded query - filter by confirmed status and date range
3. Add test coverage for markReplacementTransactionsAsRemoved

Medium Priority:

4. Improve error handling - consider retry logic or caching failed operations
5. Add validation for boostTxIds list size to prevent unbounded growth
6. Split test file into smaller, focused test files

Low Priority:

7. Add constants for magic numbers and query limits
8. Consider batch updates for marking multiple replacement transactions

---

✅ Conclusion

This is a well-implemented PR with strong test coverage and clear intent. The main concerns are:

• Potential performance issues with unbounded queries
• Race condition possibility in concurrent scenarios
• Missing test coverage for the new markReplacementTransactionsAsRemoved function

The semantic change from "delete" to "mark as removed from mempool" is architecturally sound and aligns with Bitcoin transaction lifecycle concepts.

Recommendation: Approve with minor revisions - Address the high-priority items before merging.

---

Great work on maintaining consistency with the iOS implementation! 🎉

[claude]

Pull Request Review: Save boosted txs (#470)

Overview

This PR implements tracking and display of boosted transaction IDs for both RBF (Replace-By-Fee) and CPFP (Child Pays For Parent) scenarios. The implementation aligns with the iOS versions in PRs #199 and #230.

---

✅ Strengths

1. Comprehensive Test Coverage

• Excellent test additions (+484 lines in ActivityRepoTest.kt)
• Tests cover key scenarios:
  • replaceActivity correctly marks activities as removed from mempool
  • boostPendingActivities handles parentTxId correctly
  • Edge cases for null parentTxId values
  • Preservation of existing boostTxIds when adding new ones
• Helper method createOnchainActivity() improves test maintainability

2. Semantic Improvement

• Replacing deletion with "removed from mempool" (doesExist = false) is semantically correct for RBF scenarios
• Preserves transaction history while indicating the transaction is no longer valid
• Allows for restoration if a removed transaction confirms (lines 458-463 in CoreService.kt)

3. Good Error Handling

• Proper fallback caching when operations fail (lines 275-282 in ActivityRepo.kt)
• Comprehensive try-catch blocks with appropriate logging

4. Clear Documentation

• Well-documented functions with clear intent
• Comments explain the semantic differences between RBF and CPFP

---

⚠️ Issues & Concerns

1. Performance Concern: markReplacementTransactionsAsRemoved()

Location: CoreService.kt:512-554

private suspend fun markReplacementTransactionsAsRemoved(originalTxId: String) {
    val allActivities = getActivities(
        filter = ActivityFilter.ONCHAIN,
        txType = null,
        ...
    )
    for (activity in allActivities) { ... }
}

Issue: This function fetches ALL onchain activities to find replacement transactions.

Impact:

• In a wallet with hundreds/thousands of transactions, this could be expensive
• Called synchronously when a transaction confirms
• No pagination or limit applied

Recommendation:

• Consider adding filtering by date (e.g., only recent unconfirmed transactions)
• Add a limit parameter to prevent unbounded queries
• Alternative: Maintain an index/mapping of boost relationships in the cache

2. Timestamp Inconsistency

Location: CoreService.kt:542 vs other locations

// CoreService.kt:542
updatedAt = System.currentTimeMillis().toULong() / 1000u

// vs ActivityRepo.kt (uses helper)
updatedAt = nowTimestamp().toEpochMilli().toULong()

Issue: Mixed timestamp generation methods

• CoreService uses System.currentTimeMillis()
• ActivityRepo uses nowTimestamp() helper

Recommendation: Use consistent timestamp generation throughout for maintainability and testability

3. Race Condition Risk

Location: ActivityRepo.kt:438-448

val updatedBoostTxIds = if (pendingBoostActivity.parentTxId != null) {
    newOnChainActivity.v1.boostTxIds + pendingBoostActivity.parentTxId
} else {
    newOnChainActivity.v1.boostTxIds
}

Issue: If multiple pending boost activities reference the same transaction, appending could create duplicates in boostTxIds.

Recommendation: Consider using a Set or adding deduplication:

val updatedBoostTxIds = if (pendingBoostActivity.parentTxId != null) {
    (newOnChainActivity.v1.boostTxIds + pendingBoostActivity.parentTxId).distinct()
} else {
    newOnChainActivity.v1.boostTxIds
}

4. Missing Error Context

Location: ActivityRepo.kt:476-498

The markActivityAsRemovedFromMempool() function returns generic errors:

return@withContext Result.failure(Exception("Activity $activityId not found"))

Recommendation: Use more specific exception types for better error handling upstream:

sealed class ActivityException : Exception() {
    class NotFound(activityId: String) : ActivityException()
    class InvalidType(activityId: String) : ActivityException()
}

5. UI Copy Behavior Change

Location: ActivityExploreScreen.kt:284-288

Before: Clicking boosted tx navigated to activity detail (commented as TODO)
After: Clicking copies to clipboard

Concern: This is a behavior change that may not be obvious to users. No visual indicator that clicking will copy.

Recommendation:

• Add a copy icon or visual cue
• Consider keeping navigation if the boosted transaction exists in activities
• Toast notification after copy would help user awareness

---

🔒 Security Assessment

✅ No security vulnerabilities identified

• No SQL injection risks (using type-safe APIs)
• No command injection
• Transaction IDs properly handled as strings
• No sensitive data exposure

---

🎯 Code Quality

Positive:

• ✅ Follows Kotlin conventions
• ✅ Proper use of coroutines and withContext
• ✅ Good separation of concerns
• ✅ Suppression annotations used appropriately (@Suppress("LargeClass"))

Minor Issues:

• Mixed timestamp generation methods (see issue POC wake2pay from notification #2)
• Could benefit from extracting magic numbers (e.g., / 1000u for timestamp conversion)

---

📋 Test Coverage Assessment

Excellent coverage for the new functionality:

• ✅ RBF scenarios with parentTxId
• ✅ CPFP scenarios without parentTxId
• ✅ Edge cases (null values, existing boostTxIds)
• ✅ Failure scenarios

Missing tests:

• ⚠️ No tests for markReplacementTransactionsAsRemoved() in CoreService
• ⚠️ No integration tests verifying the full RBF/CPFP flow
• ⚠️ No tests for the restored transaction scenario (line 459: shouldRestore)

---

📝 Recommendations Summary

High Priority:

1. Address performance concern in markReplacementTransactionsAsRemoved() - add filtering/limits
2. Add deduplication for boostTxIds to prevent duplicates

Medium Priority:

3. Standardize timestamp generation across the codebase
4. Add tests for markReplacementTransactionsAsRemoved()
5. Improve UI feedback for copy behavior in activity explore screen

Low Priority:

6. Consider more specific exception types for better error handling
7. Extract timestamp conversion constant (MILLIS_TO_SECONDS = 1000u)

---

✅ Verdict

Approved with recommendations ✨

This is a solid implementation with excellent test coverage for the core functionality. The main concern is the potential performance impact of markReplacementTransactionsAsRemoved() which should be addressed before merging to production. The semantic change from deletion to "removed from mempool" is correct and well-implemented.

The code quality is high, follows Kotlin best practices, and maintains consistency with the existing codebase structure. Great work on the comprehensive tests!

[claude]

Code Review: Save Boosted Txs

I've reviewed PR #470 and overall the implementation looks solid! The changes align well with the iOS implementations (PRs #199 and #230). Here's my detailed feedback:

✅ Strengths

1. Good architectural design: The shift from deleting RBFed transactions to marking them as removed (doesExist = false) is the right approach - maintains transaction history while clearly indicating status.

2. Comprehensive test coverage: The test file adds 484 lines of tests covering the new functionality, including edge cases for RBF/CPFP scenarios. Great work!

3. Proper error handling: Caching mechanisms for retry logic when operations fail (e.g., addActivityToPendingDelete, addActivityToPendingBoost) show good resilience.

4. Clear documentation: Function KDocs are helpful and explain the RBF/CPFP behavior well.

🐛 Potential Issues

1. Performance concern in markReplacementTransactionsAsRemoved (CoreService.kt:512)

val allActivities = getActivities(
    filter = ActivityFilter.ONCHAIN,
    txType = null,
    tags = null,
    search = null,
    minDate = null,
    maxDate = null,
    limit = null,  // ⚠️ No limit - fetches ALL activities
    sortDirection = null
)

Issue: This fetches ALL onchain activities without pagination, which could be expensive as the activity list grows. This operation happens every time a removed transaction confirms.

Recommendation: Consider one of:

• Add a reasonable limit (e.g., last 100 unconfirmed transactions)
• Add an index/query to find activities by boostTxIds directly if the underlying data store supports it
• Add a filter for unconfirmed transactions only since confirmed transactions wouldn't need this check

2. Inconsistent timestamp precision (CoreService.kt:542)

updatedAt = System.currentTimeMillis().toULong() / 1000u

vs. elsewhere in the codebase using:

updatedAt = nowTimestamp().toEpochMilli().toULong()

Recommendation: Use the same nowTimestamp() helper for consistency across the codebase.

3. Race condition potential (ActivityRepo.kt:282)

The replaceActivity function has a success path that returns Result.success(Unit) after marking the old activity as removed, but it doesn't propagate the failure if markActivityAsRemovedFromMempool fails - it only logs and caches it. This is actually fine for resilience, but the success result might be misleading.

Current behavior: Returns success even if marking as removed fails (relying on retry mechanism).
Recommendation: This is probably intentional for fault tolerance, but consider documenting this behavior in the function KDoc.

💡 Code Quality Suggestions

1. Test naming (ActivityRepoTest.kt): Great test names! Very descriptive.

2. Magic strings removed: Good job replacing the TODO placeholders ("todo_first_parent_txid") with real data from boostTxIds. The click behavior change from navigating to copying is more practical.

3. UI improvements: Adding the "Removed" status display in ActivityRow.kt and ActivityDetailScreen.kt improves UX clarity.

🔒 Security

No security concerns identified. The changes handle Bitcoin transaction data appropriately without introducing vulnerabilities.

📊 Test Coverage Assessment

Excellent test coverage! The test file includes:

• ✅ RBF replacement scenarios
• ✅ CPFP boost scenarios
• ✅ Edge cases (activity not found, wrong type)
• ✅ Pending boost activity handling
• ✅ Cache retry mechanisms
• ✅ Timestamp and state updates

Minor gap: Consider adding a test for markReplacementTransactionsAsRemoved to verify it correctly handles:

• Multiple replacement transactions
• Already confirmed replacements (should skip)
• Empty boostTxIds arrays

🎯 Summary

This is a well-implemented feature with strong test coverage. The main recommendation is to optimize the markReplacementTransactionsAsRemoved query to prevent performance degradation as the activity list grows. The other issues are minor and mostly about consistency.

Recommendation: ✅ Approve with minor suggestions

---

Review generated with assistance from Claude Code

[ovitrif]

ACK

Tested via RBF boost.

I added a micro change to always use the X icon instead of boost for these activities:

[ovitrif]

@ben-kaufman Need your review as well now.

Also please add this UI change to iOS as well:
#470 (review)

[claude]

Code Review - PR #470: Save boosted txs

Overview

This PR implements tracking of boosted transaction relationships for RBF and CPFP operations. The implementation replaces transaction deletion with a "removed from mempool" marker and displays boosted transaction IDs in activity details.

---

Positive Aspects

1. Excellent Test Coverage

• 484 new lines of tests added with comprehensive scenarios
• Tests cover:
  • boostPendingActivities with parentTxId handling
  • markActivityAsRemovedFromMempool functionality
  • replaceActivity changes
  • Edge cases like null parentTxId and existing boostTxIds
• Well-structured test helper functions (createOnchainActivity)

2. Good Documentation

• Clear KDoc comments explaining the purpose of functions
• Inline comments for complex logic (e.g., lines 467-470 in CoreService.kt)
• Function names are descriptive and self-documenting

3. Proper Error Handling

• Graceful failure handling with Result types
• Comprehensive logging at appropriate levels (debug, warn, error)
• Caching mechanism for retry on sync failures

4. Clean Architecture

• Good separation of concerns between ViewModel, Repository, and Service layers
• Consistent use of suspend functions for async operations
• Proper use of Kotlin coroutines (async/await)

---

Issues & Concerns

1. Critical: Potential Race Condition in markReplacementTransactionsAsRemoved

Location: CoreService.kt:512-554

Issue: The function fetches ALL onchain activities and iterates through them to find replacements. This could cause issues:

val allActivities = getActivities(
    filter = ActivityFilter.ONCHAIN,
    // ... fetches ALL activities
)

Problems:

• No pagination/limit - could fetch thousands of activities
• If another thread updates an activity during iteration, state could be inconsistent
• The check onchainActivity.doesExist && !onchainActivity.confirmed happens outside transaction boundaries

Recommendation:
Consider adding a limit or using a more targeted query. Also, document the threading model assumptions.

---

2. Performance: O(n) Iteration in markReplacementTransactionsAsRemoved

Location: CoreService.kt:525-546

Issue: Linear scan through all activities for each confirmed removed transaction.

for (activity in allActivities) {
    if (activity !is Activity.Onchain) continue
    val onchainActivity = activity.v1
    val isReplacement = onchainActivity.boostTxIds.contains(originalTxId) && ...

Impact:

• If user has 1000 activities and 10 RBF confirmations, this is 10,000 iterations
• Could cause noticeable UI lag

Recommendation:
Consider:

• Adding an index on boostTxIds if the storage layer supports it
• Implementing a reverse lookup map (txId -> activities that reference it)
• Adding early exit if no activities have boostTxIds

---

3. Bug: Timestamp Inconsistency

Location: ActivityRepo.kt:488 vs CoreService.kt:542

Issue: Two different ways to get current timestamp:

// ActivityRepo.kt:488
updatedAt = nowTimestamp().toEpochMilli().toULong()

// CoreService.kt:542
updatedAt = System.currentTimeMillis().toULong() / 1000u

Problems:

• First uses milliseconds, second uses seconds (note the division by 1000)
• Inconsistent timestamps could cause comparison issues
• Different time sources could cause clock skew issues

Recommendation:
Create a unified getCurrentTimestamp() utility function that returns consistent units.

---

4. Potential Bug: Missing Null Check

Location: ActivityRepo.kt:481-482

val existingActivity = getActivity(activityId).getOrNull()
    ?: return@withContext Result.failure(Exception("Activity $activityId not found"))

Issue: return@withContext Result.failure(...) is wrapped in runCatching block, which will catch this as an exception rather than propagating the Result.

Current behavior:

runCatching {
    return@withContext Result.failure(...) // This throws, not returns
}

Recommendation:

private suspend fun markActivityAsRemovedFromMempool(activityId: String): Result<Unit> = withContext(bgDispatcher) {
    runCatching {
        val existingActivity = getActivity(activityId).getOrNull()
            ?: throw Exception("Activity $activityId not found")
        
        // ... rest of logic
    }
}

---

5. UX: Removed Transactions Still Clickable

Location: ActivityExploreScreen.kt:261-290

Issue: Boosted transaction IDs are clickable and copy to clipboard, but removed transactions might no longer exist in blockchain explorers.

Recommendation:

• Consider disabling clicks on removed transactions
• Show a visual indicator (grayed out, strikethrough) for removed txIds
• Add a tooltip explaining "This transaction was replaced and removed from mempool"

---

6. Code Smell: Large Class Warning Added

Location: ActivityRepo.kt:46

@Suppress("LargeClass", "LongParameterList")

Issue: Adding LargeClass suppression suggests the class is growing too large. This PR adds more complexity.

Recommendation:
Consider refactoring into smaller, focused classes:

• ActivitySyncManager for sync operations
• ActivityBoostHandler for boost-related operations
• ActivityRepository for basic CRUD

---

7. Minor: Inconsistent Color for Removed Transactions

Location: ActivityIcon.kt:98 and ActivityDetailScreen.kt:584

Issue: Icon uses Colors.Brand (blue) but detail screen uses Colors.Red for removed status.

// ActivityIcon.kt:98
icon = when {
    !activity.doesExist() -> painterResource(R.drawable.ic_x)
    ...
},
iconColor = Colors.Brand,  // Always blue, even for removed

// ActivityDetailScreen.kt:584
statusIcon = painterResource(R.drawable.ic_x)
statusColor = Colors.Red  // Red for removed

Recommendation:
Make icon color conditional:

iconColor = when {
    !activity.doesExist() -> Colors.Red
    else -> Colors.Brand
}

---

Security Considerations

✅ No Major Security Issues Found

• No SQL injection risks (using type-safe APIs)
• No hardcoded sensitive data
• Proper validation of user inputs
• Safe handling of transaction IDs (no direct blockchain interaction in this PR)

---

Performance Considerations

1. Async Operations: Good use of async/awaitAll for parallel operations
2. Concern: markReplacementTransactionsAsRemoved could be slow with many activities
3. Suggestion: Consider adding telemetry to measure actual performance impact

---

Suggested Improvements

High Priority

1. Fix timestamp inconsistency (different units)
2. Fix the return logic in markActivityAsRemovedFromMempool
3. Add performance optimization or pagination for markReplacementTransactionsAsRemoved

Medium Priority

4. Make removed transaction icon color red for consistency
5. Add limit/pagination to prevent fetching all activities in one query
6. Consider refactoring ActivityRepo to reduce class size

Low Priority

7. Add visual indicators for removed boosted transactions in UI
8. Add integration test for the full RBF flow
9. Document thread safety assumptions

---

Test Coverage Assessment

Score: 9/10 - Excellent coverage of new functionality

What's tested:

• ✅ Boost pending activities with parentTxId
• ✅ Preserving existing boostTxIds
• ✅ Marking activities as removed from mempool
• ✅ Replace activity flow
• ✅ Null parentTxId handling

What could be added:

• Integration test for the full RBF confirmation flow
• Test for concurrent access to markReplacementTransactionsAsRemoved
• UI tests for removed transaction display

---

Summary

This is a well-implemented feature with excellent test coverage and good architecture. The main concerns are:

1. Timestamp unit inconsistency - should be fixed before merge
2. Performance of O(n) scan - monitor in production, optimize if needed
3. UI consistency - minor visual polish needed

The code quality is high, and the approach of marking transactions as removed rather than deleting them is the right design choice for maintaining history.

Recommendation: Approve with minor fixes (timestamp consistency and return logic).

---

Great work on this PR! The test coverage is particularly impressive. 🎉

[ovitrif]

cc

ACK

Tested via RBF boost.

I added a micro change to always use the X icon instead of boost for these activities:

cc. @jvsena42 , need a final re-review of my latest changes so we can merge this.

[ben-kaufman]

@ovitrif looks good, but I can't approve since it's my PR.

I also updated iOS with the same change

[ovitrif]

@ovitrif looks good, but I can't approve since it's my PR.

I also updated iOS with the same change

You could push one more change or rebase last commit of mine, then I can approve and voila 🙏🏻