Skip to content

Commit 53ff0c1

Browse files
committed
Expect callers to hold read locks before channel_monitor_updated
Our existing lockorder tests assume that a read lock on a thread that is already holding the same read lock is totally fine. This isn't at all true. The `std` `RwLock` behavior is platform-dependent - on most platforms readers can starve writers as readers will never block for a pending writer. However, on platforms where this is not the case, one thread trying to take a write lock may deadlock with another thread that both already has, and is attempting to take again, a read lock. Worse, our in-tree `FairRwLock` exhibits this behavior explicitly on all platforms to avoid the starvation issue. Sadly, a user ended up hitting this deadlock in production in the form of a call to `get_and_clear_pending_msg_events` which holds the `ChannelManager::total_consistency_lock` before calling `process_pending_monitor_events` and eventually `channel_monitor_updated`, which tries to take the same read lock again. Luckily, the fix is trivial, simply remove the redundand read lock in `channel_monitor_updated`. Fixes #2000
1 parent c611d5f commit 53ff0c1

File tree

1 file changed

+3
-1
lines changed

1 file changed

+3
-1
lines changed

lightning/src/ln/channelmanager.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -4101,7 +4101,7 @@ where
41014101
}
41024102

41034103
fn channel_monitor_updated(&self, funding_txo: &OutPoint, highest_applied_update_id: u64, counterparty_node_id: Option<&PublicKey>) {
4104-
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
4104+
debug_assert!(self.total_consistency_lock.try_write().is_err()); // Caller holds read lock
41054105

41064106
let htlc_forwards;
41074107
let (mut pending_failures, finalized_claims, counterparty_node_id) = {
@@ -5068,6 +5068,8 @@ where
50685068

50695069
/// Process pending events from the `chain::Watch`, returning whether any events were processed.
50705070
fn process_pending_monitor_events(&self) -> bool {
5071+
debug_assert!(self.total_consistency_lock.try_write().is_err()); // Caller holds read lock
5072+
50715073
let mut failed_channels = Vec::new();
50725074
let mut pending_monitor_events = self.chain_monitor.release_pending_monitor_events();
50735075
let has_pending_monitor_events = !pending_monitor_events.is_empty();

0 commit comments

Comments
 (0)