Rework ChannelManager::funding_transaction_signed #4336
Conversation
|
👋 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.06% -0.48%
==========================================
Files 158 156 -2
Lines 103190 102557 -633
Branches 103190 102557 -633
==========================================
- Hits 89300 88263 -1037
- Misses 11469 11788 +319
- Partials 2421 2506 +85
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:
|
wpaulino
force-pushed
the
funding-transaction-signed-rework
branch
from
c0193fe to
0fc923c
Compare
| // 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, | ||
| }); | ||
| } |
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?
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
| // We may need to regenerate [`Event::FundingTransactionReadyForSigning`] for channels that | ||
| // still need their holder `tx_signatures`. |
There was a problem hiding this comment.
Does it make sense to start persisting this?
There was a problem hiding this comment.
Matt pointed out we shouldn't #4257 (comment)
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Hmm, seems like the last commit should really get have test coverage? I guess just our dual-funding coverage isn't that great?
Also, presumably this needs documentation for how to cancel a splice instead of signing and a test that does so?
Yeah things aren't really hooked up yet to do proper testing.
Planned for a separate PR, no need for it to happen here. |
wpaulino
force-pushed
the
funding-transaction-signed-rework
branch
from
0fc923c to
734ad69
Compare
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
734ad69 to
04a7560
Compare
4 participants
Alternative version to #4257