Uh oh!
There was an error while loading. Please reload this page.
Splice channel state rework - #4061
Conversation
👋 Thanks for assigning @jkczyz as a reviewer! |
| // FIXME: Either we bring back the `OUR_TX_SIGNATURES_READY` flag for this specific | ||
| // case, or we somehow access the signing session to check it instead. | ||
| ChannelState::FundingNegotiated(flags) => !flags.is_interactive_signing(), |
There was a problem hiding this comment.
The current implementation has a potential logic issue. The FIXME comment correctly identifies that is_interactive_signing() is being used as a replacement for the removed OUR_TX_SIGNATURES_READY flag, but these represent different states:
is_interactive_signing()is true during the entire interactive signing processOUR_TX_SIGNATURES_READYwas specifically true only after our signatures were ready to send
This semantic difference could cause funding transactions to be incorrectly marked as non-broadcastable when they should be broadcastable. Consider either:
- Restoring the
OUR_TX_SIGNATURES_READYflag for this specific use case, or - Accessing the signing session directly to check if holder signatures are ready via something like
signing_session.holder_tx_signatures().is_some()
This would more accurately represent the original intent of the code.
| // FIXME: Either we bring back the `OUR_TX_SIGNATURES_READY` flag for this specific | |
| // case, or we somehow access the signing session to check it instead. | |
| ChannelState::FundingNegotiated(flags) => !flags.is_interactive_signing(), | |
| // We need to check if our signatures are ready to send, which means the transaction | |
| // is ready to be broadcast. | |
| ChannelState::FundingNegotiated(flags) => { | |
| ifletSome(signing_session) = &self.funding_tx_signing_session{ | |
| signing_session.holder_tx_signatures().is_none() | |
| }else{ | |
| !flags.is_interactive_signing() | |
| } | |
| }, |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
ldk-reviews-bot
commented
Sep 8, 2025
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
9272b39 to
23a1c29CompareCodecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #4061 +/- ##
==========================================
- Coverage 88.33% 87.84% -0.49%
==========================================
Files 177 176 -1 Lines 131896 131744 -152 Branches 131896 131744 -152 ==========================================
- Hits 116512 115733 -779 - Misses 12728 13376 +648 + Partials 2656 2635 -21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
23a1c29 to
0cbeb51Compare| debug_assert!( | ||
| false, | ||
| "A signing session must always be present while interactive signing" | ||
| ); | ||
| let err = | ||
| format!("Channel {} not expecting funding signatures", self.context.channel_id); | ||
| return Err(APIError::APIMisuseError { err }); |
There was a problem hiding this comment.
The code contains a pattern where a debug assertion claims a signing session must always be present during interactive signing, but then includes error handling for when it's None. This creates a logical contradiction - either:
- The assertion is correct and the error handling is unnecessary, or
- The error handling is necessary and the assertion is incorrect
This pattern appears multiple times throughout the code. Consider either:
- Removing the error handling if the assertion truly holds
- Adjusting the assertion to reflect the actual conditions where a signing session might be absent
This would make the code's intent clearer and avoid potential confusion about the expected state during interactive signing.
| debug_assert!( | |
| false, | |
| "A signing session must always be present while interactive signing" | |
| ); | |
| let err = | |
| format!("Channel {} not expecting funding signatures",self.context.channel_id); | |
| returnErr(APIError::APIMisuseError{ err }); | |
| // A signing session should typically be present during interactive signing, | |
| // but handle the case where it's not | |
| let err = | |
| format!("Channel {} not expecting funding signatures",self.context.channel_id); | |
| returnErr(APIError::APIMisuseError{ err }); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
ldk-reviews-bot
commented
Sep 11, 2025
🔔 1st Reminder Hey @TheBlueMatt@jkczyz! This PR has been waiting for your review. |
1 similar comment
ldk-reviews-bot
commented
Sep 11, 2025
🔔 1st Reminder Hey @TheBlueMatt@jkczyz! This PR has been waiting for your review. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Will have to finish looking later
Uh oh!
There was an error while loading. Please reload this page.
| pub fn tx_signatures(&mut self, msg: &msgs::TxSignatures) -> Result<(Option<msgs::TxSignatures>, Option<Transaction>), ChannelError> { | ||
| if !self.context.channel_state.is_interactive_signing() | ||
| || self.context.channel_state.is_their_tx_signatures_sent() | ||
| && !self.has_pending_splice_awaiting_signatures() |
There was a problem hiding this comment.
Those are the only two states when we should be processing an incoming tx_signatures.
| // FIXME: Either we bring back the `OUR_TX_SIGNATURES_READY` flag for this specific | ||
| // case, or we somehow access the signing session to check it instead. |
There was a problem hiding this comment.
Should we move this out of ChannelContext then?
There was a problem hiding this comment.
Either that, or we track the session in the context, which lets us get rid of the interactive signing flag. I'm leaning towards the latter.
There was a problem hiding this comment.
or we track the session in the context, which lets us get rid of the interactive signing flag
Pushed an update that ended up doing this.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| self.channel_state, | ||
| ChannelState::ChannelReady(f) if f.is_set(ChannelReadyFlags::QUIESCENT) | ||
| ); | ||
| debug_assert!(self.channel_state.is_interactive_signing() || is_quiescent); |
There was a problem hiding this comment.
For v2-channel establishment, is_quiescent isn't applicable, IIUC. But for splicing, wouldn't checking is_interactive_signing be sufficient?
There was a problem hiding this comment.
The interactive signing flag is only relevant to dual funding due to it being a flag on ChannelState::FundingNegotiated.
0cbeb51 to
8ca96e3Compare
jkczyz
left a comment
There was a problem hiding this comment.
LGTM other some minor fixes
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
8ca96e3 to
e60f8d7Comparee60f8d7 to
78da77bCompareTheBlueMatt
left a comment
There was a problem hiding this comment.
A few additional things, though none of them blocking this PR per se:
is_waiting_on_peer_pending_channel_update/should_disconnect_peer_awaiting_responsemay need updating to give us more time to finish a splice. RN we just get mad because we're in quiescence and its not finishing quickly.- We might still need a splicing flag or something to track which type of quiescent action is happening. It doesn't have to happen now but right now we enter quiescence and then start accepting splicing messages.
- Relatedly, we currently don't reject receiving a second
splice_initmessage from our counterparty after we're already splicing (and similarly accept it if we initiated a splice, afaict). - I'm confused why we still have this in
funding_tx_constructed:// TODO(splicing) Forced error, as the use case is not complete
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
TheBlueMatt
commented
Sep 14, 2025
Oh also |
78da77b to
8a574a5Comparewpaulino
commented
Sep 15, 2025
If the counterparty is still enforcing a stricter timeout then it may not matter what we do? I guess we could reset the timer once we get to the signing phase, or would you rather have it happen at every single step of the splice negotiation (seems overkill IMO)?
We only track the action up until we're quiescent, and at that point we're already sending the first message (if we're the initiator) for the action we want to take. When we're not the initiator, the action isn't relevant or we're not allowed to propose something anyway, so we should just accept whatever the counterparty sends. We will need to ensure once we support other quiescent actions that the counterparty is only sending messages for a specific one.
It's removed in #4054. |
The tests have become a bit painful to maintain, and they don't even fully test the production code paths, so we opt to just remove them for now. In the future, we plan to unify the dual funding code paths with splicing.
Since the `InteractiveTxSigningSession` already tracks everything we need, we can remove the duplicate interactive signing state tracking within `ChannelState`.
This commit addresses an overlap of state between `InteractiveTxSigningSession` and `ChannelState::FundingNegotiated`. The signing session already tracks whether both holder and counterparty `tx_signatures` have been produced, so tracking the state duplicatively at the `ChannelState` level is unnecessary.
| fn on_tx_signatures_exchange(&mut self, funding_tx: Transaction) { | ||
| debug_assert!(!self.context.channel_state.is_monitor_update_in_progress()); | ||
| debug_assert!(!self.context.channel_state.is_awaiting_remote_revoke()); | ||
| if let Some(pending_splice) = self.pending_splice.as_mut() { | ||
| if let Some(FundingNegotiation::AwaitingSignatures(mut funding)) = | ||
| pending_splice.funding_negotiation.take() | ||
| { | ||
| funding.funding_transaction = Some(funding_tx); | ||
| self.pending_funding.push(funding); | ||
| } else { | ||
| debug_assert!(false); | ||
| let err = format!( | ||
| "Channel {} with pending splice is not expecting funding signatures yet", | ||
| self.context.channel_id | ||
| ); | ||
| return Err(APIError::APIMisuseError { err }); | ||
| } | ||
| self.context.channel_state.clear_quiescent(); | ||
| } else { | ||
| self.funding.funding_transaction = Some(funding_tx); | ||
| self.context.channel_state = | ||
| ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new()); | ||
| } | ||
| } |
There was a problem hiding this comment.
State management bug: The on_tx_signatures_exchange function modifies channel state and moves funding to pending_funding for splices, but doesn't handle the case where pending_splice.funding_negotiation.take() returns None. The else branch only has debug_assert!(false) which is a no-op in release builds, leaving the channel in an inconsistent state when the funding negotiation is unexpectedly missing.
| fn on_tx_signatures_exchange(&mutself,funding_tx:Transaction){ | |
| debug_assert!(!self.context.channel_state.is_monitor_update_in_progress()); | |
| debug_assert!(!self.context.channel_state.is_awaiting_remote_revoke()); | |
| ifletSome(pending_splice) = self.pending_splice.as_mut(){ | |
| ifletSome(FundingNegotiation::AwaitingSignatures(mut funding)) = | |
| pending_splice.funding_negotiation.take() | |
| { | |
| funding.funding_transaction = Some(funding_tx); | |
| self.pending_funding.push(funding); | |
| }else{ | |
| debug_assert!(false); | |
| let err = format!( | |
| "Channel {} with pending splice is not expecting funding signatures yet", | |
| self.context.channel_id | |
| ); | |
| returnErr(APIError::APIMisuseError{ err }); | |
| } | |
| self.context.channel_state.clear_quiescent(); | |
| }else{ | |
| self.funding.funding_transaction = Some(funding_tx); | |
| self.context.channel_state = | |
| ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new()); | |
| } | |
| } | |
| fn on_tx_signatures_exchange(&mutself,funding_tx:Transaction){ | |
| debug_assert!(!self.context.channel_state.is_monitor_update_in_progress()); | |
| debug_assert!(!self.context.channel_state.is_awaiting_remote_revoke()); | |
| ifletSome(pending_splice) = self.pending_splice.as_mut(){ | |
| ifletSome(FundingNegotiation::AwaitingSignatures(mut funding)) = | |
| pending_splice.funding_negotiation.take() | |
| { | |
| funding.funding_transaction = Some(funding_tx); | |
| self.pending_funding.push(funding); | |
| }else{ | |
| // This should never happen - if we're in on_tx_signatures_exchange for a splice, | |
| // we must have a pending funding negotiation in the AwaitingSignatures state. | |
| panic!("Missing or invalid funding negotiation state in splice during signature exchange"); | |
| } | |
| self.context.channel_state.clear_quiescent(); | |
| }else{ | |
| self.funding.funding_transaction = Some(funding_tx); | |
| self.context.channel_state = | |
| ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new()); | |
| } | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
This is unnecessary, we always check we're in the AwaitingSignatures state prior to calling this function.
| debug_assert!(matches!( | ||
| self.context.channel_state, | ||
| ChannelState::FundingNegotiated(_) if self.context.interactive_tx_signing_session.is_none() | ||
| )); |
There was a problem hiding this comment.
Logic error: The debug_assert condition is too restrictive. It only allows unsetting funding info when interactive_tx_signing_session.is_none(), but the function should also be callable in AwaitingChannelReady state as indicated by the original logic. This overly restrictive condition will cause assertion failures in valid scenarios where funding info needs to be unset after the signing session has completed.
| debug_assert!(matches!( | |
| self.context.channel_state, | |
| ChannelState::FundingNegotiated(_)ifself.context.interactive_tx_signing_session.is_none() | |
| )); | |
| debug_assert!(matches!( | |
| self.context.channel_state, | |
| ChannelState::FundingNegotiated(_)ifself.context.interactive_tx_signing_session.is_none() | | |
| ChannelState::AwaitingChannelReady | |
| )); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
OutboundV1Channel is not expected to be in AwaitingChannelReady.
Once a channel open has become locked (i.e., we've entered `ChannelState::ChannelReady`), the channel is intended to remain within this state for the rest of its lifetime until shutdown. Previously, we had assumed a channel being spliced would go through the `ChannelState` lifecycle again starting from `NegotiatingFunding` but skipping `AwaitingChannelReady`. This inconsistency departs from what we strive to achieve with `ChannelState` and also makes the state of a channel harder to reason about. This commit ensures a channel undergoing a splice remains in `ChannelReady`, clearing the quiescent flag once the negotiation is complete. Dual funding is unaffected by this change as the channel is being opened and we want to maintain the same `ChannelState` lifecycle.
8a574a5 to
7597140Comparewpaulino
commented
Sep 15, 2025
Had to rebase due to a small conflict. |
| if let Some(funding_tx) = funding_tx_opt.clone() { | ||
| debug_assert!(tx_signatures_opt.is_some()); | ||
| self.on_tx_signatures_exchange(funding_tx); |
There was a problem hiding this comment.
Logic error: The code calls self.on_tx_signatures_exchange(funding_tx) only when funding_tx_opt is Some, and asserts that tx_signatures_opt.is_some() in this case. However, this assumes that having a funding transaction always means we have tx_signatures to send, which may not be true in all signing scenarios. This could cause assertion failures or incorrect state transitions.
| ifletSome(funding_tx) = funding_tx_opt.clone(){ | |
| debug_assert!(tx_signatures_opt.is_some()); | |
| self.on_tx_signatures_exchange(funding_tx); | |
| ifletSome(funding_tx) = funding_tx_opt.clone(){ | |
| if tx_signatures_opt.is_some(){ | |
| self.on_tx_signatures_exchange(funding_tx); | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Within the context of funding_transaction_signed, if the funding transaction is ready, it implies that the counterparty already sent their signatures first so we must always respond with ours.
TheBlueMatt
commented
Sep 16, 2025
No strong opinion, honestly. Just seemed like something to highlight that we should be at least somewhat less strict.
Right, that was my point :) |
This was pulled out of #4054 to ease review. It depends on #4060 to not bother with
cfg(splicing)anymore.