Fix debug panic in full_stack fuzz test
#3602
Conversation
d4bd56f 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.
| }) | ||
| { | ||
| 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 |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Alright, pushed a change adding the assertion. Let me know if I misunderstood what you meant.
|
Hmm, let's talk separately about adding files to |
|
Otherwise LGTM |
Whoops, accidentally added that but thought I had removed it. Will push a fix shortly. |
jkczyz
force-pushed
the
2025-02-unset-funding-info
branch
from
549be04 to
5382fc7
Compare
There was a problem hiding this comment.
Gonna go ahead and land, only changes since @wpaulino's ACK were some new debug assertions.
|
Oh nvm, the build fails in |
This is a sanity check that ChannelPhase and ChannelState do not go out of sync.
jkczyz
force-pushed
the
2025-02-unset-funding-info
branch
from
5382fc7 to
9164f7e
Compare
There was a problem hiding this comment.
Okay, now gonna land it since the only changes since @wpaulino's ack were debug assertions.
d4bd56f changed the logic for calling
unset_funding_infosuch that it may be called on a channel that was already inChannelPhase::Fundedwhen handlingfunding_signed. This caused a debug panic in thefull_stackfuzz test when callingFundedChannel::unset_funding_info. Fix this by only callingunset_funding_infoonwatch_channelerror, 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.Fixes #3597