-
Notifications
You must be signed in to change notification settings - Fork 421
Emit SplicePending
and SpliceFailed
events
#4077
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 @TheBlueMatt as a reviewer! |
Looking for high-level feedback on the approach and if all scenarios were covered. Specifically, is |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4077 +/- ##
==========================================
+ Coverage 88.73% 88.77% +0.03%
==========================================
Files 180 180
Lines 135622 136350 +728
Branches 135622 136350 +728
==========================================
+ Hits 120346 121045 +699
+ Misses 12505 12502 -3
- Partials 2771 2803 +32
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:
|
0db24f3
to
41c1e60
Compare
lightning/src/ln/channel.rs
Outdated
let splice_funding = self.validate_splice_ack(msg)?; | ||
let splice_funding = self.validate_splice_ack(msg).map_err(|err| { | ||
let splice_failed = SpliceFundingFailed { | ||
channel_id: self.context.channel_id, | ||
counterparty_node_id: self.context.counterparty_node_id, | ||
user_channel_id: self.context.user_id, | ||
funding_txo: None, | ||
channel_type: None, | ||
contributed_inputs: Vec::new(), | ||
contributed_outputs: Vec::new(), | ||
}; | ||
(err, splice_failed) | ||
})?; |
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 commit will likely need to be re-worked to have validate_splice_ack
return Option<SpliceFundingFailed>
as part of its error. But it seems we never clear pending_splice.funding_negotiation
here.
@wpaulino Should we? I'd imagine if we ever emit Event::SpliceFailed
then pending_splice.funding_negotiation
should no longer be set.
lightning/src/ln/channel.rs
Outdated
ChannelError::WarnAndDisconnect(format!( | ||
let splice_failed = SpliceFundingFailed { | ||
channel_id: self.context.channel_id, | ||
counterparty_node_id: self.context.counterparty_node_id, | ||
user_channel_id: self.context.user_id, | ||
funding_txo: splice_funding.get_funding_txo().map(|txo| txo.into_bitcoin_outpoint()), | ||
channel_type: Some(splice_funding.get_channel_type().clone()), | ||
contributed_inputs: Vec::new(), | ||
contributed_outputs: Vec::new(), | ||
}; | ||
let channel_error = ChannelError::WarnAndDisconnect(format!( | ||
"Failed to start interactive transaction construction, {:?}", | ||
err | ||
)) | ||
)); | ||
(channel_error, splice_failed) |
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.
Likewise here.
|
||
pub(super) struct InteractiveTxConstructor { | ||
state_machine: StateMachine, | ||
is_initiator: bool, |
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.
Might be able to get this from the StateMachine
, but not if it transitioned to NegotiationAborted
, it seems.
lightning/src/ln/channel.rs
Outdated
contributed_inputs: Vec::new(), | ||
contributed_outputs: Vec::new(), |
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.
One issue with populating these is if we are in FundingNegotiation::AwaitingSignatures
, then we'd need to get them from ChannelContext::interactive_tx_signing_session
, which may not have been set yet in FundedChannel::funding_tx_constructed
depending on the error.
lightning/src/ln/channel.rs
Outdated
let signing_session = interactive_tx_constructor.into_signing_session(); | ||
let commitment_signed = chan.context.funding_tx_constructed( | ||
let commitment_signed_result = chan.context.funding_tx_constructed( | ||
&mut funding, | ||
signing_session, | ||
true, | ||
chan.holder_commitment_point.next_transaction_number(), | ||
&&logger, | ||
)?; | ||
); | ||
|
||
// This must be set even if returning an Err. Otherwise, | ||
// fail_interactive_tx_negotiation won't produce a SpliceFailed event. | ||
pending_splice.funding_negotiation = | ||
Some(FundingNegotiation::AwaitingSignatures { funding }); | ||
|
||
return Ok(commitment_signed); | ||
return commitment_signed_result; |
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.
Here, specifically, we may not have set ChannelContext::interactive_tx_signing_session
for some errors.
🔔 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. |
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
🔔 2nd Reminder Hey @TheBlueMatt @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.
What's the status of this? IMO we should land it early so we can write tests and discover where we're missing coverage.
lightning/src/ln/channel.rs
Outdated
|
||
/// Information about a splice funding negotiation that has been completed. | ||
/// This is returned from channel operations and converted to an Event::SplicePending in ChannelManager. | ||
pub struct SpliceFundingNegotiated { |
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.
Meh, just return the Event
?
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.
Hmmm... our pattern has been to only create events in ChannelManager
. Though we can drop the first three fields and take them from the ChannelContext
in ChannelManager
as is done for other events.
lightning/src/ln/channel.rs
Outdated
/// Information about a splice funding negotiation that has been completed. | ||
/// This is returned from channel operations and converted to an Event::SplicePending in ChannelManager. | ||
pub struct SpliceFundingNegotiated { | ||
/// The channel_id of the channel being spliced. |
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.
Lol, these comments are great, they're exactly what I expect from an intern, they say absolutely nothing but meet the not-even-a-requirement for things having docs.
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.
Yeah... I'll take a pass and clean up Claude's work. Didn't really give it much prompting to be honest.
/// Input outpoints contributed to the splice transaction. | ||
contributed_inputs: Vec<OutPoint>, | ||
/// Outputs contributed to the splice transaction. | ||
contributed_outputs: Vec<TxOut>, |
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.
Indeed, I believe we discussed on the call last week, but we should include information about other splices still pending.
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.
Yeah, the idea was to only include inputs that can be unlocked. I need to update this still.
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.
Given we won't have RBF in the initial release, this doesn't need to be included yet.
This depends on #4097 now. |
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
41c1e60
to
a01b467
Compare
lightning/src/ln/channel.rs
Outdated
let splice_funding_failed = funding_negotiation_opt | ||
.filter(|funding_negotiation| funding_negotiation.is_initiator()) | ||
.map(|funding_negotiation| { | ||
// Create SpliceFundingFailed for the aborted splice | ||
let (funding_txo, channel_type) = match &funding_negotiation { | ||
FundingNegotiation::ConstructingTransaction { funding, .. } => { | ||
(funding.get_funding_txo().map(|txo| txo.into_bitcoin_outpoint()), Some(funding.get_channel_type().clone())) | ||
}, | ||
FundingNegotiation::AwaitingSignatures { funding, .. } => { | ||
(funding.get_funding_txo().map(|txo| txo.into_bitcoin_outpoint()), Some(funding.get_channel_type().clone())) | ||
}, | ||
FundingNegotiation::AwaitingAck { .. } => { | ||
(None, None) | ||
}, | ||
}; | ||
|
||
SpliceFundingFailed { | ||
channel_id: funded_channel.context.channel_id, | ||
counterparty_node_id: funded_channel.context.counterparty_node_id, | ||
user_channel_id: funded_channel.context.user_id, | ||
funding_txo, | ||
channel_type, | ||
contributed_inputs: Vec::new(), | ||
contributed_outputs: Vec::new(), | ||
} | ||
}); |
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.
Bug: In tx_abort handling for funded channels, when a splice negotiation is aborted during the ConstructingTransaction state, the contributed inputs and outputs are not extracted from the interactive_tx_constructor before creating the SpliceFundingFailed event. The code sets contributed_inputs and contributed_outputs to empty vectors, but if the local node had already contributed inputs/outputs to the transaction construction, this information should be preserved so users know which UTXOs they can reuse. The interactive_tx_constructor contains inputs_to_contribute and outputs_to_contribute fields that should be extracted before the constructor is dropped.
let splice_funding_failed = funding_negotiation_opt | |
.filter(|funding_negotiation| funding_negotiation.is_initiator()) | |
.map(|funding_negotiation| { | |
// Create SpliceFundingFailed for the aborted splice | |
let (funding_txo, channel_type) = match &funding_negotiation { | |
FundingNegotiation::ConstructingTransaction { funding, .. } => { | |
(funding.get_funding_txo().map(|txo| txo.into_bitcoin_outpoint()), Some(funding.get_channel_type().clone())) | |
}, | |
FundingNegotiation::AwaitingSignatures { funding, .. } => { | |
(funding.get_funding_txo().map(|txo| txo.into_bitcoin_outpoint()), Some(funding.get_channel_type().clone())) | |
}, | |
FundingNegotiation::AwaitingAck { .. } => { | |
(None, None) | |
}, | |
}; | |
SpliceFundingFailed { | |
channel_id: funded_channel.context.channel_id, | |
counterparty_node_id: funded_channel.context.counterparty_node_id, | |
user_channel_id: funded_channel.context.user_id, | |
funding_txo, | |
channel_type, | |
contributed_inputs: Vec::new(), | |
contributed_outputs: Vec::new(), | |
} | |
}); | |
let splice_funding_failed = funding_negotiation_opt | |
.filter(|funding_negotiation| funding_negotiation.is_initiator()) | |
.map(|funding_negotiation| { | |
// Create SpliceFundingFailed for the aborted splice | |
let (funding_txo, channel_type, contributed_inputs, contributed_outputs) = match &funding_negotiation { | |
FundingNegotiation::ConstructingTransaction { funding, interactive_tx_constructor, .. } => { | |
(funding.get_funding_txo().map(|txo| txo.into_bitcoin_outpoint()), | |
Some(funding.get_channel_type().clone()), | |
interactive_tx_constructor.inputs_to_contribute.clone(), | |
interactive_tx_constructor.outputs_to_contribute.clone()) | |
}, | |
FundingNegotiation::AwaitingSignatures { funding, .. } => { | |
(funding.get_funding_txo().map(|txo| txo.into_bitcoin_outpoint()), | |
Some(funding.get_channel_type().clone()), | |
Vec::new(), | |
Vec::new()) | |
}, | |
FundingNegotiation::AwaitingAck { .. } => { | |
(None, None, Vec::new(), Vec::new()) | |
}, | |
}; | |
SpliceFundingFailed { | |
channel_id: funded_channel.context.channel_id, | |
counterparty_node_id: funded_channel.context.counterparty_node_id, | |
user_channel_id: funded_channel.context.user_id, | |
funding_txo, | |
channel_type, | |
contributed_inputs, | |
contributed_outputs, | |
} | |
}); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
I pushed four new commits immediately after the |
Specifically, see the |
8ff58b9
to
3a08a0e
Compare
Opened #4123 based on some offline discussion, which this PR is now based on. It includes |
3a08a0e
to
69a4748
Compare
1165217
to
9842c35
Compare
if let Some(splice_funding_failed) = splice_funding_failed { | ||
let pending_events = &mut self.pending_events.lock().unwrap(); | ||
pending_events.push_back((events::Event::SpliceFailed { | ||
channel_id: msg.channel_id, | ||
counterparty_node_id, | ||
user_channel_id: chan.context().get_user_id(), | ||
funding_txo: splice_funding_failed.funding_txo, | ||
channel_type: splice_funding_failed.channel_type.clone(), | ||
contributed_inputs: splice_funding_failed.contributed_inputs, | ||
contributed_outputs: splice_funding_failed.contributed_outputs, | ||
}, None)); | ||
} |
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.
FYI, added a fixup to persist in this case.
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 some test coverage.
&[ExpectedCloseEvent { | ||
channel_id: Some(channel_id), | ||
discard_funding: false, | ||
splice_failed: true, |
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.
Here and below, not sure if there's a better way to test for this. Problem was having a SpliceFailed
event show up with a ChannelClosed
event.
let event = get_event!(initiator, Event::SpliceFailed); | ||
match event { | ||
Event::SpliceFailed { contributed_inputs, .. } => { | ||
assert_eq!(contributed_inputs.len(), 1); | ||
assert_eq!(contributed_inputs[0], contribution.inputs()[0].outpoint()); | ||
}, | ||
_ => panic!("Expected Event::SpliceFailed"), | ||
} |
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'm only doing some minimal checks here, so let me know if you want anything else covered.
lightning/src/ln/channel.rs
Outdated
FundingNegotiation::ConstructingTransaction { | ||
interactive_tx_constructor, | ||
.. | ||
} => interactive_tx_constructor.into_contributed_inputs_and_outputs(), |
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.
FYI, I think the SpliceFailed
tests may only be covering this case.
lightning/src/ln/channel.rs
Outdated
self.quiescent_action.take().and_then(|quiescent_action| match quiescent_action { | ||
QuiescentAction::Splice(instructions) => { | ||
self.context.channel_state.clear_awaiting_quiescence(); | ||
let (inputs, outputs) = instructions.into_contributed_inputs_and_outputs(); | ||
Some(SpliceFundingFailed { | ||
funding_txo: None, | ||
channel_type: None, | ||
contributed_inputs: inputs, | ||
contributed_outputs: outputs, | ||
}) | ||
}, | ||
#[cfg(any(test, fuzzing))] | ||
_ => { | ||
self.quiescent_action = Some(quiescent_action); | ||
None | ||
}, | ||
}) |
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.
The linter doesn't seem to like this. Says I should use map
instead of and_then
, but that won't work for the catch-all arm. May need to use if let
to get around it.
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.
Replaced the and_then
with a match
on the take
n Option<QuiescentAction>
.
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.
Can we get a test of splice failure on disconnect/reload?
Isn't that already covered by |
Yeah, though I realized that we aren't handling the reload case. Since we only persist |
9842c35
to
c2c6fdf
Compare
As discussed offline, ended up persisting |
bd15e74
to
3ad8b15
Compare
Once a splice has been negotiated and its funding transaction has been broadcast, it is considered pending until both parties have seen enough confirmations to consider the funding locked. Add an event used to indicate this.
Once a splice has been negotiated and its funding transaction has been broadcast, emit a SplicePending event. Once this occurs, the inputs contributed to the splice cannot be reused except by an RBF attempt.
Once a splice has been successfully initiated, but prior to signing any negotiated funding transaction, it may fail. Add an event used to indicate this and which UTXOs can be reused.
When interactive transaction construction fails during splice funding negotiation, emit Event::SpliceFailed to notify users of the failure such that they can reclaim any contributed UTXOs.
When a tx_abort message is successfully processed for a funded channel with an active splice negotiation, emit Event::SpliceFailed to notify users that the splice operation was aborted by the counterparty.
When a channel has a pending splice and is shutdown, generate a SpliceFailed event when necessary. This allows users to reclaim any contributed UTXOs.
Since quiescence is terminated upon disconnection, any outstanding splice negotiation should result in emitting a SpliceFailed event as long as we haven't reached FundingNegotiation::AwaitingSignatures. This may occur if we explicitly disconnect the peer (e.g., when failing to process splice_ack) or if the connection is lost.
When ChannelMangager::splice_channel returns Ok, a QuiescentAction is set. Since this is persisted with a FundedChannel, an Ok result should trigger persistence.
3ad8b15
to
f51a24b
Compare
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
let res = self.internal_tx_abort(&counterparty_node_id, msg); | ||
let persist = match &res { | ||
Err(e) if e.closes_channel() => NotifyOption::DoPersist, | ||
Err(_) => NotifyOption::SkipPersistHandleEvents, |
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 looks wrong. We should persist as a result of generating the new event.
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.
Here the event is emitted for the Ok
case. Either:
(1) we already sent our own tx_abort
because we had a ChannelError::Abort
earlier and they are responding with a tx_abort
as per the protocol, or
(2) they are sending the initial tx_abort
, so we are responding in kind.
For (2) we could return ChannelError::Abort
instead, I suppose. But it's not really an error. Doing so would be as a way to re-use that code path.
lightning/src/ln/channel.rs
Outdated
return None; | ||
} | ||
|
||
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.
Care to DRY this up with the copy in reset_pending_splice_state
?
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... FWIW, I think we'd need to use a macro. That one take
s the FundingNegotiation
and uses into_
methods (consuming self
) while this one leaves the FundingNegotiation
and uses to_
methods (making copies). I guess we could simply clone
it and use the into_
methods. But most of the contained data doesn't implement Clone
and isn't actually needed.
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.
Pushed the macro approach since it turned out to be straightforward.
lightning/src/ln/channelmanager.rs
Outdated
let peer_state = &mut *peer_state_lock; | ||
for chan in peer_state.channel_by_id.values().filter_map(Channel::as_funded) { | ||
if let Some(splice_funding_failed) = chan.maybe_splice_funding_failed() { | ||
self.pending_events.lock().unwrap().push_back(( |
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.
Why push this? We already have a loop a few lines down that iterates over the pending events and writes them one at a time, just put this after that loop.
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.
It's also written as a TLV later.
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.
Ugh, still so awkward. If we're gonna push it at least keep a single lock over pending_events
the whole time so its not unlocked with the bogs event in between.
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.
That's a lock-order violation. We could alloc a Vec
and extend
the existing one, but I assumed we wanted to avoid that.
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.
The peer_states
Vec
that is built a few lines down to hold all the per-peer locks can be moved up as well to address that, I think.
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.
Ah, yeah. My earlier attempt was retaking the lock, which is why I had moved this code above it. But I can just use peer_states.iter()
to avoid that. Pushed a new version that does this.
This also made me realize we can actually use Iterable
since the mutex problem mentioned in #4077 (comment) doesn't exist when using peer_states.iter()
. However, we'd need a second TLV because we can't chain
and Iterator
returning references with one returning values, IIUC.
I attempted this but ran into tests failing with ShortRead
errors. Haven't quite debugged it, but I pushed to a separate branch in case it's worth considering the apporach: jkczyz@cfeae18. There's some extra copies made to do the count unfortunately.
f51a24b
to
bf5f224
Compare
Similarly to when a peer is disconnected, when a node is reloaded any splice that hasn't reaching FundingNegotiation::AwaitingSignatures will be reset. This should produce a SpliceFailed event. However, since other FundingNegotiation variants are not persisted, the data to produced the SpliceFailed event upon reload is lost. Therefore, opportunistically persist a SpliceFailed event for these cases such that it is available upon reload.
Update the ChannelManager::splice_channel docs to reflect the current state of the implementation and splicing-related events.
bf5f224
to
957b742
Compare
When a splice has been negotiated, it remains pending until the funding transaction has been confirmed and locked by both sides. Emit an
Event::SplicePending
when it reaches this state. At this point, the inputs used for the splice cannot be reused except for an RBF attempt. Once the splice is locked, anEvent::DiscardFunding
will be emitted for any unsuccessful candidates.Similarly, a splice may fail before a splice has finished negotiation for various reasons. Emit an
Event::SpliceFailed
in these cases so the user may reuse the inputs.