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 102591 -599
Branches 103190 102591 -599
==========================================
- Hits 89300 88299 -1001
- Misses 11469 11783 +314
- Partials 2421 2509 +88
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
2 times, most recently
from
734ad69 to
04a7560
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Question somewhat orthogonal to this PR - do we need a monitor update before sending our tx-sigs? Once we send it, its possible for our counterparty to respond with a commitment sigs and then we can't cancel. If we restart without chanman persistence and the user tries to cancel at that point we'll end up getting force-closed on.
I continue to be somewhat annoyed at how many expects there are in the splicing logic, and would feel a lot better if we had a full_stack_target seed that spliced so that at least the fuzzer could explore it a tiny bit (though of course a larger refactor to reduce them would be nice too).
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
04a7560 to
f2dd6f2
Compare
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
4 participants
Alternative version to #4257