Integrate LDK onchain events

[ben-kaufman]

This PR integrates LDK node on chain events to replace the previous pollin and full syncs.

It also removes Address Checker, and improves handling on RBFed transactions to be recognized on both sides.

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

Screen.Recording.2025-11-26.at.9.05.11.AM.mov

Reorg removes tx from blocks demo:

Screen.Recording.2025-11-26.at.5.13.45.PM.mov

Transaction removed from mempool demo:

Screen.Recording.2025-11-26.at.5.38.36.PM.mov

[Copilot]

Pull request overview

This PR integrates LDK node onchain events to replace previous polling and full sync mechanisms for Bitcoin transaction management. It removes the AddressChecker utility class that relied on external API calls, and improves handling of RBF (Replace-By-Fee) transactions to properly distinguish them from CPFP (Child-Pays-For-Parent) transactions on both sender and receiver sides.

Key changes:

• Event-driven architecture for onchain transaction updates (received, confirmed, replaced, reorged, evicted)
• Removal of 1-second polling timer in favor of real-time LDK events
• Enhanced RBF transaction detection using doesExist flag to differentiate from CPFP

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
BitkitNotification/NotificationService.swift	Added notification handling for new onchain transaction events
Bitkit/Views/Wallets/Sheets/BoostSheet.swift	Removed redundant LDK node payment sync after boost operation
Bitkit/Views/Wallets/Activity/ActivityItemView.swift	Enhanced boost button logic to differentiate between RBF and CPFP transactions
Bitkit/Views/Wallets/Activity/ActivityExplorerView.swift	Replaced AddressChecker API calls with LDK node transaction details
Bitkit/ViewModels/WalletViewModel.swift	Removed polling mechanism and added event-driven state updates for onchain transactions
Bitkit/ViewModels/AppViewModel.swift	Added comprehensive event handling for onchain transaction lifecycle
Bitkit/Utilities/AddressChecker.swift	Removed external API-dependent utility in favor of native LDK functionality
Bitkit/Services/LightningService.swift	Added transaction detail methods and event handlers for onchain events
Bitkit/Services/CoreService.swift	Refactored payment processing to handle LDK events and removed replacement tracking maps
Bitkit/AppScene.swift	Removed balance change observer and event-based activity sync
Bitkit.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved	Updated bitkit-core and ldk-node dependencies
Bitkit.xcodeproj/project.pbxproj	Updated project references for LDK package

[ben-kaufman]

@piotr-iohk please note we will need to update the e2e tests to close the onchain received sheet.

[ovitrif]

utAck

Amazing work, I've been waiting for over a year to finally have this setup in place: event-based sync for transactions 🎉

Added a few small nit comments, the one about the localizable strings applies to all cases where we added new toast messages, not only the code where I added it.

I wanted to give you the time to already make some small updates as I am starting to test this, hence me approving without testing; but I trust you'll wait for my testing 🙏🏻

[piotr-iohk]

Ok, updated e2e tests for current state (synonymdev/bitkit-e2e-tests#56). Re-running 🤞
It would be good to coordinate merging with Android (synonymdev/bitkit-android#475)

[ben-kaufman]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

[pwltr]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

Sounds good yeah I think an optimistic UI like this would be the way to go. Assume everything went right and correct if needed when the event(s) come in from LDK. Still feel free to postpone for now.

[ben-kaufman]

Actually had a simpler solution, fixed in: #247

[pwltr]

tACK with follow-up task here

Also note @piotr-iohk's request to merge this together with the Android counterpart.

[claude]

PR Review: Integrate LDK onchain events

This is an excellent architectural improvement that modernizes the onchain transaction handling by leveraging LDK's event-driven architecture. The change from polling to event-driven updates should significantly improve performance and user experience.

---

Summary of Changes

Major Improvements:

• Event-driven onchain transaction handling via LDK Node events
• Removed AddressChecker utility (110 lines removed)
• Enhanced RBF transaction tracking and handling
• Added comprehensive onchain event handling (received, confirmed, replaced, reorged, evicted)
• Improved user notifications for transaction state changes
• Better handling of edge cases (reorgs, mempool evictions)

Impact:

• 1,031 additions, 495 deletions across 20 files
• Core architectural change in CoreService.swift (+529/-273)
• Enhanced event handling in AppViewModel.swift (+108/-4)
• Improved LightningService.swift integration (+120/-3)

---

Code Quality & Best Practices

Strengths

1. Event-Driven Architecture - Proper use of LDK onchain events eliminates polling overhead
2. Comprehensive Error Handling - Good use of try-catch blocks with descriptive logging
3. Service Queue Pattern - Correctly uses ServiceQueue.background(.core) for Core operations
4. MainActor Compliance - Proper async/await patterns with @mainactor for UI updates
5. Caching Strategy - cachedTxIdsInBoostTxIds improves performance for boost transaction lookups
6. Detailed Logging - Excellent context-aware logging throughout
7. Accessibility - Added accessibilityIdentifier to Toast model and components

Code Organization

The new event handlers follow a clean pattern:

• handleOnchainTransactionReceived
• handleOnchainTransactionConfirmed
• handleOnchainTransactionReplaced
• handleOnchainTransactionReorged
• handleOnchainTransactionEvicted

Each handler properly delegates to ServiceQueue.background(.core) and updates the activity list appropriately.

---

Potential Bugs & Issues

1. Race Condition in handleOnchainTransactionReplaced

Location: CoreService.swift:346-433

Issue: When processing replacement transactions, there is a potential race condition between checking if replacementActivity exists and processing the payment. Another event handler could process the same transaction concurrently.

Recommendation: Add a lock or transaction ID tracking to prevent concurrent processing of the same transaction.

2. Cache Invalidation Issue

Location: CoreService.swift:34-62

Issue: The cachedTxIdsInBoostTxIds cache is updated in insert and bulk refreshed in upsertList, but individual update operations don't update the cache. This could lead to stale cache data.

Recommendation: Add cache update to the update method to keep cache consistent.

3. Potential Memory Leak in Event Handlers

Location: AppScene.swift:213-215

Issue: The event handler closure captures app weakly, but the closure itself is never removed when the scene is dismissed.

Recommendation: Ensure event handlers are removed when the scene is dismissed. The addOnEvent API should support removal, or use a cleanup mechanism in deinit or onDisappear.

4. Missing nil-check in processOnchainTransaction

Location: CoreService.swift:315-332

Issue: The function logs a warning if payment is not found but doesn't throw an error. The calling code may not handle this gracefully.

Recommendation: Consider throwing a specific error so callers can decide how to handle missing payments, or add metrics to track how often this occurs.

---

Performance Considerations

Strengths

1. Eliminated Polling - Removing AddressChecker and full syncs significantly reduces CPU/network overhead
2. Efficient Caching - cachedTxIdsInBoostTxIds prevents repeated database queries
3. Lazy Cache Initialization - Cache is only built when first accessed

Concerns

1. Cache Refresh Performance - CoreService.swift:47-62 loads ALL onchain activities from the database, which could be slow for users with large transaction histories (1000+ transactions). Consider a more efficient query that only fetches boostTxIds without loading full activity objects.

2. Delay in Received Sheet - AppViewModel.swift:399 has a 500ms delay. Is this necessary? It could make the UI feel sluggish. Consider removing or reducing if not strictly required.

---

Security Concerns

Good Practices

1. Proper use of ServiceQueue prevents concurrent access to Core/LDK
2. Transactions marked as doesExist = false when replaced (prevents double-counting)
3. RBF detection prevents showing duplicate received sheets

Recommendations

1. Transaction Validation - Consider adding validation that conflicts array in handleOnchainTransactionReplaced actually contains valid transaction IDs to prevent malformed data corruption.

2. Timestamp Validation - In processOnchainPayment, ensure timestamps are within reasonable ranges to prevent time-based attacks.

---

Test Coverage

Missing Test Scenarios

1. RBF Handling - Need tests for single replacement, multiple conflicts, chain of replacements, replacement arriving before original

2. Reorg Handling - Need tests for confirmed tx becoming unconfirmed, multiple txs affected by reorg, reorg depth > 1 block

3. Edge Cases - Need tests for tx evicted from mempool that was not replaced, race conditions, cache invalidation scenarios

Testing Recommendations

Add integration tests that simulate these event sequences:

• onchainTransactionReceived → onchainTransactionReplaced → onchainTransactionConfirmed
• onchainTransactionConfirmed → onchainTransactionReorged
• onchainTransactionReceived → onchainTransactionEvicted

---

Final Recommendations

Must Fix (Before Merge)

1. Run swiftformat . to ensure code formatting compliance
2. Fix cache invalidation in update method
3. Add race condition protection in handleOnchainTransactionReplaced
4. Verify event handler cleanup to prevent memory leaks

Should Fix (High Priority)

1. Optimize refreshBoostTxIdsCache for large transaction histories
2. Add comprehensive tests for RBF and reorg scenarios
3. Review the 500ms delay in received sheet display

Nice to Have

1. Add transaction ID truncation to user-facing messages
2. Improve user-friendly language for technical events (reorg, eviction)
3. Add metrics/telemetry for tracking event frequencies

---

Conclusion

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design pattern and will eliminate polling overhead. The code quality is generally excellent with proper error handling, logging, and async patterns.

Recommendation:Approve with minor changes. Address the cache invalidation and race condition issues before merge, and plan follow-up work for comprehensive testing and performance optimization.

Great work! The videos demonstrating reorg and eviction handling are particularly impressive.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time I try it

[claude]

PR Review: Integrate LDK onchain events

Summary

This PR successfully replaces polling-based transaction monitoring with event-driven LDK onchain events, removes the AddressChecker, and improves RBF transaction handling. This is a significant architectural improvement that makes the app more reactive and efficient.

✅ Strengths

Architecture & Design

1. Event-driven architecture: The transition from polling to LDK events (onchainTransactionReceived, onchainTransactionConfirmed, onchainTransactionReplaced, onchainTransactionReorged, onchainTransactionEvicted) is excellent. This provides real-time updates and reduces unnecessary syncs.

2. Comprehensive RBF handling: The implementation properly tracks replacement transactions through boostTxIds and distinguishes between RBF (replaced transactions with doesExist=false) and CPFP (child transactions with doesExist=true).

3. Clean separation of concerns: Event handlers in LightningService.swift (708-757) delegate to ActivityService methods, maintaining good separation between Lightning layer and activity management.

4. Improved user feedback: New toast notifications with accessibility identifiers for transaction states (replaced, reorged, evicted) provide better UX.

5. Removal of polling: Removing the onChange(of: wallet.totalBalanceSats) trigger in AppScene.swift:217 and the event-based sync in AppScene.swift:217-220 eliminates redundant activity syncing.

Code Quality

1. Good error handling: All event handlers wrap operations in try-catch blocks with proper logging.
2. Proper concurrency: Uses ServiceQueue.background(.core) for all database operations.
3. Cache optimization: The cachedTxIdsInBoostTxIds cache improves performance for checking replaced transactions.

⚠️ Issues & Concerns

Critical

1. Potential cache inconsistency in CoreService.swift:36-43

func getTxIdsInBoostTxIds()async->Set<String>{if cachedTxIdsInBoostTxIds.isEmpty {awaitrefreshBoostTxIdsCache()}return cachedTxIdsInBoostTxIds
}

Issue: The cache is only refreshed when empty, but individual updates via updateBoostTxIdsCache(for:) might not be called for all updates. The update() method at line 195 doesn't update the cache.

Recommendation:

• Update the cache in the update() method, or
• Implement a TTL-based cache invalidation strategy, or
• Document why the cache only needs to be refreshed when empty

2. Race condition in RBF handling (CoreService.swift:355-441)

When handleOnchainTransactionReplaced is called, it:

1. Marks the replaced transaction as doesExist=false
2. Processes replacement transactions that might not exist yet in the payment store
3. Waits for them via processOnchainPayment

Issue: There's a timing window where a replacement transaction event arrives before LDK updates the payment store. The code handles this (line 385-403) by processing the payment, but there's a potential race if the transaction hasn't been added to the payments list yet.

Recommendation: Consider adding retry logic or ensuring LDK guarantees payment store updates before emitting replacement events.

Moderate

3. Missing cache update in update() method (CoreService.swift:193-199)

func update(id:String, activity:Activity)asyncthrows{tryawaitServiceQueue.background(.core){tryupdateActivity(activityId: id, activity: activity)self.metadataChangedSubject.send()}}

This doesn't call updateBoostTxIdsCache(for: activity) like insert() does at line 172.

Recommendation: Add cache update to maintain consistency.

4. Incomplete validation in shouldShowReceivedSheet (CoreService.swift:76-107)

The method checks if a replacement transaction has the same value as the replaced transaction to avoid showing duplicate sheets. However:

• It only checks exact value matches
• Doesn't account for fee differences in RBF
• Could show sheets for CPFP where the child transaction might have a different value

Recommendation: Consider checking isBoosted flag and the nature of the boost (RBF vs CPFP) for more precise logic.

5. Error handling in handleOnchainTransactionReplaced (CoreService.swift:355-441)

When processing replacement transactions, errors are caught and logged (line 401-405), but the loop continues. If multiple replacement transactions fail to process, the relationship tracking could be incomplete.

Recommendation: Consider collecting errors and either throwing or returning them to the caller for better error visibility.

6. CI workflow changes remove caching (integration-tests.yml, unit-tests.yml)

The PR removes DerivedData caching and restore-keys from SPM cache. While this ensures clean builds, it will increase build times significantly.

Recommendation:

• Document why caching was removed (likely due to cache invalidation issues)
• Consider using Xcode Cloud or other CI optimizations to offset the performance loss
• The -onlyUsePackageVersionsFromResolvedFile flag is good for reproducibility

Minor

7. Potential N+1 query in getBoostTxDoesExist (CoreService.swift:130-138)

func getBoostTxDoesExist(boostTxIds:[String])async->[String:Bool]{vardoesExistMap:[String:Bool]=[:]forboostTxIdin boostTxIds {
if let boostActivity =try?awaitgetOnchainActivityByTxId(txid: boostTxId){doesExistMap[boostTxId]= boostActivity.doesExist
}}return doesExistMap
}

Issue: Makes individual database queries for each boost transaction.

Recommendation: Consider adding a batch query method to BitkitCore to fetch multiple activities by txId in one query.

8. Magic number delay in AppViewModel.swift:399

try?awaitTask.sleep(nanoseconds:500_000_000) // 500ms delay

Recommendation: Extract to a named constant with documentation explaining why the delay is necessary.

9. Missing test coverage

While TxBumpingTests.swift and ActivityListTest.swift exist, there don't appear to be specific tests for:

• Onchain event handlers
• RBF replacement tracking with boostTxIds
• Reorg and eviction scenarios
• Cache consistency

Recommendation: Add integration tests for the new event handlers.

🔒 Security

No major security concerns identified. The code properly:

• Uses ServiceQueue for thread safety
• Doesn't expose sensitive transaction data in logs beyond txids
• Maintains Bitcoin/Lightning operation isolation in the service layer

Minor: Logging in LightningService.swift:711-757 includes transaction amounts. Consider whether this is acceptable for production logging.

🚀 Performance

Positive impacts:

1. Eliminates redundant polling and full activity list syncs
2. Event-driven updates only process changed transactions
3. Cache for boostTxIds lookups

Potential concerns:

1. Cache not updated consistently (see issue Project setup #1, Migrate channels from LDK to LDK-node #3)
2. N+1 queries in getBoostTxDoesExist (see issue feat: keychain #7)

🧪 Testing

Existing test coverage:

• ActivityListTest.swift: Good coverage of basic activity CRUD operations
• TxBumpingTests.swift: Tests RBF scenarios

Missing:

• Tests for new onchain event handlers
• Tests for handleOnchainTransactionReplaced, handleOnchainTransactionReorged, handleOnchainTransactionEvicted
• Tests for cache consistency
• Integration tests for the complete event flow

Recommendation: Add integration tests that simulate the event sequences shown in the PR videos.

📝 Recommendations

Must Fix Before Merge

1. Fix cache consistency issues (Issues Project setup #1, Migrate channels from LDK to LDK-node #3)
2. Add error handling for race conditions in RBF handling (Issue Push service setup and service queues  #2)

Should Fix Before Merge

1. Improve shouldShowReceivedSheet validation logic (Issue Create swift.yml #4)
2. Better error aggregation in handleOnchainTransactionReplaced (Issue Create ios.yml #5)

Consider for Follow-up

1. Add batch query support for getBoostTxDoesExist (Issue feat: keychain #7)
2. Add integration tests for event handlers (Issue Use SwiftFormat #9)
3. Extract magic constants (Issue BDK 1.0.0-alpha.11 #8)
4. Document CI caching removal reasoning (Issue Create ios.yml #6)

🎯 Overall Assessment

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design, and the RBF handling logic is well thought out. The main concerns are around cache consistency and potential race conditions in RBF processing, which should be addressed before merge.

The removal of AddressChecker and polling-based syncing will improve app performance and responsiveness.

Recommendation: Request changes to address cache consistency and race condition handling, then approve after fixes.

---

Great work on this PR! The demos showing reorg and mempool eviction handling are particularly impressive. Once the cache and race condition issues are resolved, this will be a solid improvement to the codebase.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ben-kaufman]

This PR integrates LDK node on chain events to replace the previous pollin and full syncs.

It also removes Address Checker, and improves handling on RBFed transactions to be recognized on both sides.

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

Screen.Recording.2025-11-26.at.9.05.11.AM.mov

Reorg removes tx from blocks demo:

Screen.Recording.2025-11-26.at.5.13.45.PM.mov

Transaction removed from mempool demo:

Screen.Recording.2025-11-26.at.5.38.36.PM.mov

[Copilot]

Pull request overview

This PR integrates LDK node onchain events to replace previous polling and full sync mechanisms for Bitcoin transaction management. It removes the AddressChecker utility class that relied on external API calls, and improves handling of RBF (Replace-By-Fee) transactions to properly distinguish them from CPFP (Child-Pays-For-Parent) transactions on both sender and receiver sides.

Key changes:

• Event-driven architecture for onchain transaction updates (received, confirmed, replaced, reorged, evicted)
• Removal of 1-second polling timer in favor of real-time LDK events
• Enhanced RBF transaction detection using doesExist flag to differentiate from CPFP

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
BitkitNotification/NotificationService.swift	Added notification handling for new onchain transaction events
Bitkit/Views/Wallets/Sheets/BoostSheet.swift	Removed redundant LDK node payment sync after boost operation
Bitkit/Views/Wallets/Activity/ActivityItemView.swift	Enhanced boost button logic to differentiate between RBF and CPFP transactions
Bitkit/Views/Wallets/Activity/ActivityExplorerView.swift	Replaced AddressChecker API calls with LDK node transaction details
Bitkit/ViewModels/WalletViewModel.swift	Removed polling mechanism and added event-driven state updates for onchain transactions
Bitkit/ViewModels/AppViewModel.swift	Added comprehensive event handling for onchain transaction lifecycle
Bitkit/Utilities/AddressChecker.swift	Removed external API-dependent utility in favor of native LDK functionality
Bitkit/Services/LightningService.swift	Added transaction detail methods and event handlers for onchain events
Bitkit/Services/CoreService.swift	Refactored payment processing to handle LDK events and removed replacement tracking maps
Bitkit/AppScene.swift	Removed balance change observer and event-based activity sync
Bitkit.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved	Updated bitkit-core and ldk-node dependencies
Bitkit.xcodeproj/project.pbxproj	Updated project references for LDK package

[ben-kaufman]

@piotr-iohk please note we will need to update the e2e tests to close the onchain received sheet.

[ovitrif]

utAck

Amazing work, I've been waiting for over a year to finally have this setup in place: event-based sync for transactions 🎉

Added a few small nit comments, the one about the localizable strings applies to all cases where we added new toast messages, not only the code where I added it.

I wanted to give you the time to already make some small updates as I am starting to test this, hence me approving without testing; but I trust you'll wait for my testing 🙏🏻

[piotr-iohk]

Ok, updated e2e tests for current state (synonymdev/bitkit-e2e-tests#56). Re-running 🤞
It would be good to coordinate merging with Android (synonymdev/bitkit-android#475)

[ben-kaufman]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

[pwltr]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

Sounds good yeah I think an optimistic UI like this would be the way to go. Assume everything went right and correct if needed when the event(s) come in from LDK. Still feel free to postpone for now.

[ben-kaufman]

Actually had a simpler solution, fixed in: #247

[pwltr]

tACK with follow-up task here

Also note @piotr-iohk's request to merge this together with the Android counterpart.

[claude]

PR Review: Integrate LDK onchain events

This is an excellent architectural improvement that modernizes the onchain transaction handling by leveraging LDK's event-driven architecture. The change from polling to event-driven updates should significantly improve performance and user experience.

---

Summary of Changes

Major Improvements:

• Event-driven onchain transaction handling via LDK Node events
• Removed AddressChecker utility (110 lines removed)
• Enhanced RBF transaction tracking and handling
• Added comprehensive onchain event handling (received, confirmed, replaced, reorged, evicted)
• Improved user notifications for transaction state changes
• Better handling of edge cases (reorgs, mempool evictions)

Impact:

• 1,031 additions, 495 deletions across 20 files
• Core architectural change in CoreService.swift (+529/-273)
• Enhanced event handling in AppViewModel.swift (+108/-4)
• Improved LightningService.swift integration (+120/-3)

---

Code Quality & Best Practices

Strengths

1. Event-Driven Architecture - Proper use of LDK onchain events eliminates polling overhead
2. Comprehensive Error Handling - Good use of try-catch blocks with descriptive logging
3. Service Queue Pattern - Correctly uses ServiceQueue.background(.core) for Core operations
4. MainActor Compliance - Proper async/await patterns with @mainactor for UI updates
5. Caching Strategy - cachedTxIdsInBoostTxIds improves performance for boost transaction lookups
6. Detailed Logging - Excellent context-aware logging throughout
7. Accessibility - Added accessibilityIdentifier to Toast model and components

Code Organization

The new event handlers follow a clean pattern:

• handleOnchainTransactionReceived
• handleOnchainTransactionConfirmed
• handleOnchainTransactionReplaced
• handleOnchainTransactionReorged
• handleOnchainTransactionEvicted

Each handler properly delegates to ServiceQueue.background(.core) and updates the activity list appropriately.

---

Potential Bugs & Issues

1. Race Condition in handleOnchainTransactionReplaced

Location: CoreService.swift:346-433

Issue: When processing replacement transactions, there is a potential race condition between checking if replacementActivity exists and processing the payment. Another event handler could process the same transaction concurrently.

Recommendation: Add a lock or transaction ID tracking to prevent concurrent processing of the same transaction.

2. Cache Invalidation Issue

Location: CoreService.swift:34-62

Issue: The cachedTxIdsInBoostTxIds cache is updated in insert and bulk refreshed in upsertList, but individual update operations don't update the cache. This could lead to stale cache data.

Recommendation: Add cache update to the update method to keep cache consistent.

3. Potential Memory Leak in Event Handlers

Location: AppScene.swift:213-215

Issue: The event handler closure captures app weakly, but the closure itself is never removed when the scene is dismissed.

Recommendation: Ensure event handlers are removed when the scene is dismissed. The addOnEvent API should support removal, or use a cleanup mechanism in deinit or onDisappear.

4. Missing nil-check in processOnchainTransaction

Location: CoreService.swift:315-332

Issue: The function logs a warning if payment is not found but doesn't throw an error. The calling code may not handle this gracefully.

Recommendation: Consider throwing a specific error so callers can decide how to handle missing payments, or add metrics to track how often this occurs.

---

Performance Considerations

Strengths

1. Eliminated Polling - Removing AddressChecker and full syncs significantly reduces CPU/network overhead
2. Efficient Caching - cachedTxIdsInBoostTxIds prevents repeated database queries
3. Lazy Cache Initialization - Cache is only built when first accessed

Concerns

1. Cache Refresh Performance - CoreService.swift:47-62 loads ALL onchain activities from the database, which could be slow for users with large transaction histories (1000+ transactions). Consider a more efficient query that only fetches boostTxIds without loading full activity objects.

2. Delay in Received Sheet - AppViewModel.swift:399 has a 500ms delay. Is this necessary? It could make the UI feel sluggish. Consider removing or reducing if not strictly required.

---

Security Concerns

Good Practices

1. Proper use of ServiceQueue prevents concurrent access to Core/LDK
2. Transactions marked as doesExist = false when replaced (prevents double-counting)
3. RBF detection prevents showing duplicate received sheets

Recommendations

1. Transaction Validation - Consider adding validation that conflicts array in handleOnchainTransactionReplaced actually contains valid transaction IDs to prevent malformed data corruption.

2. Timestamp Validation - In processOnchainPayment, ensure timestamps are within reasonable ranges to prevent time-based attacks.

---

Test Coverage

Missing Test Scenarios

1. RBF Handling - Need tests for single replacement, multiple conflicts, chain of replacements, replacement arriving before original

2. Reorg Handling - Need tests for confirmed tx becoming unconfirmed, multiple txs affected by reorg, reorg depth > 1 block

3. Edge Cases - Need tests for tx evicted from mempool that was not replaced, race conditions, cache invalidation scenarios

Testing Recommendations

Add integration tests that simulate these event sequences:

• onchainTransactionReceived → onchainTransactionReplaced → onchainTransactionConfirmed
• onchainTransactionConfirmed → onchainTransactionReorged
• onchainTransactionReceived → onchainTransactionEvicted

---

Final Recommendations

Must Fix (Before Merge)

1. Run swiftformat . to ensure code formatting compliance
2. Fix cache invalidation in update method
3. Add race condition protection in handleOnchainTransactionReplaced
4. Verify event handler cleanup to prevent memory leaks

Should Fix (High Priority)

1. Optimize refreshBoostTxIdsCache for large transaction histories
2. Add comprehensive tests for RBF and reorg scenarios
3. Review the 500ms delay in received sheet display

Nice to Have

1. Add transaction ID truncation to user-facing messages
2. Improve user-friendly language for technical events (reorg, eviction)
3. Add metrics/telemetry for tracking event frequencies

---

Conclusion

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design pattern and will eliminate polling overhead. The code quality is generally excellent with proper error handling, logging, and async patterns.

Recommendation:Approve with minor changes. Address the cache invalidation and race condition issues before merge, and plan follow-up work for comprehensive testing and performance optimization.

Great work! The videos demonstrating reorg and eviction handling are particularly impressive.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time I try it

[claude]

PR Review: Integrate LDK onchain events

Summary

This PR successfully replaces polling-based transaction monitoring with event-driven LDK onchain events, removes the AddressChecker, and improves RBF transaction handling. This is a significant architectural improvement that makes the app more reactive and efficient.

✅ Strengths

Architecture & Design

1. Event-driven architecture: The transition from polling to LDK events (onchainTransactionReceived, onchainTransactionConfirmed, onchainTransactionReplaced, onchainTransactionReorged, onchainTransactionEvicted) is excellent. This provides real-time updates and reduces unnecessary syncs.

2. Comprehensive RBF handling: The implementation properly tracks replacement transactions through boostTxIds and distinguishes between RBF (replaced transactions with doesExist=false) and CPFP (child transactions with doesExist=true).

3. Clean separation of concerns: Event handlers in LightningService.swift (708-757) delegate to ActivityService methods, maintaining good separation between Lightning layer and activity management.

4. Improved user feedback: New toast notifications with accessibility identifiers for transaction states (replaced, reorged, evicted) provide better UX.

5. Removal of polling: Removing the onChange(of: wallet.totalBalanceSats) trigger in AppScene.swift:217 and the event-based sync in AppScene.swift:217-220 eliminates redundant activity syncing.

Code Quality

1. Good error handling: All event handlers wrap operations in try-catch blocks with proper logging.
2. Proper concurrency: Uses ServiceQueue.background(.core) for all database operations.
3. Cache optimization: The cachedTxIdsInBoostTxIds cache improves performance for checking replaced transactions.

⚠️ Issues & Concerns

Critical

1. Potential cache inconsistency in CoreService.swift:36-43

func getTxIdsInBoostTxIds()async->Set<String>{if cachedTxIdsInBoostTxIds.isEmpty {awaitrefreshBoostTxIdsCache()}return cachedTxIdsInBoostTxIds
}

Issue: The cache is only refreshed when empty, but individual updates via updateBoostTxIdsCache(for:) might not be called for all updates. The update() method at line 195 doesn't update the cache.

Recommendation:

• Update the cache in the update() method, or
• Implement a TTL-based cache invalidation strategy, or
• Document why the cache only needs to be refreshed when empty

2. Race condition in RBF handling (CoreService.swift:355-441)

When handleOnchainTransactionReplaced is called, it:

1. Marks the replaced transaction as doesExist=false
2. Processes replacement transactions that might not exist yet in the payment store
3. Waits for them via processOnchainPayment

Issue: There's a timing window where a replacement transaction event arrives before LDK updates the payment store. The code handles this (line 385-403) by processing the payment, but there's a potential race if the transaction hasn't been added to the payments list yet.

Recommendation: Consider adding retry logic or ensuring LDK guarantees payment store updates before emitting replacement events.

Moderate

3. Missing cache update in update() method (CoreService.swift:193-199)

func update(id:String, activity:Activity)asyncthrows{tryawaitServiceQueue.background(.core){tryupdateActivity(activityId: id, activity: activity)self.metadataChangedSubject.send()}}

This doesn't call updateBoostTxIdsCache(for: activity) like insert() does at line 172.

Recommendation: Add cache update to maintain consistency.

4. Incomplete validation in shouldShowReceivedSheet (CoreService.swift:76-107)

The method checks if a replacement transaction has the same value as the replaced transaction to avoid showing duplicate sheets. However:

• It only checks exact value matches
• Doesn't account for fee differences in RBF
• Could show sheets for CPFP where the child transaction might have a different value

Recommendation: Consider checking isBoosted flag and the nature of the boost (RBF vs CPFP) for more precise logic.

5. Error handling in handleOnchainTransactionReplaced (CoreService.swift:355-441)

When processing replacement transactions, errors are caught and logged (line 401-405), but the loop continues. If multiple replacement transactions fail to process, the relationship tracking could be incomplete.

Recommendation: Consider collecting errors and either throwing or returning them to the caller for better error visibility.

6. CI workflow changes remove caching (integration-tests.yml, unit-tests.yml)

The PR removes DerivedData caching and restore-keys from SPM cache. While this ensures clean builds, it will increase build times significantly.

Recommendation:

• Document why caching was removed (likely due to cache invalidation issues)
• Consider using Xcode Cloud or other CI optimizations to offset the performance loss
• The -onlyUsePackageVersionsFromResolvedFile flag is good for reproducibility

Minor

7. Potential N+1 query in getBoostTxDoesExist (CoreService.swift:130-138)

func getBoostTxDoesExist(boostTxIds:[String])async->[String:Bool]{vardoesExistMap:[String:Bool]=[:]forboostTxIdin boostTxIds {
if let boostActivity =try?awaitgetOnchainActivityByTxId(txid: boostTxId){doesExistMap[boostTxId]= boostActivity.doesExist
}}return doesExistMap
}

Issue: Makes individual database queries for each boost transaction.

Recommendation: Consider adding a batch query method to BitkitCore to fetch multiple activities by txId in one query.

8. Magic number delay in AppViewModel.swift:399

try?awaitTask.sleep(nanoseconds:500_000_000) // 500ms delay

Recommendation: Extract to a named constant with documentation explaining why the delay is necessary.

9. Missing test coverage

While TxBumpingTests.swift and ActivityListTest.swift exist, there don't appear to be specific tests for:

• Onchain event handlers
• RBF replacement tracking with boostTxIds
• Reorg and eviction scenarios
• Cache consistency

Recommendation: Add integration tests for the new event handlers.

🔒 Security

No major security concerns identified. The code properly:

• Uses ServiceQueue for thread safety
• Doesn't expose sensitive transaction data in logs beyond txids
• Maintains Bitcoin/Lightning operation isolation in the service layer

Minor: Logging in LightningService.swift:711-757 includes transaction amounts. Consider whether this is acceptable for production logging.

🚀 Performance

Positive impacts:

1. Eliminates redundant polling and full activity list syncs
2. Event-driven updates only process changed transactions
3. Cache for boostTxIds lookups

Potential concerns:

1. Cache not updated consistently (see issue Project setup #1, Migrate channels from LDK to LDK-node #3)
2. N+1 queries in getBoostTxDoesExist (see issue feat: keychain #7)

🧪 Testing

Existing test coverage:

• ActivityListTest.swift: Good coverage of basic activity CRUD operations
• TxBumpingTests.swift: Tests RBF scenarios

Missing:

• Tests for new onchain event handlers
• Tests for handleOnchainTransactionReplaced, handleOnchainTransactionReorged, handleOnchainTransactionEvicted
• Tests for cache consistency
• Integration tests for the complete event flow

Recommendation: Add integration tests that simulate the event sequences shown in the PR videos.

📝 Recommendations

Must Fix Before Merge

1. Fix cache consistency issues (Issues Project setup #1, Migrate channels from LDK to LDK-node #3)
2. Add error handling for race conditions in RBF handling (Issue Push service setup and service queues  #2)

Should Fix Before Merge

1. Improve shouldShowReceivedSheet validation logic (Issue Create swift.yml #4)
2. Better error aggregation in handleOnchainTransactionReplaced (Issue Create ios.yml #5)

Consider for Follow-up

1. Add batch query support for getBoostTxDoesExist (Issue feat: keychain #7)
2. Add integration tests for event handlers (Issue Use SwiftFormat #9)
3. Extract magic constants (Issue BDK 1.0.0-alpha.11 #8)
4. Document CI caching removal reasoning (Issue Create ios.yml #6)

🎯 Overall Assessment

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design, and the RBF handling logic is well thought out. The main concerns are around cache consistency and potential race conditions in RBF processing, which should be addressed before merge.

The removal of AddressChecker and polling-based syncing will improve app performance and responsiveness.

Recommendation: Request changes to address cache consistency and race condition handling, then approve after fixes.

---

Great work on this PR! The demos showing reorg and mempool eviction handling are particularly impressive. Once the cache and race condition issues are resolved, this will be a solid improvement to the codebase.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ben-kaufman]

This PR integrates LDK node on chain events to replace the previous pollin and full syncs.

It also removes Address Checker, and improves handling on RBFed transactions to be recognized on both sides.

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

Screen.Recording.2025-11-26.at.9.05.11.AM.mov

Reorg removes tx from blocks demo:

Screen.Recording.2025-11-26.at.5.13.45.PM.mov

Transaction removed from mempool demo:

Screen.Recording.2025-11-26.at.5.38.36.PM.mov

[Copilot]

Pull request overview

This PR integrates LDK node onchain events to replace previous polling and full sync mechanisms for Bitcoin transaction management. It removes the AddressChecker utility class that relied on external API calls, and improves handling of RBF (Replace-By-Fee) transactions to properly distinguish them from CPFP (Child-Pays-For-Parent) transactions on both sender and receiver sides.

Key changes:

• Event-driven architecture for onchain transaction updates (received, confirmed, replaced, reorged, evicted)
• Removal of 1-second polling timer in favor of real-time LDK events
• Enhanced RBF transaction detection using doesExist flag to differentiate from CPFP

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
BitkitNotification/NotificationService.swift	Added notification handling for new onchain transaction events
Bitkit/Views/Wallets/Sheets/BoostSheet.swift	Removed redundant LDK node payment sync after boost operation
Bitkit/Views/Wallets/Activity/ActivityItemView.swift	Enhanced boost button logic to differentiate between RBF and CPFP transactions
Bitkit/Views/Wallets/Activity/ActivityExplorerView.swift	Replaced AddressChecker API calls with LDK node transaction details
Bitkit/ViewModels/WalletViewModel.swift	Removed polling mechanism and added event-driven state updates for onchain transactions
Bitkit/ViewModels/AppViewModel.swift	Added comprehensive event handling for onchain transaction lifecycle
Bitkit/Utilities/AddressChecker.swift	Removed external API-dependent utility in favor of native LDK functionality
Bitkit/Services/LightningService.swift	Added transaction detail methods and event handlers for onchain events
Bitkit/Services/CoreService.swift	Refactored payment processing to handle LDK events and removed replacement tracking maps
Bitkit/AppScene.swift	Removed balance change observer and event-based activity sync
Bitkit.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved	Updated bitkit-core and ldk-node dependencies
Bitkit.xcodeproj/project.pbxproj	Updated project references for LDK package

[ben-kaufman]

@piotr-iohk please note we will need to update the e2e tests to close the onchain received sheet.

[ovitrif]

utAck

Amazing work, I've been waiting for over a year to finally have this setup in place: event-based sync for transactions 🎉

Added a few small nit comments, the one about the localizable strings applies to all cases where we added new toast messages, not only the code where I added it.

I wanted to give you the time to already make some small updates as I am starting to test this, hence me approving without testing; but I trust you'll wait for my testing 🙏🏻

[piotr-iohk]

Ok, updated e2e tests for current state (synonymdev/bitkit-e2e-tests#56). Re-running 🤞
It would be good to coordinate merging with Android (synonymdev/bitkit-android#475)

[ben-kaufman]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

[pwltr]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

Sounds good yeah I think an optimistic UI like this would be the way to go. Assume everything went right and correct if needed when the event(s) come in from LDK. Still feel free to postpone for now.

[ben-kaufman]

Actually had a simpler solution, fixed in: #247

[pwltr]

tACK with follow-up task here

Also note @piotr-iohk's request to merge this together with the Android counterpart.

[claude]

PR Review: Integrate LDK onchain events

This is an excellent architectural improvement that modernizes the onchain transaction handling by leveraging LDK's event-driven architecture. The change from polling to event-driven updates should significantly improve performance and user experience.

---

Summary of Changes

Major Improvements:

• Event-driven onchain transaction handling via LDK Node events
• Removed AddressChecker utility (110 lines removed)
• Enhanced RBF transaction tracking and handling
• Added comprehensive onchain event handling (received, confirmed, replaced, reorged, evicted)
• Improved user notifications for transaction state changes
• Better handling of edge cases (reorgs, mempool evictions)

Impact:

• 1,031 additions, 495 deletions across 20 files
• Core architectural change in CoreService.swift (+529/-273)
• Enhanced event handling in AppViewModel.swift (+108/-4)
• Improved LightningService.swift integration (+120/-3)

---

Code Quality & Best Practices

Strengths

1. Event-Driven Architecture - Proper use of LDK onchain events eliminates polling overhead
2. Comprehensive Error Handling - Good use of try-catch blocks with descriptive logging
3. Service Queue Pattern - Correctly uses ServiceQueue.background(.core) for Core operations
4. MainActor Compliance - Proper async/await patterns with @mainactor for UI updates
5. Caching Strategy - cachedTxIdsInBoostTxIds improves performance for boost transaction lookups
6. Detailed Logging - Excellent context-aware logging throughout
7. Accessibility - Added accessibilityIdentifier to Toast model and components

Code Organization

The new event handlers follow a clean pattern:

• handleOnchainTransactionReceived
• handleOnchainTransactionConfirmed
• handleOnchainTransactionReplaced
• handleOnchainTransactionReorged
• handleOnchainTransactionEvicted

Each handler properly delegates to ServiceQueue.background(.core) and updates the activity list appropriately.

---

Potential Bugs & Issues

1. Race Condition in handleOnchainTransactionReplaced

Location: CoreService.swift:346-433

Issue: When processing replacement transactions, there is a potential race condition between checking if replacementActivity exists and processing the payment. Another event handler could process the same transaction concurrently.

Recommendation: Add a lock or transaction ID tracking to prevent concurrent processing of the same transaction.

2. Cache Invalidation Issue

Location: CoreService.swift:34-62

Issue: The cachedTxIdsInBoostTxIds cache is updated in insert and bulk refreshed in upsertList, but individual update operations don't update the cache. This could lead to stale cache data.

Recommendation: Add cache update to the update method to keep cache consistent.

3. Potential Memory Leak in Event Handlers

Location: AppScene.swift:213-215

Issue: The event handler closure captures app weakly, but the closure itself is never removed when the scene is dismissed.

Recommendation: Ensure event handlers are removed when the scene is dismissed. The addOnEvent API should support removal, or use a cleanup mechanism in deinit or onDisappear.

4. Missing nil-check in processOnchainTransaction

Location: CoreService.swift:315-332

Issue: The function logs a warning if payment is not found but doesn't throw an error. The calling code may not handle this gracefully.

Recommendation: Consider throwing a specific error so callers can decide how to handle missing payments, or add metrics to track how often this occurs.

---

Performance Considerations

Strengths

1. Eliminated Polling - Removing AddressChecker and full syncs significantly reduces CPU/network overhead
2. Efficient Caching - cachedTxIdsInBoostTxIds prevents repeated database queries
3. Lazy Cache Initialization - Cache is only built when first accessed

Concerns

1. Cache Refresh Performance - CoreService.swift:47-62 loads ALL onchain activities from the database, which could be slow for users with large transaction histories (1000+ transactions). Consider a more efficient query that only fetches boostTxIds without loading full activity objects.

2. Delay in Received Sheet - AppViewModel.swift:399 has a 500ms delay. Is this necessary? It could make the UI feel sluggish. Consider removing or reducing if not strictly required.

---

Security Concerns

Good Practices

1. Proper use of ServiceQueue prevents concurrent access to Core/LDK
2. Transactions marked as doesExist = false when replaced (prevents double-counting)
3. RBF detection prevents showing duplicate received sheets

Recommendations

1. Transaction Validation - Consider adding validation that conflicts array in handleOnchainTransactionReplaced actually contains valid transaction IDs to prevent malformed data corruption.

2. Timestamp Validation - In processOnchainPayment, ensure timestamps are within reasonable ranges to prevent time-based attacks.

---

Test Coverage

Missing Test Scenarios

1. RBF Handling - Need tests for single replacement, multiple conflicts, chain of replacements, replacement arriving before original

2. Reorg Handling - Need tests for confirmed tx becoming unconfirmed, multiple txs affected by reorg, reorg depth > 1 block

3. Edge Cases - Need tests for tx evicted from mempool that was not replaced, race conditions, cache invalidation scenarios

Testing Recommendations

Add integration tests that simulate these event sequences:

• onchainTransactionReceived → onchainTransactionReplaced → onchainTransactionConfirmed
• onchainTransactionConfirmed → onchainTransactionReorged
• onchainTransactionReceived → onchainTransactionEvicted

---

Final Recommendations

Must Fix (Before Merge)

1. Run swiftformat . to ensure code formatting compliance
2. Fix cache invalidation in update method
3. Add race condition protection in handleOnchainTransactionReplaced
4. Verify event handler cleanup to prevent memory leaks

Should Fix (High Priority)

1. Optimize refreshBoostTxIdsCache for large transaction histories
2. Add comprehensive tests for RBF and reorg scenarios
3. Review the 500ms delay in received sheet display

Nice to Have

1. Add transaction ID truncation to user-facing messages
2. Improve user-friendly language for technical events (reorg, eviction)
3. Add metrics/telemetry for tracking event frequencies

---

Conclusion

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design pattern and will eliminate polling overhead. The code quality is generally excellent with proper error handling, logging, and async patterns.

Recommendation:Approve with minor changes. Address the cache invalidation and race condition issues before merge, and plan follow-up work for comprehensive testing and performance optimization.

Great work! The videos demonstrating reorg and eviction handling are particularly impressive.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time I try it

[claude]

PR Review: Integrate LDK onchain events

Summary

This PR successfully replaces polling-based transaction monitoring with event-driven LDK onchain events, removes the AddressChecker, and improves RBF transaction handling. This is a significant architectural improvement that makes the app more reactive and efficient.

✅ Strengths

Architecture & Design

1. Event-driven architecture: The transition from polling to LDK events (onchainTransactionReceived, onchainTransactionConfirmed, onchainTransactionReplaced, onchainTransactionReorged, onchainTransactionEvicted) is excellent. This provides real-time updates and reduces unnecessary syncs.

2. Comprehensive RBF handling: The implementation properly tracks replacement transactions through boostTxIds and distinguishes between RBF (replaced transactions with doesExist=false) and CPFP (child transactions with doesExist=true).

3. Clean separation of concerns: Event handlers in LightningService.swift (708-757) delegate to ActivityService methods, maintaining good separation between Lightning layer and activity management.

4. Improved user feedback: New toast notifications with accessibility identifiers for transaction states (replaced, reorged, evicted) provide better UX.

5. Removal of polling: Removing the onChange(of: wallet.totalBalanceSats) trigger in AppScene.swift:217 and the event-based sync in AppScene.swift:217-220 eliminates redundant activity syncing.

Code Quality

1. Good error handling: All event handlers wrap operations in try-catch blocks with proper logging.
2. Proper concurrency: Uses ServiceQueue.background(.core) for all database operations.
3. Cache optimization: The cachedTxIdsInBoostTxIds cache improves performance for checking replaced transactions.

⚠️ Issues & Concerns

Critical

1. Potential cache inconsistency in CoreService.swift:36-43

func getTxIdsInBoostTxIds()async->Set<String>{if cachedTxIdsInBoostTxIds.isEmpty {awaitrefreshBoostTxIdsCache()}return cachedTxIdsInBoostTxIds
}

Issue: The cache is only refreshed when empty, but individual updates via updateBoostTxIdsCache(for:) might not be called for all updates. The update() method at line 195 doesn't update the cache.

Recommendation:

• Update the cache in the update() method, or
• Implement a TTL-based cache invalidation strategy, or
• Document why the cache only needs to be refreshed when empty

2. Race condition in RBF handling (CoreService.swift:355-441)

When handleOnchainTransactionReplaced is called, it:

1. Marks the replaced transaction as doesExist=false
2. Processes replacement transactions that might not exist yet in the payment store
3. Waits for them via processOnchainPayment

Issue: There's a timing window where a replacement transaction event arrives before LDK updates the payment store. The code handles this (line 385-403) by processing the payment, but there's a potential race if the transaction hasn't been added to the payments list yet.

Recommendation: Consider adding retry logic or ensuring LDK guarantees payment store updates before emitting replacement events.

Moderate

3. Missing cache update in update() method (CoreService.swift:193-199)

func update(id:String, activity:Activity)asyncthrows{tryawaitServiceQueue.background(.core){tryupdateActivity(activityId: id, activity: activity)self.metadataChangedSubject.send()}}

This doesn't call updateBoostTxIdsCache(for: activity) like insert() does at line 172.

Recommendation: Add cache update to maintain consistency.

4. Incomplete validation in shouldShowReceivedSheet (CoreService.swift:76-107)

The method checks if a replacement transaction has the same value as the replaced transaction to avoid showing duplicate sheets. However:

• It only checks exact value matches
• Doesn't account for fee differences in RBF
• Could show sheets for CPFP where the child transaction might have a different value

Recommendation: Consider checking isBoosted flag and the nature of the boost (RBF vs CPFP) for more precise logic.

5. Error handling in handleOnchainTransactionReplaced (CoreService.swift:355-441)

When processing replacement transactions, errors are caught and logged (line 401-405), but the loop continues. If multiple replacement transactions fail to process, the relationship tracking could be incomplete.

Recommendation: Consider collecting errors and either throwing or returning them to the caller for better error visibility.

6. CI workflow changes remove caching (integration-tests.yml, unit-tests.yml)

The PR removes DerivedData caching and restore-keys from SPM cache. While this ensures clean builds, it will increase build times significantly.

Recommendation:

• Document why caching was removed (likely due to cache invalidation issues)
• Consider using Xcode Cloud or other CI optimizations to offset the performance loss
• The -onlyUsePackageVersionsFromResolvedFile flag is good for reproducibility

Minor

7. Potential N+1 query in getBoostTxDoesExist (CoreService.swift:130-138)

func getBoostTxDoesExist(boostTxIds:[String])async->[String:Bool]{vardoesExistMap:[String:Bool]=[:]forboostTxIdin boostTxIds {
if let boostActivity =try?awaitgetOnchainActivityByTxId(txid: boostTxId){doesExistMap[boostTxId]= boostActivity.doesExist
}}return doesExistMap
}

Issue: Makes individual database queries for each boost transaction.

Recommendation: Consider adding a batch query method to BitkitCore to fetch multiple activities by txId in one query.

8. Magic number delay in AppViewModel.swift:399

try?awaitTask.sleep(nanoseconds:500_000_000) // 500ms delay

Recommendation: Extract to a named constant with documentation explaining why the delay is necessary.

9. Missing test coverage

While TxBumpingTests.swift and ActivityListTest.swift exist, there don't appear to be specific tests for:

• Onchain event handlers
• RBF replacement tracking with boostTxIds
• Reorg and eviction scenarios
• Cache consistency

Recommendation: Add integration tests for the new event handlers.

🔒 Security

No major security concerns identified. The code properly:

• Uses ServiceQueue for thread safety
• Doesn't expose sensitive transaction data in logs beyond txids
• Maintains Bitcoin/Lightning operation isolation in the service layer

Minor: Logging in LightningService.swift:711-757 includes transaction amounts. Consider whether this is acceptable for production logging.

🚀 Performance

Positive impacts:

1. Eliminates redundant polling and full activity list syncs
2. Event-driven updates only process changed transactions
3. Cache for boostTxIds lookups

Potential concerns:

1. Cache not updated consistently (see issue Project setup #1, Migrate channels from LDK to LDK-node #3)
2. N+1 queries in getBoostTxDoesExist (see issue feat: keychain #7)

🧪 Testing

Existing test coverage:

• ActivityListTest.swift: Good coverage of basic activity CRUD operations
• TxBumpingTests.swift: Tests RBF scenarios

Missing:

• Tests for new onchain event handlers
• Tests for handleOnchainTransactionReplaced, handleOnchainTransactionReorged, handleOnchainTransactionEvicted
• Tests for cache consistency
• Integration tests for the complete event flow

Recommendation: Add integration tests that simulate the event sequences shown in the PR videos.

📝 Recommendations

Must Fix Before Merge

1. Fix cache consistency issues (Issues Project setup #1, Migrate channels from LDK to LDK-node #3)
2. Add error handling for race conditions in RBF handling (Issue Push service setup and service queues  #2)

Should Fix Before Merge

1. Improve shouldShowReceivedSheet validation logic (Issue Create swift.yml #4)
2. Better error aggregation in handleOnchainTransactionReplaced (Issue Create ios.yml #5)

Consider for Follow-up

1. Add batch query support for getBoostTxDoesExist (Issue feat: keychain #7)
2. Add integration tests for event handlers (Issue Use SwiftFormat #9)
3. Extract magic constants (Issue BDK 1.0.0-alpha.11 #8)
4. Document CI caching removal reasoning (Issue Create ios.yml #6)

🎯 Overall Assessment

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design, and the RBF handling logic is well thought out. The main concerns are around cache consistency and potential race conditions in RBF processing, which should be addressed before merge.

The removal of AddressChecker and polling-based syncing will improve app performance and responsiveness.

Recommendation: Request changes to address cache consistency and race condition handling, then approve after fixes.

---

Great work on this PR! The demos showing reorg and mempool eviction handling are particularly impressive. Once the cache and race condition issues are resolved, this will be a solid improvement to the codebase.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ben-kaufman]

This PR integrates LDK node on chain events to replace the previous pollin and full syncs.

It also removes Address Checker, and improves handling on RBFed transactions to be recognized on both sides.

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

Screen.Recording.2025-11-26.at.9.05.11.AM.mov

Reorg removes tx from blocks demo:

Screen.Recording.2025-11-26.at.5.13.45.PM.mov

Transaction removed from mempool demo:

Screen.Recording.2025-11-26.at.5.38.36.PM.mov

[Copilot]

Pull request overview

This PR integrates LDK node onchain events to replace previous polling and full sync mechanisms for Bitcoin transaction management. It removes the AddressChecker utility class that relied on external API calls, and improves handling of RBF (Replace-By-Fee) transactions to properly distinguish them from CPFP (Child-Pays-For-Parent) transactions on both sender and receiver sides.

Key changes:

• Event-driven architecture for onchain transaction updates (received, confirmed, replaced, reorged, evicted)
• Removal of 1-second polling timer in favor of real-time LDK events
• Enhanced RBF transaction detection using doesExist flag to differentiate from CPFP

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
BitkitNotification/NotificationService.swift	Added notification handling for new onchain transaction events
Bitkit/Views/Wallets/Sheets/BoostSheet.swift	Removed redundant LDK node payment sync after boost operation
Bitkit/Views/Wallets/Activity/ActivityItemView.swift	Enhanced boost button logic to differentiate between RBF and CPFP transactions
Bitkit/Views/Wallets/Activity/ActivityExplorerView.swift	Replaced AddressChecker API calls with LDK node transaction details
Bitkit/ViewModels/WalletViewModel.swift	Removed polling mechanism and added event-driven state updates for onchain transactions
Bitkit/ViewModels/AppViewModel.swift	Added comprehensive event handling for onchain transaction lifecycle
Bitkit/Utilities/AddressChecker.swift	Removed external API-dependent utility in favor of native LDK functionality
Bitkit/Services/LightningService.swift	Added transaction detail methods and event handlers for onchain events
Bitkit/Services/CoreService.swift	Refactored payment processing to handle LDK events and removed replacement tracking maps
Bitkit/AppScene.swift	Removed balance change observer and event-based activity sync
Bitkit.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved	Updated bitkit-core and ldk-node dependencies
Bitkit.xcodeproj/project.pbxproj	Updated project references for LDK package

[ben-kaufman]

@piotr-iohk please note we will need to update the e2e tests to close the onchain received sheet.

[ovitrif]

utAck

Amazing work, I've been waiting for over a year to finally have this setup in place: event-based sync for transactions 🎉

Added a few small nit comments, the one about the localizable strings applies to all cases where we added new toast messages, not only the code where I added it.

I wanted to give you the time to already make some small updates as I am starting to test this, hence me approving without testing; but I trust you'll wait for my testing 🙏🏻

[piotr-iohk]

Ok, updated e2e tests for current state (synonymdev/bitkit-e2e-tests#56). Re-running 🤞
It would be good to coordinate merging with Android (synonymdev/bitkit-android#475)

[ben-kaufman]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

[pwltr]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

Sounds good yeah I think an optimistic UI like this would be the way to go. Assume everything went right and correct if needed when the event(s) come in from LDK. Still feel free to postpone for now.

[ben-kaufman]

Actually had a simpler solution, fixed in: #247

[pwltr]

tACK with follow-up task here

Also note @piotr-iohk's request to merge this together with the Android counterpart.

[claude]

PR Review: Integrate LDK onchain events

This is an excellent architectural improvement that modernizes the onchain transaction handling by leveraging LDK's event-driven architecture. The change from polling to event-driven updates should significantly improve performance and user experience.

---

Summary of Changes

Major Improvements:

• Event-driven onchain transaction handling via LDK Node events
• Removed AddressChecker utility (110 lines removed)
• Enhanced RBF transaction tracking and handling
• Added comprehensive onchain event handling (received, confirmed, replaced, reorged, evicted)
• Improved user notifications for transaction state changes
• Better handling of edge cases (reorgs, mempool evictions)

Impact:

• 1,031 additions, 495 deletions across 20 files
• Core architectural change in CoreService.swift (+529/-273)
• Enhanced event handling in AppViewModel.swift (+108/-4)
• Improved LightningService.swift integration (+120/-3)

---

Code Quality & Best Practices

Strengths

1. Event-Driven Architecture - Proper use of LDK onchain events eliminates polling overhead
2. Comprehensive Error Handling - Good use of try-catch blocks with descriptive logging
3. Service Queue Pattern - Correctly uses ServiceQueue.background(.core) for Core operations
4. MainActor Compliance - Proper async/await patterns with @mainactor for UI updates
5. Caching Strategy - cachedTxIdsInBoostTxIds improves performance for boost transaction lookups
6. Detailed Logging - Excellent context-aware logging throughout
7. Accessibility - Added accessibilityIdentifier to Toast model and components

Code Organization

The new event handlers follow a clean pattern:

• handleOnchainTransactionReceived
• handleOnchainTransactionConfirmed
• handleOnchainTransactionReplaced
• handleOnchainTransactionReorged
• handleOnchainTransactionEvicted

Each handler properly delegates to ServiceQueue.background(.core) and updates the activity list appropriately.

---

Potential Bugs & Issues

1. Race Condition in handleOnchainTransactionReplaced

Location: CoreService.swift:346-433

Issue: When processing replacement transactions, there is a potential race condition between checking if replacementActivity exists and processing the payment. Another event handler could process the same transaction concurrently.

Recommendation: Add a lock or transaction ID tracking to prevent concurrent processing of the same transaction.

2. Cache Invalidation Issue

Location: CoreService.swift:34-62

Issue: The cachedTxIdsInBoostTxIds cache is updated in insert and bulk refreshed in upsertList, but individual update operations don't update the cache. This could lead to stale cache data.

Recommendation: Add cache update to the update method to keep cache consistent.

3. Potential Memory Leak in Event Handlers

Location: AppScene.swift:213-215

Issue: The event handler closure captures app weakly, but the closure itself is never removed when the scene is dismissed.

Recommendation: Ensure event handlers are removed when the scene is dismissed. The addOnEvent API should support removal, or use a cleanup mechanism in deinit or onDisappear.

4. Missing nil-check in processOnchainTransaction

Location: CoreService.swift:315-332

Issue: The function logs a warning if payment is not found but doesn't throw an error. The calling code may not handle this gracefully.

Recommendation: Consider throwing a specific error so callers can decide how to handle missing payments, or add metrics to track how often this occurs.

---

Performance Considerations

Strengths

1. Eliminated Polling - Removing AddressChecker and full syncs significantly reduces CPU/network overhead
2. Efficient Caching - cachedTxIdsInBoostTxIds prevents repeated database queries
3. Lazy Cache Initialization - Cache is only built when first accessed

Concerns

1. Cache Refresh Performance - CoreService.swift:47-62 loads ALL onchain activities from the database, which could be slow for users with large transaction histories (1000+ transactions). Consider a more efficient query that only fetches boostTxIds without loading full activity objects.

2. Delay in Received Sheet - AppViewModel.swift:399 has a 500ms delay. Is this necessary? It could make the UI feel sluggish. Consider removing or reducing if not strictly required.

---

Security Concerns

Good Practices

1. Proper use of ServiceQueue prevents concurrent access to Core/LDK
2. Transactions marked as doesExist = false when replaced (prevents double-counting)
3. RBF detection prevents showing duplicate received sheets

Recommendations

1. Transaction Validation - Consider adding validation that conflicts array in handleOnchainTransactionReplaced actually contains valid transaction IDs to prevent malformed data corruption.

2. Timestamp Validation - In processOnchainPayment, ensure timestamps are within reasonable ranges to prevent time-based attacks.

---

Test Coverage

Missing Test Scenarios

1. RBF Handling - Need tests for single replacement, multiple conflicts, chain of replacements, replacement arriving before original

2. Reorg Handling - Need tests for confirmed tx becoming unconfirmed, multiple txs affected by reorg, reorg depth > 1 block

3. Edge Cases - Need tests for tx evicted from mempool that was not replaced, race conditions, cache invalidation scenarios

Testing Recommendations

Add integration tests that simulate these event sequences:

• onchainTransactionReceived → onchainTransactionReplaced → onchainTransactionConfirmed
• onchainTransactionConfirmed → onchainTransactionReorged
• onchainTransactionReceived → onchainTransactionEvicted

---

Final Recommendations

Must Fix (Before Merge)

1. Run swiftformat . to ensure code formatting compliance
2. Fix cache invalidation in update method
3. Add race condition protection in handleOnchainTransactionReplaced
4. Verify event handler cleanup to prevent memory leaks

Should Fix (High Priority)

1. Optimize refreshBoostTxIdsCache for large transaction histories
2. Add comprehensive tests for RBF and reorg scenarios
3. Review the 500ms delay in received sheet display

Nice to Have

1. Add transaction ID truncation to user-facing messages
2. Improve user-friendly language for technical events (reorg, eviction)
3. Add metrics/telemetry for tracking event frequencies

---

Conclusion

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design pattern and will eliminate polling overhead. The code quality is generally excellent with proper error handling, logging, and async patterns.

Recommendation:Approve with minor changes. Address the cache invalidation and race condition issues before merge, and plan follow-up work for comprehensive testing and performance optimization.

Great work! The videos demonstrating reorg and eviction handling are particularly impressive.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time I try it

[claude]

PR Review: Integrate LDK onchain events

Summary

This PR successfully replaces polling-based transaction monitoring with event-driven LDK onchain events, removes the AddressChecker, and improves RBF transaction handling. This is a significant architectural improvement that makes the app more reactive and efficient.

✅ Strengths

Architecture & Design

1. Event-driven architecture: The transition from polling to LDK events (onchainTransactionReceived, onchainTransactionConfirmed, onchainTransactionReplaced, onchainTransactionReorged, onchainTransactionEvicted) is excellent. This provides real-time updates and reduces unnecessary syncs.

2. Comprehensive RBF handling: The implementation properly tracks replacement transactions through boostTxIds and distinguishes between RBF (replaced transactions with doesExist=false) and CPFP (child transactions with doesExist=true).

3. Clean separation of concerns: Event handlers in LightningService.swift (708-757) delegate to ActivityService methods, maintaining good separation between Lightning layer and activity management.

4. Improved user feedback: New toast notifications with accessibility identifiers for transaction states (replaced, reorged, evicted) provide better UX.

5. Removal of polling: Removing the onChange(of: wallet.totalBalanceSats) trigger in AppScene.swift:217 and the event-based sync in AppScene.swift:217-220 eliminates redundant activity syncing.

Code Quality

1. Good error handling: All event handlers wrap operations in try-catch blocks with proper logging.
2. Proper concurrency: Uses ServiceQueue.background(.core) for all database operations.
3. Cache optimization: The cachedTxIdsInBoostTxIds cache improves performance for checking replaced transactions.

⚠️ Issues & Concerns

Critical

1. Potential cache inconsistency in CoreService.swift:36-43

func getTxIdsInBoostTxIds()async->Set<String>{if cachedTxIdsInBoostTxIds.isEmpty {awaitrefreshBoostTxIdsCache()}return cachedTxIdsInBoostTxIds
}

Issue: The cache is only refreshed when empty, but individual updates via updateBoostTxIdsCache(for:) might not be called for all updates. The update() method at line 195 doesn't update the cache.

Recommendation:

• Update the cache in the update() method, or
• Implement a TTL-based cache invalidation strategy, or
• Document why the cache only needs to be refreshed when empty

2. Race condition in RBF handling (CoreService.swift:355-441)

When handleOnchainTransactionReplaced is called, it:

1. Marks the replaced transaction as doesExist=false
2. Processes replacement transactions that might not exist yet in the payment store
3. Waits for them via processOnchainPayment

Issue: There's a timing window where a replacement transaction event arrives before LDK updates the payment store. The code handles this (line 385-403) by processing the payment, but there's a potential race if the transaction hasn't been added to the payments list yet.

Recommendation: Consider adding retry logic or ensuring LDK guarantees payment store updates before emitting replacement events.

Moderate

3. Missing cache update in update() method (CoreService.swift:193-199)

func update(id:String, activity:Activity)asyncthrows{tryawaitServiceQueue.background(.core){tryupdateActivity(activityId: id, activity: activity)self.metadataChangedSubject.send()}}

This doesn't call updateBoostTxIdsCache(for: activity) like insert() does at line 172.

Recommendation: Add cache update to maintain consistency.

4. Incomplete validation in shouldShowReceivedSheet (CoreService.swift:76-107)

The method checks if a replacement transaction has the same value as the replaced transaction to avoid showing duplicate sheets. However:

• It only checks exact value matches
• Doesn't account for fee differences in RBF
• Could show sheets for CPFP where the child transaction might have a different value

Recommendation: Consider checking isBoosted flag and the nature of the boost (RBF vs CPFP) for more precise logic.

5. Error handling in handleOnchainTransactionReplaced (CoreService.swift:355-441)

When processing replacement transactions, errors are caught and logged (line 401-405), but the loop continues. If multiple replacement transactions fail to process, the relationship tracking could be incomplete.

Recommendation: Consider collecting errors and either throwing or returning them to the caller for better error visibility.

6. CI workflow changes remove caching (integration-tests.yml, unit-tests.yml)

The PR removes DerivedData caching and restore-keys from SPM cache. While this ensures clean builds, it will increase build times significantly.

Recommendation:

• Document why caching was removed (likely due to cache invalidation issues)
• Consider using Xcode Cloud or other CI optimizations to offset the performance loss
• The -onlyUsePackageVersionsFromResolvedFile flag is good for reproducibility

Minor

7. Potential N+1 query in getBoostTxDoesExist (CoreService.swift:130-138)

func getBoostTxDoesExist(boostTxIds:[String])async->[String:Bool]{vardoesExistMap:[String:Bool]=[:]forboostTxIdin boostTxIds {
if let boostActivity =try?awaitgetOnchainActivityByTxId(txid: boostTxId){doesExistMap[boostTxId]= boostActivity.doesExist
}}return doesExistMap
}

Issue: Makes individual database queries for each boost transaction.

Recommendation: Consider adding a batch query method to BitkitCore to fetch multiple activities by txId in one query.

8. Magic number delay in AppViewModel.swift:399

try?awaitTask.sleep(nanoseconds:500_000_000) // 500ms delay

Recommendation: Extract to a named constant with documentation explaining why the delay is necessary.

9. Missing test coverage

While TxBumpingTests.swift and ActivityListTest.swift exist, there don't appear to be specific tests for:

• Onchain event handlers
• RBF replacement tracking with boostTxIds
• Reorg and eviction scenarios
• Cache consistency

Recommendation: Add integration tests for the new event handlers.

🔒 Security

No major security concerns identified. The code properly:

• Uses ServiceQueue for thread safety
• Doesn't expose sensitive transaction data in logs beyond txids
• Maintains Bitcoin/Lightning operation isolation in the service layer

Minor: Logging in LightningService.swift:711-757 includes transaction amounts. Consider whether this is acceptable for production logging.

🚀 Performance

Positive impacts:

1. Eliminates redundant polling and full activity list syncs
2. Event-driven updates only process changed transactions
3. Cache for boostTxIds lookups

Potential concerns:

1. Cache not updated consistently (see issue Project setup #1, Migrate channels from LDK to LDK-node #3)
2. N+1 queries in getBoostTxDoesExist (see issue feat: keychain #7)

🧪 Testing

Existing test coverage:

• ActivityListTest.swift: Good coverage of basic activity CRUD operations
• TxBumpingTests.swift: Tests RBF scenarios

Missing:

• Tests for new onchain event handlers
• Tests for handleOnchainTransactionReplaced, handleOnchainTransactionReorged, handleOnchainTransactionEvicted
• Tests for cache consistency
• Integration tests for the complete event flow

Recommendation: Add integration tests that simulate the event sequences shown in the PR videos.

📝 Recommendations

Must Fix Before Merge

1. Fix cache consistency issues (Issues Project setup #1, Migrate channels from LDK to LDK-node #3)
2. Add error handling for race conditions in RBF handling (Issue Push service setup and service queues  #2)

Should Fix Before Merge

1. Improve shouldShowReceivedSheet validation logic (Issue Create swift.yml #4)
2. Better error aggregation in handleOnchainTransactionReplaced (Issue Create ios.yml #5)

Consider for Follow-up

1. Add batch query support for getBoostTxDoesExist (Issue feat: keychain #7)
2. Add integration tests for event handlers (Issue Use SwiftFormat #9)
3. Extract magic constants (Issue BDK 1.0.0-alpha.11 #8)
4. Document CI caching removal reasoning (Issue Create ios.yml #6)

🎯 Overall Assessment

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design, and the RBF handling logic is well thought out. The main concerns are around cache consistency and potential race conditions in RBF processing, which should be addressed before merge.

The removal of AddressChecker and polling-based syncing will improve app performance and responsiveness.

Recommendation: Request changes to address cache consistency and race condition handling, then approve after fixes.

---

Great work on this PR! The demos showing reorg and mempool eviction handling are particularly impressive. Once the cache and race condition issues are resolved, this will be a solid improvement to the codebase.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ben-kaufman]

This PR integrates LDK node on chain events to replace the previous pollin and full syncs.

It also removes Address Checker, and improves handling on RBFed transactions to be recognized on both sides.

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

Screen.Recording.2025-11-26.at.9.05.11.AM.mov

Reorg removes tx from blocks demo:

Screen.Recording.2025-11-26.at.5.13.45.PM.mov

Transaction removed from mempool demo:

Screen.Recording.2025-11-26.at.5.38.36.PM.mov

[Copilot]

Pull request overview

This PR integrates LDK node onchain events to replace previous polling and full sync mechanisms for Bitcoin transaction management. It removes the AddressChecker utility class that relied on external API calls, and improves handling of RBF (Replace-By-Fee) transactions to properly distinguish them from CPFP (Child-Pays-For-Parent) transactions on both sender and receiver sides.

Key changes:

• Event-driven architecture for onchain transaction updates (received, confirmed, replaced, reorged, evicted)
• Removal of 1-second polling timer in favor of real-time LDK events
• Enhanced RBF transaction detection using doesExist flag to differentiate from CPFP

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
BitkitNotification/NotificationService.swift	Added notification handling for new onchain transaction events
Bitkit/Views/Wallets/Sheets/BoostSheet.swift	Removed redundant LDK node payment sync after boost operation
Bitkit/Views/Wallets/Activity/ActivityItemView.swift	Enhanced boost button logic to differentiate between RBF and CPFP transactions
Bitkit/Views/Wallets/Activity/ActivityExplorerView.swift	Replaced AddressChecker API calls with LDK node transaction details
Bitkit/ViewModels/WalletViewModel.swift	Removed polling mechanism and added event-driven state updates for onchain transactions
Bitkit/ViewModels/AppViewModel.swift	Added comprehensive event handling for onchain transaction lifecycle
Bitkit/Utilities/AddressChecker.swift	Removed external API-dependent utility in favor of native LDK functionality
Bitkit/Services/LightningService.swift	Added transaction detail methods and event handlers for onchain events
Bitkit/Services/CoreService.swift	Refactored payment processing to handle LDK events and removed replacement tracking maps
Bitkit/AppScene.swift	Removed balance change observer and event-based activity sync
Bitkit.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved	Updated bitkit-core and ldk-node dependencies
Bitkit.xcodeproj/project.pbxproj	Updated project references for LDK package

[ben-kaufman]

@piotr-iohk please note we will need to update the e2e tests to close the onchain received sheet.

[ovitrif]

utAck

Amazing work, I've been waiting for over a year to finally have this setup in place: event-based sync for transactions 🎉

Added a few small nit comments, the one about the localizable strings applies to all cases where we added new toast messages, not only the code where I added it.

I wanted to give you the time to already make some small updates as I am starting to test this, hence me approving without testing; but I trust you'll wait for my testing 🙏🏻

[piotr-iohk]

Ok, updated e2e tests for current state (synonymdev/bitkit-e2e-tests#56). Re-running 🤞
It would be good to coordinate merging with Android (synonymdev/bitkit-android#475)

[ben-kaufman]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

[pwltr]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

Sounds good yeah I think an optimistic UI like this would be the way to go. Assume everything went right and correct if needed when the event(s) come in from LDK. Still feel free to postpone for now.

[ben-kaufman]

Actually had a simpler solution, fixed in: #247

[pwltr]

tACK with follow-up task here

Also note @piotr-iohk's request to merge this together with the Android counterpart.

[claude]

PR Review: Integrate LDK onchain events

This is an excellent architectural improvement that modernizes the onchain transaction handling by leveraging LDK's event-driven architecture. The change from polling to event-driven updates should significantly improve performance and user experience.

---

Summary of Changes

Major Improvements:

• Event-driven onchain transaction handling via LDK Node events
• Removed AddressChecker utility (110 lines removed)
• Enhanced RBF transaction tracking and handling
• Added comprehensive onchain event handling (received, confirmed, replaced, reorged, evicted)
• Improved user notifications for transaction state changes
• Better handling of edge cases (reorgs, mempool evictions)

Impact:

• 1,031 additions, 495 deletions across 20 files
• Core architectural change in CoreService.swift (+529/-273)
• Enhanced event handling in AppViewModel.swift (+108/-4)
• Improved LightningService.swift integration (+120/-3)

---

Code Quality & Best Practices

Strengths

1. Event-Driven Architecture - Proper use of LDK onchain events eliminates polling overhead
2. Comprehensive Error Handling - Good use of try-catch blocks with descriptive logging
3. Service Queue Pattern - Correctly uses ServiceQueue.background(.core) for Core operations
4. MainActor Compliance - Proper async/await patterns with @mainactor for UI updates
5. Caching Strategy - cachedTxIdsInBoostTxIds improves performance for boost transaction lookups
6. Detailed Logging - Excellent context-aware logging throughout
7. Accessibility - Added accessibilityIdentifier to Toast model and components

Code Organization

The new event handlers follow a clean pattern:

• handleOnchainTransactionReceived
• handleOnchainTransactionConfirmed
• handleOnchainTransactionReplaced
• handleOnchainTransactionReorged
• handleOnchainTransactionEvicted

Each handler properly delegates to ServiceQueue.background(.core) and updates the activity list appropriately.

---

Potential Bugs & Issues

1. Race Condition in handleOnchainTransactionReplaced

Location: CoreService.swift:346-433

Issue: When processing replacement transactions, there is a potential race condition between checking if replacementActivity exists and processing the payment. Another event handler could process the same transaction concurrently.

Recommendation: Add a lock or transaction ID tracking to prevent concurrent processing of the same transaction.

2. Cache Invalidation Issue

Location: CoreService.swift:34-62

Issue: The cachedTxIdsInBoostTxIds cache is updated in insert and bulk refreshed in upsertList, but individual update operations don't update the cache. This could lead to stale cache data.

Recommendation: Add cache update to the update method to keep cache consistent.

3. Potential Memory Leak in Event Handlers

Location: AppScene.swift:213-215

Issue: The event handler closure captures app weakly, but the closure itself is never removed when the scene is dismissed.

Recommendation: Ensure event handlers are removed when the scene is dismissed. The addOnEvent API should support removal, or use a cleanup mechanism in deinit or onDisappear.

4. Missing nil-check in processOnchainTransaction

Location: CoreService.swift:315-332

Issue: The function logs a warning if payment is not found but doesn't throw an error. The calling code may not handle this gracefully.

Recommendation: Consider throwing a specific error so callers can decide how to handle missing payments, or add metrics to track how often this occurs.

---

Performance Considerations

Strengths

1. Eliminated Polling - Removing AddressChecker and full syncs significantly reduces CPU/network overhead
2. Efficient Caching - cachedTxIdsInBoostTxIds prevents repeated database queries
3. Lazy Cache Initialization - Cache is only built when first accessed

Concerns

1. Cache Refresh Performance - CoreService.swift:47-62 loads ALL onchain activities from the database, which could be slow for users with large transaction histories (1000+ transactions). Consider a more efficient query that only fetches boostTxIds without loading full activity objects.

2. Delay in Received Sheet - AppViewModel.swift:399 has a 500ms delay. Is this necessary? It could make the UI feel sluggish. Consider removing or reducing if not strictly required.

---

Security Concerns

Good Practices

1. Proper use of ServiceQueue prevents concurrent access to Core/LDK
2. Transactions marked as doesExist = false when replaced (prevents double-counting)
3. RBF detection prevents showing duplicate received sheets

Recommendations

1. Transaction Validation - Consider adding validation that conflicts array in handleOnchainTransactionReplaced actually contains valid transaction IDs to prevent malformed data corruption.

2. Timestamp Validation - In processOnchainPayment, ensure timestamps are within reasonable ranges to prevent time-based attacks.

---

Test Coverage

Missing Test Scenarios

1. RBF Handling - Need tests for single replacement, multiple conflicts, chain of replacements, replacement arriving before original

2. Reorg Handling - Need tests for confirmed tx becoming unconfirmed, multiple txs affected by reorg, reorg depth > 1 block

3. Edge Cases - Need tests for tx evicted from mempool that was not replaced, race conditions, cache invalidation scenarios

Testing Recommendations

Add integration tests that simulate these event sequences:

• onchainTransactionReceived → onchainTransactionReplaced → onchainTransactionConfirmed
• onchainTransactionConfirmed → onchainTransactionReorged
• onchainTransactionReceived → onchainTransactionEvicted

---

Final Recommendations

Must Fix (Before Merge)

1. Run swiftformat . to ensure code formatting compliance
2. Fix cache invalidation in update method
3. Add race condition protection in handleOnchainTransactionReplaced
4. Verify event handler cleanup to prevent memory leaks

Should Fix (High Priority)

1. Optimize refreshBoostTxIdsCache for large transaction histories
2. Add comprehensive tests for RBF and reorg scenarios
3. Review the 500ms delay in received sheet display

Nice to Have

1. Add transaction ID truncation to user-facing messages
2. Improve user-friendly language for technical events (reorg, eviction)
3. Add metrics/telemetry for tracking event frequencies

---

Conclusion

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design pattern and will eliminate polling overhead. The code quality is generally excellent with proper error handling, logging, and async patterns.

Recommendation:Approve with minor changes. Address the cache invalidation and race condition issues before merge, and plan follow-up work for comprehensive testing and performance optimization.

Great work! The videos demonstrating reorg and eviction handling are particularly impressive.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time I try it

[claude]

PR Review: Integrate LDK onchain events

Summary

This PR successfully replaces polling-based transaction monitoring with event-driven LDK onchain events, removes the AddressChecker, and improves RBF transaction handling. This is a significant architectural improvement that makes the app more reactive and efficient.

✅ Strengths

Architecture & Design

1. Event-driven architecture: The transition from polling to LDK events (onchainTransactionReceived, onchainTransactionConfirmed, onchainTransactionReplaced, onchainTransactionReorged, onchainTransactionEvicted) is excellent. This provides real-time updates and reduces unnecessary syncs.

2. Comprehensive RBF handling: The implementation properly tracks replacement transactions through boostTxIds and distinguishes between RBF (replaced transactions with doesExist=false) and CPFP (child transactions with doesExist=true).

3. Clean separation of concerns: Event handlers in LightningService.swift (708-757) delegate to ActivityService methods, maintaining good separation between Lightning layer and activity management.

4. Improved user feedback: New toast notifications with accessibility identifiers for transaction states (replaced, reorged, evicted) provide better UX.

5. Removal of polling: Removing the onChange(of: wallet.totalBalanceSats) trigger in AppScene.swift:217 and the event-based sync in AppScene.swift:217-220 eliminates redundant activity syncing.

Code Quality

1. Good error handling: All event handlers wrap operations in try-catch blocks with proper logging.
2. Proper concurrency: Uses ServiceQueue.background(.core) for all database operations.
3. Cache optimization: The cachedTxIdsInBoostTxIds cache improves performance for checking replaced transactions.

⚠️ Issues & Concerns

Critical

1. Potential cache inconsistency in CoreService.swift:36-43

func getTxIdsInBoostTxIds()async->Set<String>{if cachedTxIdsInBoostTxIds.isEmpty {awaitrefreshBoostTxIdsCache()}return cachedTxIdsInBoostTxIds
}

Issue: The cache is only refreshed when empty, but individual updates via updateBoostTxIdsCache(for:) might not be called for all updates. The update() method at line 195 doesn't update the cache.

Recommendation:

• Update the cache in the update() method, or
• Implement a TTL-based cache invalidation strategy, or
• Document why the cache only needs to be refreshed when empty

2. Race condition in RBF handling (CoreService.swift:355-441)

When handleOnchainTransactionReplaced is called, it:

1. Marks the replaced transaction as doesExist=false
2. Processes replacement transactions that might not exist yet in the payment store
3. Waits for them via processOnchainPayment

Issue: There's a timing window where a replacement transaction event arrives before LDK updates the payment store. The code handles this (line 385-403) by processing the payment, but there's a potential race if the transaction hasn't been added to the payments list yet.

Recommendation: Consider adding retry logic or ensuring LDK guarantees payment store updates before emitting replacement events.

Moderate

3. Missing cache update in update() method (CoreService.swift:193-199)

func update(id:String, activity:Activity)asyncthrows{tryawaitServiceQueue.background(.core){tryupdateActivity(activityId: id, activity: activity)self.metadataChangedSubject.send()}}

This doesn't call updateBoostTxIdsCache(for: activity) like insert() does at line 172.

Recommendation: Add cache update to maintain consistency.

4. Incomplete validation in shouldShowReceivedSheet (CoreService.swift:76-107)

The method checks if a replacement transaction has the same value as the replaced transaction to avoid showing duplicate sheets. However:

• It only checks exact value matches
• Doesn't account for fee differences in RBF
• Could show sheets for CPFP where the child transaction might have a different value

Recommendation: Consider checking isBoosted flag and the nature of the boost (RBF vs CPFP) for more precise logic.

5. Error handling in handleOnchainTransactionReplaced (CoreService.swift:355-441)

When processing replacement transactions, errors are caught and logged (line 401-405), but the loop continues. If multiple replacement transactions fail to process, the relationship tracking could be incomplete.

Recommendation: Consider collecting errors and either throwing or returning them to the caller for better error visibility.

6. CI workflow changes remove caching (integration-tests.yml, unit-tests.yml)

The PR removes DerivedData caching and restore-keys from SPM cache. While this ensures clean builds, it will increase build times significantly.

Recommendation:

• Document why caching was removed (likely due to cache invalidation issues)
• Consider using Xcode Cloud or other CI optimizations to offset the performance loss
• The -onlyUsePackageVersionsFromResolvedFile flag is good for reproducibility

Minor

7. Potential N+1 query in getBoostTxDoesExist (CoreService.swift:130-138)

func getBoostTxDoesExist(boostTxIds:[String])async->[String:Bool]{vardoesExistMap:[String:Bool]=[:]forboostTxIdin boostTxIds {
if let boostActivity =try?awaitgetOnchainActivityByTxId(txid: boostTxId){doesExistMap[boostTxId]= boostActivity.doesExist
}}return doesExistMap
}

Issue: Makes individual database queries for each boost transaction.

Recommendation: Consider adding a batch query method to BitkitCore to fetch multiple activities by txId in one query.

8. Magic number delay in AppViewModel.swift:399

try?awaitTask.sleep(nanoseconds:500_000_000) // 500ms delay

Recommendation: Extract to a named constant with documentation explaining why the delay is necessary.

9. Missing test coverage

While TxBumpingTests.swift and ActivityListTest.swift exist, there don't appear to be specific tests for:

• Onchain event handlers
• RBF replacement tracking with boostTxIds
• Reorg and eviction scenarios
• Cache consistency

Recommendation: Add integration tests for the new event handlers.

🔒 Security

No major security concerns identified. The code properly:

• Uses ServiceQueue for thread safety
• Doesn't expose sensitive transaction data in logs beyond txids
• Maintains Bitcoin/Lightning operation isolation in the service layer

Minor: Logging in LightningService.swift:711-757 includes transaction amounts. Consider whether this is acceptable for production logging.

🚀 Performance

Positive impacts:

1. Eliminates redundant polling and full activity list syncs
2. Event-driven updates only process changed transactions
3. Cache for boostTxIds lookups

Potential concerns:

1. Cache not updated consistently (see issue Project setup #1, Migrate channels from LDK to LDK-node #3)
2. N+1 queries in getBoostTxDoesExist (see issue feat: keychain #7)

🧪 Testing

Existing test coverage:

• ActivityListTest.swift: Good coverage of basic activity CRUD operations
• TxBumpingTests.swift: Tests RBF scenarios

Missing:

• Tests for new onchain event handlers
• Tests for handleOnchainTransactionReplaced, handleOnchainTransactionReorged, handleOnchainTransactionEvicted
• Tests for cache consistency
• Integration tests for the complete event flow

Recommendation: Add integration tests that simulate the event sequences shown in the PR videos.

📝 Recommendations

Must Fix Before Merge

1. Fix cache consistency issues (Issues Project setup #1, Migrate channels from LDK to LDK-node #3)
2. Add error handling for race conditions in RBF handling (Issue Push service setup and service queues  #2)

Should Fix Before Merge

1. Improve shouldShowReceivedSheet validation logic (Issue Create swift.yml #4)
2. Better error aggregation in handleOnchainTransactionReplaced (Issue Create ios.yml #5)

Consider for Follow-up

1. Add batch query support for getBoostTxDoesExist (Issue feat: keychain #7)
2. Add integration tests for event handlers (Issue Use SwiftFormat #9)
3. Extract magic constants (Issue BDK 1.0.0-alpha.11 #8)
4. Document CI caching removal reasoning (Issue Create ios.yml #6)

🎯 Overall Assessment

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design, and the RBF handling logic is well thought out. The main concerns are around cache consistency and potential race conditions in RBF processing, which should be addressed before merge.

The removal of AddressChecker and polling-based syncing will improve app performance and responsiveness.

Recommendation: Request changes to address cache consistency and race condition handling, then approve after fixes.

---

Great work on this PR! The demos showing reorg and mempool eviction handling are particularly impressive. Once the cache and race condition issues are resolved, this will be a solid improvement to the codebase.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ben-kaufman]

This PR integrates LDK node on chain events to replace the previous pollin and full syncs.

It also removes Address Checker, and improves handling on RBFed transactions to be recognized on both sides.

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

Screen.Recording.2025-11-26.at.9.05.11.AM.mov

Reorg removes tx from blocks demo:

Screen.Recording.2025-11-26.at.5.13.45.PM.mov

Transaction removed from mempool demo:

Screen.Recording.2025-11-26.at.5.38.36.PM.mov

[Copilot]

Pull request overview

This PR integrates LDK node onchain events to replace previous polling and full sync mechanisms for Bitcoin transaction management. It removes the AddressChecker utility class that relied on external API calls, and improves handling of RBF (Replace-By-Fee) transactions to properly distinguish them from CPFP (Child-Pays-For-Parent) transactions on both sender and receiver sides.

Key changes:

• Event-driven architecture for onchain transaction updates (received, confirmed, replaced, reorged, evicted)
• Removal of 1-second polling timer in favor of real-time LDK events
• Enhanced RBF transaction detection using doesExist flag to differentiate from CPFP

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
BitkitNotification/NotificationService.swift	Added notification handling for new onchain transaction events
Bitkit/Views/Wallets/Sheets/BoostSheet.swift	Removed redundant LDK node payment sync after boost operation
Bitkit/Views/Wallets/Activity/ActivityItemView.swift	Enhanced boost button logic to differentiate between RBF and CPFP transactions
Bitkit/Views/Wallets/Activity/ActivityExplorerView.swift	Replaced AddressChecker API calls with LDK node transaction details
Bitkit/ViewModels/WalletViewModel.swift	Removed polling mechanism and added event-driven state updates for onchain transactions
Bitkit/ViewModels/AppViewModel.swift	Added comprehensive event handling for onchain transaction lifecycle
Bitkit/Utilities/AddressChecker.swift	Removed external API-dependent utility in favor of native LDK functionality
Bitkit/Services/LightningService.swift	Added transaction detail methods and event handlers for onchain events
Bitkit/Services/CoreService.swift	Refactored payment processing to handle LDK events and removed replacement tracking maps
Bitkit/AppScene.swift	Removed balance change observer and event-based activity sync
Bitkit.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved	Updated bitkit-core and ldk-node dependencies
Bitkit.xcodeproj/project.pbxproj	Updated project references for LDK package

[ben-kaufman]

@piotr-iohk please note we will need to update the e2e tests to close the onchain received sheet.

[ovitrif]

utAck

Amazing work, I've been waiting for over a year to finally have this setup in place: event-based sync for transactions 🎉

Added a few small nit comments, the one about the localizable strings applies to all cases where we added new toast messages, not only the code where I added it.

I wanted to give you the time to already make some small updates as I am starting to test this, hence me approving without testing; but I trust you'll wait for my testing 🙏🏻

[piotr-iohk]

Ok, updated e2e tests for current state (synonymdev/bitkit-e2e-tests#56). Re-running 🤞
It would be good to coordinate merging with Android (synonymdev/bitkit-android#475)

[ben-kaufman]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

[pwltr]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

Sounds good yeah I think an optimistic UI like this would be the way to go. Assume everything went right and correct if needed when the event(s) come in from LDK. Still feel free to postpone for now.

[ben-kaufman]

Actually had a simpler solution, fixed in: #247

[pwltr]

tACK with follow-up task here

Also note @piotr-iohk's request to merge this together with the Android counterpart.

[claude]

PR Review: Integrate LDK onchain events

This is an excellent architectural improvement that modernizes the onchain transaction handling by leveraging LDK's event-driven architecture. The change from polling to event-driven updates should significantly improve performance and user experience.

---

Summary of Changes

Major Improvements:

• Event-driven onchain transaction handling via LDK Node events
• Removed AddressChecker utility (110 lines removed)
• Enhanced RBF transaction tracking and handling
• Added comprehensive onchain event handling (received, confirmed, replaced, reorged, evicted)
• Improved user notifications for transaction state changes
• Better handling of edge cases (reorgs, mempool evictions)

Impact:

• 1,031 additions, 495 deletions across 20 files
• Core architectural change in CoreService.swift (+529/-273)
• Enhanced event handling in AppViewModel.swift (+108/-4)
• Improved LightningService.swift integration (+120/-3)

---

Code Quality & Best Practices

Strengths

1. Event-Driven Architecture - Proper use of LDK onchain events eliminates polling overhead
2. Comprehensive Error Handling - Good use of try-catch blocks with descriptive logging
3. Service Queue Pattern - Correctly uses ServiceQueue.background(.core) for Core operations
4. MainActor Compliance - Proper async/await patterns with @mainactor for UI updates
5. Caching Strategy - cachedTxIdsInBoostTxIds improves performance for boost transaction lookups
6. Detailed Logging - Excellent context-aware logging throughout
7. Accessibility - Added accessibilityIdentifier to Toast model and components

Code Organization

The new event handlers follow a clean pattern:

• handleOnchainTransactionReceived
• handleOnchainTransactionConfirmed
• handleOnchainTransactionReplaced
• handleOnchainTransactionReorged
• handleOnchainTransactionEvicted

Each handler properly delegates to ServiceQueue.background(.core) and updates the activity list appropriately.

---

Potential Bugs & Issues

1. Race Condition in handleOnchainTransactionReplaced

Location: CoreService.swift:346-433

Issue: When processing replacement transactions, there is a potential race condition between checking if replacementActivity exists and processing the payment. Another event handler could process the same transaction concurrently.

Recommendation: Add a lock or transaction ID tracking to prevent concurrent processing of the same transaction.

2. Cache Invalidation Issue

Location: CoreService.swift:34-62

Issue: The cachedTxIdsInBoostTxIds cache is updated in insert and bulk refreshed in upsertList, but individual update operations don't update the cache. This could lead to stale cache data.

Recommendation: Add cache update to the update method to keep cache consistent.

3. Potential Memory Leak in Event Handlers

Location: AppScene.swift:213-215

Issue: The event handler closure captures app weakly, but the closure itself is never removed when the scene is dismissed.

Recommendation: Ensure event handlers are removed when the scene is dismissed. The addOnEvent API should support removal, or use a cleanup mechanism in deinit or onDisappear.

4. Missing nil-check in processOnchainTransaction

Location: CoreService.swift:315-332

Issue: The function logs a warning if payment is not found but doesn't throw an error. The calling code may not handle this gracefully.

Recommendation: Consider throwing a specific error so callers can decide how to handle missing payments, or add metrics to track how often this occurs.

---

Performance Considerations

Strengths

1. Eliminated Polling - Removing AddressChecker and full syncs significantly reduces CPU/network overhead
2. Efficient Caching - cachedTxIdsInBoostTxIds prevents repeated database queries
3. Lazy Cache Initialization - Cache is only built when first accessed

Concerns

1. Cache Refresh Performance - CoreService.swift:47-62 loads ALL onchain activities from the database, which could be slow for users with large transaction histories (1000+ transactions). Consider a more efficient query that only fetches boostTxIds without loading full activity objects.

2. Delay in Received Sheet - AppViewModel.swift:399 has a 500ms delay. Is this necessary? It could make the UI feel sluggish. Consider removing or reducing if not strictly required.

---

Security Concerns

Good Practices

1. Proper use of ServiceQueue prevents concurrent access to Core/LDK
2. Transactions marked as doesExist = false when replaced (prevents double-counting)
3. RBF detection prevents showing duplicate received sheets

Recommendations

1. Transaction Validation - Consider adding validation that conflicts array in handleOnchainTransactionReplaced actually contains valid transaction IDs to prevent malformed data corruption.

2. Timestamp Validation - In processOnchainPayment, ensure timestamps are within reasonable ranges to prevent time-based attacks.

---

Test Coverage

Missing Test Scenarios

1. RBF Handling - Need tests for single replacement, multiple conflicts, chain of replacements, replacement arriving before original

2. Reorg Handling - Need tests for confirmed tx becoming unconfirmed, multiple txs affected by reorg, reorg depth > 1 block

3. Edge Cases - Need tests for tx evicted from mempool that was not replaced, race conditions, cache invalidation scenarios

Testing Recommendations

Add integration tests that simulate these event sequences:

• onchainTransactionReceived → onchainTransactionReplaced → onchainTransactionConfirmed
• onchainTransactionConfirmed → onchainTransactionReorged
• onchainTransactionReceived → onchainTransactionEvicted

---

Final Recommendations

Must Fix (Before Merge)

1. Run swiftformat . to ensure code formatting compliance
2. Fix cache invalidation in update method
3. Add race condition protection in handleOnchainTransactionReplaced
4. Verify event handler cleanup to prevent memory leaks

Should Fix (High Priority)

1. Optimize refreshBoostTxIdsCache for large transaction histories
2. Add comprehensive tests for RBF and reorg scenarios
3. Review the 500ms delay in received sheet display

Nice to Have

1. Add transaction ID truncation to user-facing messages
2. Improve user-friendly language for technical events (reorg, eviction)
3. Add metrics/telemetry for tracking event frequencies

---

Conclusion

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design pattern and will eliminate polling overhead. The code quality is generally excellent with proper error handling, logging, and async patterns.

Recommendation:Approve with minor changes. Address the cache invalidation and race condition issues before merge, and plan follow-up work for comprehensive testing and performance optimization.

Great work! The videos demonstrating reorg and eviction handling are particularly impressive.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time I try it

[claude]

PR Review: Integrate LDK onchain events

Summary

This PR successfully replaces polling-based transaction monitoring with event-driven LDK onchain events, removes the AddressChecker, and improves RBF transaction handling. This is a significant architectural improvement that makes the app more reactive and efficient.

✅ Strengths

Architecture & Design

1. Event-driven architecture: The transition from polling to LDK events (onchainTransactionReceived, onchainTransactionConfirmed, onchainTransactionReplaced, onchainTransactionReorged, onchainTransactionEvicted) is excellent. This provides real-time updates and reduces unnecessary syncs.

2. Comprehensive RBF handling: The implementation properly tracks replacement transactions through boostTxIds and distinguishes between RBF (replaced transactions with doesExist=false) and CPFP (child transactions with doesExist=true).

3. Clean separation of concerns: Event handlers in LightningService.swift (708-757) delegate to ActivityService methods, maintaining good separation between Lightning layer and activity management.

4. Improved user feedback: New toast notifications with accessibility identifiers for transaction states (replaced, reorged, evicted) provide better UX.

5. Removal of polling: Removing the onChange(of: wallet.totalBalanceSats) trigger in AppScene.swift:217 and the event-based sync in AppScene.swift:217-220 eliminates redundant activity syncing.

Code Quality

1. Good error handling: All event handlers wrap operations in try-catch blocks with proper logging.
2. Proper concurrency: Uses ServiceQueue.background(.core) for all database operations.
3. Cache optimization: The cachedTxIdsInBoostTxIds cache improves performance for checking replaced transactions.

⚠️ Issues & Concerns

Critical

1. Potential cache inconsistency in CoreService.swift:36-43

func getTxIdsInBoostTxIds()async->Set<String>{if cachedTxIdsInBoostTxIds.isEmpty {awaitrefreshBoostTxIdsCache()}return cachedTxIdsInBoostTxIds
}

Issue: The cache is only refreshed when empty, but individual updates via updateBoostTxIdsCache(for:) might not be called for all updates. The update() method at line 195 doesn't update the cache.

Recommendation:

• Update the cache in the update() method, or
• Implement a TTL-based cache invalidation strategy, or
• Document why the cache only needs to be refreshed when empty

2. Race condition in RBF handling (CoreService.swift:355-441)

When handleOnchainTransactionReplaced is called, it:

1. Marks the replaced transaction as doesExist=false
2. Processes replacement transactions that might not exist yet in the payment store
3. Waits for them via processOnchainPayment

Issue: There's a timing window where a replacement transaction event arrives before LDK updates the payment store. The code handles this (line 385-403) by processing the payment, but there's a potential race if the transaction hasn't been added to the payments list yet.

Recommendation: Consider adding retry logic or ensuring LDK guarantees payment store updates before emitting replacement events.

Moderate

3. Missing cache update in update() method (CoreService.swift:193-199)

func update(id:String, activity:Activity)asyncthrows{tryawaitServiceQueue.background(.core){tryupdateActivity(activityId: id, activity: activity)self.metadataChangedSubject.send()}}

This doesn't call updateBoostTxIdsCache(for: activity) like insert() does at line 172.

Recommendation: Add cache update to maintain consistency.

4. Incomplete validation in shouldShowReceivedSheet (CoreService.swift:76-107)

The method checks if a replacement transaction has the same value as the replaced transaction to avoid showing duplicate sheets. However:

• It only checks exact value matches
• Doesn't account for fee differences in RBF
• Could show sheets for CPFP where the child transaction might have a different value

Recommendation: Consider checking isBoosted flag and the nature of the boost (RBF vs CPFP) for more precise logic.

5. Error handling in handleOnchainTransactionReplaced (CoreService.swift:355-441)

When processing replacement transactions, errors are caught and logged (line 401-405), but the loop continues. If multiple replacement transactions fail to process, the relationship tracking could be incomplete.

Recommendation: Consider collecting errors and either throwing or returning them to the caller for better error visibility.

6. CI workflow changes remove caching (integration-tests.yml, unit-tests.yml)

The PR removes DerivedData caching and restore-keys from SPM cache. While this ensures clean builds, it will increase build times significantly.

Recommendation:

• Document why caching was removed (likely due to cache invalidation issues)
• Consider using Xcode Cloud or other CI optimizations to offset the performance loss
• The -onlyUsePackageVersionsFromResolvedFile flag is good for reproducibility

Minor

7. Potential N+1 query in getBoostTxDoesExist (CoreService.swift:130-138)

func getBoostTxDoesExist(boostTxIds:[String])async->[String:Bool]{vardoesExistMap:[String:Bool]=[:]forboostTxIdin boostTxIds {
if let boostActivity =try?awaitgetOnchainActivityByTxId(txid: boostTxId){doesExistMap[boostTxId]= boostActivity.doesExist
}}return doesExistMap
}

Issue: Makes individual database queries for each boost transaction.

Recommendation: Consider adding a batch query method to BitkitCore to fetch multiple activities by txId in one query.

8. Magic number delay in AppViewModel.swift:399

try?awaitTask.sleep(nanoseconds:500_000_000) // 500ms delay

Recommendation: Extract to a named constant with documentation explaining why the delay is necessary.

9. Missing test coverage

While TxBumpingTests.swift and ActivityListTest.swift exist, there don't appear to be specific tests for:

• Onchain event handlers
• RBF replacement tracking with boostTxIds
• Reorg and eviction scenarios
• Cache consistency

Recommendation: Add integration tests for the new event handlers.

🔒 Security

No major security concerns identified. The code properly:

• Uses ServiceQueue for thread safety
• Doesn't expose sensitive transaction data in logs beyond txids
• Maintains Bitcoin/Lightning operation isolation in the service layer

Minor: Logging in LightningService.swift:711-757 includes transaction amounts. Consider whether this is acceptable for production logging.

🚀 Performance

Positive impacts:

1. Eliminates redundant polling and full activity list syncs
2. Event-driven updates only process changed transactions
3. Cache for boostTxIds lookups

Potential concerns:

1. Cache not updated consistently (see issue Project setup #1, Migrate channels from LDK to LDK-node #3)
2. N+1 queries in getBoostTxDoesExist (see issue feat: keychain #7)

🧪 Testing

Existing test coverage:

• ActivityListTest.swift: Good coverage of basic activity CRUD operations
• TxBumpingTests.swift: Tests RBF scenarios

Missing:

• Tests for new onchain event handlers
• Tests for handleOnchainTransactionReplaced, handleOnchainTransactionReorged, handleOnchainTransactionEvicted
• Tests for cache consistency
• Integration tests for the complete event flow

Recommendation: Add integration tests that simulate the event sequences shown in the PR videos.

📝 Recommendations

Must Fix Before Merge

1. Fix cache consistency issues (Issues Project setup #1, Migrate channels from LDK to LDK-node #3)
2. Add error handling for race conditions in RBF handling (Issue Push service setup and service queues  #2)

Should Fix Before Merge

1. Improve shouldShowReceivedSheet validation logic (Issue Create swift.yml #4)
2. Better error aggregation in handleOnchainTransactionReplaced (Issue Create ios.yml #5)

Consider for Follow-up

1. Add batch query support for getBoostTxDoesExist (Issue feat: keychain #7)
2. Add integration tests for event handlers (Issue Use SwiftFormat #9)
3. Extract magic constants (Issue BDK 1.0.0-alpha.11 #8)
4. Document CI caching removal reasoning (Issue Create ios.yml #6)

🎯 Overall Assessment

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design, and the RBF handling logic is well thought out. The main concerns are around cache consistency and potential race conditions in RBF processing, which should be addressed before merge.

The removal of AddressChecker and polling-based syncing will improve app performance and responsiveness.

Recommendation: Request changes to address cache consistency and race condition handling, then approve after fixes.

---

Great work on this PR! The demos showing reorg and mempool eviction handling are particularly impressive. Once the cache and race condition issues are resolved, this will be a solid improvement to the codebase.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ben-kaufman]

This PR integrates LDK node on chain events to replace the previous pollin and full syncs.

It also removes Address Checker, and improves handling on RBFed transactions to be recognized on both sides.

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

Screen.Recording.2025-11-26.at.9.05.11.AM.mov

Reorg removes tx from blocks demo:

Screen.Recording.2025-11-26.at.5.13.45.PM.mov

Transaction removed from mempool demo:

Screen.Recording.2025-11-26.at.5.38.36.PM.mov

[Copilot]

Pull request overview

This PR integrates LDK node onchain events to replace previous polling and full sync mechanisms for Bitcoin transaction management. It removes the AddressChecker utility class that relied on external API calls, and improves handling of RBF (Replace-By-Fee) transactions to properly distinguish them from CPFP (Child-Pays-For-Parent) transactions on both sender and receiver sides.

Key changes:

• Event-driven architecture for onchain transaction updates (received, confirmed, replaced, reorged, evicted)
• Removal of 1-second polling timer in favor of real-time LDK events
• Enhanced RBF transaction detection using doesExist flag to differentiate from CPFP

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
BitkitNotification/NotificationService.swift	Added notification handling for new onchain transaction events
Bitkit/Views/Wallets/Sheets/BoostSheet.swift	Removed redundant LDK node payment sync after boost operation
Bitkit/Views/Wallets/Activity/ActivityItemView.swift	Enhanced boost button logic to differentiate between RBF and CPFP transactions
Bitkit/Views/Wallets/Activity/ActivityExplorerView.swift	Replaced AddressChecker API calls with LDK node transaction details
Bitkit/ViewModels/WalletViewModel.swift	Removed polling mechanism and added event-driven state updates for onchain transactions
Bitkit/ViewModels/AppViewModel.swift	Added comprehensive event handling for onchain transaction lifecycle
Bitkit/Utilities/AddressChecker.swift	Removed external API-dependent utility in favor of native LDK functionality
Bitkit/Services/LightningService.swift	Added transaction detail methods and event handlers for onchain events
Bitkit/Services/CoreService.swift	Refactored payment processing to handle LDK events and removed replacement tracking maps
Bitkit/AppScene.swift	Removed balance change observer and event-based activity sync
Bitkit.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved	Updated bitkit-core and ldk-node dependencies
Bitkit.xcodeproj/project.pbxproj	Updated project references for LDK package

[ben-kaufman]

@piotr-iohk please note we will need to update the e2e tests to close the onchain received sheet.

[ovitrif]

utAck

Amazing work, I've been waiting for over a year to finally have this setup in place: event-based sync for transactions 🎉

Added a few small nit comments, the one about the localizable strings applies to all cases where we added new toast messages, not only the code where I added it.

I wanted to give you the time to already make some small updates as I am starting to test this, hence me approving without testing; but I trust you'll wait for my testing 🙏🏻

[piotr-iohk]

Ok, updated e2e tests for current state (synonymdev/bitkit-e2e-tests#56). Re-running 🤞
It would be good to coordinate merging with Android (synonymdev/bitkit-android#475)

[ben-kaufman]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

[pwltr]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

Sounds good yeah I think an optimistic UI like this would be the way to go. Assume everything went right and correct if needed when the event(s) come in from LDK. Still feel free to postpone for now.

[ben-kaufman]

Actually had a simpler solution, fixed in: #247

[pwltr]

tACK with follow-up task here

Also note @piotr-iohk's request to merge this together with the Android counterpart.

[claude]

PR Review: Integrate LDK onchain events

This is an excellent architectural improvement that modernizes the onchain transaction handling by leveraging LDK's event-driven architecture. The change from polling to event-driven updates should significantly improve performance and user experience.

---

Summary of Changes

Major Improvements:

• Event-driven onchain transaction handling via LDK Node events
• Removed AddressChecker utility (110 lines removed)
• Enhanced RBF transaction tracking and handling
• Added comprehensive onchain event handling (received, confirmed, replaced, reorged, evicted)
• Improved user notifications for transaction state changes
• Better handling of edge cases (reorgs, mempool evictions)

Impact:

• 1,031 additions, 495 deletions across 20 files
• Core architectural change in CoreService.swift (+529/-273)
• Enhanced event handling in AppViewModel.swift (+108/-4)
• Improved LightningService.swift integration (+120/-3)

---

Code Quality & Best Practices

Strengths

1. Event-Driven Architecture - Proper use of LDK onchain events eliminates polling overhead
2. Comprehensive Error Handling - Good use of try-catch blocks with descriptive logging
3. Service Queue Pattern - Correctly uses ServiceQueue.background(.core) for Core operations
4. MainActor Compliance - Proper async/await patterns with @mainactor for UI updates
5. Caching Strategy - cachedTxIdsInBoostTxIds improves performance for boost transaction lookups
6. Detailed Logging - Excellent context-aware logging throughout
7. Accessibility - Added accessibilityIdentifier to Toast model and components

Code Organization

The new event handlers follow a clean pattern:

• handleOnchainTransactionReceived
• handleOnchainTransactionConfirmed
• handleOnchainTransactionReplaced
• handleOnchainTransactionReorged
• handleOnchainTransactionEvicted

Each handler properly delegates to ServiceQueue.background(.core) and updates the activity list appropriately.

---

Potential Bugs & Issues

1. Race Condition in handleOnchainTransactionReplaced

Location: CoreService.swift:346-433

Issue: When processing replacement transactions, there is a potential race condition between checking if replacementActivity exists and processing the payment. Another event handler could process the same transaction concurrently.

Recommendation: Add a lock or transaction ID tracking to prevent concurrent processing of the same transaction.

2. Cache Invalidation Issue

Location: CoreService.swift:34-62

Issue: The cachedTxIdsInBoostTxIds cache is updated in insert and bulk refreshed in upsertList, but individual update operations don't update the cache. This could lead to stale cache data.

Recommendation: Add cache update to the update method to keep cache consistent.

3. Potential Memory Leak in Event Handlers

Location: AppScene.swift:213-215

Issue: The event handler closure captures app weakly, but the closure itself is never removed when the scene is dismissed.

Recommendation: Ensure event handlers are removed when the scene is dismissed. The addOnEvent API should support removal, or use a cleanup mechanism in deinit or onDisappear.

4. Missing nil-check in processOnchainTransaction

Location: CoreService.swift:315-332

Issue: The function logs a warning if payment is not found but doesn't throw an error. The calling code may not handle this gracefully.

Recommendation: Consider throwing a specific error so callers can decide how to handle missing payments, or add metrics to track how often this occurs.

---

Performance Considerations

Strengths

1. Eliminated Polling - Removing AddressChecker and full syncs significantly reduces CPU/network overhead
2. Efficient Caching - cachedTxIdsInBoostTxIds prevents repeated database queries
3. Lazy Cache Initialization - Cache is only built when first accessed

Concerns

1. Cache Refresh Performance - CoreService.swift:47-62 loads ALL onchain activities from the database, which could be slow for users with large transaction histories (1000+ transactions). Consider a more efficient query that only fetches boostTxIds without loading full activity objects.

2. Delay in Received Sheet - AppViewModel.swift:399 has a 500ms delay. Is this necessary? It could make the UI feel sluggish. Consider removing or reducing if not strictly required.

---

Security Concerns

Good Practices

1. Proper use of ServiceQueue prevents concurrent access to Core/LDK
2. Transactions marked as doesExist = false when replaced (prevents double-counting)
3. RBF detection prevents showing duplicate received sheets

Recommendations

1. Transaction Validation - Consider adding validation that conflicts array in handleOnchainTransactionReplaced actually contains valid transaction IDs to prevent malformed data corruption.

2. Timestamp Validation - In processOnchainPayment, ensure timestamps are within reasonable ranges to prevent time-based attacks.

---

Test Coverage

Missing Test Scenarios

1. RBF Handling - Need tests for single replacement, multiple conflicts, chain of replacements, replacement arriving before original

2. Reorg Handling - Need tests for confirmed tx becoming unconfirmed, multiple txs affected by reorg, reorg depth > 1 block

3. Edge Cases - Need tests for tx evicted from mempool that was not replaced, race conditions, cache invalidation scenarios

Testing Recommendations

Add integration tests that simulate these event sequences:

• onchainTransactionReceived → onchainTransactionReplaced → onchainTransactionConfirmed
• onchainTransactionConfirmed → onchainTransactionReorged
• onchainTransactionReceived → onchainTransactionEvicted

---

Final Recommendations

Must Fix (Before Merge)

1. Run swiftformat . to ensure code formatting compliance
2. Fix cache invalidation in update method
3. Add race condition protection in handleOnchainTransactionReplaced
4. Verify event handler cleanup to prevent memory leaks

Should Fix (High Priority)

1. Optimize refreshBoostTxIdsCache for large transaction histories
2. Add comprehensive tests for RBF and reorg scenarios
3. Review the 500ms delay in received sheet display

Nice to Have

1. Add transaction ID truncation to user-facing messages
2. Improve user-friendly language for technical events (reorg, eviction)
3. Add metrics/telemetry for tracking event frequencies

---

Conclusion

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design pattern and will eliminate polling overhead. The code quality is generally excellent with proper error handling, logging, and async patterns.

Recommendation:Approve with minor changes. Address the cache invalidation and race condition issues before merge, and plan follow-up work for comprehensive testing and performance optimization.

Great work! The videos demonstrating reorg and eviction handling are particularly impressive.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time I try it

[claude]

PR Review: Integrate LDK onchain events

Summary

This PR successfully replaces polling-based transaction monitoring with event-driven LDK onchain events, removes the AddressChecker, and improves RBF transaction handling. This is a significant architectural improvement that makes the app more reactive and efficient.

✅ Strengths

Architecture & Design

1. Event-driven architecture: The transition from polling to LDK events (onchainTransactionReceived, onchainTransactionConfirmed, onchainTransactionReplaced, onchainTransactionReorged, onchainTransactionEvicted) is excellent. This provides real-time updates and reduces unnecessary syncs.

2. Comprehensive RBF handling: The implementation properly tracks replacement transactions through boostTxIds and distinguishes between RBF (replaced transactions with doesExist=false) and CPFP (child transactions with doesExist=true).

3. Clean separation of concerns: Event handlers in LightningService.swift (708-757) delegate to ActivityService methods, maintaining good separation between Lightning layer and activity management.

4. Improved user feedback: New toast notifications with accessibility identifiers for transaction states (replaced, reorged, evicted) provide better UX.

5. Removal of polling: Removing the onChange(of: wallet.totalBalanceSats) trigger in AppScene.swift:217 and the event-based sync in AppScene.swift:217-220 eliminates redundant activity syncing.

Code Quality

1. Good error handling: All event handlers wrap operations in try-catch blocks with proper logging.
2. Proper concurrency: Uses ServiceQueue.background(.core) for all database operations.
3. Cache optimization: The cachedTxIdsInBoostTxIds cache improves performance for checking replaced transactions.

⚠️ Issues & Concerns

Critical

1. Potential cache inconsistency in CoreService.swift:36-43

func getTxIdsInBoostTxIds()async->Set<String>{if cachedTxIdsInBoostTxIds.isEmpty {awaitrefreshBoostTxIdsCache()}return cachedTxIdsInBoostTxIds
}

Issue: The cache is only refreshed when empty, but individual updates via updateBoostTxIdsCache(for:) might not be called for all updates. The update() method at line 195 doesn't update the cache.

Recommendation:

• Update the cache in the update() method, or
• Implement a TTL-based cache invalidation strategy, or
• Document why the cache only needs to be refreshed when empty

2. Race condition in RBF handling (CoreService.swift:355-441)

When handleOnchainTransactionReplaced is called, it:

1. Marks the replaced transaction as doesExist=false
2. Processes replacement transactions that might not exist yet in the payment store
3. Waits for them via processOnchainPayment

Issue: There's a timing window where a replacement transaction event arrives before LDK updates the payment store. The code handles this (line 385-403) by processing the payment, but there's a potential race if the transaction hasn't been added to the payments list yet.

Recommendation: Consider adding retry logic or ensuring LDK guarantees payment store updates before emitting replacement events.

Moderate

3. Missing cache update in update() method (CoreService.swift:193-199)

func update(id:String, activity:Activity)asyncthrows{tryawaitServiceQueue.background(.core){tryupdateActivity(activityId: id, activity: activity)self.metadataChangedSubject.send()}}

This doesn't call updateBoostTxIdsCache(for: activity) like insert() does at line 172.

Recommendation: Add cache update to maintain consistency.

4. Incomplete validation in shouldShowReceivedSheet (CoreService.swift:76-107)

The method checks if a replacement transaction has the same value as the replaced transaction to avoid showing duplicate sheets. However:

• It only checks exact value matches
• Doesn't account for fee differences in RBF
• Could show sheets for CPFP where the child transaction might have a different value

Recommendation: Consider checking isBoosted flag and the nature of the boost (RBF vs CPFP) for more precise logic.

5. Error handling in handleOnchainTransactionReplaced (CoreService.swift:355-441)

When processing replacement transactions, errors are caught and logged (line 401-405), but the loop continues. If multiple replacement transactions fail to process, the relationship tracking could be incomplete.

Recommendation: Consider collecting errors and either throwing or returning them to the caller for better error visibility.

6. CI workflow changes remove caching (integration-tests.yml, unit-tests.yml)

The PR removes DerivedData caching and restore-keys from SPM cache. While this ensures clean builds, it will increase build times significantly.

Recommendation:

• Document why caching was removed (likely due to cache invalidation issues)
• Consider using Xcode Cloud or other CI optimizations to offset the performance loss
• The -onlyUsePackageVersionsFromResolvedFile flag is good for reproducibility

Minor

7. Potential N+1 query in getBoostTxDoesExist (CoreService.swift:130-138)

func getBoostTxDoesExist(boostTxIds:[String])async->[String:Bool]{vardoesExistMap:[String:Bool]=[:]forboostTxIdin boostTxIds {
if let boostActivity =try?awaitgetOnchainActivityByTxId(txid: boostTxId){doesExistMap[boostTxId]= boostActivity.doesExist
}}return doesExistMap
}

Issue: Makes individual database queries for each boost transaction.

Recommendation: Consider adding a batch query method to BitkitCore to fetch multiple activities by txId in one query.

8. Magic number delay in AppViewModel.swift:399

try?awaitTask.sleep(nanoseconds:500_000_000) // 500ms delay

Recommendation: Extract to a named constant with documentation explaining why the delay is necessary.

9. Missing test coverage

While TxBumpingTests.swift and ActivityListTest.swift exist, there don't appear to be specific tests for:

• Onchain event handlers
• RBF replacement tracking with boostTxIds
• Reorg and eviction scenarios
• Cache consistency

Recommendation: Add integration tests for the new event handlers.

🔒 Security

No major security concerns identified. The code properly:

• Uses ServiceQueue for thread safety
• Doesn't expose sensitive transaction data in logs beyond txids
• Maintains Bitcoin/Lightning operation isolation in the service layer

Minor: Logging in LightningService.swift:711-757 includes transaction amounts. Consider whether this is acceptable for production logging.

🚀 Performance

Positive impacts:

1. Eliminates redundant polling and full activity list syncs
2. Event-driven updates only process changed transactions
3. Cache for boostTxIds lookups

Potential concerns:

1. Cache not updated consistently (see issue Project setup #1, Migrate channels from LDK to LDK-node #3)
2. N+1 queries in getBoostTxDoesExist (see issue feat: keychain #7)

🧪 Testing

Existing test coverage:

• ActivityListTest.swift: Good coverage of basic activity CRUD operations
• TxBumpingTests.swift: Tests RBF scenarios

Missing:

• Tests for new onchain event handlers
• Tests for handleOnchainTransactionReplaced, handleOnchainTransactionReorged, handleOnchainTransactionEvicted
• Tests for cache consistency
• Integration tests for the complete event flow

Recommendation: Add integration tests that simulate the event sequences shown in the PR videos.

📝 Recommendations

Must Fix Before Merge

1. Fix cache consistency issues (Issues Project setup #1, Migrate channels from LDK to LDK-node #3)
2. Add error handling for race conditions in RBF handling (Issue Push service setup and service queues  #2)

Should Fix Before Merge

1. Improve shouldShowReceivedSheet validation logic (Issue Create swift.yml #4)
2. Better error aggregation in handleOnchainTransactionReplaced (Issue Create ios.yml #5)

Consider for Follow-up

1. Add batch query support for getBoostTxDoesExist (Issue feat: keychain #7)
2. Add integration tests for event handlers (Issue Use SwiftFormat #9)
3. Extract magic constants (Issue BDK 1.0.0-alpha.11 #8)
4. Document CI caching removal reasoning (Issue Create ios.yml #6)

🎯 Overall Assessment

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design, and the RBF handling logic is well thought out. The main concerns are around cache consistency and potential race conditions in RBF processing, which should be addressed before merge.

The removal of AddressChecker and polling-based syncing will improve app performance and responsiveness.

Recommendation: Request changes to address cache consistency and race condition handling, then approve after fixes.

---

Great work on this PR! The demos showing reorg and mempool eviction handling are particularly impressive. Once the cache and race condition issues are resolved, this will be a solid improvement to the codebase.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ben-kaufman]

This PR integrates LDK node on chain events to replace the previous pollin and full syncs.

It also removes Address Checker, and improves handling on RBFed transactions to be recognized on both sides.

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

Screen.Recording.2025-11-26.at.9.05.11.AM.mov

Reorg removes tx from blocks demo:

Screen.Recording.2025-11-26.at.5.13.45.PM.mov

Transaction removed from mempool demo:

Screen.Recording.2025-11-26.at.5.38.36.PM.mov

[Copilot]

Pull request overview

This PR integrates LDK node onchain events to replace previous polling and full sync mechanisms for Bitcoin transaction management. It removes the AddressChecker utility class that relied on external API calls, and improves handling of RBF (Replace-By-Fee) transactions to properly distinguish them from CPFP (Child-Pays-For-Parent) transactions on both sender and receiver sides.

Key changes:

• Event-driven architecture for onchain transaction updates (received, confirmed, replaced, reorged, evicted)
• Removal of 1-second polling timer in favor of real-time LDK events
• Enhanced RBF transaction detection using doesExist flag to differentiate from CPFP

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
BitkitNotification/NotificationService.swift	Added notification handling for new onchain transaction events
Bitkit/Views/Wallets/Sheets/BoostSheet.swift	Removed redundant LDK node payment sync after boost operation
Bitkit/Views/Wallets/Activity/ActivityItemView.swift	Enhanced boost button logic to differentiate between RBF and CPFP transactions
Bitkit/Views/Wallets/Activity/ActivityExplorerView.swift	Replaced AddressChecker API calls with LDK node transaction details
Bitkit/ViewModels/WalletViewModel.swift	Removed polling mechanism and added event-driven state updates for onchain transactions
Bitkit/ViewModels/AppViewModel.swift	Added comprehensive event handling for onchain transaction lifecycle
Bitkit/Utilities/AddressChecker.swift	Removed external API-dependent utility in favor of native LDK functionality
Bitkit/Services/LightningService.swift	Added transaction detail methods and event handlers for onchain events
Bitkit/Services/CoreService.swift	Refactored payment processing to handle LDK events and removed replacement tracking maps
Bitkit/AppScene.swift	Removed balance change observer and event-based activity sync
Bitkit.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved	Updated bitkit-core and ldk-node dependencies
Bitkit.xcodeproj/project.pbxproj	Updated project references for LDK package

[ben-kaufman]

@piotr-iohk please note we will need to update the e2e tests to close the onchain received sheet.

[ovitrif]

utAck

Amazing work, I've been waiting for over a year to finally have this setup in place: event-based sync for transactions 🎉

Added a few small nit comments, the one about the localizable strings applies to all cases where we added new toast messages, not only the code where I added it.

I wanted to give you the time to already make some small updates as I am starting to test this, hence me approving without testing; but I trust you'll wait for my testing 🙏🏻

[piotr-iohk]

Ok, updated e2e tests for current state (synonymdev/bitkit-e2e-tests#56). Re-running 🤞
It would be good to coordinate merging with Android (synonymdev/bitkit-android#475)

[ben-kaufman]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

[pwltr]

@pwltr it's not due to race condition, it's just because the event of the new tx takes a few seconds to come in from the LDK node.

We can solve it by immediately caching the "intermediate" state in memory until the event comes in.

Sounds good yeah I think an optimistic UI like this would be the way to go. Assume everything went right and correct if needed when the event(s) come in from LDK. Still feel free to postpone for now.

[ben-kaufman]

Actually had a simpler solution, fixed in: #247

[pwltr]

tACK with follow-up task here

Also note @piotr-iohk's request to merge this together with the Android counterpart.

[claude]

PR Review: Integrate LDK onchain events

This is an excellent architectural improvement that modernizes the onchain transaction handling by leveraging LDK's event-driven architecture. The change from polling to event-driven updates should significantly improve performance and user experience.

---

Summary of Changes

Major Improvements:

• Event-driven onchain transaction handling via LDK Node events
• Removed AddressChecker utility (110 lines removed)
• Enhanced RBF transaction tracking and handling
• Added comprehensive onchain event handling (received, confirmed, replaced, reorged, evicted)
• Improved user notifications for transaction state changes
• Better handling of edge cases (reorgs, mempool evictions)

Impact:

• 1,031 additions, 495 deletions across 20 files
• Core architectural change in CoreService.swift (+529/-273)
• Enhanced event handling in AppViewModel.swift (+108/-4)
• Improved LightningService.swift integration (+120/-3)

---

Code Quality & Best Practices

Strengths

1. Event-Driven Architecture - Proper use of LDK onchain events eliminates polling overhead
2. Comprehensive Error Handling - Good use of try-catch blocks with descriptive logging
3. Service Queue Pattern - Correctly uses ServiceQueue.background(.core) for Core operations
4. MainActor Compliance - Proper async/await patterns with @mainactor for UI updates
5. Caching Strategy - cachedTxIdsInBoostTxIds improves performance for boost transaction lookups
6. Detailed Logging - Excellent context-aware logging throughout
7. Accessibility - Added accessibilityIdentifier to Toast model and components

Code Organization

The new event handlers follow a clean pattern:

• handleOnchainTransactionReceived
• handleOnchainTransactionConfirmed
• handleOnchainTransactionReplaced
• handleOnchainTransactionReorged
• handleOnchainTransactionEvicted

Each handler properly delegates to ServiceQueue.background(.core) and updates the activity list appropriately.

---

Potential Bugs & Issues

1. Race Condition in handleOnchainTransactionReplaced

Location: CoreService.swift:346-433

Issue: When processing replacement transactions, there is a potential race condition between checking if replacementActivity exists and processing the payment. Another event handler could process the same transaction concurrently.

Recommendation: Add a lock or transaction ID tracking to prevent concurrent processing of the same transaction.

2. Cache Invalidation Issue

Location: CoreService.swift:34-62

Issue: The cachedTxIdsInBoostTxIds cache is updated in insert and bulk refreshed in upsertList, but individual update operations don't update the cache. This could lead to stale cache data.

Recommendation: Add cache update to the update method to keep cache consistent.

3. Potential Memory Leak in Event Handlers

Location: AppScene.swift:213-215

Issue: The event handler closure captures app weakly, but the closure itself is never removed when the scene is dismissed.

Recommendation: Ensure event handlers are removed when the scene is dismissed. The addOnEvent API should support removal, or use a cleanup mechanism in deinit or onDisappear.

4. Missing nil-check in processOnchainTransaction

Location: CoreService.swift:315-332

Issue: The function logs a warning if payment is not found but doesn't throw an error. The calling code may not handle this gracefully.

Recommendation: Consider throwing a specific error so callers can decide how to handle missing payments, or add metrics to track how often this occurs.

---

Performance Considerations

Strengths

1. Eliminated Polling - Removing AddressChecker and full syncs significantly reduces CPU/network overhead
2. Efficient Caching - cachedTxIdsInBoostTxIds prevents repeated database queries
3. Lazy Cache Initialization - Cache is only built when first accessed

Concerns

1. Cache Refresh Performance - CoreService.swift:47-62 loads ALL onchain activities from the database, which could be slow for users with large transaction histories (1000+ transactions). Consider a more efficient query that only fetches boostTxIds without loading full activity objects.

2. Delay in Received Sheet - AppViewModel.swift:399 has a 500ms delay. Is this necessary? It could make the UI feel sluggish. Consider removing or reducing if not strictly required.

---

Security Concerns

Good Practices

1. Proper use of ServiceQueue prevents concurrent access to Core/LDK
2. Transactions marked as doesExist = false when replaced (prevents double-counting)
3. RBF detection prevents showing duplicate received sheets

Recommendations

1. Transaction Validation - Consider adding validation that conflicts array in handleOnchainTransactionReplaced actually contains valid transaction IDs to prevent malformed data corruption.

2. Timestamp Validation - In processOnchainPayment, ensure timestamps are within reasonable ranges to prevent time-based attacks.

---

Test Coverage

Missing Test Scenarios

1. RBF Handling - Need tests for single replacement, multiple conflicts, chain of replacements, replacement arriving before original

2. Reorg Handling - Need tests for confirmed tx becoming unconfirmed, multiple txs affected by reorg, reorg depth > 1 block

3. Edge Cases - Need tests for tx evicted from mempool that was not replaced, race conditions, cache invalidation scenarios

Testing Recommendations

Add integration tests that simulate these event sequences:

• onchainTransactionReceived → onchainTransactionReplaced → onchainTransactionConfirmed
• onchainTransactionConfirmed → onchainTransactionReorged
• onchainTransactionReceived → onchainTransactionEvicted

---

Final Recommendations

Must Fix (Before Merge)

1. Run swiftformat . to ensure code formatting compliance
2. Fix cache invalidation in update method
3. Add race condition protection in handleOnchainTransactionReplaced
4. Verify event handler cleanup to prevent memory leaks

Should Fix (High Priority)

1. Optimize refreshBoostTxIdsCache for large transaction histories
2. Add comprehensive tests for RBF and reorg scenarios
3. Review the 500ms delay in received sheet display

Nice to Have

1. Add transaction ID truncation to user-facing messages
2. Improve user-friendly language for technical events (reorg, eviction)
3. Add metrics/telemetry for tracking event frequencies

---

Conclusion

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design pattern and will eliminate polling overhead. The code quality is generally excellent with proper error handling, logging, and async patterns.

Recommendation:Approve with minor changes. Address the cache invalidation and race condition issues before merge, and plan follow-up work for comprehensive testing and performance optimization.

Great work! The videos demonstrating reorg and eviction handling are particularly impressive.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time I try it

[claude]

PR Review: Integrate LDK onchain events

Summary

This PR successfully replaces polling-based transaction monitoring with event-driven LDK onchain events, removes the AddressChecker, and improves RBF transaction handling. This is a significant architectural improvement that makes the app more reactive and efficient.

✅ Strengths

Architecture & Design

1. Event-driven architecture: The transition from polling to LDK events (onchainTransactionReceived, onchainTransactionConfirmed, onchainTransactionReplaced, onchainTransactionReorged, onchainTransactionEvicted) is excellent. This provides real-time updates and reduces unnecessary syncs.

2. Comprehensive RBF handling: The implementation properly tracks replacement transactions through boostTxIds and distinguishes between RBF (replaced transactions with doesExist=false) and CPFP (child transactions with doesExist=true).

3. Clean separation of concerns: Event handlers in LightningService.swift (708-757) delegate to ActivityService methods, maintaining good separation between Lightning layer and activity management.

4. Improved user feedback: New toast notifications with accessibility identifiers for transaction states (replaced, reorged, evicted) provide better UX.

5. Removal of polling: Removing the onChange(of: wallet.totalBalanceSats) trigger in AppScene.swift:217 and the event-based sync in AppScene.swift:217-220 eliminates redundant activity syncing.

Code Quality

1. Good error handling: All event handlers wrap operations in try-catch blocks with proper logging.
2. Proper concurrency: Uses ServiceQueue.background(.core) for all database operations.
3. Cache optimization: The cachedTxIdsInBoostTxIds cache improves performance for checking replaced transactions.

⚠️ Issues & Concerns

Critical

1. Potential cache inconsistency in CoreService.swift:36-43

func getTxIdsInBoostTxIds()async->Set<String>{if cachedTxIdsInBoostTxIds.isEmpty {awaitrefreshBoostTxIdsCache()}return cachedTxIdsInBoostTxIds
}

Issue: The cache is only refreshed when empty, but individual updates via updateBoostTxIdsCache(for:) might not be called for all updates. The update() method at line 195 doesn't update the cache.

Recommendation:

• Update the cache in the update() method, or
• Implement a TTL-based cache invalidation strategy, or
• Document why the cache only needs to be refreshed when empty

2. Race condition in RBF handling (CoreService.swift:355-441)

When handleOnchainTransactionReplaced is called, it:

1. Marks the replaced transaction as doesExist=false
2. Processes replacement transactions that might not exist yet in the payment store
3. Waits for them via processOnchainPayment

Issue: There's a timing window where a replacement transaction event arrives before LDK updates the payment store. The code handles this (line 385-403) by processing the payment, but there's a potential race if the transaction hasn't been added to the payments list yet.

Recommendation: Consider adding retry logic or ensuring LDK guarantees payment store updates before emitting replacement events.

Moderate

3. Missing cache update in update() method (CoreService.swift:193-199)

func update(id:String, activity:Activity)asyncthrows{tryawaitServiceQueue.background(.core){tryupdateActivity(activityId: id, activity: activity)self.metadataChangedSubject.send()}}

This doesn't call updateBoostTxIdsCache(for: activity) like insert() does at line 172.

Recommendation: Add cache update to maintain consistency.

4. Incomplete validation in shouldShowReceivedSheet (CoreService.swift:76-107)

The method checks if a replacement transaction has the same value as the replaced transaction to avoid showing duplicate sheets. However:

• It only checks exact value matches
• Doesn't account for fee differences in RBF
• Could show sheets for CPFP where the child transaction might have a different value

Recommendation: Consider checking isBoosted flag and the nature of the boost (RBF vs CPFP) for more precise logic.

5. Error handling in handleOnchainTransactionReplaced (CoreService.swift:355-441)

When processing replacement transactions, errors are caught and logged (line 401-405), but the loop continues. If multiple replacement transactions fail to process, the relationship tracking could be incomplete.

Recommendation: Consider collecting errors and either throwing or returning them to the caller for better error visibility.

6. CI workflow changes remove caching (integration-tests.yml, unit-tests.yml)

The PR removes DerivedData caching and restore-keys from SPM cache. While this ensures clean builds, it will increase build times significantly.

Recommendation:

• Document why caching was removed (likely due to cache invalidation issues)
• Consider using Xcode Cloud or other CI optimizations to offset the performance loss
• The -onlyUsePackageVersionsFromResolvedFile flag is good for reproducibility

Minor

7. Potential N+1 query in getBoostTxDoesExist (CoreService.swift:130-138)

func getBoostTxDoesExist(boostTxIds:[String])async->[String:Bool]{vardoesExistMap:[String:Bool]=[:]forboostTxIdin boostTxIds {
if let boostActivity =try?awaitgetOnchainActivityByTxId(txid: boostTxId){doesExistMap[boostTxId]= boostActivity.doesExist
}}return doesExistMap
}

Issue: Makes individual database queries for each boost transaction.

Recommendation: Consider adding a batch query method to BitkitCore to fetch multiple activities by txId in one query.

8. Magic number delay in AppViewModel.swift:399

try?awaitTask.sleep(nanoseconds:500_000_000) // 500ms delay

Recommendation: Extract to a named constant with documentation explaining why the delay is necessary.

9. Missing test coverage

While TxBumpingTests.swift and ActivityListTest.swift exist, there don't appear to be specific tests for:

• Onchain event handlers
• RBF replacement tracking with boostTxIds
• Reorg and eviction scenarios
• Cache consistency

Recommendation: Add integration tests for the new event handlers.

🔒 Security

No major security concerns identified. The code properly:

• Uses ServiceQueue for thread safety
• Doesn't expose sensitive transaction data in logs beyond txids
• Maintains Bitcoin/Lightning operation isolation in the service layer

Minor: Logging in LightningService.swift:711-757 includes transaction amounts. Consider whether this is acceptable for production logging.

🚀 Performance

Positive impacts:

1. Eliminates redundant polling and full activity list syncs
2. Event-driven updates only process changed transactions
3. Cache for boostTxIds lookups

Potential concerns:

1. Cache not updated consistently (see issue Project setup #1, Migrate channels from LDK to LDK-node #3)
2. N+1 queries in getBoostTxDoesExist (see issue feat: keychain #7)

🧪 Testing

Existing test coverage:

• ActivityListTest.swift: Good coverage of basic activity CRUD operations
• TxBumpingTests.swift: Tests RBF scenarios

Missing:

• Tests for new onchain event handlers
• Tests for handleOnchainTransactionReplaced, handleOnchainTransactionReorged, handleOnchainTransactionEvicted
• Tests for cache consistency
• Integration tests for the complete event flow

Recommendation: Add integration tests that simulate the event sequences shown in the PR videos.

📝 Recommendations

Must Fix Before Merge

1. Fix cache consistency issues (Issues Project setup #1, Migrate channels from LDK to LDK-node #3)
2. Add error handling for race conditions in RBF handling (Issue Push service setup and service queues  #2)

Should Fix Before Merge

1. Improve shouldShowReceivedSheet validation logic (Issue Create swift.yml #4)
2. Better error aggregation in handleOnchainTransactionReplaced (Issue Create ios.yml #5)

Consider for Follow-up

1. Add batch query support for getBoostTxDoesExist (Issue feat: keychain #7)
2. Add integration tests for event handlers (Issue Use SwiftFormat #9)
3. Extract magic constants (Issue BDK 1.0.0-alpha.11 #8)
4. Document CI caching removal reasoning (Issue Create ios.yml #6)

🎯 Overall Assessment

This is a high-quality PR that represents a significant architectural improvement. The event-driven approach is the correct design, and the RBF handling logic is well thought out. The main concerns are around cache consistency and potential race conditions in RBF processing, which should be addressed before merge.

The removal of AddressChecker and polling-based syncing will improve app performance and responsiveness.

Recommendation: Request changes to address cache consistency and race condition handling, then approve after fixes.

---

Great work on this PR! The demos showing reorg and mempool eviction handling are particularly impressive. Once the cache and race condition issues are resolved, this will be a solid improvement to the codebase.

[ovitrif]

tAck

now it really shows the boosted TX update instantly every time

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.

[ovitrif]

Android PR should succeed e2e once Electrum on staging is fixed. Merging this.