Integrate LDK events

[ben-kaufman]

This PR integrates LDK node onchain events to replace the previous polling and full syncs.

It also removes Address Checker, and improves handling of RBF and CPFP transactions.

Testing:
Should test all actions, send, receive (onchain and LN), transfer to savings, transfer to spendings, etc.

[ovitrif]

utAck

I pushed a commit for a a few formatting fixes and the nit comment about the outdated docs for a function.

Starting to test 🧪

[ovitrif]

Pushed another commit for the few lint issues: 2f0122f, no lint issue remaining to fix

[claude]

Pull Request Review: LDK Events Integration

Overview

This PR successfully integrates LDK node onchain events to replace the previous polling and full syncs, removes AddressChecker, and improves handling of RBF and CPFP transactions. The changes span 27 files with 962 additions and 769 deletions.

---

✅ Strengths

Architecture Improvements

1. Event-Driven Architecture: The migration from polling to LDK events is a significant improvement that will reduce unnecessary API calls and improve responsiveness
2. Code Simplification: Removed 131 lines from AddressChecker.kt and simplified the activity syncing logic
3. Better Separation of Concerns: Event handlers in AppViewModel delegate to appropriate repositories, maintaining clean MVVM architecture

Code Quality

1. Comprehensive Event Handling: All onchain transaction events are properly handled (received, confirmed, replaced, reorged, evicted)
2. Error Handling: Consistent use of runCatching blocks with proper logging throughout the codebase
3. Null Safety: Good use of safe calls and early returns instead of not-null assertions

---

🔍 Issues & Recommendations

High Priority

1. Race Condition Risk in Event Handlers (CoreService.kt:782-808)

suspend fun handleOnchainTransactionReceived(txid: String, details: TransactionDetails) {
    ServiceQueue.CORE.background {
        runCatching {
            val payments = lightningService.payments ?: run {
                Logger.warn("No payments available for transaction $txid", context = TAG)
                return@background
            }

Issue: When a transaction event arrives, lightningService.payments might not yet contain the payment if LDK hasn't updated its internal state. This could cause legitimate transactions to be ignored.

Recommendation:

• Consider implementing a retry mechanism with a short delay
• Or sync payments explicitly before processing the event
• Add metrics/logging to track how often this condition occurs

2. Potential Data Loss in RBF Handling (CoreService.kt:838-854)

suspend fun handleOnchainTransactionReplaced(txid: String, conflicts: List<String>) {
    // ... 
    markReplacedActivity(txid, replacedActivity, conflicts)
    for (conflictTxid in conflicts) {
        val replacementActivity = getOrCreateReplacementActivity(conflictTxid)

Issue: If multiple conflicts are provided and processing one fails, the others may not be processed due to early return in runCatching block.

Recommendation: Move the runCatching inside the loop to ensure all conflicts are attempted even if one fails.

3. Missing Transaction Details Handling (CoreService.kt:529-533)

val details = transactionDetails ?: lightningService.getTransactionDetails(kind.txid)
if (details == null) {
    Logger.verbose("Transaction details not available for txid: ${kind.txid}", context = TAG)
    return null
}

Issue: For inbound payments, if transaction details aren't available, the address won't be resolved. This could lead to "Loading..." being permanently shown.

Recommendation: Add a fallback mechanism or retry logic for critical fields like address resolution.

Medium Priority

4. Inconsistent Channel ID Resolution (ActivityRepo.kt:133-168 vs CoreService.kt:642-652)

Channel finding logic exists in both ActivityRepo.findChannelForTransaction and CoreService.findChannelForTransaction.

Recommendation: Consolidate this logic in one place to avoid inconsistencies.

5. Large Class Warning (CoreService.kt:207)

ActivityService is marked with @Suppress("LargeClass") and has grown significantly with the new event handlers.

Recommendation: Consider splitting into separate services:

• ActivitySyncService for payment syncing
• ActivityEventService for event handling
• Keep ActivityService for basic CRUD operations

6. Removed Functionality Without Clear Migration (ActivityRepo.kt:166-191)

The shouldShowPaymentReceived, findClosedChannelForTransaction, and related methods were significantly changed or delegated.

Recommendation: Verify that all edge cases previously handled are still covered in the new implementation within CoreService.

Low Priority

7. Magic Numbers (AppViewModel.kt:292)

delay(DELAY_FOR_ACTIVITY_SYNC_MS)

Recommendation: Ensure this constant is well-documented explaining why the delay is necessary.

8. Test Coverage Reduction (ActivityRepoTest.kt, WalletRepoTest.kt)

Test files show significant line reductions (140 lines removed from ActivityRepoTest, 43 from WalletRepoTest).

Recommendation: Verify that the removed tests are no longer applicable and not just deleted without replacement. Add tests for new event handling logic.

9. String Resources (strings.xml)

Only 2 new strings were added. Verify all user-facing messages for the new events are using string resources and not hardcoded.

---

🔒 Security Considerations

1. Transaction Validation: ✅ Good - transactions are validated through LDK events
2. Input Sanitization: ✅ Good - txids and payment hashes come from trusted LDK node
3. Error Messages: ✅ Good - no sensitive information leaked in error logs

---

⚡ Performance Considerations

1. Event Processing: Events are processed sequentially which is good for data consistency
2. ServiceQueue Usage: ✅ Proper use of background queues for expensive operations
3. Chunked Processing: ✅ Payment sync uses chunking (CHUNK_SIZE = 50) to prevent memory issues

Recommendation: Monitor the performance impact of processing each event. If events come in bursts, consider batching updates.

---

🧪 Testing Recommendations

1. Add integration tests for event handling flow:

   • Test transaction received → confirmed flow
   • Test RBF replacement with multiple conflicts
   • Test reorg handling
   • Test eviction handling

2. Add edge case tests:

   • Events arriving out of order
   • Events for unknown transactions
   • Rapid succession of RBF replacements

3. Add tests for race conditions:

   • Event arrives before payment list is updated
   • Multiple events for same transaction

---

📝 Code Style & Best Practices

✅ Follows Guidelines:

• Proper use of StateFlow and coroutines
• Repository pattern maintained
• No ViewModels injected into services
• Error handling with runCatching
• Proper logging throughout

⚠️ Minor Issues:

• Some files could benefit from more inline documentation explaining the event flow
• Consider adding KDoc comments for public event handler methods

---

🎯 Summary

Overall Assessment: ✅ Approve with recommendations

This is a solid refactoring that modernizes the transaction handling architecture. The event-driven approach is the right direction. The main concerns are around edge cases in event handling and ensuring data consistency during rapid state changes (like multiple RBF replacements).

Critical Action Items:

1. Address the race condition in payment availability (High Priority Android - Project setup #1)
2. Fix the RBF conflict handling to process all conflicts (High Priority POC wake2pay from notification #2)
3. Add integration tests for the event handling flows
4. Verify test coverage hasn't been unintentionally reduced

Recommendation: Once the high-priority issues are addressed, this PR should be good to merge. Consider a staged rollout with monitoring to catch any edge cases in production.

---

📋 Testing Checklist (as mentioned in PR description)

Please confirm testing of:

• ✅ Send onchain
• ✅ Receive onchain
• ✅ Send Lightning
• ✅ Receive Lightning
• ✅ Transfer to savings
• ✅ Transfer to spendings
• ✅ RBF transactions
• ✅ CPFP transactions
• ✅ Transaction confirmations
• ✅ Transaction replacements

---

Review conducted with focus on code quality, architecture, security, and adherence to the repository's coding standards as defined in .github/copilot-instructions.md

[ovitrif]

Tests

1. Boost
   1. RBF 🟢
   2. CPFP 🟢
2. Activity
   1. Home Screen 🟢
   2. Activity List 🟢
   3. Activity List (and filtering) regression test 🟢
   4. Activity Detail 🟢
   5. Activity Explore 🟢
      • When electrum was very slow, an optimisation idea surfaced to me:
        • we should cache transaction details in the viewmodel of activity details
          • I went back to detail screen then returned, I had to wait again too much for the spinner to load the transaction details on the explore screen.
3. Transfers
   1. Transfer to savings 🟢
   2. Transfer to spending 🟢
4. Toasts
   5. RBF sent transaction replaced 🟢
   6. RBF from sender’s side (toast + removed from mempool activity for receiver) 🟢
5. Payment Received 🟢
   1. Onchain
      1. fg 🟢
      2. bg 🟢
   2. LN
      1. fg 🟢
      2. bg 🟢
      3. wake2pay ⚪
         • couldn’t test because right in that moment Electrum server on STAG died again.
         • but on the bright side, the wake was already starting as seen in logs

[claude]

Pull Request Review: Integrate LDK Events

I've completed a thorough review of this PR. Overall, this is a well-architected and significant improvement to the codebase. The integration of LDK node onchain events to replace polling is a solid architectural decision that should improve performance and reliability.

---

✅ Strengths

1. Excellent Architecture

• Event-driven architecture using LDK node events is the right approach
• Clean separation of concerns between ActivityRepo, CoreService.ActivityService, and AppViewModel
• Proper use of coroutines and background dispatchers throughout

2. Comprehensive Event Handling

The event handlers in AppViewModel.kt cover all critical transaction lifecycle events:

• OnchainTransactionReceived - Initial detection
• OnchainTransactionConfirmed - Confirmation handling
• OnchainTransactionReplaced - RBF handling
• OnchainTransactionReorged - Blockchain reorganization
• OnchainTransactionEvicted - Mempool eviction

3. Good Test Coverage

The ActivityRepoTest.kt file shows solid unit test coverage with tests for:

• Sync workflows
• Boost transaction handling
• Tag management
• Activity lifecycle operations

4. Removal of AddressChecker

Removing the external polling dependency (AddressChecker) in favor of LDK events is excellent for:

• Reduced external API calls
• Better performance
• More reliable data source

---

⚠️ Issues & Recommendations

1. Critical: Potential Null Pointer in Event Handlers (CoreService.kt:782-836)

Issue: The event handlers don't validate that TransactionDetails contains expected data before processing.

Recommendation:

• Consider logging at ERROR level or propagating errors when critical data is missing
• Add validation for TransactionDetails fields before use

Location: app/src/main/java/to/bitkit/services/CoreService.kt:782-836

---

2. Performance: Inefficient Loop in getTxIdsInBoostTxIds (CoreService.kt:1064-1082)

Issue: This method loads ALL onchain activities to build a set of boost transaction IDs. This could be slow with many transactions.

Recommendation:

• Consider caching this data or implementing a database query that directly returns boost transaction IDs
• Add pagination if the activity count grows large

Location: app/src/main/java/to/bitkit/services/CoreService.kt:1064-1082

---

3. Concurrency: Race Condition in processSinglePayment (CoreService.kt:623-680)

Issue: The check for existing activity and subsequent update/insert may have a race condition if multiple events fire for the same transaction.

Recommendation:

• Consider using database-level upsert or transaction to ensure atomicity
• Or use a mutex/lock per transaction ID to serialize processing

Location: app/src/main/java/to/bitkit/services/CoreService.kt:623-680

---

4. Code Quality: Large Class Warning (CoreService.kt:204-1200+)

Issue: The ActivityService class is marked with @Suppress("LargeClass") and contains 1000+ lines.

Recommendation:

• Consider splitting into smaller, focused service classes:
  • OnchainActivityService - Handle onchain transaction events
  • LightningActivityService - Handle Lightning payment events
  • ActivityQueryService - Handle reads/queries
  • ActivityTransactionService - Handle boost/RBF logic

Location: app/src/main/java/to/bitkit/services/CoreService.kt:204

---

5. Missing Error Propagation in AppViewModel (AppViewModel.kt:272-313)

Issue: Event handlers launch coroutines but don't handle failures in a user-visible way.

Recommendation:

• Add .onFailure handlers or try-catch blocks
• Log errors or show user-facing error messages when critical events fail to process

Location: app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:272-313

---

6. Test Coverage Gap: Missing Integration Tests

Observation: While unit tests are good, I don't see integration tests for the event flow:

• LDK event → ActivityRepo → CoreService → Database

Recommendation:

• Add integration tests that verify the complete event handling pipeline
• Test edge cases like:
  • Multiple rapid events for same transaction
  • Events arriving out of order
  • Events for unknown transactions

---

7. Documentation: Missing KDoc for Public Methods

Issue: Several public suspend functions lack documentation explaining their behavior, especially around edge cases.

Examples:

• handleOnchainTransactionReplaced - What happens if conflicts list is empty?
• shouldShowReceivedSheet - When exactly is this false?
• findClosedChannelForTransaction - What if details are null?

Recommendation:

• Add KDoc comments for all public API methods
• Document preconditions, postconditions, and edge case behavior

---

🔒 Security Considerations

✅ Good Security Practices

1. No sensitive data logged
2. Proper error handling prevents crashes from malformed events
3. Transaction validation before processing

⚠️ Minor Concerns

1. No rate limiting on event processing - malicious events could overwhelm the system
2. Transaction value overflow - All values use ULong which is good, but ensure arithmetic operations check for overflow

---

🎯 Performance Considerations

Improvements

• ✅ Removing polling reduces network overhead
• ✅ Event-driven updates are more efficient
• ✅ Background dispatcher usage prevents UI blocking

Concerns

• ⚠️ getTxIdsInBoostTxIds loads all activities (see issue POC wake2pay from notification #2)
• ⚠️ No obvious batching for multiple rapid events
• ⚠️ Each event triggers a separate database operation

Recommendation: Consider implementing an event buffer that batches rapid events for the same transaction.

---

📋 Test Coverage Assessment

Current Coverage: Good ⭐⭐⭐⭐☆

Strengths:

• Comprehensive unit tests for ActivityRepo
• Tests cover boost transaction logic, tag management, sync workflows
• Good use of mocking

Gaps:

• Missing tests for new event handler methods in ActivityService
• No tests for handleOnchainTransaction* methods
• Limited testing of error scenarios in event processing

---

🎨 Code Quality & Best Practices

✅ Follows Best Practices

• Proper use of Kotlin coroutines
• Immutable data classes
• Sealed class hierarchies for events
• Dependency injection with Hilt

🔧 Minor Improvements Suggested

1. Consistency: Some functions use runCatching { }.onFailure { } while others use try-catch. Pick one pattern.
2. Magic Numbers: CHUNK_SIZE = 50 and timeouts should be documented or made configurable
3. String Literals: Error messages could be moved to string resources for i18n

---

🏁 Conclusion

This is a high-quality PR that makes meaningful architectural improvements. The event-driven approach is correct and the implementation is mostly solid.

Approval Status: ✅ Approve with Minor Changes

Priority Fixes Before Merge:

1. Add error handling in AppViewModel event handlers (Issue Android - LDK state & Slashtags metadata #5)
2. Add validation for null/missing data in event handlers (Issue Android - Project setup #1)
3. Add basic tests for new event handler methods

Post-Merge Improvements:

1. Refactor ActivityService into smaller classes (Issue Migrate to ldk-node #4)
2. Optimize getTxIdsInBoostTxIds (Issue POC wake2pay from notification #2)
3. Add integration tests for event flow (Issue Migrate channels from LDK to ldk-node #6)
4. Add KDoc documentation (Issue Setup Keychain #7)

Great work on this significant refactor! The move to event-driven architecture will pay dividends in reliability and performance.

---

Reviewed using Claude Code
Automated code review assistant

[jvsena42]

Tested

• Open and close channels ✅
• Send and receive ✅
• CPFP and RBF for send and receive ✅
• Transaction details ✅
• Received sheet ✅
• All activities ✅
• Filter activities ✅

[ovitrif]

Merging this PR with its parent, then we'll make sure e2e pass there.