LSPS2ServiceHandler API: Avoid explictly dropping locks

We previously added some explict `drop`s to make sure to drop locks before persisting. While clippy warned us about them, they seemed perfectly functional. However, it weirdly seems that `cargo` will run the same static analysis on dependencies, which has LDK Node builds fail with many dreaded 'isn't `Send`' error whenever we touch the async `LSPS2ServiceHandler` API. Here, we simply believe clippy that this is a bad idea and use scoping over explict `drop`s to ensure we don't hold them anymore when persisting.

Author: tnull

lightning-liquidity/src/lsps2/service.rs

@@ -771,16 +771,14 @@ where
 /// [`ChannelManager::create_channel`]: lightning::ln::channelmanager::ChannelManager::create_channel
 /// [`ChannelManager::get_intercept_scid`]: lightning::ln::channelmanager::ChannelManager::get_intercept_scid
 /// [`LSPS2ServiceEvent::BuyRequest`]: crate::lsps2::event::LSPS2ServiceEvent::BuyRequest
-#[allow(clippy::await_holding_lock)]
 pub async fn invoice_parameters_generated(
 &self, counterparty_node_id: &PublicKey, request_id: LSPSRequestId, intercept_scid: u64,
 cltv_expiry_delta: u32, client_trusts_lsp: bool, user_channel_id: u128,
 ) -> Result<(), APIError> {
 let mut message_queue_notifier = self.pending_messages.notifier();
 let mut should_persist = false;
 
-let outer_state_lock = self.per_peer_state.read().unwrap();
-match outer_state_lock.get(counterparty_node_id) {
+match self.per_peer_state.read().unwrap().get(counterparty_node_id) {
 Some(inner_state_lock) => {
 let mut peer_state_lock = inner_state_lock.lock().unwrap();
 
@@ -827,8 +825,6 @@ where
 },
 };
 
-drop(outer_state_lock);
-
 if should_persist {
 self.persist_peer_state(*counterparty_node_id).await.map_err(|e| {
 APIError::APIMisuseError {
@@ -855,16 +851,16 @@ where
 ///
 /// [`Event::HTLCIntercepted`]: lightning::events::Event::HTLCIntercepted
 /// [`LSPS2ServiceEvent::OpenChannel`]: crate::lsps2::event::LSPS2ServiceEvent::OpenChannel
-#[allow(clippy::await_holding_lock)]
 pub async fn htlc_intercepted(
 &self, intercept_scid: u64, intercept_id: InterceptId, expected_outbound_amount_msat: u64,
 payment_hash: PaymentHash,
 ) -> Result<(), APIError> {
 let event_queue_notifier = self.pending_events.notifier();
 let mut should_persist = None;
 
-let peer_by_intercept_scid = self.peer_by_intercept_scid.read().unwrap();
-if let Some(counterparty_node_id) = peer_by_intercept_scid.get(&intercept_scid) {
+if let Some(counterparty_node_id) =
+self.peer_by_intercept_scid.read().unwrap().get(&intercept_scid)
+{
 let outer_state_lock = self.per_peer_state.read().unwrap();
 match outer_state_lock.get(counterparty_node_id) {
 Some(inner_state_lock) => {
@@ -939,8 +935,6 @@ where
 }
 }
 
-drop(peer_by_intercept_scid);
-
 if let Some(counterparty_node_id) = should_persist {
 self.persist_peer_state(counterparty_node_id).await.map_err(|e| {
 APIError::APIMisuseError {
@@ -1131,55 +1125,57 @@ where
 /// open, as it only affects the local LSPS2 state and doesn't affect any channels that
 /// might already exist on-chain. Any pending channel open attempts must be managed
 /// separately.
-#[allow(clippy::await_holding_lock)]
 pub async fn channel_open_abandoned(
 &self, counterparty_node_id: &PublicKey, user_channel_id: u128,
 ) -> Result<(), APIError> {
-let outer_state_lock = self.per_peer_state.read().unwrap();
-let inner_state_lock =
-outer_state_lock.get(counterparty_node_id).ok_or_else(|| APIError::APIMisuseError {
-err: format!("No counterparty state for: {}", counterparty_node_id),
-})?;
-let mut peer_state = inner_state_lock.lock().unwrap();
-
-let intercept_scid = peer_state
-.intercept_scid_by_user_channel_id
-.get(&user_channel_id)
-.copied()
-.ok_or_else(|| APIError::APIMisuseError {
-err: format!("Could not find a channel with user_channel_id {}", user_channel_id),
+{
+let outer_state_lock = self.per_peer_state.read().unwrap();
+let inner_state_lock = outer_state_lock.get(counterparty_node_id).ok_or_else(|| {
+APIError::APIMisuseError {
+err: format!("No counterparty state for: {}", counterparty_node_id),
+}
 })?;
+let mut peer_state = inner_state_lock.lock().unwrap();
 
-let jit_channel = peer_state
-.outbound_channels_by_intercept_scid
-.get(&intercept_scid)
-.ok_or_else(|| APIError::APIMisuseError {
+let intercept_scid = peer_state
+.intercept_scid_by_user_channel_id
+.get(&user_channel_id)
+.copied()
+.ok_or_else(|| APIError::APIMisuseError {
+err: format!(
+"Could not find a channel with user_channel_id {}",
+user_channel_id
+),
+})?;
+
+let jit_channel = peer_state
+.outbound_channels_by_intercept_scid
+.get(&intercept_scid)
+.ok_or_else(|| APIError::APIMisuseError {
 err: format!(
 "Failed to map intercept_scid {} for user_channel_id {} to a channel.",
 intercept_scid, user_channel_id,
 ),
 })?;
 
-let is_pending = matches!(
-jit_channel.state,
-OutboundJITChannelState::PendingInitialPayment { .. }
-| OutboundJITChannelState::PendingChannelOpen { .. }
-);
-
-if !is_pending {
-return Err(APIError::APIMisuseError {
-err: "Cannot abandon channel open after channel creation or payment forwarding"
-.to_string(),
-});
-}
+let is_pending = matches!(
+jit_channel.state,
+OutboundJITChannelState::PendingInitialPayment { .. }
+| OutboundJITChannelState::PendingChannelOpen { .. }
+);
 
-peer_state.intercept_scid_by_user_channel_id.remove(&user_channel_id);
-peer_state.outbound_channels_by_intercept_scid.remove(&intercept_scid);
-peer_state.intercept_scid_by_channel_id.retain(|_, &mut scid| scid != intercept_scid);
-peer_state.needs_persist |= true;
+if !is_pending {
+return Err(APIError::APIMisuseError {
+err: "Cannot abandon channel open after channel creation or payment forwarding"
+.to_string(),
+});
+}
 
-drop(peer_state);
-drop(outer_state_lock);
+peer_state.intercept_scid_by_user_channel_id.remove(&user_channel_id);
+peer_state.outbound_channels_by_intercept_scid.remove(&intercept_scid);
+peer_state.intercept_scid_by_channel_id.retain(|_, &mut scid| scid != intercept_scid);
+peer_state.needs_persist |= true;
+}
 
 self.persist_peer_state(*counterparty_node_id).await.map_err(|e| {
 APIError::APIMisuseError {
@@ -1197,62 +1193,63 @@ where
 /// state so that the payer may try the payment again.
 ///
 /// [`LSPS2ServiceEvent::OpenChannel`]: crate::lsps2::event::LSPS2ServiceEvent::OpenChannel
-#[allow(clippy::await_holding_lock)]
 pub async fn channel_open_failed(
 &self, counterparty_node_id: &PublicKey, user_channel_id: u128,
 ) -> Result<(), APIError> {
-let outer_state_lock = self.per_peer_state.read().unwrap();
+{
+let outer_state_lock = self.per_peer_state.read().unwrap();
 
-let inner_state_lock =
-outer_state_lock.get(counterparty_node_id).ok_or_else(|| APIError::APIMisuseError {
-err: format!("No counterparty state for: {}", counterparty_node_id),
+let inner_state_lock = outer_state_lock.get(counterparty_node_id).ok_or_else(|| {
+APIError::APIMisuseError {
+err: format!("No counterparty state for: {}", counterparty_node_id),
+}
 })?;
 
-let mut peer_state = inner_state_lock.lock().unwrap();
+let mut peer_state = inner_state_lock.lock().unwrap();
 
-let intercept_scid = peer_state
-.intercept_scid_by_user_channel_id
-.get(&user_channel_id)
-.copied()
-.ok_or_else(|| APIError::APIMisuseError {
-err: format!("Could not find a channel with user_channel_id {}", user_channel_id),
-})?;
+let intercept_scid = peer_state
+.intercept_scid_by_user_channel_id
+.get(&user_channel_id)
+.copied()
+.ok_or_else(|| APIError::APIMisuseError {
+err: format!(
+"Could not find a channel with user_channel_id {}",
+user_channel_id
+),
+})?;
 
-let jit_channel = peer_state
-.outbound_channels_by_intercept_scid
-.get_mut(&intercept_scid)
-.ok_or_else(|| APIError::APIMisuseError {
-err: format!(
-"Failed to map intercept_scid {} for user_channel_id {} to a channel.",
-intercept_scid, user_channel_id,
-),
-})?;
+let jit_channel = peer_state
+.outbound_channels_by_intercept_scid
+.get_mut(&intercept_scid)
+.ok_or_else(|| APIError::APIMisuseError {
+err: format!(
+"Failed to map intercept_scid {} for user_channel_id {} to a channel.",
+intercept_scid, user_channel_id,
+),
+})?;
+
+if let OutboundJITChannelState::PendingChannelOpen { payment_queue, .. } =
+&mut jit_channel.state
+{
+let intercepted_htlcs = payment_queue.clear();
+for htlc in intercepted_htlcs {
+self.channel_manager.get_cm().fail_htlc_backwards_with_reason(
+&htlc.payment_hash,
+FailureCode::TemporaryNodeFailure,
+);
+}
 
-if let OutboundJITChannelState::PendingChannelOpen { payment_queue, .. } =
-&mut jit_channel.state
-{
-let intercepted_htlcs = payment_queue.clear();
-for htlc in intercepted_htlcs {
-self.channel_manager.get_cm().fail_htlc_backwards_with_reason(
-&htlc.payment_hash,
-FailureCode::TemporaryNodeFailure,
-);
+jit_channel.state = OutboundJITChannelState::PendingInitialPayment {
+payment_queue: PaymentQueue::new(),
+};
+} else {
+return Err(APIError::APIMisuseError {
+err: "Channel is not in the PendingChannelOpen state.".to_string(),
+});
 }
 
-jit_channel.state = OutboundJITChannelState::PendingInitialPayment {
-payment_queue: PaymentQueue::new(),
-};
-} else {
-return Err(APIError::APIMisuseError {
-err: "Channel is not in the PendingChannelOpen state.".to_string(),
-});
+peer_state.needs_persist |= true;
 }
-
-peer_state.needs_persist |= true;
-
-drop(peer_state);
-drop(outer_state_lock);
-
 self.persist_peer_state(*counterparty_node_id).await.map_err(|e| {
 APIError::APIMisuseError {
 err: format!("Failed to persist peer state for {}: {}", counterparty_node_id, e),
@@ -1268,7 +1265,6 @@ where
 /// we need to forward a payment over otherwise it will be ignored.
 ///
 /// [`Event::ChannelReady`]: lightning::events::Event::ChannelReady
-#[allow(clippy::await_holding_lock)]
 pub async fn channel_ready(
 &self, user_channel_id: u128, channel_id: &ChannelId, counterparty_node_id: &PublicKey,
 ) -> Result<(), APIError> {
@@ -1277,8 +1273,7 @@ where
 let mut peer_by_channel_id = self.peer_by_channel_id.write().unwrap();
 peer_by_channel_id.insert(*channel_id, *counterparty_node_id);
 }
-let outer_state_lock = self.per_peer_state.read().unwrap();
-match outer_state_lock.get(counterparty_node_id) {
+match self.per_peer_state.read().unwrap().get(counterparty_node_id) {
 Some(inner_state_lock) => {
 let mut peer_state = inner_state_lock.lock().unwrap();
 if let Some(intercept_scid) =