From ff9840ea35d90bb0ceb004be2ebe16a4c62c5ae0 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Viktor=20Tigerstr=C3=B6m?= <11711198+ViktorTigerstrom@users.noreply.github.com> Date: Fri, 9 Dec 2022 00:59:21 +0100 Subject: [PATCH] Avoid unnecessary immediate retake `per_peer_state` lock --- lightning/src/ln/channelmanager.rs | 118 ++++++++++++++--------------- 1 file changed, 55 insertions(+), 63 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 7b2865bb1..b5fa26098 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1515,12 +1515,19 @@ where return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) }); } - let channel = { - let per_peer_state = self.per_peer_state.read().unwrap(); - match per_peer_state.get(&their_network_key) { - Some(peer_state) => { + let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier); + // We want to make sure the lock is actually acquired by PersistenceNotifierGuard. + debug_assert!(&self.total_consistency_lock.try_write().is_err()); + + let mut channel_state = self.channel_state.lock().unwrap(); + let per_peer_state = self.per_peer_state.read().unwrap(); + + match per_peer_state.get(&their_network_key) { + None => return Err(APIError::ChannelUnavailable { err: format!("Not connected to node: {}", their_network_key) }), + Some(peer_state_mutex) => { + let mut peer_state = peer_state_mutex.lock().unwrap(); + let channel = { let outbound_scid_alias = self.create_and_insert_outbound_scid_alias(); - let peer_state = peer_state.lock().unwrap(); let their_features = &peer_state.latest_features; let config = if override_config.is_some() { override_config.as_ref().unwrap() } else { &self.default_configuration }; match Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, @@ -1533,38 +1540,28 @@ where return Err(e); }, } - }, - None => return Err(APIError::ChannelUnavailable { err: format!("Not connected to node: {}", their_network_key) }), - } - }; - let res = channel.get_open_channel(self.genesis_hash.clone()); - - let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier); - // We want to make sure the lock is actually acquired by PersistenceNotifierGuard. - debug_assert!(&self.total_consistency_lock.try_write().is_err()); + }; + let res = channel.get_open_channel(self.genesis_hash.clone()); - let temporary_channel_id = channel.channel_id(); - let mut channel_state = self.channel_state.lock().unwrap(); - let per_peer_state = self.per_peer_state.read().unwrap(); - if let Some(peer_state_mutex) = per_peer_state.get(&their_network_key){ - let mut peer_state_lock = peer_state_mutex.lock().unwrap(); - let peer_state = &mut *peer_state_lock; - match peer_state.channel_by_id.entry(temporary_channel_id) { - hash_map::Entry::Occupied(_) => { - if cfg!(fuzzing) { - return Err(APIError::APIMisuseError { err: "Fuzzy bad RNG".to_owned() }); - } else { - panic!("RNG is bad???"); + let temporary_channel_id = channel.channel_id(); + match peer_state.channel_by_id.entry(temporary_channel_id) { + hash_map::Entry::Occupied(_) => { + if cfg!(fuzzing) { + return Err(APIError::APIMisuseError { err: "Fuzzy bad RNG".to_owned() }); + } else { + panic!("RNG is bad???"); + } + }, + hash_map::Entry::Vacant(entry) => { entry.insert(channel); } } + + channel_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel { + node_id: their_network_key, + msg: res, + }); + Ok(temporary_channel_id) }, - hash_map::Entry::Vacant(entry) => { entry.insert(channel); } - } - } else { return Err(APIError::ChannelUnavailable { err: format!("Not connected to node: {}", their_network_key) }) } - channel_state.pending_msg_events.push(events::MessageSendEvent::SendOpenChannel { - node_id: their_network_key, - msg: res, - }); - Ok(temporary_channel_id) + } } fn list_channels_with_filter::Signer>)) -> bool + Copy>(&self, f: Fn) -> Vec { @@ -2533,12 +2530,13 @@ where fn funding_transaction_generated_intern::Signer>, &Transaction) -> Result>( &self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, funding_transaction: Transaction, find_funding_output: FundingOutput ) -> Result<(), APIError> { - let (chan, msg) = { - let (res, chan) = { - let per_peer_state = self.per_peer_state.read().unwrap(); - if let Some(peer_state_mutex) = per_peer_state.get(counterparty_node_id) { - let mut peer_state_lock = peer_state_mutex.lock().unwrap(); - let peer_state = &mut *peer_state_lock; + let mut channel_state = self.channel_state.lock().unwrap(); + let per_peer_state = self.per_peer_state.read().unwrap(); + if let Some(peer_state_mutex) = per_peer_state.get(counterparty_node_id) { + let mut peer_state_lock = peer_state_mutex.lock().unwrap(); + let peer_state = &mut *peer_state_lock; + let (chan, msg) = { + let (res, chan) = { match peer_state.channel_by_id.remove(temporary_channel_id) { Some(mut chan) => { let funding_txo = find_funding_output(&chan, &funding_transaction)?; @@ -2551,30 +2549,22 @@ where }, None => { return Err(APIError::ChannelUnavailable { err: "No such channel".to_owned() }) }, } - } else { - return Err(APIError::APIMisuseError { err: format!("Can't find a peer with a node_id matching the passed counterparty_node_id {}", counterparty_node_id) }) + }; + match handle_error!(self, res, chan.get_counterparty_node_id()) { + Ok(funding_msg) => { + (chan, funding_msg) + }, + Err(_) => { return Err(APIError::ChannelUnavailable { + err: "Error deriving keys or signing initial commitment transactions - either our RNG or our counterparty's RNG is broken or the Signer refused to sign".to_owned() + }) }, } }; - match handle_error!(self, res, chan.get_counterparty_node_id()) { - Ok(funding_msg) => { - (chan, funding_msg) - }, - Err(_) => { return Err(APIError::ChannelUnavailable { - err: "Error deriving keys or signing initial commitment transactions - either our RNG or our counterparty's RNG is broken or the Signer refused to sign".to_owned() - }) }, - } - }; - let mut channel_state = self.channel_state.lock().unwrap(); - channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingCreated { - node_id: chan.get_counterparty_node_id(), - msg, - }); - mem::drop(channel_state); - let per_peer_state = self.per_peer_state.read().unwrap(); - if let Some(peer_state_mutex) = per_peer_state.get(counterparty_node_id) { - let mut peer_state_lock = peer_state_mutex.lock().unwrap(); - let peer_state = &mut *peer_state_lock; + channel_state.pending_msg_events.push(events::MessageSendEvent::SendFundingCreated { + node_id: chan.get_counterparty_node_id(), + msg, + }); + mem::drop(channel_state); match peer_state.channel_by_id.entry(chan.channel_id()) { hash_map::Entry::Occupied(_) => { panic!("Generated duplicate funding txid?"); @@ -2587,8 +2577,10 @@ where e.insert(chan); } } - } else { return Err(APIError::ChannelUnavailable { err: format!("Peer with counterparty_node_id {} disconnected and closed the channel", counterparty_node_id) }) } - Ok(()) + Ok(()) + } else { + return Err(APIError::APIMisuseError { err: format!("Can't find a peer with a node_id matching the passed counterparty_node_id {}", counterparty_node_id) }) + } } #[cfg(test)] -- 2.39.5