Skip to content
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

Add deadline height to UrgentOnChainSweep #3563

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add deadline height to UrgentOnChainSweep #3563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa64a20
Add target delta to UrgentOnChainSweep
arik-so Jan 27, 2025
56b73e7
f: return global minimum
arik-so Feb 11, 2025
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
2 changes: 1 addition & 1 deletion fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ impl FeeEstimator for FuzzEstimator {
// always return a HighPriority feerate here which is >= the maximum Normal feerate and a
// Background feerate which is <= the minimum Normal feerate.
match conf_target {
ConfirmationTarget::MaximumFeeEstimate | ConfirmationTarget::UrgentOnChainSweep => {
ConfirmationTarget::MaximumFeeEstimate | ConfirmationTarget::UrgentOnChainSweep(_) => {
MAX_FEE
},
ConfirmationTarget::ChannelCloseMinimum
Expand Down
4 changes: 3 additions & 1 deletion lightning/src/chain/chaininterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ pub enum ConfirmationTarget {
/// Generally we have in the high tens to low hundreds of blocks to get our transaction
/// on-chain (it doesn't have to happen in the next few blocks!), but we shouldn't risk too low
/// a fee - this should be a relatively high priority feerate.
UrgentOnChainSweep,
///
/// If a target confirmation delta is known, it can be set as the parameter.
UrgentOnChainSweep(Option<u32>),
Copy link
Contributor

Choose a reason for hiding this comment

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

Fee estimation is usually based on deltas (and the docs above even point to such).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I think I misinterpreted the target time in the issue. Will adjust

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't buy that we need an Option here, we should be able to remove it, AFAICT.

/// This is the lowest feerate we will allow our channel counterparty to have in an anchor
/// channel in order to close the channel if a channel party goes away.
///
Expand Down
37 changes: 31 additions & 6 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit concerned about performance here, as well as whether setting the CLTV expiry itself as the deadline is prudent, or if it should be offset by some constant value.

Copy link
Contributor

Choose a reason for hiding this comment

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

In most cases, you'll find the same HTLCs across the commitments, so we could just have a single set/vec to filter out duplicates.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be careful about the deadlines. Since we often need to get 2 transactions confirmed to claim HTLCs (commitment + HTLC claim), we can't always just set the deadline to the CLTV expiry.

Ideally I'd suggest something like:

  • HTLC claim deadline: CLTV expiry
  • commitment tx deadline: (current_height + ((min_cltv_expiry - current_height) / 2) (i.e. half the earliest CLTV expiry)

This wouldn't work properly with the current logic, since conf_targets are re-evaluated every block (and on rebroadcast IIRC), which would cause the commitment tx deadlines to be pushed back by 1 every 2 blocks. But if we keep track of the start height or the previous deadline, we'd be able to properly adjust the deadline each block.

Alternatively we could choose a fixed delta to subtract from CLTV expiry for the commitment deadline, but that seems less clean IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if the cleanest way to accomplish this is by having this method return a tuple instead of a single confirmation target enum variant, or if we should break this up into commitment and HTLC conf target methods.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we just have two different variants - a "we need to get this confirmed by X" and a "we need to get two transactions confirmed by X" variant? Seems to communicate it all to the user and we can let them figure it out.

Original file line number Diff line number Diff line change
Expand Up @@ -2743,22 +2743,47 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
fn closure_conf_target(&self) -> ConfirmationTarget {
// Treat the sweep as urgent as long as there is at least one HTLC which is pending on a
// valid commitment transaction.
let mut minimum_expiry = None;
let update_minimum_expiry = |local_minimum: Option<u32>, global_minimum: &mut Option<u32>| {
if let Some(expiry) = local_minimum {
*global_minimum = Some(global_minimum.map_or(expiry, |m| cmp::min(expiry, m)));
}
};
Comment on lines +2747 to +2751
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think the following refactor is more readable:

Suggested change
let update_minimum_expiry = |local_minimum: Option<u32>, global_minimum: &mut Option<u32>| {
if let Some(expiry) = local_minimum {
*global_minimum = Some(global_minimum.map_or(expiry, |m| cmp::min(expiry, m)));
}
};
let mut update_minimum_expiry = |local_minimum: Option<u32>| {
if let Some(expiry) = local_minimum {
minimum_expiry = Some(cmp::min(expiry, minimum_expiry.unwrap_or(expiry)));
}
};


if !self.current_holder_commitment_tx.htlc_outputs.is_empty() {
return ConfirmationTarget::UrgentOnChainSweep;
let local_minimum_expiry = self.current_holder_commitment_tx.htlc_outputs
.iter()
.map(|o| o.0.cltv_expiry)
.min();
update_minimum_expiry(local_minimum_expiry, &mut minimum_expiry);
}
if self.prev_holder_signed_commitment_tx.as_ref().map(|t| !t.htlc_outputs.is_empty()).unwrap_or(false) {
return ConfirmationTarget::UrgentOnChainSweep;
let local_minimum_expiry = self.prev_holder_signed_commitment_tx.as_ref().map(|t| t.htlc_outputs
.iter()
.map(|o| o.0.cltv_expiry)
.min()
).flatten();
update_minimum_expiry(local_minimum_expiry, &mut minimum_expiry);
}
if let Some(txid) = self.current_counterparty_commitment_txid {
if !self.counterparty_claimable_outpoints.get(&txid).unwrap().is_empty() {
return ConfirmationTarget::UrgentOnChainSweep;
let claimable_outpoints = self.counterparty_claimable_outpoints.get(&txid).unwrap();
if !claimable_outpoints.is_empty() {
let local_minimum_expiry = claimable_outpoints.iter().map(|o|o.0.cltv_expiry).min();
update_minimum_expiry(local_minimum_expiry, &mut minimum_expiry);
}
}
if let Some(txid) = self.prev_counterparty_commitment_txid {
if !self.counterparty_claimable_outpoints.get(&txid).unwrap().is_empty() {
return ConfirmationTarget::UrgentOnChainSweep;
let claimable_outpoints = self.counterparty_claimable_outpoints.get(&txid).unwrap();
if !claimable_outpoints.is_empty() {
let local_minimum_expiry = claimable_outpoints.iter().map(|o|o.0.cltv_expiry).min();
update_minimum_expiry(local_minimum_expiry, &mut minimum_expiry);
}
}

if let Some(global_minimum) = minimum_expiry {
return ConfirmationTarget::UrgentOnChainSweep(Some(global_minimum))
}

ConfirmationTarget::OutputSpendingFee
}

Expand Down
4 changes: 2 additions & 2 deletions lightning/src/chain/onchaintx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1417,7 +1417,7 @@ mod tests {
1,
1,
&&broadcaster,
ConfirmationTarget::UrgentOnChainSweep,
ConfirmationTarget::UrgentOnChainSweep(Some(2)),
&fee_estimator,
&logger,
);
Expand All @@ -1440,7 +1440,7 @@ mod tests {
2,
2,
&&broadcaster,
ConfirmationTarget::UrgentOnChainSweep,
ConfirmationTarget::UrgentOnChainSweep(Some(3)),
&fee_estimator,
&logger,
);
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/chain/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1689,7 +1689,7 @@ mod tests {
let test_fee_estimator = &TestFeeEstimator { sat_per_kw };
let fee_estimator = LowerBoundedFeeEstimator::new(test_fee_estimator);
let fee_rate_strategy = FeerateStrategy::ForceBump;
let confirmation_target = ConfirmationTarget::UrgentOnChainSweep;
let confirmation_target = ConfirmationTarget::UrgentOnChainSweep(None);

{
// Check underflow doesn't occur
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/monitor_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2556,7 +2556,7 @@ fn do_test_yield_anchors_events(have_htlcs: bool) {
// Note that if we use the wrong target, we will immediately broadcast the commitment
// transaction as no bump is required.
if have_htlcs {
nodes[0].fee_estimator.target_override.lock().unwrap().insert(ConfirmationTarget::UrgentOnChainSweep, 500);
nodes[0].fee_estimator.target_override.lock().unwrap().insert(ConfirmationTarget::UrgentOnChainSweep(Some(81)), 500);
} else {
nodes[0].fee_estimator.target_override.lock().unwrap().insert(ConfirmationTarget::OutputSpendingFee, 500);
}
Expand Down
Loading