Skip to content

Fix debug panic in full_stack fuzz test #3602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 14, 2025
Merged

Fix debug panic in full_stack fuzz test #3602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e93aa01
Fix debug panic in full_stack fuzz test
jkczyz Feb 13, 2025
9164f7e
Add debug assertion in Channel::funding_signed
jkczyz Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 3 additions & 23 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1392,13 +1392,16 @@ impl<SP: Deref> Channel<SP> 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)
},
Expand All @@ -1412,29 +1415,6 @@ impl<SP: Deref> Channel<SP> 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<L: Deref>(
&mut self, signing_session: InteractiveTxSigningSession, logger: &L
) -> Result<(msgs::CommitmentSigned, Option<Event>), ChannelError>
Expand Down
16 changes: 7 additions & 9 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

@TheBlueMatt TheBlueMatt Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make any sense to me. Going by the comment, try_channel_entry will, when it hits a ChannelError::Close, call Channel::force_shutdown, which will create a ShutdownResult (ultimately stored as shutdown_finish) with a ChannelMonitorUpdate (since !self.channel_state.is_pre_funded_state()) but when we try to apply it we'll fail (as we never added the original monitor).

Copy link
Contributor

@wpaulino wpaulino Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I think this was also the case prior to d4bd56f. Wouldn't the monitor update be applied to the original monitor (since we found a duplicate, the original monitor must still be around)?

Copy link
Contributor

@wpaulino wpaulino Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this actually shouldn't be reachable because the monitor update isn't generated without a funding_txo, which is unset in unset_funding_info

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, hmm, maybe the !self.channel_state.is_pre_funded_state check didn't pass then (and doesn't pass now) because initial_commitment_signed properly only advances the state after checks...so I guess its right, but it certainly is confusing.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is quite annoying that we have the channel_state which can be out of whack with the ChannelPhase...can we add an assertion in funding_signed that the channel_state matches the phase we decide to go with after we call the inner funding_signed?

Copy link
Contributor Author

@jkczyz jkczyz Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, pushed a change adding the assertion. Let me know if I misunderstood what you meant.

// 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))
Expand Down
Loading