Uh oh!
There was an error while loading. Please reload this page.
Fix minor miscellaneous splicing issues - #4060
Conversation
👋 Thanks for assigning @jkczyz as a reviewer! |
ldk-reviews-bot
commented
Sep 8, 2025
🔔 1st Reminder Hey @TheBlueMatt@jkczyz! This PR has been waiting for your review. |
1 similar comment
ldk-reviews-bot
commented
Sep 8, 2025
🔔 1st Reminder Hey @TheBlueMatt@jkczyz! This PR has been waiting for your review. |
| } | ||
| } | ||
| } | ||
| removed_fulfilled_htlcs = true; |
There was a problem hiding this comment.
Could we just run the loop below at this point and return rather than needing this variable?
There was a problem hiding this comment.
It's just a small optimization to not remove HTLCs that have already been removed since we have duplicate data across FundingScopes. The loop below does need to run for every FundingScope though.
There was a problem hiding this comment.
Right, but that is already inside the closure, which is called on each FundingScope.
There was a problem hiding this comment.
Oh, nevermind. Didn't see that removed_fulfilled_htlcs is used across all calls.
| { | ||
| Some(interactive_tx_constructor) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
I think we actually don't want any unreachables since they can be triggered by a buggy/malicious counterparty.
| ChannelPhase::UnfundedOutboundV1(_) => unreachable!(), | ||
| ChannelPhase::UnfundedInboundV1(_) => unreachable!(), | ||
| ChannelPhase::UnfundedV2(pending_v2_channel) => { | ||
| pending_v2_channel.interactive_tx_constructor.take() |
There was a problem hiding this comment.
Can't recall if we eventually want to use FundingNegotiation here, too. Can wait, of course, but we may be able avoid these unreachables if we can take that at the call sites below.
There was a problem hiding this comment.
We're not quite using that yet for dual funding so I'd rather delay it until we do.
Uh oh!
There was an error while loading. Please reload this page.
| pub fn funding_tx_constructed<L: Deref>( | ||
| &mut self, logger: &L, | ||
| ) -> Result<msgs::CommitmentSigned, msgs::TxAbort> | ||
| ) -> Result<msgs::CommitmentSigned, AbortReason> |
There was a problem hiding this comment.
Wanna make these methods non-pub now? Makes the logic more readable.
There was a problem hiding this comment.
Done, I think it was just funding_tx_constructed and interactive_tx_constructor_mut?
b1d97a1 to
9540edeCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Otherwise we'll hit an assert that we do not already have an alternative funding confirmation when reprocessing the transaction.
…cked The `ChannelMonitor` now tracks its own set of channel parameters, but in the event they change after a splice, we want to ensure they are updated accordingly at the `OnchainTxHandler` level as well in case the user downgrades after a locked splice has already occurred.
We only need to track the HTLC sources for the previous and current counterparty commitments.
This commit reworks all interactive transaction construction methods to mark the negotiation as failed upon a local/remote `TxAbort`, ensuring the `InteractiveTxConstructor` is consumed. Along the way, we refactor the handling of `tx_complete` such that we only have a single call into the `Channel` from the `ChannelManager`.
The splice shared input weight should always be composed of the base input weight (prevout & sequence), an empty `script_sig` weight, and the multisig channel-type specific witness weight.
Although a splice out doesn't include inputs to sign, users still need to call back with `funding_transaction_signed` to sign the shared splice input.
9540ede to
1dc190dCompare| let events = initiator_node.node.get_and_clear_pending_msg_events(); | ||
| assert_eq!(events.len(), 2); | ||
| assert_eq!(events.len(), 1); | ||
| match events[0] { | ||
| MessageSendEvent::SendTxComplete { .. } => {}, | ||
| _ => panic!("Unexpected event {:?}", events[0]), | ||
| } | ||
| match events[1] { | ||
| MessageSendEvent::SendTxAbort { .. } => {}, | ||
| _ => panic!("Unexpected event {:?}", events[1]), | ||
| _ => panic!("Unexpected event {:?}", events[0]), | ||
| } |
There was a problem hiding this comment.
The test is using the wrong variable in the handle_tx_complete call. It captures the latest tx_complete message as _tx_complete_msg (with an underscore), but then passes the earlier tx_complete_msg from line 244 to the handler. This could lead to incorrect behavior since it's processing the wrong message. The variable names should be consistent, and the latest message should be used in the handler call.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
This looks to predate the PR, but shouldn't affect behavior given tx_complete only contains a channel id.
There was a problem hiding this comment.
Yeah this test is getting removed completely in #4054 anyway.
| let events = initiator_node.node.get_and_clear_pending_msg_events(); | ||
| assert_eq!(events.len(), 2); | ||
| assert_eq!(events.len(), 1); | ||
| match events[0] { | ||
| MessageSendEvent::SendTxComplete { .. } => {}, | ||
| _ => panic!("Unexpected event {:?}", events[0]), | ||
| } | ||
| match events[1] { | ||
| MessageSendEvent::SendTxAbort { .. } => {}, | ||
| _ => panic!("Unexpected event {:?}", events[1]), | ||
| _ => panic!("Unexpected event {:?}", events[0]), | ||
| } |
There was a problem hiding this comment.
This looks to predate the PR, but shouldn't affect behavior given tx_complete only contains a channel id.
TheBlueMatt
left a comment
There was a problem hiding this comment.
Gonna land this but think we need to fix the dual-funding failure handling.
| ChannelPhase::Undefined => unreachable!(), | ||
| ChannelPhase::UnfundedOutboundV1(_) | ChannelPhase::UnfundedInboundV1(_) => None, | ||
| ChannelPhase::UnfundedV2(pending_v2_channel) => { | ||
| pending_v2_channel.interactive_tx_constructor.take() |
There was a problem hiding this comment.
Can we really just blindly take this? Its set up in new for inbound channels, so we probably should be outright failing the channel here, not resetting the signing session?
There was a problem hiding this comment.
Same applies further down.
There was a problem hiding this comment.
Dual funding hasn't been updated to use the new interactive tx path, it shouldn't even have an interactive tx constructor in new yet without calling FundingNegotiationContext::into_interactive_tx_constructor.
This was pulled out of #4054 to ease review. It fixes an assortment of issues I ran into while writing the tests there.