Skip to content

Conversation

@pwltr
Copy link
Contributor

@pwltr pwltr commented Oct 22, 2025

Fix: Critical Channel Monitor Persistence Bug

🚨 Critical Issue Fixed

This PR resolves a critical bug that could lead to unrecoverable fund loss in multiple failure scenarios involving remote backup operations.

🐛 Problem

  • Channel monitor persistence was conditionally dependent on remote backup success
  • If remote backup failed (any reason), local persistence was completely skipped
  • Multiple failure scenarios could cause fund loss:
    • App killed during backup retries
    • No network connection (permanent backup failure)
    • Network issues during backup
    • Backup server down
  • When channel monitor was lost, channels would force-close with "unknown channel" errors
  • Result: Permanent loss of channel state and unrecoverable funds

✅ Solution

  • Always persist channel monitors locally first (before any remote backup attempts)
  • Remote backup failures no longer affect local persistence
  • Fixed threading issues in chainMonitor.channelMonitorUpdated() calls
  • Channel monitors now survive all network failure scenarios

🔧 Technical Changes

  • Move local file write before remote backup queue
  • Fix threading issues in chainMonitor.channelMonitorUpdated() calls
  • Return ChannelMonitorUpdateStatus.Completed immediately after local persistence
  • Bump version

🧪 Testing

  • ✅ Reproduced original bug with simulated remote backup failures
  • ✅ Verified channel monitors persist despite remote backup failures
  • ✅ Confirmed channels remain recoverable after app restart
  • ✅ No threading crashes or force-close errors

@pwltr
Copy link
Contributor Author

pwltr commented Oct 22, 2025

This is the PR that initially introduced the bug: #204

@jvsena42 jvsena42 requested a review from Copilot October 22, 2025 15:16
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a critical bug where channel monitor persistence was conditional on remote backup success, potentially causing unrecoverable fund loss. The fix ensures channel monitors are always persisted locally first, regardless of remote backup status.

Key Changes:

  • Local channel monitor persistence now occurs before remote backup attempts
  • Remote backup failures no longer block or skip local persistence
  • Threading issues in chainMonitor.channelMonitorUpdated() calls are fixed by dispatching to main/UI thread

Reviewed Changes

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

File Description
lib/package.json Version bump from 0.0.160 to 0.0.161
lib/ios/Classes/LdkPersist.swift Reordered persistence to write locally first, moved remote backup to async callback, fixed threading for chain monitor update
lib/android/src/main/java/com/reactnativeldk/classes/LdkPersister.kt Reordered persistence to write locally first, moved remote backup to async callback, fixed threading for chain monitor update

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, didn't test but the changeset reads easy, and I trust the author :D

@coreyphillips
Copy link
Collaborator

As the copilot comment mentioned, is there any reason we shouldn't re-add the new channel event emitters beneath the local writes?

Android:

file.writeBytes(serialized)
if (isNew) {
      LdkEventEmitter.send(EventTypes.new_channel, body)
}

iOS:

try serialized.write(to: channelStoragePath)
if isNew {
      LdkEventEmitter.shared.send(withEvent: .new_channel, body: body)
}

@jvsena42 jvsena42 requested a review from ben-kaufman October 27, 2025 11:33
@ben-kaufman
Copy link

@coreyphillips I'm not so familiar with the code so maybe I'm wrong but I think it does call the new channel event since it no longer returns right after saving locally, so new channel event is now emitted in line 38 in Android and line 46 iOS. So that seems like a false positive issue from copilot.

Copy link

@ben-kaufman ben-kaufman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@ovitrif ovitrif merged commit bae91a6 into master Oct 27, 2025
1 of 6 checks passed
@ovitrif
Copy link
Contributor

ovitrif commented Oct 27, 2025

@pwltr Will you add a PR to integrate this fix into Bitkit? Thx 🙏🏻 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants