From e93aa019eff7a0d7545a622de091c49f55bf19f5 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Thu, 13 Feb 2025 11:03:28 -0600 Subject: [PATCH 1/2] Fix debug panic in full_stack fuzz test d4bd56fc41e8714574407ffcd064be21cb42e539 changed the logic for calling unset_funding_info such that it may be called on a channel that was already in ChannelPhase::Funded when handling funding_signed. This caused a debug panic in the full_stack fuzz test when calling FundedChannel::unset_funding_info. Fix this by only calling unset_funding_info on watch_channel error, as was previously the case. This also reverts moving the channel back into ChannelPhase::UnfundedOutboundV1, which should be fine since the channel is about to be removed. --- lightning/src/ln/channel.rs | 23 ----------------------- lightning/src/ln/channelmanager.rs | 16 +++++++--------- 2 files changed, 7 insertions(+), 32 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index e76cc0deb1d..a779ce7481d 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1412,29 +1412,6 @@ impl Channel where result.map(|monitor| (self.as_funded_mut().expect("Channel should be funded"), monitor)) } - pub fn unset_funding_info(&mut self) { - let phase = core::mem::replace(&mut self.phase, ChannelPhase::Undefined); - if let ChannelPhase::Funded(mut funded_chan) = phase { - funded_chan.unset_funding_info(); - - let context = funded_chan.context; - let unfunded_context = UnfundedChannelContext { - unfunded_channel_age_ticks: 0, - holder_commitment_point: HolderCommitmentPoint::new(&context.holder_signer, &context.secp_ctx), - }; - let unfunded_chan = OutboundV1Channel { - context, - unfunded_context, - signer_pending_open_channel: false, - }; - self.phase = ChannelPhase::UnfundedOutboundV1(unfunded_chan); - } else { - self.phase = phase; - }; - - debug_assert!(!matches!(self.phase, ChannelPhase::Undefined)); - } - pub fn funding_tx_constructed( &mut self, signing_session: InteractiveTxSigningSession, logger: &L ) -> Result<(msgs::CommitmentSigned, Option), ChannelError> diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index eb5bd89575a..04b57fb68f7 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -8241,24 +8241,22 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ .and_then(|(funded_chan, monitor)| { self.chain_monitor .watch_channel(funded_chan.context.channel_id(), monitor) - .map(|persist_status| (funded_chan, persist_status)) .map_err(|()| { + // We weren't able to watch the channel to begin with, so no + // updates should be made on it. Previously, full_stack_target + // found an (unreachable) panic when the monitor update contained + // within `shutdown_finish` was applied. + funded_chan.unset_funding_info(); ChannelError::close("Channel ID was a duplicate".to_owned()) }) + .map(|persist_status| (funded_chan, persist_status)) }) { Ok((funded_chan, persist_status)) => { handle_new_monitor_update!(self, persist_status, peer_state_lock, peer_state, per_peer_state, funded_chan, INITIAL_MONITOR); Ok(()) }, - Err(e) => { - // We weren't able to watch the channel to begin with, so no - // updates should be made on it. Previously, full_stack_target - // found an (unreachable) panic when the monitor update contained - // within `shutdown_finish` was applied. - chan.unset_funding_info(); - try_channel_entry!(self, peer_state, Err(e), chan_entry) - }, + Err(e) => try_channel_entry!(self, peer_state, Err(e), chan_entry), } }, hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id)) From 9164f7e13f74fc60484c4bac33d5d9ae2785ff31 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Thu, 13 Feb 2025 18:04:56 -0600 Subject: [PATCH 2/2] Add debug assertion in Channel::funding_signed This is a sanity check that ChannelPhase and ChannelState do not go out of sync. --- lightning/src/ln/channel.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index a779ce7481d..29ebb5be36c 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1392,13 +1392,16 @@ impl Channel where { let phase = core::mem::replace(&mut self.phase, ChannelPhase::Undefined); let result = if let ChannelPhase::UnfundedOutboundV1(chan) = phase { + let channel_state = chan.context.channel_state; let logger = WithChannelContext::from(logger, &chan.context, None); match chan.funding_signed(msg, best_block, signer_provider, &&logger) { Ok((chan, monitor)) => { + debug_assert!(matches!(chan.context.channel_state, ChannelState::AwaitingChannelReady(_))); self.phase = ChannelPhase::Funded(chan); Ok(monitor) }, Err((chan, e)) => { + debug_assert_eq!(chan.context.channel_state, channel_state); self.phase = ChannelPhase::UnfundedOutboundV1(chan); Err(e) },