Correct and update confirmation target constant definitions #3608
+195
−145
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpaulino
reviewed
Feb 24, 2025
TheBlueMatt
force-pushed
the
2025-02-better-block-constants
branch
2 times, most recently
from
5314c7d to
dac2cc2
Compare
|
Feel free to squash |
TheBlueMatt
force-pushed
the
2025-02-better-block-constants
branch
from
dac2cc2 to
84ce4f0
Compare
|
Squashed without changes. |
|
Needs rustfmt |
TheBlueMatt
force-pushed
the
2025-02-better-block-constants
branch
from
84ce4f0 to
7fb9ba1
Compare
|
Grrr, done. |
wpaulino
previously approved these changes
Feb 25, 2025
| // in with enough time left to fail the corresponding HTLC back to our inbound edge before they | ||
| // force-close on us. | ||
| // In other words, if the next-hop peer fails the HTLC LATENCY_GRACE_PERIOD_BLOCKS after our | ||
| // CLTV_CLAIM_BUFFER (because that's how many blocks we allow them after expiry), we'll still have |
There was a problem hiding this comment.
s/after expiry/before expiry?
There was a problem hiding this comment.
No after. We FC a channel several blocks after an HTLC expires.
| // In other words, if the next-hop peer fails the HTLC LATENCY_GRACE_PERIOD_BLOCKS after our | ||
| // CLTV_CLAIM_BUFFER (because that's how many blocks we allow them after expiry), we'll still have | ||
| // 2*MAX_BLOCKS_FOR_CONF + ANTI_REORG_DELAY left to get two transactions on chain and the second | ||
| // fully locked in before the peer force-closes on us (LATENCY_GRACE_PERIOD_BLOCKS before the |
There was a problem hiding this comment.
suggestion:
Suggested change
| // fully locked in before the peer force-closes on us (LATENCY_GRACE_PERIOD_BLOCKS before the | |
| // fully locked in before the inbound peer force-closes on us (LATENCY_GRACE_PERIOD_BLOCKS before the |
There was a problem hiding this comment.
That reads kinda confusing to me? Makes it sound like the channel was inbound from the peer, rather than the HTLC was relayed inbound from the peer?
Comment on lines
233
to
235
| /// If an HTLC expires within this many blocks, force-close the channel to broadcast the | ||
| /// HTLC-Success transaction. |
There was a problem hiding this comment.
"HTLC-Success transaction" phrasing seems to suggest this const is only used in the context of channels with inbound HTLC(s) where we have the preimage. But it seems to be used for inbounds where we don't have the preimage as well, and/or other contexts? I wonder if this could be clarified?
There was a problem hiding this comment.
Not directly? Its only used directly as !htlc_outbound && htlc.cltv_expiry <= height + CLTV_CLAIM_BUFFER && self.payment_preimages.contains_key(&htlc.payment_hash) and indirectly to calculate HTLC_FAIL_BACK_BUFFER which is the same concept.
The `CHECK_CLTV_EXPIRY_SANITY_2` check actually made no sense - its use of `2*CLTV_CLAIM_BUFFER` implicitly assumed that we were failing HTLCs `CLTV_CLAIM_BUFFER` after they expired, which is nonsense. Instead, we improve the readability of the docs on `_CHECK_CLTV_EXPIRY_SANITY` and add a new `_CHECK_CLTV_EXPIRY_OFFCHAIN` that correctly tests for what the `CHECK_CLTV_EXPIRY_SANITY_2` check was supposed to look at.
TheBlueMatt
force-pushed
the
2025-02-better-block-constants
branch
from
7fb9ba1 to
d078b10
Compare
Over the years we've built up a few tests which rely on block count constants but without making direct references to those constants. Here we fix three such tests to ensure changing block count constants in the next commit do not break tests.
`test_duplicate_payment_hash_one_failure_one_success` makes a number of assumptions which rely on our specific CLTV constants, which we intend to change. Specifically, it relies on forwarding two HTLCs, one which goes one hop longer, and that it can close the channel with both HTLCs sitting on chain with enough time to claim that it doesn't give up on them but also that they get claimed in separate transactions. Here we clean up, simplify, and better document the test such that it is more resilient to future CLTV constant changes.
`CLTV_CLAIM_BUFFER`'s definition stated "this is an upper bound on how many blocks we think it can take us to get a transaction confirmed". This was mostly okay for pre-anchor channels, where we broadcasted HTLC claim transactions at the same time as the commitment transactions themselves, but for anchor channels we can no longer do that - HTLC transactions are always CSV 1'd. Further, when we do go to broadcast HTLC transactions, we start the feerate estimate for them back at the users' feerate estimator, rather than whatever feerate we ended up using to get the commitment transaction confirmed. While we should maybe consider changing that, for now that means that we really need to run the whole "get a transaction confirmed" process from start to finish *twice* in order to claim an HTLC. Thus, `CLTV_CLAIM_BUFFER` is here redefined to be two times "the upper bound on how many blocks we think it can take for us to get a transaction confirmed", with a new `MAX_BLOCKS_FOR_CONF` constant defining the expected max blocks.
In the previous commit it was observed that we actually have to run the whole "get a transaction confirmed" process from start to finish twice to claim an HTLC on an anchor channel. This leaves us with only 9 blocks to get each transaction confirmed, which is quite aggressive. Here we double this threshold, force-closing channels which have an expiring HTLC 36 blocks before expiry instead of 18. We also increase the minimum CLTV expiry delta to 48 to ensure we have at least a few blocks after the transactions get confirmed before we need to fail the inbound edge of a forwarded HTLC back. We do not change the default CLTV expiry delta of 72 blocks.
|
Cleaned up the commit history with the following additional diff, plus rebased. diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 66f004356..d01d25bbc 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -2816,3 +2816,3 @@ pub(crate) const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 2 * 6 * 24 * 7;
/// The minimum number of blocks between an inbound HTLC's CLTV and the corresponding outbound
-/// HTLC's CLTV. The current default represents roughly seven hours of blocks at six blocks/hour.
+/// HTLC's CLTV. The current default represents roughly eight hours of blocks at six blocks/hour.
///
@@ -2845,3 +2845,3 @@ pub const MIN_FINAL_CLTV_EXPIRY_DELTA: u16 = HTLC_FAIL_BACK_BUFFER as u16 + 3;
// force-close on us.
-// In other words, if the next-hop peer fails the HTLC LATENCY_GRACE_PERIOD_BLOCKS after our
+// In other words, if the next-hop peer fails HTLC LATENCY_GRACE_PERIOD_BLOCKS after our
// CLTV_CLAIM_BUFFER (because that's how many blocks we allow them after expiry), we'll still have |
TheBlueMatt
force-pushed
the
2025-02-better-block-constants
branch
from
d078b10 to
e7c2a61
Compare
valentinewallace
approved these changes
Mar 5, 2025
wpaulino
approved these changes
Mar 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the process of #3340 I noticed a bunch of our confirmation target constants don't make sense anymore post-anchor (and one of our checks made no sense ever). So here we update them, taking this opportunity to also increase a few windows.