From 9606f94caf451d9eb3a690b559e2955486a3e76a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 25 Mar 2018 15:47:47 -0400 Subject: [PATCH] Stop freeing holding cell in updates, fix freeing in revoke --- src/ln/channel.rs | 33 +++++------------------- src/ln/channelmanager.rs | 55 +++++++++++++++++++--------------------- src/ln/msgs.rs | 8 +++--- src/ln/peer_handler.rs | 45 ++++++++++---------------------- 4 files changed, 49 insertions(+), 92 deletions(-) diff --git a/src/ln/channel.rs b/src/ln/channel.rs index 0af02e9c8..361dd89be 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -1105,23 +1105,7 @@ impl Channel { } } - /// Checks if there are any LocalAnnounced HTLCs remaining and sets - /// ChannelState::AwaitingRemoteRevoke accordingly, possibly calling free_holding_cell_htlcs. - fn check_and_free_holding_cell_htlcs(&mut self) -> Result, msgs::CommitmentSigned)>, HandleError> { - if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) { - for htlc in self.pending_htlcs.iter() { - if htlc.state == HTLCState::LocalAnnounced { - return Ok(None); - } - } - self.channel_state &= !(ChannelState::AwaitingRemoteRevoke as u32); - self.free_holding_cell_htlcs() - } else { - Ok(None) - } - } - - pub fn update_fulfill_htlc(&mut self, msg: &msgs::UpdateFulfillHTLC) -> Result, msgs::CommitmentSigned)>, HandleError> { + pub fn update_fulfill_htlc(&mut self, msg: &msgs::UpdateFulfillHTLC) -> Result<(), HandleError> { if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) { return Err(HandleError{err: "Got add HTLC message when channel was not in an operational state", msg: None}); } @@ -1139,11 +1123,10 @@ impl Channel { self.value_to_self_msat -= htlc.amount_msat; } } - - self.check_and_free_holding_cell_htlcs() + Ok(()) } - pub fn update_fail_htlc(&mut self, msg: &msgs::UpdateFailHTLC) -> Result<([u8; 32], Option<(Vec, msgs::CommitmentSigned)>), HandleError> { + pub fn update_fail_htlc(&mut self, msg: &msgs::UpdateFailHTLC) -> Result<[u8; 32], HandleError> { if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) { return Err(HandleError{err: "Got add HTLC message when channel was not in an operational state", msg: None}); } @@ -1156,12 +1139,10 @@ impl Channel { htlc.payment_hash } }; - - let holding_cell_freedom = self.check_and_free_holding_cell_htlcs()?; - Ok((payment_hash, holding_cell_freedom)) + Ok(payment_hash) } - pub fn update_fail_malformed_htlc(&mut self, msg: &msgs::UpdateFailMalformedHTLC) -> Result<([u8; 32], Option<(Vec, msgs::CommitmentSigned)>), HandleError> { + pub fn update_fail_malformed_htlc(&mut self, msg: &msgs::UpdateFailMalformedHTLC) -> Result<[u8; 32], HandleError> { if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) { return Err(HandleError{err: "Got add HTLC message when channel was not in an operational state", msg: None}); } @@ -1174,9 +1155,7 @@ impl Channel { htlc.payment_hash } }; - - let holding_cell_freedom = self.check_and_free_holding_cell_htlcs()?; - Ok((payment_hash, holding_cell_freedom)) + Ok(payment_hash) } pub fn commitment_signed(&mut self, msg: &msgs::CommitmentSigned) -> Result<(msgs::RevokeAndACK, Vec), HandleError> { diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index e0e9000a2..d7bd9ff9d 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -1212,54 +1212,52 @@ impl ChannelMessageHandler for ChannelManager { Ok(res) } - fn handle_update_fulfill_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFulfillHTLC) -> Result, msgs::CommitmentSigned)>, HandleError> { - let res = { + fn handle_update_fulfill_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFulfillHTLC) -> Result<(), HandleError> { + { let mut channel_state = self.channel_state.lock().unwrap(); match channel_state.by_id.get_mut(&msg.channel_id) { Some(chan) => { if chan.get_their_node_id() != *their_node_id { return Err(HandleError{err: "Got a message for a channel from the wrong node!", msg: None}) } - chan.update_fulfill_htlc(&msg) + chan.update_fulfill_htlc(&msg)?; }, None => return Err(HandleError{err: "Failed to find corresponding channel", msg: None}) } - }; + } //TODO: Delay the claimed_funds relaying just like we do outbound relay! self.claim_funds_internal(msg.payment_preimage.clone(), false); - res + Ok(()) } - fn handle_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFailHTLC) -> Result, msgs::CommitmentSigned)>, HandleError> { + fn handle_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFailHTLC) -> Result<(), HandleError> { let mut channel_state = self.channel_state.lock().unwrap(); - let res; - match channel_state.by_id.get_mut(&msg.channel_id) { + let payment_hash = match channel_state.by_id.get_mut(&msg.channel_id) { Some(chan) => { if chan.get_their_node_id() != *their_node_id { return Err(HandleError{err: "Got a message for a channel from the wrong node!", msg: None}) } - res = chan.update_fail_htlc(&msg)?; + chan.update_fail_htlc(&msg)? }, None => return Err(HandleError{err: "Failed to find corresponding channel", msg: None}) - } - self.fail_htlc_backwards_internal(channel_state, &res.0, HTLCFailReason::ErrorPacket { err: &msg.reason }); - Ok(res.1) + }; + self.fail_htlc_backwards_internal(channel_state, &payment_hash, HTLCFailReason::ErrorPacket { err: &msg.reason }); + Ok(()) } - fn handle_update_fail_malformed_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFailMalformedHTLC) -> Result, msgs::CommitmentSigned)>, HandleError> { + fn handle_update_fail_malformed_htlc(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFailMalformedHTLC) -> Result<(), HandleError> { let mut channel_state = self.channel_state.lock().unwrap(); - let res; - match channel_state.by_id.get_mut(&msg.channel_id) { + let payment_hash = match channel_state.by_id.get_mut(&msg.channel_id) { Some(chan) => { if chan.get_their_node_id() != *their_node_id { return Err(HandleError{err: "Got a message for a channel from the wrong node!", msg: None}) } - res = chan.update_fail_malformed_htlc(&msg)?; + chan.update_fail_malformed_htlc(&msg)? }, None => return Err(HandleError{err: "Failed to find corresponding channel", msg: None}) - } - self.fail_htlc_backwards_internal(channel_state, &res.0, HTLCFailReason::Reason { failure_code: msg.failure_code, data: &[0;0] }); - Ok(res.1) + }; + self.fail_htlc_backwards_internal(channel_state, &payment_hash, HTLCFailReason::Reason { failure_code: msg.failure_code, data: &[0;0] }); + Ok(()) } fn handle_commitment_signed(&self, their_node_id: &PublicKey, msg: &msgs::CommitmentSigned) -> Result { @@ -1310,22 +1308,21 @@ impl ChannelMessageHandler for ChannelManager { Ok(res) } - fn handle_revoke_and_ack(&self, their_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result<(), HandleError> { - let monitor = { + fn handle_revoke_and_ack(&self, their_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result, msgs::CommitmentSigned)>, HandleError> { + let (res, monitor) = { let mut channel_state = self.channel_state.lock().unwrap(); match channel_state.by_id.get_mut(&msg.channel_id) { Some(chan) => { if chan.get_their_node_id() != *their_node_id { return Err(HandleError{err: "Got a message for a channel from the wrong node!", msg: None}) } - chan.revoke_and_ack(&msg)?; - chan.channel_monitor() + (chan.revoke_and_ack(&msg)?, chan.channel_monitor()) }, None => return Err(HandleError{err: "Failed to find corresponding channel", msg: None}) } }; self.monitor.add_update_monitor(monitor.get_funding_txo().unwrap(), monitor)?; - Ok(()) + Ok(res) } fn handle_update_fee(&self, their_node_id: &PublicKey, msg: &msgs::UpdateFee) -> Result<(), HandleError> { @@ -1711,7 +1708,7 @@ mod tests { node.handle_update_add_htlc(&prev_node.get_our_node_id(), &payment_event.msgs[0]).unwrap(); let revoke_and_ack = node.handle_commitment_signed(&prev_node.get_our_node_id(), &payment_event.commitment_msg).unwrap(); - prev_node.handle_revoke_and_ack(&node.get_our_node_id(), &revoke_and_ack).unwrap(); + assert!(prev_node.handle_revoke_and_ack(&node.get_our_node_id(), &revoke_and_ack).unwrap().is_none()); let events_1 = node.get_and_clear_pending_events(); assert_eq!(events_1.len(), 1); @@ -1756,7 +1753,7 @@ mod tests { assert_eq!(expected_next_node, node.get_our_node_id()); match next_msg { Some(msg) => { - assert!(node.handle_update_fulfill_htlc(&prev_node.get_our_node_id(), &msg).unwrap().is_none()); + node.handle_update_fulfill_htlc(&prev_node.get_our_node_id(), &msg).unwrap(); }, None => {} } @@ -1774,7 +1771,7 @@ mod tests { } assert_eq!(expected_next_node, origin_node.get_our_node_id()); - assert!(origin_node.handle_update_fulfill_htlc(&expected_route.first().unwrap().get_our_node_id(), &next_msg.unwrap()).unwrap().is_none()); + origin_node.handle_update_fulfill_htlc(&expected_route.first().unwrap().get_our_node_id(), &next_msg.unwrap()).unwrap(); let events = origin_node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); @@ -1839,7 +1836,7 @@ mod tests { assert_eq!(expected_next_node, node.get_our_node_id()); match next_msg { Some(msg) => { - assert!(node.handle_update_fail_htlc(&prev_node.get_our_node_id(), &msg).unwrap().is_none()); + node.handle_update_fail_htlc(&prev_node.get_our_node_id(), &msg).unwrap(); }, None => {} } @@ -1857,7 +1854,7 @@ mod tests { } assert_eq!(expected_next_node, origin_node.get_our_node_id()); - assert!(origin_node.handle_update_fail_htlc(&expected_route.first().unwrap().get_our_node_id(), &next_msg.unwrap()).unwrap().is_none()); + origin_node.handle_update_fail_htlc(&expected_route.first().unwrap().get_our_node_id(), &next_msg.unwrap()).unwrap(); let events = origin_node.get_and_clear_pending_events(); assert_eq!(events.len(), 1); diff --git a/src/ln/msgs.rs b/src/ln/msgs.rs index af13d4cd0..6bb900967 100644 --- a/src/ln/msgs.rs +++ b/src/ln/msgs.rs @@ -371,11 +371,11 @@ pub trait ChannelMessageHandler : events::EventsProvider { // HTLC handling: fn handle_update_add_htlc(&self, their_node_id: &PublicKey, msg: &UpdateAddHTLC) -> Result<(), HandleError>; - fn handle_update_fulfill_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFulfillHTLC) -> Result, CommitmentSigned)>, HandleError>; - fn handle_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFailHTLC) -> Result, CommitmentSigned)>, HandleError>; - fn handle_update_fail_malformed_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFailMalformedHTLC) -> Result, CommitmentSigned)>, HandleError>; + fn handle_update_fulfill_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFulfillHTLC) -> Result<(), HandleError>; + fn handle_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFailHTLC) -> Result<(), HandleError>; + fn handle_update_fail_malformed_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFailMalformedHTLC) -> Result<(), HandleError>; fn handle_commitment_signed(&self, their_node_id: &PublicKey, msg: &CommitmentSigned) -> Result; - fn handle_revoke_and_ack(&self, their_node_id: &PublicKey, msg: &RevokeAndACK) -> Result<(), HandleError>; + fn handle_revoke_and_ack(&self, their_node_id: &PublicKey, msg: &RevokeAndACK) -> Result, CommitmentSigned)>, HandleError>; fn handle_update_fee(&self, their_node_id: &PublicKey, msg: &UpdateFee) -> Result<(), HandleError>; diff --git a/src/ln/peer_handler.rs b/src/ln/peer_handler.rs index 2b60bbf12..9715ee965 100644 --- a/src/ln/peer_handler.rs +++ b/src/ln/peer_handler.rs @@ -370,42 +370,15 @@ impl PeerManager { }, 130 => { let msg = try_potential_decodeerror!(msgs::UpdateFulfillHTLC::decode(&msg_data[2..])); - let resp_option = try_potential_handleerror!(self.message_handler.chan_handler.handle_update_fulfill_htlc(&peer.their_node_id.unwrap(), &msg)); - match resp_option { - Some(resps) => { - for resp in resps.0 { - encode_and_send_msg!(resp, 128); - } - encode_and_send_msg!(resps.1, 132); - }, - None => {}, - } + try_potential_handleerror!(self.message_handler.chan_handler.handle_update_fulfill_htlc(&peer.their_node_id.unwrap(), &msg)); }, 131 => { let msg = try_potential_decodeerror!(msgs::UpdateFailHTLC::decode(&msg_data[2..])); - let resp_option = try_potential_handleerror!(self.message_handler.chan_handler.handle_update_fail_htlc(&peer.their_node_id.unwrap(), &msg)); - match resp_option { - Some(resps) => { - for resp in resps.0 { - encode_and_send_msg!(resp, 128); - } - encode_and_send_msg!(resps.1, 132); - }, - None => {}, - } + try_potential_handleerror!(self.message_handler.chan_handler.handle_update_fail_htlc(&peer.their_node_id.unwrap(), &msg)); }, 135 => { let msg = try_potential_decodeerror!(msgs::UpdateFailMalformedHTLC::decode(&msg_data[2..])); - let resp_option = try_potential_handleerror!(self.message_handler.chan_handler.handle_update_fail_malformed_htlc(&peer.their_node_id.unwrap(), &msg)); - match resp_option { - Some(resps) => { - for resp in resps.0 { - encode_and_send_msg!(resp, 128); - } - encode_and_send_msg!(resps.1, 132); - }, - None => {}, - } + try_potential_handleerror!(self.message_handler.chan_handler.handle_update_fail_malformed_htlc(&peer.their_node_id.unwrap(), &msg)); }, 132 => { @@ -415,9 +388,17 @@ impl PeerManager { }, 133 => { let msg = try_potential_decodeerror!(msgs::RevokeAndACK::decode(&msg_data[2..])); - try_potential_handleerror!(self.message_handler.chan_handler.handle_revoke_and_ack(&peer.their_node_id.unwrap(), &msg)); + let resp_option = try_potential_handleerror!(self.message_handler.chan_handler.handle_revoke_and_ack(&peer.their_node_id.unwrap(), &msg)); + match resp_option { + Some(resps) => { + for resp in resps.0 { + encode_and_send_msg!(resp, 128); + } + encode_and_send_msg!(resps.1, 132); + }, + None => {}, + } }, - 134 => { let msg = try_potential_decodeerror!(msgs::UpdateFee::decode(&msg_data[2..])); try_potential_handleerror!(self.message_handler.chan_handler.handle_update_fee(&peer.their_node_id.unwrap(), &msg)); -- 2.39.5