Rework ChannelManager::funding_transaction_signed #4336
+900
−508
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
👋 Thanks for assigning @jkczyz as a reviewer! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4336 +/- ##
==========================================
+ Coverage 86.53% 86.76% +0.22%
==========================================
Files 158 158
Lines 103190 102927 -263
Branches 103190 102927 -263
==========================================
+ Hits 89300 89309 +9
+ Misses 11469 11258 -211
+ Partials 2421 2360 -61
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:
|
Previously, we'd emit a FundingTransactionReadyForSigning event once the initial commitment_signed is exchanged for a splicing/dual-funding attempt and require users to call back with their signed inputs using ChannelManager::funding_transaction_signed. While this approach worked in practice, it prevents us from abandoning a splice if we cannot or no longer wish to sign as the splice has already been committed to by this point. This commit reworks the API such that this is now possible. After exchanging tx_complete, we will no longer immediately send our initial commitment_signed. We will now emit the FundingTransactionReadyForSigning event and wait for the user to call back before releasing both our initial commitment_signed and our tx_signatures. As a result, the event is now persisted, as there is only one possible path in which it is generated. Note that we continue to only emit the event if a local contribution to negotiated transaction was made. Future work will expose a cancellation API such that we can abandon splice attempts safely (we can just force close the channel with dual-funding).
This is crucial to enable the splice cancellation use case. When we process the initial commitment signed from our counterparty, we queue a monitor update that cannot be undone. To give the user a chance to abort the splice negotiation before it's committed to, we buffer the message until a successful call to `Channel::funding_transaction_signed` and process it then. Note that this is currently only done for splice and RBF attempts, as if we want to abort a dual funding negotiation, we can just force close the channel as it hasn't been funded yet.
Now that we require users to first call `ChannelManager::funding_transaction_signed` before releasing any signatures, it's possible that it is called before we receive the initial commitment signed from our counterparty, which would transition the channel to funded. Because of this, we need to support the API call while the channel is still in the unfunded phase. Note that this commit is mostly a code move of `FundedChannel::funding_transaction_signed` to `Channel::funding_transaction_signed` that doesn't alter the signing logic.
wpaulino
force-pushed
the
funding-transaction-signed-rework
branch
from
c0193fe to
0fc923c
Compare
jkczyz
reviewed
Jan 23, 2026
Comment on lines
2101
to
2113
| if let FundingNegotiation::ConstructingTransaction { | ||
| funding, | ||
| mut funding, | ||
| interactive_tx_constructor, | ||
| } = funding_negotiation | ||
| { | ||
| let is_initiator = interactive_tx_constructor.is_initiator(); | ||
| Some((is_initiator, funding, interactive_tx_constructor)) | ||
| funding.channel_transaction_parameters.funding_outpoint = | ||
| Some(funding_outpoint); | ||
| pending_splice.funding_negotiation = | ||
| Some(FundingNegotiation::AwaitingSignatures { | ||
| is_initiator, | ||
| funding, | ||
| }); |
Contributor
There was a problem hiding this comment.
I guess it's preexisting, but consider avoiding these side-effects using if let instead of and_then:
let funding_negotiation = pending_splice.funding_negotiation.take();
if let Some(FundingNegotiation::ConstructingTransaction { .. }) = funding_negotiation {
Comment on lines
11265
to
11274
| // We should never be sending a `commitment_signed` in response to their | ||
| // `tx_signatures`. | ||
| debug_assert!(commitment_signed.is_none()); | ||
|
|
||
| if let Some(tx_signatures) = tx_signatures { | ||
| peer_state.pending_msg_events.push(MessageSendEvent::SendTxSignatures { | ||
| node_id: *counterparty_node_id, | ||
| msg: tx_signatures, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
I'm a little confused by this. Why would we send our tx_signatures if we aren't sending our commitment_signed? Didn't we want to send both at once?
Contributor
Author
There was a problem hiding this comment.
We've already sent it by this point, we're sending our tx_signatures here in response to theirs
Comment on lines
+18998
to
+18999
| // We may need to regenerate [`Event::FundingTransactionReadyForSigning`] for channels that | ||
| // still need their holder `tx_signatures`. |
Contributor
There was a problem hiding this comment.
Does it make sense to start persisting this?
Contributor
Author
There was a problem hiding this comment.
Matt pointed out we shouldn't #4257 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative version to #4257