Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 480
Include failure context in splice events#4514
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
f0a8ceb450eb6414c6819cc7fb0f2eb939b9c6cca6db45c83367ffaab44076effbb8fe51e6079File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -25,6 +25,7 @@ use crate::blinded_path::payment::{ | ||
| use crate::chain::transaction; | ||
| use crate::ln::channel::FUNDING_CONF_DEADLINE_BLOCKS; | ||
| use crate::ln::channelmanager::{InterceptId, PaymentId}; | ||
| use crate::ln::funding::FundingContribution; | ||
| use crate::ln::msgs; | ||
| use crate::ln::onion_utils::LocalHTLCFailureReason; | ||
| use crate::ln::outbound_payment::RecipientOnionFields; | ||
| @@ -99,6 +100,119 @@ impl_writeable_tlv_based_enum!(FundingInfo, | ||
| } | ||
| ); | ||
| /// The reason a funding negotiation round failed. | ||
| /// | ||
| /// Each negotiation attempt (initial or RBF) resolves to either success or failure. This enum | ||
| /// indicates what caused the failure. Use [`is_retriable`] to determine whether the splice can | ||
| /// be reattempted on this channel by calling [`ChannelManager::splice_channel`]. | ||
| /// | ||
| /// [`is_retriable`]: Self::is_retriable | ||
| /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub enum NegotiationFailureReason { | ||
| /// The reason was not available (e.g., from an older serialization). | ||
| Unknown, | ||
| /// The peer disconnected during negotiation. Wait for the peer to reconnect, then retry. | ||
| PeerDisconnected, | ||
| /// The counterparty explicitly aborted the negotiation by sending `tx_abort`. Retrying with | ||
| /// the same parameters is unlikely to succeed — consider adjusting the contribution or | ||
| /// waiting for the counterparty to initiate. | ||
| CounterpartyAborted { | ||
| /// The counterparty's abort message. | ||
| /// | ||
| /// This is counterparty-provided data. Use `Display` on [`UntrustedString`] for safe | ||
| /// logging. | ||
| msg: UntrustedString, | ||
| }, | ||
| /// An error occurred during interactive transaction negotiation (e.g., the counterparty sent | ||
| /// an invalid message). The negotiation was aborted. | ||
| NegotiationError { | ||
| /// A developer-readable error message. | ||
| msg: String, | ||
| }, | ||
| /// The funding contribution was invalid (e.g., insufficient balance for the splice amount). | ||
| /// Call [`ChannelManager::splice_channel`] for a fresh [`FundingTemplate`] and build a new | ||
| /// contribution with adjusted parameters. | ||
| /// | ||
| /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel | ||
| /// [`FundingTemplate`]: crate::ln::funding::FundingTemplate | ||
| ContributionInvalid, | ||
| /// The negotiation was locally abandoned via `ChannelManager::abandon_splice`. | ||
| LocallyAbandoned, | ||
| /// The channel is closing, so the negotiation cannot continue. See [`Event::ChannelClosed`] | ||
| /// for the closure reason. | ||
| ChannelClosing, | ||
| /// The contribution's feerate was too low for RBF. Call [`ChannelManager::splice_channel`] | ||
| /// for a fresh [`FundingTemplate`] (which includes the updated minimum feerate) and build a | ||
| /// new contribution with a higher feerate. | ||
| /// | ||
| /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel | ||
| /// [`FundingTemplate`]: crate::ln::funding::FundingTemplate | ||
| FeeRateTooLow, | ||
| /// An RBF attempt could not be initiated (e.g., a prior splice transaction already | ||
| /// confirmed). The channel remains operational — start a new splice with | ||
| /// [`ChannelManager::splice_channel`] if further changes are needed. | ||
| /// | ||
| /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel | ||
| CannotInitiateRbf, | ||
| } | ||
| impl NegotiationFailureReason { | ||
| /// Whether the splice negotiation is likely to succeed if retried on this channel. When `true`, | ||
| /// call [`ChannelManager::splice_channel`] to obtain a fresh [`FundingTemplate`] and retry. | ||
| /// | ||
| /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel | ||
| /// [`FundingTemplate`]: crate::ln::funding::FundingTemplate | ||
| pub fn is_retriable(&self) -> bool { | ||
| match self { | ||
| Self::Unknown | ||
| | Self::PeerDisconnected | ||
| | Self::ContributionInvalid | ||
| | Self::FeeRateTooLow => true, | ||
| Self::CounterpartyAborted { .. } | ||
| | Self::NegotiationError { .. } | ||
| | Self::LocallyAbandoned | ||
| | Self::ChannelClosing | ||
| | Self::CannotInitiateRbf => false, | ||
| } | ||
| } | ||
| } | ||
| impl core::fmt::Display for NegotiationFailureReason { | ||
| fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
| match self { | ||
| Self::Unknown => f.write_str("unknown reason"), | ||
| Self::PeerDisconnected => f.write_str("peer disconnected during negotiation"), | ||
| Self::CounterpartyAborted { msg } => { | ||
| write!(f, "counterparty aborted: {}", msg) | ||
| }, | ||
| Self::NegotiationError { msg } => write!(f, "negotiation error: {}", msg), | ||
| Self::ContributionInvalid => f.write_str("funding contribution was invalid"), | ||
| Self::LocallyAbandoned => f.write_str("splice locally abandoned"), | ||
| Self::ChannelClosing => f.write_str("channel is closing"), | ||
| Self::FeeRateTooLow => f.write_str("feerate too low for RBF"), | ||
| Self::CannotInitiateRbf => f.write_str("cannot initiate RBF"), | ||
| } | ||
| } | ||
| } | ||
| impl_writeable_tlv_based_enum_upgradable!(NegotiationFailureReason, | ||
| (1, Unknown) => {}, | ||
| (3, PeerDisconnected) => {}, | ||
| (5, CounterpartyAborted) => { | ||
| (1, msg, required), | ||
| }, | ||
| (7, NegotiationError) => { | ||
| (1, msg, required), | ||
| }, | ||
| (9, ContributionInvalid) => {}, | ||
| (11, LocallyAbandoned) => {}, | ||
| (13, ChannelClosing) => {}, | ||
| (15, FeeRateTooLow) => {}, | ||
| (17, CannotInitiateRbf) => {}, | ||
| ); | ||
| /// Some information provided on receipt of payment depends on whether the payment received is a | ||
| /// spontaneous payment or a "conventional" lightning payment that's paying an invoice. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| @@ -1541,8 +1655,8 @@ pub enum Event { | ||
| /// # Failure Behavior and Persistence | ||
| /// This event will eventually be replayed after failures-to-handle (i.e., the event handler | ||
| /// returning `Err(ReplayEvent ())`) and will be persisted across restarts. | ||
| SplicePending { | ||
| /// The `channel_id` of the channel that has a pending splice funding transaction. | ||
| SpliceNegotiated { | ||
| /// The `channel_id` of the channel with the negotiated splice funding transaction. | ||
| channel_id: ChannelId, | ||
| /// The `user_channel_id` value passed in to [`ChannelManager::create_channel`] for outbound | ||
| /// channels, or to [`ChannelManager::accept_inbound_channel`] for inbound channels. | ||
| @@ -1560,19 +1674,20 @@ pub enum Event { | ||
| /// The witness script that is used to lock the channel's funding output to commitment transactions. | ||
| new_funding_redeem_script: ScriptBuf, | ||
| }, | ||
| /// Used to indicate that a splice for the given `channel_id` has failed. | ||
| /// Used to indicate that a splice negotiation round for the given `channel_id` has failed. | ||
| /// | ||
| /// This event may be emitted if a splice fails after it has been initiated but prior to signing | ||
| /// any negotiated funding transaction. | ||
| /// 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. | ||
| /// | ||
| /// Any UTXOs contributed to be spent by the funding transaction may be reused and will be | ||
| /// given in `contributed_inputs`. | ||
| /// 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`]. | ||
| /// | ||
| /// # Failure Behavior and Persistence | ||
| /// This event will eventually be replayed after failures-to-handle (i.e., the event handler | ||
| /// returning `Err(ReplayEvent ())`) and will be persisted across restarts. | ||
| SpliceFailed { | ||
| /// The `channel_id` of the channel for which the splice failed. | ||
| SpliceNegotiationFailed { | ||
| /// The `channel_id` of the channel for which the splice negotiation round failed. | ||
| channel_id: ChannelId, | ||
| /// The `user_channel_id` value passed in to [`ChannelManager::create_channel`] for outbound | ||
| /// channels, or to [`ChannelManager::accept_inbound_channel`] for inbound channels. | ||
| @@ -1582,10 +1697,17 @@ pub enum Event { | ||
| user_channel_id: u128, | ||
| /// The `node_id` of the channel counterparty. | ||
| counterparty_node_id: PublicKey, | ||
| /// The outpoint of the channel's splice funding transaction, if one was created. | ||
| abandoned_funding_txo: Option<OutPoint>, | ||
| /// The features that this channel will operate with, if available. | ||
| channel_type: Option<ChannelTypeFeatures>, | ||
Comment on lines
-1585
to
-1588
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't feel like there was much value to these.
| ||
| /// The reason the splice negotiation failed. | ||
| reason: NegotiationFailureReason, | ||
| /// The funding contribution from the failed negotiation round, if available. This can be | ||
| /// fed back to [`ChannelManager::funding_contributed`] to retry with the same parameters. | ||
| /// Alternatively, call [`ChannelManager::splice_channel`] to obtain a fresh | ||
| /// [`FundingTemplate`] and build a new contribution. | ||
| /// | ||
| /// [`ChannelManager::funding_contributed`]: crate::ln::channelmanager::ChannelManager::funding_contributed | ||
| /// [`ChannelManager::splice_channel`]: crate::ln::channelmanager::ChannelManager::splice_channel | ||
| /// [`FundingTemplate`]: crate::ln::funding::FundingTemplate | ||
| contribution: Option<FundingContribution>, | ||
| }, | ||
| /// Used to indicate to the user that they can abandon the funding transaction and recycle the | ||
| /// inputs for another purpose. | ||
| @@ -2355,7 +2477,7 @@ impl Writeable for Event { | ||
| // We never write out FundingTransactionReadyForSigning events as they will be regenerated when | ||
| // necessary. | ||
| }, | ||
| &Event::SplicePending { | ||
| &Event::SpliceNegotiated { | ||
| ref channel_id, | ||
| ref user_channel_id, | ||
| ref counterparty_node_id, | ||
| @@ -2373,20 +2495,20 @@ impl Writeable for Event { | ||
| (11, new_funding_redeem_script, required), | ||
| }); | ||
| }, | ||
| &Event::SpliceFailed { | ||
| &Event::SpliceNegotiationFailed { | ||
| ref channel_id, | ||
| ref user_channel_id, | ||
| ref counterparty_node_id, | ||
| ref abandoned_funding_txo, | ||
| ref channel_type, | ||
| ref reason, | ||
| ref contribution, | ||
| } => { | ||
| 52u8.write(writer)?; | ||
| write_tlv_fields!(writer, { | ||
| (1, channel_id, required), | ||
| (3, channel_type, option), | ||
| (5, user_channel_id, required), | ||
| (7, counterparty_node_id, required), | ||
| (9, abandoned_funding_txo, option), | ||
| (11, reason, required), | ||
| (13, contribution, option), | ||
| }); | ||
| }, | ||
| // Note that, going forward, all new events must only write data inside of | ||
| @@ -3012,7 +3134,7 @@ impl MaybeReadable for Event { | ||
| (11, new_funding_redeem_script, required), | ||
| }); | ||
| Ok(Some(Event::SplicePending { | ||
| Ok(Some(Event::SpliceNegotiated { | ||
| channel_id: channel_id.0.unwrap(), | ||
| user_channel_id: user_channel_id.0.unwrap(), | ||
| counterparty_node_id: counterparty_node_id.0.unwrap(), | ||
| @@ -3027,18 +3149,18 @@ impl MaybeReadable for Event { | ||
| let mut f = || { | ||
| _init_and_read_len_prefixed_tlv_fields!(reader, { | ||
| (1, channel_id, required), | ||
| (3, channel_type, option), | ||
| (5, user_channel_id, required), | ||
| (7, counterparty_node_id, required), | ||
| (9, abandoned_funding_txo, option), | ||
| (11, reason, upgradable_option), | ||
| (13, contribution, option), | ||
| }); | ||
| Ok(Some(Event::SpliceFailed { | ||
| Ok(Some(Event::SpliceNegotiationFailed { | ||
| channel_id: channel_id.0.unwrap(), | ||
| user_channel_id: user_channel_id.0.unwrap(), | ||
| counterparty_node_id: counterparty_node_id.0.unwrap(), | ||
| abandoned_funding_txo, | ||
| channel_type, | ||
| reason: reason.unwrap_or(NegotiationFailureReason::Unknown), | ||
| contribution, | ||
| })) | ||
| }; | ||
| f() | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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 does seem like some of these could use more details (error message string?). As-is given the recommendation for ~all enum variants is "retry by calling
splice_channelagain" its not really clear to me why we should bother with an enum - an enum is useful if the downstream code has different handling for different cases, but here that doesn't really seem likely/possible. If we can't expose more details that are useful for automated action, we could probably do with an enum with three variants and a developer-readable string.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 docs could be improved here. Maybe re-calling
splice_channelisn't always necessary? In some cases, callingfunding_contributedat a later time may be sufficient. We are providing theFundingContributionafter all.So it does seem like different actions are required depending on the variant.
PeerDisconnected: retry upon re-connectionChannelNotReady: retry when channel is in a usable state (assuming it's not closing)FeeRateTooLow: retry with a higher fee rateContributionInvalid: retry with a re-computed contributionNegotiationError: probably not retryable as it indicates a counterparty errorCounterpartyAborted: might be able to retry but would need to examine thetx_abortmessage (e.g., counterparty doesn't like your fee rate).We could have a top-level
Retryablethat holds one of these instead, if you prefer? We could go as far as only putting theFundingContributionin applicable sub-variants. Though a user may want to see it for other variants.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 assume
ChannelNotReadyis really "you can't retry" (why were you splicing an unopened channel anyway? but probably its closing),FeeRateTooLowandContributionInvalidare really "retry now with a freshsplice_channelcall as that'll tell you what the new required feerate is to do a fresh rbf from scracth or will fix your contribution, hopefully". So it does seem like its basically "retry with a freshsplice_channelcall, or not", with the one maybe-exception ofPeerDisconnected? We could leave the enum and include a function that returns "retryable or not", but I'm not really sure why we shouldn't just return a string+bool pair (in a struct) - do we anticipate downstream code will have any different behavior based on anything beyond "should I retry this" or is it just gonna log/print an error?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.
Probably not other than when / how should you retry in case of the
PeerDisconnected.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 suppose given we don't know when the peer will reconnect, we might as well just process it the same and call
splice_channelagain upon re-connection. Otherwise, we'd need to make sure not to process the associatedDiscardFundingwhich would mean locking up the UTXOs indefinitely. Instead, the client can simply always queue up a "retry when connected".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.
Discussed offline. Updated to have an
is_retryablemethod that the counterparty calls rather than examining the variant. Would prevent allocating strings in many cases.