feat: allow configurable force-close buffer for claimable HTLCs#4401
feat: allow configurable force-close buffer for claimable HTLCs#4401okekefrancis112 wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
👋 I see @valentinewallace was un-assigned. |
okekefrancis112
force-pushed
the
claimable_htcl
branch
from
ca4bccc to
59439dc
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4401 +/- ##
==========================================
+ Coverage 85.97% 86.06% +0.09%
==========================================
Files 159 159
Lines 104722 105220 +498
Branches 104722 105220 +498
==========================================
+ Hits 90030 90555 +525
+ Misses 12191 12147 -44
- Partials 2501 2518 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
okekefrancis112
force-pushed
the
claimable_htcl
branch
from
59439dc to
c894222
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
There's a bunch of checks in channelmanager.rs that need to be equivalently enforce on settings. In any case, please carefully go through every use of CLTV_CLAIM_BUFFER and maybe all the block-count-constants read the associated comments, and consider if we should update anything or if it might imply changes somewhere.
| htlc: SentHTLCId, | ||
| }, | ||
| /// Used to update the configurable force-close CLTV buffer for claimable HTLCs. | ||
| ChannelConfigUpdated { |
There was a problem hiding this comment.
Bleh, pushing through a monitor update is a heavy persistence operation. Instead, let's just make this configurable on the ChainMonitor and keep the settings memory-only.
There was a problem hiding this comment.
Thank you for pointing this out. Your review is greatly appreciated, I will work on this and push it.
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Oops, sorry I let this one drop. Feel free to ping a PR if no one has looked at it in a week or two and you're expecting review.
You still need to calculate exact min/max bounds for this based on the various assertions at the top of channelmanager.rs.
lightning/src/chain/chainmonitor.rs
Outdated
| /// Memory-only per-channel configuration for the CLTV buffer used when deciding | ||
| /// to force-close channels with claimable inbound HTLCs. This is not persisted | ||
| /// and is rebuilt from channel state on restart. | ||
| channel_force_close_buffers: RwLock<HashMap<ChannelId, u32>>, |
There was a problem hiding this comment.
Hmm, is there a reason to want this per-channel vs just setting a global value and calling it done?
lightning/src/chain/chainmonitor.rs
Outdated
| /// Returns an error if the buffer value is below [`CLTV_CLAIM_BUFFER`]. | ||
| /// | ||
| /// [`CLTV_CLAIM_BUFFER`]: channelmonitor::CLTV_CLAIM_BUFFER | ||
| pub fn update_channel_force_close_buffer( |
There was a problem hiding this comment.
Rather than a setter, can we just have this be a parameter on the constructor?
lightning/src/util/config.rs
Outdated
| /// | ||
| /// [`cltv_expiry_delta`]: ChannelConfig::cltv_expiry_delta | ||
| /// [`CLTV_CLAIM_BUFFER`]: crate::chain::channelmonitor::CLTV_CLAIM_BUFFER | ||
| pub force_close_claimable_htlc_cltv_buffer: u32, |
There was a problem hiding this comment.
We can't have two separate values as we can't allow these to conflict.
On the min/max bounds: tracing through the assertions at the top of channelmanager.rs, I arrive at: Min = CLTV_CLAIM_BUFFER (36) — need at least 2 * MAX_BLOCKS_FOR_CONF blocks to get both transactions confirmed on-chain. |
okekefrancis112
force-pushed
the
claimable_htcl
branch
from
cd3b598 to
d4a5e60
Compare
…o global ChainMonitor param Remove `force_close_claimable_htlc_cltv_buffer` from `ChannelConfig`, `ChannelConfigUpdate`, `ChannelMonitor`, and the `Watch` trait. Instead, accept it as a constructor parameter on `ChainMonitor`, making it a single global, memory-only setting. The valid range is [CLTV_CLAIM_BUFFER, HTLC_FAIL_BACK_BUFFER] (36-39 blocks), enforced via assert at construction time. The lower bound ensures enough time to confirm both commitment and HTLC-Success transactions. The upper bound ensures the monitor doesn't force-close before ChannelManager has a chance to fail back other pending HTLCs. This addresses review feedback to: - Avoid two conflicting values (ChannelConfig vs ChainMonitor) - Use a global value rather than per-channel configuration - Pass via constructor rather than a setter method - Keep the setting memory-only (no persistence overhead)
okekefrancis112
force-pushed
the
claimable_htcl
branch
from
d4a5e60 to
3720a18
Compare
3 participants
This PR adds a configurable
force_close_claimable_htlc_cltv_buffertoChannelConfig, allowing users to control how far in advance an inbound channel is force-closed when an HTLC becomes claimable. Previously, this buffer was hardcoded toCLTV_CLAIM_BUFFER(36 blocks), limiting users’ ability to tolerate their own downtime. The new field defaults to the existing constant, enforces the same minimum, and is validated againstcltv_expiry_deltato preserve safety guarantees. The value is propagated toChannelMonitor, persisted via monitor updates and serialization, and used in place of the hardcoded constant when deciding whether to broadcast a holder commitment transaction. This change improves configurability without altering default behavior.Closes: #3837