Follow-ups #3741: Exchange splice_locked messages
#3873
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
lightning/src/ln/channel.rs
Outdated
| @@ -9420,6 +9421,9 @@ where | |||
| let pending_splice = self.pending_splice.as_ref().unwrap(); | |||
| let funding = self.pending_funding.get(confirmed_funding_index).unwrap(); | |||
| if let Some(splice_locked) = self.check_get_splice_locked(pending_splice, funding, height) { | |||
| let pending_splice = self.pending_splice.as_mut().unwrap(); | |||
There was a problem hiding this comment.
Why not just DRY this up and do this in check_get_splice_locked so that we don't forget to do it at its callsites?
There was a problem hiding this comment.
Hmmm... a few reasons:
- it introduces a side effect into a "check" function, though it seems we already do that for
check_get_channel_ready - we would update state before knowing if we actually will send
splice_lockedfor the call intransactions_confirmed - it requires a bit of refactoring to placate the borrow checker, which may be a problem later (see below)
I made the change in a fixup to see what it looks like. However, I think we'll have a problem once we move ChannelFunded::pending_funding into PendingSplice because we'll need a mutable reference to PendingSplice and an immutable reference to one of its parts (FundingScope). Elsewhere, we get around this by passing confirmed_funding_index to maybe_promote_splice_funding. We'd likely need to do something similar, which is kinda meh. What do you think?
There was a problem hiding this comment.
it introduces a side effect into a "check" function, though it seems we already do that for check_get_channel_ready
Indeed, this is what we've done elsewhere, really the "get" part of the thing...Maybe a new name would make it clearer
we would update state before knowing if we actually will send splice_locked for the call in transactions_confirmed
Sure, but we can't be sure we ever send a message. Generally our message generators are treated as "this message has now been generated, it will be sent to our peer as the next message and if it doesn't make it we'll figure it out later and retransmit", so I think its still in line with what we do otherwise.
it requires a bit of refactoring to placate the borrow checker, which may be a problem later (see below)
However, I think we'll have a problem once we move ChannelFunded::pending_funding into PendingSplice because we'll need a mutable reference to PendingSplice and an immutable reference to one of its parts (FundingScope).
Shouldn't that simply imply that the method can move to be a method on PendingSplice at that point, where we wouldn't have that issue because it just takes a &mut self?
Put another way, this feels like an indication that our data model is wrong - we shouldn't need to be passing two references to different parts of a struct to a method, the method should be on that struct.
There was a problem hiding this comment.
Shouldn't that simply imply that the method can move to be a method on
PendingSpliceat that point, where we wouldn't have that issue because it just takes a&mut self?
Yup, that's what the fixup does...
Put another way, this feels like an indication that our data model is wrong - we shouldn't need to be passing two references to different parts of a struct to a method, the method should be on that struct.
... though now we need to pass &ChannelContext. The future refactor will need to remove &FundingScope in favor of confirmed_funding_index because of the issue mentioned above.
|
👋 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. |
jkczyz
force-pushed
the
2025-06-splice-locked-fixes
branch
from
e0386ba to
8ea5689
Compare
jkczyz
force-pushed
the
2025-06-splice-locked-fixes
branch
from
8ea5689 to
e08c2ff
Compare
jkczyz
force-pushed
the
2025-06-splice-locked-fixes
branch
2 times, most recently
from
1c0d324 to
061a02a
Compare
When sending splice_locked, set PendingSplice::sent_funding_txid prior to calling FundedChannel::maybe_promote_splice_funding. This makes the latter idempotent when the splice is not promoted. Otherwise, successive calls could override the previously set sent_funding_txid.
This will help make the code more compact when using rustfmt.
Use of a macro as a function parameter prevented this method from being formatted by rustfmt. Extract out a variable such is now can be.
When sending or receiving splice_locked results in promoting a FundingScope, return the new funding_txo to ChannelManager. This is used to determine if Event::ChannelReady should be emitted. This is deemed safer than checking the channel if there are any pending splices after it handles splice_locked or after checking funding confirmations.
The spec is being changed to keep around a channel_announcement when the funding_txo is spent. This allows spliced channels more time to exchange splice_locked messages before the channel is dropped from the network graph. While LDK does not drop such channels, it uses this constant to allow forwarding HTLCs over the channel using SCIDs from previous funding transactions. Here, the increase from 12 to 144 reflects double the spec change of 72 blocks.
jkczyz
force-pushed
the
2025-06-splice-locked-fixes
branch
from
061a02a to
db69ec7
Compare
Addresses remaining feedback from #3741.