-
Notifications
You must be signed in to change notification settings - Fork 412
Exchange splice_locked
messages
#3741
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
👋 Thanks for assigning @wpaulino as a reviewer! |
The logic around determining if the |
dfbc04e
to
e4c0566
Compare
@wpaulino Ok, this is in better shape for a high-level look. I don't believe it correctly handles unconfirmed splice transactions yet. Also, doesn't yet re-send |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3741 +/- ##
==========================================
+ Coverage 89.10% 90.34% +1.24%
==========================================
Files 156 158 +2
Lines 123431 135603 +12172
Branches 123431 135603 +12172
==========================================
+ Hits 109985 122515 +12530
+ Misses 10760 10568 -192
+ Partials 2686 2520 -166 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The preparational changes are very clear. The WIP part also makes sense so far.
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
e4c0566
to
49f8ef6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed most comments other than adding an event and for re-sending splice_locked
. See comment replies for open questions.
Also, added code to insert the new scid in short_to_chan_info
. Should we remove the old one?
ef048e6
to
f3b0989
Compare
Squashed fixups |
An offline discussion we had around |
lightning/src/ln/channelmanager.rs
Outdated
@@ -3026,7 +3026,7 @@ macro_rules! locked_close_channel { | |||
// into the map (which prevents the `PeerState` from being cleaned up) for channels that | |||
// never even got confirmations (which would open us up to DoS attacks). | |||
let update_id = $channel_context.get_latest_monitor_update_id(); | |||
if $channel_context.get_funding_tx_confirmation_height().is_some() || $channel_context.minimum_depth() == Some(0) || update_id > 1 { | |||
if $channel_funding.get_funding_tx_confirmation_height().is_some() || $channel_context.minimum_depth() == Some(0) || update_id > 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we agreed to try to keep the concept of funding scopes outside of ChannelManager
? I think for this we can move to a is_funding_confirmed_or_0conf
call or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This macro doesn't have access to the channel, only the context which doesn't have reference to the funding scope. So it would require much more re-work than I'd like to do in this PR.
Instead, I'd rather make a separate PR after all the changes needed for FundingScope
are complete. Hopefully in a couple PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, also forgot to drop these two commits since one reverts the previous one. But the general idea still stands for other occurrences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to avoid this but the problem is that locked_close_channel
is called throughout the file including by convert_channel_err
, which likewise is called throughout. Additionally, convert_channel_err
has a rule that expects a FundedChannel
so that it can pass it to get_channel_update_for_broadcast
.
Might be a way to refactor this but I think it should wait for a follow-up given it's not going to be straightforward.
@@ -3031,7 +3031,7 @@ macro_rules! locked_close_channel { | |||
$peer_state.closed_channel_monitor_update_ids.insert(chan_id, update_id); | |||
} | |||
let mut short_to_chan_info = $self.short_to_chan_info.write().unwrap(); | |||
if let Some(short_id) = $channel_context.get_short_channel_id() { | |||
if let Some(short_id) = $channel_funding.get_short_channel_id() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, can we avoid this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, we'd need this to take a Channel
instead of a ChannelContext
, but the problem is that convert_channel_err
needs a FundedChannel
for one rule as mentioned in another comment. Maybe we eventually just add any methods called from those macros on both Channel
and each sub-struct so that it can be used either way?
Separate but related to this, I'm wondering now which Currently, we use the pre-splice
After that So in the scenario described above, should we transition to the pending Then that way for Related discussion: #3592 (comment) |
f3b0989
to
bd1a788
Compare
Discussed offline with @TheBlueMatt and @wpaulino. The two scenarios are closing while funding negotiation takes place and while waiting for the splice to confirm. For the latter, it would be better to a have general approach in For the former, we won't have a |
b4e2b1a
to
a57c034
Compare
Pushed some fixups to hopefully fix CI. Also, updated the WIP commit message. |
One more fixup... |
🔔 1st Reminder Hey @TheBlueMatt @dunxen @wpaulino! This PR has been waiting for your review. |
1 similar comment
🔔 1st Reminder Hey @TheBlueMatt @dunxen @wpaulino! This PR has been waiting for your review. |
0-conf channels always meet the funding minimum depth once accepted. Special case this in check_funding_meets_minimum_depth such that it isn't implicit in later calculations. Since a minimum depth is always set when the channel is accepted, expect this to be the case in the method since it should only be called on a ChannelContext in a FundedChannel.
When transactions confirm or the best block is updated, check if any pending splice funding transactions have confirmed to an acceptable depth. If so, send a splice_locked message to the counterparty and -- if the counterparty has exchanged a splice_locked message for the same funding txid -- promote the corresponding FundingScope such that the new funding can be utilized.
bff123e
to
2129d53
Compare
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
funding, block_hash, height, index_in_block, &mut confirmed_tx, logger, | ||
)? { | ||
if confirmed_funding_index.is_some() { | ||
let err_reason = "splice tx of another pending funding already confirmed"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also check to make sure no other funding is already confirmed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a funding_already_confirmed
to track and check.
}, | ||
#[cfg(splicing)] | ||
Some(FundingConfirmedMessage::Splice(splice_locked)) => { | ||
if announcement_sigs.is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, don't we normally keep all (including non-public) channels in short_to_chan_info
? I'd buy that we don't actually need to, but seems weird to end up with different state here than we get on restart/for a normal channel open. (same goes for the inbound-locked case afaict)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure if I understand this comment. We use announcement_sigs
to determine if we already received splice_locked
from our counterparty. If so, the funding is now locked as we are about to send our own splice_locked
. Thus, we need to update short_to_chan_info
with the new SCID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't have announcement_sigs
if the channel is private, so we won't update short_to_chan_info
even though we should.
lightning/src/ln/channel.rs
Outdated
// transaction until it is seen on chain. Set it so that minimum_depth | ||
// checks can tell if the coinbase transaction was used. | ||
if funding.funding_transaction.is_none() { | ||
funding.funding_transaction = Some(tx.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to remove this line now, right?
lightning/src/ln/channel.rs
Outdated
|
||
// Check if we sent splice_locked for the unconfirmed transaction | ||
#[cfg(splicing)] | ||
if let Some(pending_splice) = &mut self.pending_splice { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only called for chain::Confirm
listeners, but chain::Listen
folks only get best_block_updated
calls. We should probably move this hunk to best_block_updated
(with appropriate changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a fixup. Let me know if it looks sane.
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
This method is only applicable for FundedChannel, so it shouldn't be accessible from ChannelContext.
Now that FundedScope::minimum_depth_override is used to override the minimum depth with COINBASE_MATURITY when the funding transaction is the coinbase transaction, use this in ChannelContext::minimum_depth method. Also, add a minimum_depth to Channel. The one on ChannelContext can become private once FudningScope doesn't need to be accessed directly from a ChannelManager macro. This fixes ChannelDetails showing an incorrect minimum depth when the coinbase transaction is used to fund the channel.
When a splice funding transaction is unconfirmed, update the corresponding FundingScope just as is done when the initial funding transaction is unconfirmed.
Pending funding transactions for splices should be monitored for appearance on chain. Include these in ChannelManager::get_relevant_txids so that they can be watched.
When a splice is locked, the SCID from the previous funding transaction needs to be remembered so that pending HTLCs can be handled properly. Additionally, when they need to be cleaned up once they should no longer be used. Track these SCIDs as splices are locked and clean any up as blocks are connected.
Once both parties have exchanged splice_locked messages, the splice funding is ready for use. Emit an event to the user indicating as much.
A ChannelReady event is used for both channel establishment and splicing to indicate that the funding transaction is confirmed to an acceptable depth and thus the channel can be used with the funding. An upcoming SplicePending event will be emitted for each pending splice (i.e., both the initial splice attempt and any RBF attempts). Thus, when a ChannelReady event is emitted, the funding_txo must be included to differentiate between which ChannelPending -- which also contains the funding_txo -- that the event corresponds to.
2129d53
to
6a0fb94
Compare
// Note that transaction_unconfirmed may have reset funding_tx_confirmation_height | ||
// to zero if it has been reorged out, however in either case, our state flags | ||
// indicate we've already sent a channel_ready | ||
let funding_tx_confirmations = self.funding.get_funding_tx_confirmations(height); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep things consistent with the splicing case, maybe we should also reset the confirmation height to 0 in do_best_block_updated
for the current scope so we don't have to do it in transaction_unconfirmed
. Though keep in mind that we should reset it regardless of whether we've sent ChannelReady
.
After a splice has been negotiated, each party must send a
splice_locked
message to the other party once the splice transaction has had an acceptable number of confirmations. Update the logic for processing newly confirmed transactions and updated best block to sendsplice_locked
when appropriate.Likewise, handle
splice_locked
and promote the channel'sFundingScope
once bothsplice_locked
messages have been exchanged.