Skip to content

Splice with mempool fuzz fixes #4708

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.

Sign up for GitHub

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

Open
wpaulino wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Splice with mempool fuzz fixes #4708

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
baa25d3
Only emit Event::SpliceNegotiated when contributing
wpaulino Jun 17, 2026
c671f20
Always emit SpliceNegotiationFailed when contributing
wpaulino Jun 16, 2026
3856a64
Allow invalid contribution error upon quiescence
wpaulino Jun 17, 2026
1c9c3b5
Prefer tx_abort over disconnection for splice negotiation errors
wpaulino Jun 16, 2026
397f2c4
Prefer tx_abort over disconnection for inability to RBF
wpaulino Jun 17, 2026
a120ac1
Check channel is live while handling counterparty tx_init_rbf
wpaulino Jun 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -936,7 +936,8 @@ fn assert_disconnect_action(action: &msgs::ErrorAction) -> (&msgs::WarningMessag
// Since sending/receiving messages may be delayed, `timer_tick_occurred` may cause a node to
// disconnect their counterparty if they're expecting a timely response.
if let msgs::ErrorAction::DisconnectPeerWithWarning { ref msg } = action {
let is_quiescent_msg = msg.data.contains("already sent splice_locked, cannot RBF");
let is_quiescent_msg = msg.data.contains("already sent splice_locked, cannot RBF")
|| msg.data.contains("contribution no longer valid at quiescence");
if !msg.data.contains("Disconnecting due to timeout awaiting response") && !is_quiescent_msg
{
panic!("Unexpected disconnect case: {}", msg.data);
Expand Down
14 changes: 9 additions & 5 deletions lightning/src/events/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1647,8 +1647,12 @@ pub enum Event {
/// [`ChainMonitor::get_claimable_balances`]: crate::chain::chainmonitor::ChainMonitor::get_claimable_balances
last_local_balance_msat: Option<u64>,
},
/// Used to indicate that a splice for the given `channel_id` has been negotiated and its
/// funding transaction has been broadcast.
/// Used to indicate that a splice for the given `channel_id` has been negotiated, its
/// funding transaction has been broadcast, and local inputs or outputs were contributed to
/// it.
///
/// This event is not emitted if the counterparty negotiated a splice without using a local
/// contribution.
///
/// The splice is then considered pending until both parties have seen enough confirmations to
/// consider the funding locked. Once this occurs, an [`Event::ChannelReady`] will be emitted.
Expand Down Expand Up @@ -1679,9 +1683,9 @@ pub enum Event {
},
/// Used to indicate that a splice negotiation round for the given `channel_id` has failed.
///
/// Each splice attempt (initial or RBF) resolves to either [`Event::SpliceNegotiated`] on
/// success or this event on failure. Prior successfully negotiated splice transactions are
/// unaffected.
/// Each splice attempt (initial or RBF) resolves to this event on failure. On success,
/// [`Event::SpliceNegotiated`] is emitted if the negotiated transaction includes local
/// inputs or outputs. Prior successfully negotiated splice transactions are unaffected.
///
/// Any UTXOs contributed to the failed round that are not committed to a prior negotiated
/// splice transaction will be returned via a preceding [`Event::DiscardFunding`].
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/ln/async_signer_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1853,7 +1853,7 @@ fn test_async_splice_initial_commit_sig() {
acceptor.node.handle_tx_signatures(initiator_node_id, &tx_signatures);

let _ = get_event!(initiator, Event::SpliceNegotiated);
let _ = get_event!(acceptor, Event::SpliceNegotiated);
assert!(acceptor.node.get_and_clear_pending_events().is_empty());
}

#[test]
Expand Down Expand Up @@ -1945,7 +1945,7 @@ fn test_async_splice_initial_commit_sig_waits_for_monitor_before_tx_signatures()
acceptor.node.handle_tx_signatures(initiator_node_id, &tx_signatures);

let _ = get_event!(initiator, Event::SpliceNegotiated);
let _ = get_event!(acceptor, Event::SpliceNegotiated);
assert!(acceptor.node.get_and_clear_pending_events().is_empty());
}

#[test]
Expand Down Expand Up @@ -2022,5 +2022,5 @@ fn test_async_splice_shared_input_signature_released_on_unblock() {
acceptor.node.handle_tx_signatures(initiator_node_id, &tx_signatures);

let _ = get_event!(initiator, Event::SpliceNegotiated);
let _ = get_event!(acceptor, Event::SpliceNegotiated);
assert!(acceptor.node.get_and_clear_pending_events().is_empty());
}
152 changes: 68 additions & 84 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7182,6 +7182,9 @@ pub struct SpliceFundingNegotiated {
/// The outpoint of the channel's splice funding transaction.
pub funding_txo: bitcoin::OutPoint,

/// Whether the holder contributed local inputs or outputs to the negotiated splice.
pub has_local_contribution: bool,

Comment on lines +7185 to +7187

@jkczyz jkczyz Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we simply return None instead of Some(SpliceFundingNegotiated{ .. })?

@wpaulino wpaulino Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No because there's a case where we rely on SpliceFundingNegotiated being set to free the holding cell after quiescence terminates.

/// The features that this channel will operate with.
pub channel_type: ChannelTypeFeatures,

Expand Down Expand Up @@ -7221,8 +7224,7 @@ impl SpliceFundingFailed {
}

macro_rules! splice_funding_failed_for {
($self: expr, $is_initiator: expr, $contribution: expr,
$contributed_inputs: ident, $contributed_outputs: ident) => {{
($self: expr, $contribution: expr, $contributed_inputs: ident, $contributed_outputs: ident) => {{
let contribution = $contribution;
let existing_inputs =
$self.pending_splice.as_ref().into_iter().flat_map(|ps| ps.$contributed_inputs());
Expand All @@ -7231,17 +7233,16 @@ macro_rules! splice_funding_failed_for {
let filtered =
contribution.clone().into_unique_contributions(existing_inputs, existing_outputs);
match filtered {
None if !$is_initiator => None,
None => Some(SpliceFundingFailed {
None => SpliceFundingFailed {
contributed_inputs: vec![],
contributed_outputs: vec![],
contribution: Some(contribution),
}),
Some((contributed_inputs, contributed_outputs)) => Some(SpliceFundingFailed {
},
Some((contributed_inputs, contributed_outputs)) => SpliceFundingFailed {
contributed_inputs,
contributed_outputs,
contribution: Some(contribution),
}),
},
}
}};
}
Expand Down Expand Up @@ -7274,14 +7275,7 @@ where
fn splice_funding_failed_for(&self, contribution: FundingContribution) -> SpliceFundingFailed {
// The contribution was never pushed to `contributions`, so `contributed_inputs()` and
// `contributed_outputs()` return only prior rounds' entries for filtering.
splice_funding_failed_for!(
self,
true,
contribution,
contributed_inputs,
contributed_outputs
)
.expect("is_initiator is true so this always returns Some")
splice_funding_failed_for!(self, contribution, contributed_inputs, contributed_outputs)
}

fn abandon_quiescent_action(&mut self) -> Option<SpliceFundingFailed> {
Expand Down Expand Up @@ -7423,11 +7417,7 @@ where
pending_splice.funding_negotiation.is_some(),
"reset_pending_splice_state requires an active funding negotiation"
);
let is_initiator = pending_splice
.funding_negotiation
.take()
.map(|negotiation| negotiation.is_initiator())
.unwrap_or(false);
pending_splice.funding_negotiation.take();
let contribution = pending_splice.contributions.pop();
if let Some(ref contribution) = contribution {
debug_assert!(
Expand All @@ -7441,14 +7431,8 @@ where

// After pop, `contributed_inputs()` / `contributed_outputs()` return only prior
// rounds for filtering.
let splice_funding_failed = contribution.and_then(|contribution| {
splice_funding_failed_for!(
self,
is_initiator,
contribution,
contributed_inputs,
contributed_outputs
)
let splice_funding_failed = contribution.map(|contribution| {
splice_funding_failed_for!(self, contribution, contributed_inputs, contributed_outputs)
});

if self.pending_funding().is_empty() {
Expand All @@ -7473,19 +7457,13 @@ where
pending_splice.funding_negotiation.is_some(),
"maybe_splice_funding_failed requires an active funding negotiation"
);
let is_initiator = pending_splice
.funding_negotiation
.as_ref()
.map(|negotiation| negotiation.is_initiator())
.unwrap_or(false);
let contribution = pending_splice.contributions.last().cloned()?;
splice_funding_failed_for!(
Some(splice_funding_failed_for!(
self,
is_initiator,
contribution,
prior_contributed_inputs,
prior_contributed_outputs
)
))
}

#[rustfmt::skip]
Expand Down Expand Up @@ -9545,11 +9523,18 @@ where
funding.get_funding_txo().expect("funding outpoint should be set");
let channel_type = funding.get_channel_type().clone();
let funding_redeem_script = funding.get_funding_redeemscript();
let has_local_contribution = self
.context
.interactive_tx_signing_session
.as_ref()
.map(|signing_session| signing_session.has_local_contribution())
.unwrap_or(false);

pending_splice.negotiated_candidates.push(funding);

let splice_negotiated = SpliceFundingNegotiated {
funding_txo: funding_txo.into_bitcoin_outpoint(),
has_local_contribution,
channel_type,
funding_redeem_script,
};
Expand Down Expand Up @@ -13148,11 +13133,13 @@ where

/// Checks during handling splice_init
pub fn validate_splice_init(&self, msg: &msgs::SpliceInit) -> Result<(), ChannelError> {
if self.holder_commitment_point.current_point().is_none() {
return Err(ChannelError::WarnAndDisconnect(format!(
"Channel {} commitment point needs to be advanced once before spliced",
self.context.channel_id(),
)));
Comment on lines -13136 to -13140

@jkczyz jkczyz Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't this performed early because it indicates the channel was created before we persisted the current commitment point? (a7ba4dd) Any specific reason to move it later?

@wpaulino wpaulino Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why the ordering should matter as long as we're still failing because of it. I just wanted to keep the WarnAndDisconnect paths together, and since we can use tx_abort to fail this specific case, we must be quiescent first to do so.

// - If it has received shutdown:
// MUST send a warning and close the connection or send an error
// and fail the channel.
if !self.context.is_live() {
return Err(ChannelError::WarnAndDisconnect(
"Splicing requested on a channel that is not live".to_owned(),
));
}

if !self.context.channel_state.is_quiescent() {
Expand All @@ -13167,15 +13154,6 @@ where
)));
}

// - If it has received shutdown:
// MUST send a warning and close the connection or send an error
// and fail the channel.
if !self.context.is_live() {
return Err(ChannelError::WarnAndDisconnect(
"Splicing requested on a channel that is not live".to_owned(),
));
}

let their_funding_contribution = SignedAmount::from_sat(msg.funding_contribution_satoshis);
if their_funding_contribution == SignedAmount::ZERO {
return Err(ChannelError::WarnAndDisconnect(format!(
Expand All @@ -13184,6 +13162,12 @@ where
)));
}

if self.holder_commitment_point.current_point().is_none() {
return Err(ChannelError::Abort(AbortReason::InternalError(
"Commitment point needs to be advanced once before spliced".into(),
)));
}

Ok(())
}

Expand All @@ -13200,13 +13184,10 @@ where
counterparty_funding_pubkey,
our_new_holder_keys,
min_funding_satoshis,
)
.map_err(|e| format!("Channel {} cannot be spliced; {}", self.context.channel_id(), e))?;
)?;

let (post_splice_holder_balance, post_splice_counterparty_balance) =
self.get_holder_counterparty_balances_floor_incl_fee(&candidate_scope).map_err(
|e| format!("Channel {} cannot be spliced; {}", self.context.channel_id(), e),
)?;
self.get_holder_counterparty_balances_floor_incl_fee(&candidate_scope)?;

let holder_selected_channel_reserve =
Amount::from_sat(candidate_scope.holder_selected_channel_reserve_satoshis);
Expand All @@ -13216,25 +13197,23 @@ where

// We allow parties to draw from their previous reserve, as long as they satisfy their v2 reserve
if our_funding_contribution != SignedAmount::ZERO {
post_splice_holder_balance.checked_sub(counterparty_selected_channel_reserve)
.ok_or(format!(
"Channel {} cannot be {}; our post-splice channel balance {} is smaller than their selected v2 reserve {}",
self.context.channel_id(),
if our_funding_contribution.is_positive() { "spliced in" } else { "spliced out" },
post_splice_holder_balance,
counterparty_selected_channel_reserve,
))?;
post_splice_holder_balance.checked_sub(counterparty_selected_channel_reserve).ok_or(
format!(
"Our post-splice channel balance {} is smaller than their selected v2 reserve {}",
post_splice_holder_balance,
counterparty_selected_channel_reserve,
),
)?;
}

if their_funding_contribution != SignedAmount::ZERO {
post_splice_counterparty_balance.checked_sub(holder_selected_channel_reserve)
.ok_or(format!(
"Channel {} cannot be {}; their post-splice channel balance {} is smaller than our selected v2 reserve {}",
self.context.channel_id(),
if their_funding_contribution.is_positive() { "spliced in" } else { "spliced out" },
post_splice_counterparty_balance,
holder_selected_channel_reserve,
))?;
post_splice_counterparty_balance.checked_sub(holder_selected_channel_reserve).ok_or(
format!(
"Their post-splice channel balance {} is smaller than our selected v2 reserve {}",
post_splice_counterparty_balance,
holder_selected_channel_reserve,
),
)?;
}

#[cfg(debug_assertions)]
Expand Down Expand Up @@ -13345,7 +13324,11 @@ where
holder_pubkeys,
min_funding_satoshis,
)
.map_err(|e| self.quiescent_negotiation_err(ChannelError::WarnAndDisconnect(e)))?;
.map_err(|e| {
self.quiescent_negotiation_err(ChannelError::Abort(
AbortReason::InvalidContribution(e),
))
})?;

// Adjust for the feerate and clone so we can store it for future RBF re-use.
let (adjusted_contribution, our_funding_inputs, our_funding_outputs) =
Expand Down Expand Up @@ -13404,17 +13387,16 @@ where
fn validate_tx_init_rbf<F: FeeEstimator>(
&self, msg: &msgs::TxInitRbf, fee_estimator: &LowerBoundedFeeEstimator<F>,
) -> Result<(ChannelPublicKeys, PublicKey), ChannelError> {
if self.holder_commitment_point.current_point().is_none() {
return Err(ChannelError::WarnAndDisconnect(format!(
"Channel {} commitment point needs to be advanced once before RBF",
self.context.channel_id(),
)));
}
Comment thread
wpaulino marked this conversation as resolved.

if !self.context.channel_state.is_quiescent() {
return Err(ChannelError::WarnAndDisconnect("Quiescence needed for RBF".to_owned()));
}

if self.holder_commitment_point.current_point().is_none() {
return Err(ChannelError::Abort(AbortReason::InternalError(
"Commitment point needs to be advanced once before RBF".into(),
)));
}

self.is_rbf_compatible().map_err(|msg| ChannelError::WarnAndDisconnect(msg))?;

let pending_splice = match &self.pending_splice {
Expand Down Expand Up @@ -13528,7 +13510,11 @@ where
holder_pubkeys,
min_funding_satoshis,
)
.map_err(|e| self.quiescent_negotiation_err(ChannelError::WarnAndDisconnect(e)))?;
.map_err(|e| {
self.quiescent_negotiation_err(ChannelError::Abort(
AbortReason::InvalidContribution(e),
))
})?;

// Consume the appropriate contribution source.
let (our_funding_inputs, our_funding_outputs) = if queued_net_value.is_some() {
Expand Down Expand Up @@ -13628,7 +13614,7 @@ where
holder_pubkeys,
min_funding_satoshis,
)
.map_err(|e| ChannelError::WarnAndDisconnect(e))?;
.map_err(|e| ChannelError::Abort(AbortReason::InvalidContribution(e)))?;

Ok(new_funding)
}
Expand Down Expand Up @@ -13705,8 +13691,6 @@ where
fn validate_splice_ack(
&self, msg: &msgs::SpliceAck, min_funding_satoshis: u64,
) -> Result<FundingScope, ChannelError> {
// TODO(splicing): Add check that we are the splice (quiescence) initiator

let pending_splice = self
.pending_splice
.as_ref()
Expand All @@ -13729,7 +13713,7 @@ where
new_keys,
min_funding_satoshis,
)
.map_err(|e| ChannelError::WarnAndDisconnect(e))?;
.map_err(|e| ChannelError::Abort(AbortReason::InvalidContribution(e)))?;

Ok(new_funding)
}
Expand Down
Loading