Uh oh!
There was an error while loading. Please reload this page.
Integrate Splicing with Quiescence - #4019
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## main #4019 +/- ##
==========================================
- Coverage 88.76% 88.71% -0.05%
==========================================
Files 176 176 Lines 128139 128891 +752 Branches 128139 128891 +752 ==========================================
+ Hits 113743 114348 +605 - Misses 11822 11947 +125 - Partials 2574 2596 +22
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:
|
be3b93a to
ddb3b37Compare| let tx16 = TransactionU16LenLimited::new(tx) | ||
| .map_err(|_e| APIError::APIMisuseError { err: format!("Too large transaction") })?; | ||
| // TODO(splicing): Check that transactions aren't too big for the splice_init message here. |
There was a problem hiding this comment.
We probably just want to do all input validation here before we even attempt quiescence so that we can fail early and fast. No point in going through the whole stfu dance just to immediately fail back to the user with "insufficient fees" or something similar.
There was a problem hiding this comment.
Yep, that's definitely the goal. Kinda leaving this until the parameters are clear and I'll clean it up then.
👋 Thanks for assigning @jkczyz as a reviewer! |
When doing an outbound splice, we check some of the instructions first in utility methods, then convert errors to `APIError`s. These utility methods should thus either return an `APIError` or more generic (string or `()`) error type, but they currently return a `ChannelError`, which is only approprite when the calling code will do what the `ChannelError` instructs (including closing the channel). Here we fix that by returning `String`s instead.
ddb3b37 to
d9a278cCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| // Assumes we are either awaiting quiescence or our counterparty has requested quiescence. | ||
| #[rustfmt::skip] | ||
| pub fn send_stfu<L: Deref>(&mut self, logger: &L) -> Result<msgs::Stfu, ChannelError> | ||
| pub fn send_stfu<L: Deref>(&mut self, logger: &L) -> Result<msgs::Stfu, &'static str> |
There was a problem hiding this comment.
May as well return String if we'll just need to call to_owned.
There was a problem hiding this comment.
I think we'd end up with strictly more to_owneds, and we'll already have the map at the callsites to convert to a ChannelError, plus rustfmt is much more likely to blow up an Err(long string.to_owned()) than a map(|e| e.to_string()). Seems simpler to just keep the &'static str?
Uh oh!
There was an error while loading. Please reload this page.
| // TODO(splicing): if post_quiescence_action is set, integrate what the user wants to do | ||
| // into the counterparty-initiated splice. For always-on nodes this probably isn't a useful | ||
| // optimization, but for often-offline nodes it may be, as we may connect and immediately | ||
| // go into splicing from both sides. |
There was a problem hiding this comment.
Would this happen when both counterparties initiated quiescence simultaneously? Ties should go to the channel opener. Would we lose the user-initiated quiescent_action in that case? Or rather would we possibly be stuck when the user tries to initiate the splice again since quiescent_action is Some?
There was a problem hiding this comment.
If we lose the tie, we don't seem to consume the quiescent_action, but we also don't seem to attempt quiescence again once we exit so that we can perform ours. We only seem to retry quiescence with a pending quescient_action after a reconnection.
There was a problem hiding this comment.
We wouldn't lose it immediately, we'd wait until the counterparty's splice completes, then try to initialize our own splice and then fail at that point (because another splice is now in progress).
There was a problem hiding this comment.
What I mean here is that we don't retry quiescence after we've exited if we still have a quiescent_action lingering. It's generally applicable once we have other quiescence protocols, but also relevant for zero conf splices since it'll be immediately locked in and we can attempt another splice.
There was a problem hiding this comment.
We should retry quiescence after we've exited, it'll just fail. We don't yet have any exiting-quiescence logic yet, however, so there's nowhere to put it yet...
There was a problem hiding this comment.
Should we address the TODO now (i.e., contribute using splice_ack) instead of adding retry or SpliceFailed event? Only issue would be if there were another quiescence protocol that we understood. Then we'd again have a stuck action, IIUC.
There was a problem hiding this comment.
We'll ultimately need a splice failed event for the general case even if, eg a peer simply nak's a splice, no? I'm definitely happy to address the TODO now, however, is doing it trivial? Its not quite clear to me it is.
There was a problem hiding this comment.
I spent some time looking at addressing the TODO and I think it really needs to come with a SpliceInstructions refactor - we need to add a max-fee and min-fee field, with some default (ie push things through a constructor rather than a public enum) and then want to move the fee-calculation logic so that we can recalculate fees if we have to. Would rather wait on this.
ldk-reviews-bot
commented
Aug 27, 2025
👋 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. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| if self.funding.get_funding_txo() != Some(original_funding_txo) { | ||
| // This should be unreachable once we opportunistically merge splices if the | ||
| // counterparty initializes a splice. | ||
| return Err("Funding changed out from under us".to_owned()); |
There was a problem hiding this comment.
So this is preventing a user-initiated splice from happening if the counterparty happens to splice first. Why do we want this? Presumably, if they spliced, they still want to splice-in/out funds from their side of the channel and that hasn't happened yet.
There was a problem hiding this comment.
We can't splice in that case anyway until the first splice locks, so I'm not really sure its worth holding the pending splice around in that case? Also the current code would break cause we need to recalculate if the splice as proposed is possible, and what the fees should be. But I didn't bother fixing it cause I'm not sure we should wait 6 blocks then do it.
There was a problem hiding this comment.
We could initiate an RBF to do our splice but that's not supported yet either. Without that or being able to contribute on an incoming splice_init, then it seems we should queue a SpliceFailed event to the user?
There was a problem hiding this comment.
Yep, seems like we should. We don't yet have a SpliceFailed event upstream yet, tho.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| // TODO(splicing): if post_quiescence_action is set, integrate what the user wants to do | ||
| // into the counterparty-initiated splice. For always-on nodes this probably isn't a useful | ||
| // optimization, but for often-offline nodes it may be, as we may connect and immediately | ||
| // go into splicing from both sides. |
There was a problem hiding this comment.
If we lose the tie, we don't seem to consume the quiescent_action, but we also don't seem to attempt quiescence again once we exit so that we can perform ours. We only seem to retry quiescence with a pending quescient_action after a reconnection.
d9a278c to
b37b5e4CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Now that we have a `QuiescentAction` to track what we intend to do once we reach quiescence, we need to use it to initiate splices. Here we do so, adding a new `SpliceInstructions` to track the arguments that are currently passed to `splice_channel`. While these may not be exactly the right arguments to track in the end, a lot of the splice logic is still in flight, so we can worry about it later.
While we have a test to disconnect a peer if we're waiting on an `stfu` message, we also disconnect if we've reached quiescence but we're waiting on a peer to do "something fundamental" and they take too long to do so. We test that behavior here.
b37b5e4 to
9200308CompareTheBlueMatt
commented
Aug 28, 2025
Squashed and addressed @wpaulino's feeedback, plus a rustfmt thing $ git diff-tree -U3 b37b5e4e7 9200308e8
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 0ee9319df8..12d3b53905 100644
--- a/lightning/src/ln/channel.rs+++ b/lightning/src/ln/channel.rs@@ -10799,7 +10799,7 @@ where
// Check if a splice has been initiated already.
// Note: only a single outstanding splice is supported (per spec)
- if self.pending_splice.is_some() {+ if self.pending_splice.is_some() || self.quiescent_action.is_some() {
return Err(APIError::APIMisuseError {
err: format!(
"Channel {} cannot be spliced, as it has already a splice pending",
@@ -10913,9 +10913,7 @@ where
let original_funding_txo = self.funding.get_funding_txo().ok_or_else(|| {
debug_assert!(false);
- APIError::APIMisuseError {- err: "Channel isn't yet fully funded".to_owned(),- }+ APIError::APIMisuseError { err: "Channel isn't yet fully funded".to_owned() }
})?;
let (our_funding_inputs, our_funding_outputs, change_script) = contribution.into_tx_parts();
@@ -10947,12 +10945,6 @@ where
original_funding_txo,
} = instructions;
- if self.funding.get_funding_txo() != Some(original_funding_txo) {- // This should be unreachable once we opportunistically merge splices if the- // counterparty initializes a splice.- return Err("Funding changed out from under us".to_owned());- }-
// Check if a splice has been initiated already.
// Note: only a single outstanding splice is supported (per spec)
if self.pending_splice.is_some() {
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 81c507b7d2..89992dd831 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -4495,8 +4495,12 @@ where
let locktime = locktime.unwrap_or_else(|| self.current_best_block().height);
if let Some(chan) = chan_phase_entry.get_mut().as_funded_mut() {
let logger = WithChannelContext::from(&self.logger, &chan.context, None);
- let msg_opt = chan- .splice_channel(contribution, funding_feerate_per_kw, locktime, &&logger)?;+ let msg_opt = chan.splice_channel(+ contribution,+ funding_feerate_per_kw,+ locktime,+ &&logger,+ )?;
if let Some(msg) = msg_opt {
peer_state.pending_msg_events.push(MessageSendEvent::SendStfu {
node_id: *counterparty_node_id, |
| change_script: Option<ScriptBuf>, | ||
| funding_feerate_per_kw: u32, | ||
| locktime: u32, | ||
| original_funding_txo: OutPoint, |
There was a problem hiding this comment.
Will do in a followup, thanks.
Uh oh!
There was an error while loading. Please reload this page.
Based on #4007this does the actual integration. Will probably go after #3979 so drafting for now.