Follow-ups #3741: Exchange `splice_locked` messages by jkczyz · Pull Request #3873 · lightningdevkit/rust-lightning

jkczyz · 2025-06-17T22:42:15Z

Addresses remaining feedback from #3741.

ldk-reviews-bot · 2025-06-17T22:42:18Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt · 2025-06-17T23:29:54Z

 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();


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?

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_locked for the call in transactions_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?

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.

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?
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.

ldk-reviews-bot · 2025-06-18T00:33:33Z

👋 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.

TheBlueMatt

Feel free to squash, looks fine to me.

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 · 2025-06-17T22:42:15Z

Addresses remaining feedback from #3741.

ldk-reviews-bot · 2025-06-17T22:42:18Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt · 2025-06-17T23:29:54Z

 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();


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?

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_locked for the call in transactions_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?

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.

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?
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.

ldk-reviews-bot · 2025-06-18T00:33:33Z

👋 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.

TheBlueMatt

Feel free to squash, looks fine to me.

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 · 2025-06-17T22:42:15Z

Addresses remaining feedback from #3741.

ldk-reviews-bot · 2025-06-17T22:42:18Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt · 2025-06-17T23:29:54Z

 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();


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?

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_locked for the call in transactions_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?

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.

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?
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.

ldk-reviews-bot · 2025-06-18T00:33:33Z

👋 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.

TheBlueMatt

Feel free to squash, looks fine to me.

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 · 2025-06-17T22:42:15Z

Addresses remaining feedback from #3741.

ldk-reviews-bot · 2025-06-17T22:42:18Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt · 2025-06-17T23:29:54Z

 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();


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?

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_locked for the call in transactions_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?

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.

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?
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.

ldk-reviews-bot · 2025-06-18T00:33:33Z

👋 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.

TheBlueMatt

Feel free to squash, looks fine to me.

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 · 2025-06-17T22:42:15Z

Addresses remaining feedback from #3741.

ldk-reviews-bot · 2025-06-17T22:42:18Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt · 2025-06-17T23:29:54Z

 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();


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?

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_locked for the call in transactions_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?

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.

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?
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.

ldk-reviews-bot · 2025-06-18T00:33:33Z

👋 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.

TheBlueMatt

Feel free to squash, looks fine to me.

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 · 2025-06-17T22:42:15Z

Addresses remaining feedback from #3741.

ldk-reviews-bot · 2025-06-17T22:42:18Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt · 2025-06-17T23:29:54Z

 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();


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?

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_locked for the call in transactions_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?

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.

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?
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.

ldk-reviews-bot · 2025-06-18T00:33:33Z

👋 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.

TheBlueMatt

Feel free to squash, looks fine to me.

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 · 2025-06-17T22:42:15Z

Addresses remaining feedback from #3741.

ldk-reviews-bot · 2025-06-17T22:42:18Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt · 2025-06-17T23:29:54Z

 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();


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?

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_locked for the call in transactions_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?

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.

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?
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.

ldk-reviews-bot · 2025-06-18T00:33:33Z

👋 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.

TheBlueMatt

Feel free to squash, looks fine to me.

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 · 2025-06-17T22:42:15Z

Addresses remaining feedback from #3741.

ldk-reviews-bot · 2025-06-17T22:42:18Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt · 2025-06-17T23:29:54Z

 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();


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?

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_locked for the call in transactions_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?

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.

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?
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.

ldk-reviews-bot · 2025-06-18T00:33:33Z

👋 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.

TheBlueMatt

Feel free to squash, looks fine to me.

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 requested a review from TheBlueMatt June 17, 2025 22:42

TheBlueMatt reviewed Jun 18, 2025
View reviewed changes

jkczyz force-pushed the 2025-06-splice-locked-fixes branch from e0386ba to 8ea5689Compare June 18, 2025 14:44

wpaulino reviewed Jun 18, 2025
View reviewed changes

Comment threadlightning/src/ln/channel.rs

wpaulino reviewed Jun 18, 2025
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

jkczyz force-pushed the 2025-06-splice-locked-fixes branch from 8ea5689 to e08c2ffCompare June 18, 2025 16:50

jkczyz requested review from TheBlueMatt and wpaulino June 18, 2025 16:50

jkczyz force-pushed the 2025-06-splice-locked-fixes branch 2 times, most recently from 1c0d324 to 061a02aCompare June 18, 2025 17:08

TheBlueMatt reviewed Jun 18, 2025
View reviewed changes

jkczyz added 5 commits June 18, 2025 18:23

Don't inline call to FundedChannel::splice_locked
eeee50e
This will help make the code more compact when using rustfmt.

rustfmt ChannelManager::internal_splice_locked
5170373
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.

jkczyz force-pushed the 2025-06-splice-locked-fixes branch from 061a02a to db69ec7Compare June 18, 2025 23:23

jkczyz requested a review from TheBlueMatt June 18, 2025 23:23

TheBlueMatt approved these changes Jun 19, 2025
View reviewed changes

wpaulino approved these changes Jun 20, 2025
View reviewed changes

wpaulino merged commit bed20cf into lightningdevkit:mainJun 20, 2025

jkczyz mentioned this pull request Jun 21, 2025
Dual-funded channels and Splicing Project Tracking #1621
Closed
7 tasks

jkczyz mentioned this pull request Jul 1, 2025
Allow non-strict forwarding for spliced/closed channels #3821
Open

notmandatory mentioned this pull request Jul 7, 2025
Socratic Seminar #33 Topics NashBitDevs/nashbitdevs.org#97
Closed