← Watch feed
Drop lockorder comments on `ChannelManager`
Public commit record
78/100 · Adequate
What the developer wrote
Authored by Matt Corallo
Drop lockorder comments on `ChannelManager`
While its nice to document things, the lockorder comment at the top
of `ChannelManager` is just annoying to always update and doesn't
add all that much value. Developers likely shouldn't be checking it
while writing code, our automated lockorder issue detection
framework more than suffices to catch any bugs in test-reachable
code. That makes it basically write-only which isn't exactly a
useful comment.
✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Mentions testing or verification
What changed, and why it matters
This commit only removes documentation comments describing the order in which internal locks should be acquired in the ChannelManager. No code logic, lock usage, or behavior changed. It is a cleanup of stale developer comments, not a security fix or vulnerability.
Recommended action
No security action needed. Treat as routine documentation cleanup.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The patch deletes a 40-line lock-order comment tree and several per-field ‘See ChannelManager struct-level documentation for lock order requirements’ doc lines from lightning/src/ln/channelmanager.rs. The commit message explicitly states the comments were high-maintenance, write-only, and redundant because automated lockorder detection is used instead. No struct definitions, types, visibility, or lock acquisition code were modified.
Changed components
lightning/src/ln/channelmanager.rs documentation onlyInspect captured patch +0 / −62
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 10c7750..95442ea 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -2629,46 +2629,6 @@ where
/// [`update_channel`]: chain::Watch::update_channel
/// [`ChannelUpdate`]: msgs::ChannelUpdate
/// [`read`]: ReadableArgs::read
-//
-// Lock order:
-// The tree structure below illustrates the lock order requirements for the different locks of the
-// `ChannelManager`. Locks can be held at the same time if they are on the same branch in the tree,
-// and should then be taken in the order of the lowest to the highest level in the tree.
-// Note that locks on different branches shall not be taken at the same time, as doing so will
-// create a new lock order for those specific locks in the order they were taken.
-//
-// Lock order tree:
-//
-// `pending_offers_messages`
-//
-// `pending_async_payments_messages`
-//
-// `total_consistency_lock`
-// |
-// |__`forward_htlcs`
-// |
-// |__`pending_intercepted_htlcs`
-// |
-// |__`decode_update_add_htlcs`
-// |
-// |__`per_peer_state`
-// |
-// |__`claimable_payments`
-// |
-// |__`pending_outbound_payments` // This field's struct contains a map of pending outbounds
-// |
-// |__`peer_state`
-// |
-// |__`short_to_chan_info`
-// |
-// |__`outbound_scid_aliases`
-// |
-// |__`best_block`
-// |
-// |__`pending_events`
-// |
-// |__`pending_background_events`
-//
pub struct ChannelManager<
M: Deref,
T: Deref,
@@ -2702,11 +2662,9 @@ pub struct ChannelManager<
#[cfg(not(test))]
flow: OffersMessageFlow<MR, L>,
- /// See `ChannelManager` struct-level documentation for lock order requirements.
#[cfg(any(test, feature = "_test_utils"))]
pub(super) best_block: RwLock<BestBlock>,
#[cfg(not(any(test, feature = "_test_utils")))]
- /// See `ChannelManager` struct-level documentation for lock order requirements.
best_block: RwLock<BestBlock>,
pub(super) secp_ctx: Secp256k1<secp256k1::All>,
@@ -2720,8 +2678,6 @@ pub struct ChannelManager<
/// after reloading from disk while replaying blocks against ChannelMonitors.
///
/// See `PendingOutboundPayment` documentation for more info.
- ///
- /// See `ChannelManager` struct-level documentation for lock order requirements.
pending_outbound_payments: OutboundPayments<L>,
/// SCID/SCID Alias -> forward infos. Key of 0 means payments received.
@@ -2732,8 +2688,6 @@ pub struct ChannelManager<
///
/// Note that no consistency guarantees are made about the existence of a channel with the
/// `short_channel_id` here, nor the `short_channel_id` in the `PendingHTLCInfo`!
- ///
- /// See `ChannelManager` struct-level documentation for lock order requirements.
#[cfg(test)]
pub(super) forward_htlcs: Mutex<HashMap<u64, Vec<HTLCForwardInfo>>>,
#[cfg(not(test))]
@@ -2746,8 +2700,6 @@ pub struct ChannelManager<
/// (or timeout)
/// 2. HTLCs that are being held on behalf of an often-offline sender until receipt of a
/// [`ReleaseHeldHtlc`] onion message from an often-offline recipient
- ///
- /// See `ChannelManager` struct-level documentation for lock order requirements.
pending_intercepted_htlcs: Mutex<HashMap<InterceptId, PendingAddHTLCInfo>>,
/// Outbound SCID Alias -> pending `update_add_htlc`s to decode.
@@ -2755,22 +2707,16 @@ pub struct ChannelManager<
///
/// Note that no consistency guarantees are made about the existence of a channel with the
/// `short_channel_id` here, nor the `channel_id` in `UpdateAddHTLC`!
- ///
- /// See `ChannelManager` struct-level documentation for lock order requirements.
decode_update_add_htlcs: Mutex<HashMap<u64, Vec<msgs::UpdateAddHTLC>>>,
/// The sets of payments which are claimable or currently being claimed. See
/// [`ClaimablePayments`]' individual field docs for more info.
- ///
- /// See `ChannelManager` struct-level documentation for lock order requirements.
claimable_payments: Mutex<ClaimablePayments>,
/// The set of outbound SCID aliases across all our channels, including unconfirmed channels
/// and some closed channels which reached a usable state prior to being closed. This is used
/// only to avoid duplicates, and is not persisted explicitly to disk, but rebuilt from the
/// active channel list on load.
- ///
- /// See `ChannelManager` struct-level documentation for lock order requirements.
outbound_scid_aliases: Mutex<HashSet<u64>>,
/// SCIDs (and outbound SCID aliases) -> `counterparty_node_id`s and `channel_id`s.
@@ -2782,8 +2728,6 @@ pub struct ChannelManager<
/// Note that while this holds `counterparty_node_id`s and `channel_id`s, no consistency
/// guarantees are made about the existence of a peer with the `counterparty_node_id` nor a
/// channel with the `channel_id` in our other maps.
- ///
- /// See `ChannelManager` struct-level documentation for lock order requirements.
#[cfg(test)]
pub(super) short_to_chan_info: FairRwLock<HashMap<u64, (PublicKey, ChannelId)>>,
#[cfg(not(test))]
@@ -2824,8 +2768,6 @@ pub struct ChannelManager<
/// channels.
///
/// Note that the same thread must never acquire two inner `PeerState` locks at the same time.
- ///
- /// See `ChannelManager` struct-level documentation for lock order requirements.
#[cfg(not(any(test, feature = "_test_utils")))]
per_peer_state: FairRwLock<HashMap<PublicKey, Mutex<PeerState<SP>>>>,
#[cfg(any(test, feature = "_test_utils"))]
@@ -2846,8 +2788,6 @@ pub struct ChannelManager<
///
/// Note that events MUST NOT be removed from pending_events after deserialization, as they
/// could be in the middle of being processed without the direct mutex held.
- ///
- /// See `ChannelManager` struct-level documentation for lock order requirements.
#[cfg(not(any(test, feature = "_test_utils")))]
pending_events: Mutex<VecDeque<(events::Event, Option<EventCompletionAction>)>>,
#[cfg(any(test, feature = "_test_utils"))]
@@ -2868,8 +2808,6 @@ pub struct ChannelManager<
///
/// Thus, we place them here to be handled as soon as possible once we are running normally.
///
- /// See `ChannelManager` struct-level documentation for lock order requirements.
- ///
/// [`ChainMonitor`]: crate::chain::chainmonitor::ChainMonitor
pending_background_events: Mutex<Vec<BackgroundEvent>>,
/// Used when we have to take a BIG lock to make sure everything is self-consistent.
Risk score
Our methodology →Why this scored 15/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.