-
Notifications
You must be signed in to change notification settings - Fork 150
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
305a54d
bba9652
61d2b19
bc2b070
af7dc4c
9acb2ad
5668ca9
008b937
da75376
66711db
06a83cf
dc6b89f
4da7335
737b4b2
02575ed
5ed2d43
b6f32aa
243c2be
95b3720
5b1571d
8d0b58b
f2990d5
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 |
|---|---|---|
|
|
@@ -1578,7 +1578,7 @@ impl Node { | |
| { | ||
| let min_feerate = | ||
| 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 max_feerate = max_funding_feerate(min_feerate); | ||
|
|
||
| let splice_amount_sats = match splice_amount_sats { | ||
| FundingAmount::Exact { amount_sats } => amount_sats, | ||
|
|
@@ -1647,16 +1647,26 @@ 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); | ||
| } | ||
|
|
||
| // When contributing to a pending splice, the funding template requires at least the RBF | ||
| // minimum feerate to replace the in-flight transaction. Use it in place of our funding | ||
| // feerate estimate when it's higher, as long as it stays within our max. | ||
| let feerate = match funding_template.min_rbf_feerate() { | ||
| Some(min_rbf_feerate) if min_rbf_feerate <= max_feerate => { | ||
| min_feerate.max(min_rbf_feerate) | ||
| }, | ||
| _ => min_feerate, | ||
| }; | ||
|
|
||
| let contribution = self | ||
| .runtime | ||
| .block_on(funding_template.splice_in( | ||
| Amount::from_sat(splice_amount_sats), | ||
| min_feerate, | ||
| feerate, | ||
| max_feerate, | ||
| Arc::clone(&self.wallet), | ||
| )) | ||
|
|
@@ -1757,7 +1767,7 @@ impl Node { | |
|
|
||
| let min_feerate = | ||
| 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 max_feerate = max_funding_feerate(min_feerate); | ||
|
|
||
| let funding_template = self | ||
| .channel_manager | ||
|
|
@@ -1770,17 +1780,27 @@ 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.
|
||
| } | ||
|
|
||
| // When contributing to a pending splice, the funding template requires at least the RBF | ||
| // minimum feerate to replace the in-flight transaction. Use it in place of our funding | ||
| // feerate estimate when it's higher, as long as it stays within our max. | ||
| let feerate = match funding_template.min_rbf_feerate() { | ||
| Some(min_rbf_feerate) if min_rbf_feerate <= max_feerate => { | ||
| min_feerate.max(min_rbf_feerate) | ||
| }, | ||
| _ => min_feerate, | ||
| }; | ||
|
|
||
| let outputs = vec![bitcoin::TxOut { | ||
| value: Amount::from_sat(splice_amount_sats), | ||
| script_pubkey: address.script_pubkey(), | ||
| }]; | ||
| let contribution = | ||
| funding_template.splice_out(outputs, min_feerate, max_feerate).map_err(|e| { | ||
| funding_template.splice_out(outputs, feerate, max_feerate).map_err(|e| { | ||
| log_error!(self.logger, "Failed to splice channel: {}", e); | ||
| Error::ChannelSplicingFailed | ||
| })?; | ||
|
|
@@ -1807,6 +1827,77 @@ impl Node { | |
| } | ||
| } | ||
|
|
||
| /// Fee-bumps the pending splice on a channel by replacing its in-flight funding transaction | ||
| /// (RBF). The splice's amount and destination are preserved; only the fee rate is raised. | ||
| /// Errors if the channel has no pending splice to bump. | ||
| 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.
Contributor
Author
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. Note that we currently limit how much we'll pay as an acceptor (i.e., if we lose quiescence) to 1.5x the channel funding fee rate. This is I'd imagine yes as anything above that means we'd be overpaying based on our fee estimator. Unless of course we want to allow users to target something quicker than 12 blocks (
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. I am mainly questioning whether this blind "push the button" api is optimal. Applying the max seems like a good safe-guard for that button, and indeed, also not if you need your splice quickly. Might be useful to think through what the end-user UI looks like for splicing, and make sure the API allows that. What does the end-user UI look like?
Contributor
Author
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. Depends on the timeframe, I suppose. Eventually, splicing is more of an implementation detail. Need to make an on-chain payment but your balance is in a channel? LDK Node should just splice out using |
||
| &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:
Contributor
Author
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. Yeah, good catch. Done. |
||
| self.fee_estimator.estimate_fee_rate(ConfirmationTarget::ChannelFunding); | ||
|
|
||
| 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 | ||
| })?; | ||
|
|
||
| let Some(min_rbf_feerate) = funding_template.min_rbf_feerate() else { | ||
| log_error!(self.logger, "Failed to RBF channel: no pending splice to replace"); | ||
| return Err(Error::ChannelSplicingFailed); | ||
| }; | ||
|
|
||
| let Some((target_feerate, max_feerate)) = | ||
| rbf_splice_feerates(min_feerate, min_rbf_feerate) | ||
| else { | ||
| log_error!( | ||
| self.logger, | ||
| "Failed to RBF channel: the RBF minimum feerate exceeds our maximum" | ||
| ); | ||
| return Err(Error::ChannelSplicingFailed); | ||
| }; | ||
|
|
||
| let contribution = self | ||
| .runtime | ||
| .block_on(funding_template.rbf_prior_contribution( | ||
| Some(target_feerate), | ||
| 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. | ||
| /// | ||
|
|
@@ -2316,12 +2407,44 @@ pub(crate) fn new_channel_anchor_reserve_sats( | |
| }) | ||
| } | ||
|
|
||
| /// The most we are willing to pay for a channel funding transaction: `1.5x` our funding feerate | ||
| /// estimate. Used as the `max_feerate` ceiling for splices and their RBF fee bumps. | ||
| fn max_funding_feerate(estimate: FeeRate) -> FeeRate { | ||
| FeeRate::from_sat_per_kwu(estimate.to_sat_per_kwu() * 3 / 2) | ||
| } | ||
|
|
||
| /// Picks the `(target, max)` feerates for replacing a pending splice's in-flight funding | ||
| /// transaction via RBF, or `None` if the RBF can't be done within our fee ceiling. | ||
| /// | ||
| /// `max` is the most we are willing to pay (see [`max_funding_feerate`]), which tracks our current | ||
| /// estimate and so may have risen or fallen since the original splice; it is never inflated to meet | ||
| /// the RBF minimum. `target` is what we actually pay — our current estimate, or the template's RBF | ||
| /// minimum if that is higher (required to replace the transaction). If that minimum exceeds `max`, | ||
| /// we can't RBF. | ||
| fn rbf_splice_feerates(estimate: FeeRate, min_rbf_feerate: FeeRate) -> Option<(FeeRate, FeeRate)> { | ||
| let max = max_funding_feerate(estimate); | ||
| let target = estimate.max(min_rbf_feerate); | ||
| (target <= max).then_some((target, max)) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use lightning::util::ser::{Readable, Writeable}; | ||
|
|
||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn rbf_splice_feerates_target_and_max() { | ||
| let kwu = FeeRate::from_sat_per_kwu; | ||
| // Estimate below the RBF minimum but within our ceiling: pay the minimum to replace the | ||
| // transaction; the max stays 1.5x the estimate (never inflated) and already clears it. | ||
| assert_eq!(rbf_splice_feerates(kwu(253), kwu(278)), Some((kwu(278), kwu(253 * 3 / 2)))); | ||
| // Estimate risen above the RBF minimum: pay the higher estimate, not the stale minimum. | ||
| assert_eq!(rbf_splice_feerates(kwu(500), kwu(278)), Some((kwu(500), kwu(500 * 3 / 2)))); | ||
| // RBF minimum above our max (1.5x a fallen estimate): we can't RBF within our ceiling. | ||
| assert_eq!(rbf_splice_feerates(kwu(100), kwu(278)), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn node_metrics_reads_legacy_rgs_snapshot_timestamp() { | ||
| // Pre-#615, `NodeMetrics` persisted `latest_rgs_snapshot_timestamp` as an optional | ||
|
|
||
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.