fix: persist channel locally in case remote backup fails #294
Conversation
Jasonvdb
left a comment
Jasonvdb
left a comment
There was a problem hiding this comment.
Logic looks good to me. Shouldn't have to run channelMonitorUpdated or anything on the main/ui thread in swift or kotlin though. Don't think it does any harm but also not needed.
| if isNew { | ||
| try serialized.write(to: channelStoragePath) | ||
|
|
||
| // Notify chain monitor on main thread |
There was a problem hiding this comment.
Shouldn't need to call channelMonitorUpdated on main thread. Not sure it matters if you do but it shouldn't be required.
There was a problem hiding this comment.
If main thread is needed then do the same below for the other call to channelMonitorUpdated
There was a problem hiding this comment.
The threading wrapper is indeed necessary, after another test. It only coincidentally works in the persistQueue because of the timing not conflicting with LDK's read-write lock, I guess. cc @ovitrif
Short write-up by the agent:
The channelMonitorUpdated() method must be called on the main thread due to LDK's internal thread safety requirements.
Crash occurred in std::sys_common::rwlock::MovableRwLock::read when called from a background thread. LDK's ChainMonitor uses internal read-write locks that are not thread-safe when accessed from background threads.
Call Stack:
Thread 13 (crashed):
├── LdkPersister.handleChannel()
├── channelMonitorUpdated()
├── LDK internal ChainMonitor code
└── Read-write lock panic
There was a problem hiding this comment.
I updated the other call to channelMonitorUpdated() to also run on the main thread, as suggested.
ovitrif
left a comment
ovitrif
left a comment
There was a problem hiding this comment.
utACK, looks good with one nit about not having to run the update on main thread, same as Jay's remark ^^
pwltr
force-pushed
the
fix/new-channel-backup
branch
from
d2202da to
0ec14f1
Compare
ovitrif
left a comment
ovitrif
left a comment
There was a problem hiding this comment.
ACK, nice way to answer to our threading remarks: nothing beats testing 🙌🏻 !
Similar to (reverted) #286, but only change the logic (writing locally first) for new channels, to cover push balance channels.
Fix: Critical Channel Monitor Persistence Bug
🚨 Critical Issue Fixed
This PR resolves a critical bug that could lead to unrecoverable fund loss when new channels were created and remote backup failed or the app was killed during backup operations. LDK normally ensures that the channel material is durably persisted before it will ever accept an HTLC on that channel, but it does not do so on the channel open itself. So any balance pushed to us in the channel open is not secured. This PR adds extra logic to handle this case.
🐛 Problem
✅ Solution
🔧 Technical Changes
ChannelMonitorUpdateStatus.Completedimmediately for new channels (after local write)ChannelMonitorUpdateStatus.InProgressfor updates (wait for remote backup)