Add deadline height to UrgentOnChainSweep #3563
Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| UrgentOnChainSweep, | ||
| /// | ||
| /// If a target confirmation height is known, it can be set as the parameter. | ||
| UrgentOnChainSweep(Option<u32>), |
There was a problem hiding this comment.
Fee estimation is usually based on deltas (and the docs above even point to such).
There was a problem hiding this comment.
yeah, I think I misinterpreted the target time in the issue. Will adjust
| .iter() | ||
| .map(|o| o.0.cltv_expiry) | ||
| .min(); | ||
| return ConfirmationTarget::UrgentOnChainSweep(minimum_expiry); |
There was a problem hiding this comment.
If we have two HTLCs: A with expiry 100 and B with expiry 300, this will always result in overpaying for HTLC B even though we can afford to pay less for a bit. It'd be nice to keep track of the resolved HTLCs, if any, so we can filter them out.
There was a problem hiding this comment.
We should distinguish between commitment tx deadlines and HTLC claim deadlines. The commitment deadline needs to consider earliest expiry of all active HTLCs, while individual HTLC claims have separate deadlines.
| .iter() | ||
| .map(|o| o.0.cltv_expiry) | ||
| .min(); | ||
| return ConfirmationTarget::UrgentOnChainSweep(minimum_expiry); |
There was a problem hiding this comment.
We should distinguish between commitment tx deadlines and HTLC claim deadlines. The commitment deadline needs to consider earliest expiry of all active HTLCs, while individual HTLC claims have separate deadlines.
| .map(|o| o.0.cltv_expiry) | ||
| .min() | ||
| ).flatten(); | ||
| return ConfirmationTarget::UrgentOnChainSweep(minimum_expiry); |
There was a problem hiding this comment.
This current logic stops evaluating as soon as any HTLCs are found on a single commitment tx.
For force closing, we need to find the minimum expiry across all active commitment transactions, not just on the first one we find HTLCs on.
There was a problem hiding this comment.
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.
arik-so
force-pushed
the
urgent-sweep-deadline
branch
from
faa33f1 to
56b73e7
Compare
| UrgentOnChainSweep, | ||
| /// | ||
| /// If a target confirmation delta is known, it can be set as the parameter. | ||
| UrgentOnChainSweep(Option<u32>), |
There was a problem hiding this comment.
I don't buy that we need an Option here, we should be able to remove it, AFAICT.
| 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))); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Nit: I think the following refactor is more readable:
| 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))); | |
| } | |
| }; |
WIP: Address #3527 to store actual deadline.