-
Notifications
You must be signed in to change notification settings - Fork 145
Add splice RBF support #888
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?
Changes from all commits
3d8d042
9e6114b
f1a0a79
c8e0713
9a8a938
b8ca9c4
88be9ce
cf78007
328bdc3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1647,7 +1647,7 @@ impl Node { | |
| if funding_template.prior_contribution().is_some() { | ||
| log_error!( | ||
| self.logger, | ||
| "Failed to splice channel: a prior splice contribution is pending" | ||
| "Failed to splice channel: a prior splice contribution is pending; use bump_channel_funding_fee to bump its fee" | ||
| ); | ||
| return Err(Error::ChannelSplicingFailed); | ||
| } | ||
|
|
@@ -1770,7 +1770,7 @@ impl Node { | |
| if funding_template.prior_contribution().is_some() { | ||
| log_error!( | ||
| self.logger, | ||
| "Failed to splice channel: a prior splice contribution is pending" | ||
| "Failed to splice channel: a prior splice contribution is pending; use bump_channel_funding_fee to bump its fee" | ||
| ); | ||
| return Err(Error::ChannelSplicingFailed); | ||
|
jkczyz marked this conversation as resolved.
|
||
| } | ||
|
|
@@ -1807,6 +1807,73 @@ impl Node { | |
| } | ||
| } | ||
|
|
||
| /// Replace a pending splice's funding transaction with a higher-feerate version. | ||
| /// | ||
| /// If a prior splice negotiation is pending, this bumps its feerate via RBF. The prior | ||
| /// contribution is reused when possible; otherwise, coin selection is re-run. | ||
| /// | ||
| /// # Experimental API | ||
| /// | ||
| /// This API is experimental and may change in the future. | ||
|
Collaborator
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. Given we don't offer API stability in general, what does concretely mean beyond the API signature possibly changing going forward? If that's all that's already expected at this point, so maybe we can drop this note? |
||
| pub fn bump_channel_funding_fee( | ||
|
Contributor
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. Is the plan for ldk-node to eventually monitor pending splice candidates and automatically bump them? It seems a bit awkward to make users/applications notice that a pending splice is stuck and manually call this API.
Contributor
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. Wonder how this method can be used to bump the fee much higher and not only to the next step. Invoke repeatedly while they monitor what the current pending splice fee rate is?
Collaborator
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.
Hmm, maybe. We previously considered that for regular onchain payments but for now opted to keep it manual. But indeed some auto-bumping could make sense, though we'd of course need to work out a sane API so the user can clearly limit how much they are willing to spend on fees..
Contributor
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. Manual is a good first step of course, even though not being able to provide a fee rate to this call might feel uncontrolled. Eventually I don't think you can ask an end-user to make any kind of decision about how and when to RBF. |
||
| &self, user_channel_id: &UserChannelId, counterparty_node_id: PublicKey, | ||
| ) -> Result<(), Error> { | ||
| let open_channels = | ||
| self.channel_manager.list_channels_with_counterparty(&counterparty_node_id); | ||
| if let Some(channel_details) = | ||
| open_channels.iter().find(|c| c.user_channel_id == user_channel_id.0) | ||
| { | ||
| let min_feerate = | ||
|
Collaborator
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. Codex:
|
||
| self.fee_estimator.estimate_fee_rate(ConfirmationTarget::ChannelFunding); | ||
| let max_feerate = FeeRate::from_sat_per_kwu(min_feerate.to_sat_per_kwu() * 3 / 2); | ||
|
|
||
| let funding_template = self | ||
| .channel_manager | ||
| .splice_channel(&channel_details.channel_id, &counterparty_node_id) | ||
| .map_err(|e| { | ||
| log_error!(self.logger, "Failed to RBF channel: {:?}", e); | ||
| Error::ChannelSplicingFailed | ||
| })?; | ||
|
|
||
| if funding_template.min_rbf_feerate().is_none() { | ||
| log_error!(self.logger, "Failed to RBF channel: no pending splice to replace"); | ||
| return Err(Error::ChannelSplicingFailed); | ||
| } | ||
|
|
||
| let contribution = self | ||
| .runtime | ||
| .block_on(funding_template.rbf_prior_contribution( | ||
| None, | ||
| max_feerate, | ||
| Arc::clone(&self.wallet), | ||
| )) | ||
| .map_err(|e| { | ||
| log_error!(self.logger, "Failed to RBF channel: {}", e); | ||
| Error::ChannelSplicingFailed | ||
| })?; | ||
|
|
||
| self.channel_manager | ||
| .funding_contributed( | ||
| &channel_details.channel_id, | ||
| &counterparty_node_id, | ||
| contribution, | ||
| None, | ||
| ) | ||
| .map_err(|e| { | ||
| log_error!(self.logger, "Failed to RBF channel: {:?}", e); | ||
| Error::ChannelSplicingFailed | ||
| }) | ||
| } else { | ||
| log_error!( | ||
| self.logger, | ||
| "Channel not found for user_channel_id {} and counterparty {}", | ||
| user_channel_id, | ||
| counterparty_node_id | ||
| ); | ||
| Err(Error::ChannelSplicingFailed) | ||
| } | ||
| } | ||
|
|
||
| /// Manually sync the LDK and BDK wallets with the current chain state and update the fee rate | ||
| /// cache. | ||
| /// | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,25 +6,52 @@ | |
| // accordance with one or both of these licenses. | ||
|
|
||
| use bitcoin::Txid; | ||
| use lightning::chain::chaininterface::FundingCandidate; | ||
| use lightning::impl_writeable_tlv_based; | ||
| use lightning::ln::channelmanager::PaymentId; | ||
|
|
||
| use crate::data_store::{StorableObject, StorableObjectUpdate}; | ||
| use crate::payment::store::PaymentDetailsUpdate; | ||
| use crate::payment::PaymentDetails; | ||
|
|
||
| /// Marks an on-chain payment as belonging to an interactive-funding negotiation. The | ||
| /// last entry in `candidates` is the currently-broadcast tx; earlier entries are RBF | ||
| /// predecessors that may still confirm if reorgs intervene. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct FundingDetails { | ||
| /// Every negotiated candidate, oldest first. | ||
| pub candidates: Vec<FundingCandidate>, | ||
| } | ||
|
|
||
| impl_writeable_tlv_based!(FundingDetails, { | ||
| (0, candidates, optional_vec), | ||
| }); | ||
|
|
||
| /// Represents a pending payment | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct PendingPaymentDetails { | ||
| /// The full payment details | ||
| pub details: PaymentDetails, | ||
| /// Transaction IDs that have replaced or conflict with this payment. | ||
| pub conflicting_txids: Vec<Txid>, | ||
| /// Set when the payment's transaction is an interactive-funding broadcast (channel | ||
| /// open or splice). The record transitions to [`PaymentStatus::Succeeded`] on | ||
| /// `ChannelReady` instead of after [`ANTI_REORG_DELAY`] confirmations. | ||
|
Collaborator
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. Hmm, could you expand on why this is necessary? It seems a rather unintuitive change given we only require |
||
| /// | ||
| /// [`PaymentStatus::Succeeded`]: crate::payment::store::PaymentStatus::Succeeded | ||
| /// [`ANTI_REORG_DELAY`]: lightning::chain::channelmonitor::ANTI_REORG_DELAY | ||
| pub funding_details: Option<FundingDetails>, | ||
| } | ||
|
|
||
| impl PendingPaymentDetails { | ||
| pub(crate) fn new(details: PaymentDetails, conflicting_txids: Vec<Txid>) -> Self { | ||
| Self { details, conflicting_txids } | ||
| Self { details, conflicting_txids, funding_details: None } | ||
| } | ||
|
|
||
| pub(crate) fn with_funding_details( | ||
| details: PaymentDetails, conflicting_txids: Vec<Txid>, funding_details: FundingDetails, | ||
| ) -> Self { | ||
| Self { details, conflicting_txids, funding_details: Some(funding_details) } | ||
| } | ||
|
|
||
| /// Convert to finalized payment for the main payment store | ||
|
|
@@ -36,13 +63,15 @@ impl PendingPaymentDetails { | |
| impl_writeable_tlv_based!(PendingPaymentDetails, { | ||
| (0, details, required), | ||
| (2, conflicting_txids, optional_vec), | ||
| (4, funding_details, option), | ||
| }); | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub(crate) struct PendingPaymentDetailsUpdate { | ||
| pub id: PaymentId, | ||
| pub payment_update: Option<PaymentDetailsUpdate>, | ||
| pub conflicting_txids: Option<Vec<Txid>>, | ||
| pub funding_details: Option<Option<FundingDetails>>, | ||
| } | ||
|
|
||
| impl StorableObject for PendingPaymentDetails { | ||
|
|
@@ -68,6 +97,13 @@ impl StorableObject for PendingPaymentDetails { | |
| } | ||
| } | ||
|
|
||
| if let Some(new_funding_details) = update.funding_details { | ||
| if self.funding_details != new_funding_details { | ||
| self.funding_details = new_funding_details; | ||
| updated = true; | ||
| } | ||
| } | ||
|
|
||
| updated | ||
| } | ||
|
|
||
|
|
@@ -89,6 +125,11 @@ impl From<&PendingPaymentDetails> for PendingPaymentDetailsUpdate { | |
| } else { | ||
| Some(value.conflicting_txids.clone()) | ||
| }; | ||
| Self { id: value.id(), payment_update: Some(value.details.to_update()), conflicting_txids } | ||
| Self { | ||
| id: value.id(), | ||
| payment_update: Some(value.details.to_update()), | ||
| conflicting_txids, | ||
| funding_details: Some(value.funding_details.clone()), | ||
| } | ||
| } | ||
| } | ||
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 says use
rbf_channelinstead but that function doesn't let us change the amount in/out. Some likeuse rbf_channel to bump feewould be more accurate. Also would be nice if this had a separate 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.
Fixed the message, but I'm not sure it justifies a new error type. Callers will be able to check
SpliceDetailsonce lightningdevkit/rust-lightning#4687 is included. @tnull Any preference?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.
Should that be added to the 0.3 milestone?
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, added.
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.
No preference, fine to leave it as-is.