Rework ChannelManager::funding_transaction_signed #4257
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
wpaulino
force-pushed
the
funding-transaction-signed-rework
branch
from
61719cf to
135605f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4257 +/- ##
==========================================
+ Coverage 86.59% 86.60% +0.01%
==========================================
Files 158 158
Lines 102368 102376 +8
Branches 102368 102376 +8
==========================================
+ Hits 88641 88659 +18
+ Misses 11313 11297 -16
- Partials 2414 2420 +6
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:
|
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
| Some(channel) => match channel.as_funded_mut() { | ||
| Some(chan) => { |
There was a problem hiding this comment.
IIUC, we can write a FundingTransactionReadyForSigning for dual funding, but upon restart the channel will be forgotten as it will still be considered pending. So calling funding_transaction_signed will fail in that case. Is this a problem? Or should we at least note this in the docs?
There was a problem hiding this comment.
Added a note in the docs that it can happen
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
lightning/src/ln/channel.rs
Outdated
| // TODO(splicing): Support async signing | ||
| .ok_or("Failed to compute commitment_signed signatures".to_owned())?; |
There was a problem hiding this comment.
Ha, I thought you said this was supporting async signing :p
lightning/src/ln/channelmanager.rs
Outdated
| )); | ||
| } | ||
| if channel.context().is_connected() { | ||
| if let Some(commit_sig) = commit_sig { |
There was a problem hiding this comment.
Can you add a test that checks that we avoid sending the commit sigs here if the funding_transaction_signed call comes in before the monitor is persisted (async)?
There was a problem hiding this comment.
We can actually send our commit sig immediately after funding_transaction_signed, there's no reason to wait for the monitor persist to complete first. We just have to make sure we don't process a counterparty one before it completes.
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).
We delay processing it until the user manually approves the splice via `Channel::funding_transaction_signed`, as otherwise, there would be a [`ChannelMonitorUpdateStep::RenegotiatedFunding`] committed that we would need to undo if they no longer wish to proceed. Note that this doesn't need to be done with dual-funded channels as there is no equivalent monitor update for them.
wpaulino
force-pushed
the
funding-transaction-signed-rework
branch
from
135605f to
e502ce3
Compare
4 participants
Previously, we'd emit a
FundingTransactionReadyForSigningevent once the initialcommitment_signedis exchanged for a splicing/dual-funding attempt and require users to call back with their signed inputs usingChannelManager::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 initialcommitment_signed. We will now emit theFundingTransactionReadyForSigningevent and wait for the user to call back before releasing both our initialcommitment_signedand ourtx_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).