Skip to content

Make cltv_expiry_delta configurable and reduce the min/default some #849

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

Merged
merged 3 commits into from
Mar 20, 2021
Merged

Make cltv_expiry_delta configurable and reduce the min/default some #849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c8d4536
Make cltv_expiry_delta configurable and reduce the min/default some
TheBlueMatt Mar 17, 2021
7503e2a
Tweak our_to_self_delay documentation wording to make it flow better
TheBlueMatt Mar 19, 2021
e640b93
Ignore patch codecov as long as total coverage is within 1% of base
TheBlueMatt Mar 18, 2021
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
7 changes: 6 additions & 1 deletion codecov.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ coverage:
project:
default:
target: auto
threshold: 2%
threshold: 1%
base: auto
informational: false
patch:
default:
target: auto
threshold: 100%
base: auto
6 changes: 5 additions & 1 deletion lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use bitcoin::secp256k1;
use ln::features::{ChannelFeatures, InitFeatures};
use ln::msgs;
use ln::msgs::{DecodeError, OptionalField, DataLossProtect};
use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentPreimage, PaymentHash, BREAKDOWN_TIMEOUT, MAX_LOCAL_BREAKDOWN_TIMEOUT};
use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentPreimage, PaymentHash, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
use ln::chan_utils::{CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, get_commitment_transaction_number_obscure_factor};
use ln::chan_utils;
use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
Expand Down Expand Up @@ -3359,6 +3359,10 @@ impl<Signer: Sign> Channel<Signer> {
self.config.fee_proportional_millionths
}

pub fn get_cltv_expiry_delta(&self) -> u16 {
cmp::max(self.config.cltv_expiry_delta, MIN_CLTV_EXPIRY_DELTA)
}

#[cfg(test)]
pub fn get_feerate(&self) -> u32 {
self.feerate_per_kw
Expand Down
23 changes: 14 additions & 9 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,11 +523,16 @@ pub const BREAKDOWN_TIMEOUT: u16 = 6 * 24;
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. This should always be a few blocks greater than channelmonitor::CLTV_CLAIM_BUFFER,
/// ie the node we forwarded the payment on to should always have enough room to reliably time out
/// the HTLC via a full update_fail_htlc/commitment_signed dance before we hit the
/// CLTV_CLAIM_BUFFER point (we static assert that it's at least 3 blocks more).
const CLTV_EXPIRY_DELTA: u16 = 6 * 12; //TODO?
/// HTLC's CLTV. The current default represents roughly six hours of blocks at six blocks/hour.
///
/// This can be increased (but not decreased) through [`ChannelConfig::cltv_expiry_delta`]
///
/// [`ChannelConfig::cltv_expiry_delta`]: crate::util::config::ChannelConfig::cltv_expiry_delta
// This should always be a few blocks greater than channelmonitor::CLTV_CLAIM_BUFFER,
// i.e. the node we forwarded the payment on to should always have enough room to reliably time out
// the HTLC via a full update_fail_htlc/commitment_signed dance before we hit the
// CLTV_CLAIM_BUFFER point (we static assert that it's at least 3 blocks more).
pub const MIN_CLTV_EXPIRY_DELTA: u16 = 6 * 6;
Copy link

Choose a reason for hiding this comment

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

I don't think the comment matches exactly this timelock purpose.

IIRC, "CLTV_EXPIRY_DELTA serves to give the forwarding node (us/this_local_node) a block delay between an onchain HTLC timeout on the forward channel and expiration of an inbound HTLC on the backward channel. Differing settlement result (success on the forward channel, failure on the backward channel) is a loss of fund for the forwarding node."

I think 36 is quite low (let's consider the case of a few hours power outage/node upgrade) and I'm more confident with the 72 blocks before. IIRC, Eclair default is 144, CL default is 20, lnd default is 40. Also consider that this timelock won't bother mobile clients, as we can assume they won't forward HTLCs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I feel like 36 is probably a reasonable lower-bound to enforce, but maybe we can change the default to a higher value and allow the user to reduce it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As to your comment about the purpose - are you referring to the comment or the documentation here? THe comment, indeed, is just noting one additional requirement, not the purpose, but its not a part of the public documentation.

Copy link

Choose a reason for hiding this comment

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

Hmm, I feel like 36 is probably a reasonable lower-bound to enforce, but maybe we can change the > default to a higher value and allow the user to reduce it?

Yes good idea to split between lower-bound/default value.

Reading the comment, I don't think the main requirement is about giving a time buffer to your counterparty to relay the HTLC forward. Otherwise a single-digit block would be enough. It's about protecting the forwarding node against multiple risks, like lengthy onchain confirmations or elongated offliness.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I split the lower-bound/default some. Discussion on IRC (I think) concluded the wording is acceptable.

pub(super) const CLTV_FAR_FAR_AWAY: u32 = 6 * 24 * 7; //TODO?

// Check that our CLTV_EXPIRY is at least CLTV_CLAIM_BUFFER + ANTI_REORG_DELAY + LATENCY_GRACE_PERIOD_BLOCKS,
Expand All @@ -538,13 +543,13 @@ pub(super) const CLTV_FAR_FAR_AWAY: u32 = 6 * 24 * 7; //TODO?
// LATENCY_GRACE_PERIOD_BLOCKS.
#[deny(const_err)]
#[allow(dead_code)]
const CHECK_CLTV_EXPIRY_SANITY: u32 = CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - CLTV_CLAIM_BUFFER - ANTI_REORG_DELAY - LATENCY_GRACE_PERIOD_BLOCKS;
const CHECK_CLTV_EXPIRY_SANITY: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - CLTV_CLAIM_BUFFER - ANTI_REORG_DELAY - LATENCY_GRACE_PERIOD_BLOCKS;

// Check for ability of an attacker to make us fail on-chain by delaying inbound claim. See
// ChannelMontior::would_broadcast_at_height for a description of why this is needed.
#[deny(const_err)]
#[allow(dead_code)]
const CHECK_CLTV_EXPIRY_SANITY_2: u32 = CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - 2*CLTV_CLAIM_BUFFER;
const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - 2*CLTV_CLAIM_BUFFER;

/// Details of a channel, as returned by ChannelManager::list_channels and ChannelManager::list_usable_channels
#[derive(Clone)]
Expand Down Expand Up @@ -1271,7 +1276,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
}
if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + CLTV_EXPIRY_DELTA as u64 { // incorrect_cltv_expiry
if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + chan.get_cltv_expiry_delta() as u64 { // incorrect_cltv_expiry
break Some(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, Some(self.get_channel_update(chan).unwrap())));
}
let cur_height = self.latest_block_height.load(Ordering::Acquire) as u32 + 1;
Expand Down Expand Up @@ -1329,7 +1334,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
short_channel_id,
timestamp: chan.get_update_time_counter(),
flags: (!were_node_one) as u8 | ((!chan.is_live() as u8) << 1),
cltv_expiry_delta: CLTV_EXPIRY_DELTA,
cltv_expiry_delta: chan.get_cltv_expiry_delta(),
htlc_minimum_msat: chan.get_counterparty_htlc_minimum_msat(),
htlc_maximum_msat: OptionalField::Present(chan.get_announced_htlc_max_msat()),
fee_base_msat: chan.get_holder_fee_base_msat(&self.fee_estimator),
Expand Down
39 changes: 32 additions & 7 deletions lightning/src/util/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,21 @@ pub struct ChannelHandshakeConfig {
///
/// Default value: 6.
pub minimum_depth: u32,
/// Set to the amount of time we require our counterparty to wait to claim their money.
/// Set to the number of blocks we require our counterparty to wait to claim their money (ie
/// the number of blocks we have to punish our counterparty if they broadcast a revoked
/// transaction).
///
/// It's one of the main parameter of our security model. We (or one of our watchtowers) MUST
/// be online to check for peer having broadcast a revoked transaction to steal our funds
/// at least once every our_to_self_delay blocks.
/// This is one of the main parameters of our security model. We (or one of our watchtowers) MUST
/// be online to check for revoked transactions on-chain at least once every our_to_self_delay
/// blocks (minus some margin to allow us enough time to broadcast and confirm a transaction,
/// possibly with time in between to RBF the spending transaction).
///
/// Meanwhile, asking for a too high delay, we bother peer to freeze funds for nothing in
/// case of an honest unilateral channel close, which implicitly decrease the economic value of
/// our channel.
///
/// Default value: [`BREAKDOWN_TIMEOUT`] (currently 144), we enforce it as a minimum at channel
/// opening so you can tweak config to ask for more security, not less.
/// Default value: [`BREAKDOWN_TIMEOUT`], we enforce it as a minimum at channel opening so you
/// can tweak config to ask for more security, not less.
pub our_to_self_delay: u16,
/// Set to the smallest value HTLC we will accept to process.
///
Expand Down Expand Up @@ -161,6 +164,26 @@ pub struct ChannelConfig {
///
/// Default value: 0.
pub fee_proportional_millionths: u32,
/// The difference in the CLTV value between incoming HTLCs and an outbound HTLC forwarded over
/// the channel this config applies to.
///
/// This is analogous to [`ChannelHandshakeConfig::our_to_self_delay`] but applies to in-flight
/// HTLC balance when a channel appears on-chain whereas
/// [`ChannelHandshakeConfig::our_to_self_delay`] applies to the remaining
/// (non-HTLC-encumbered) balance.
Copy link

Choose a reason for hiding this comment

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

The analogy isn't straighforward.

CSV delay protects against the risk of a revoked commitment state and offer to the offended counterparty a delay to punish and claim back funds.

CLTV delta protects against the risk of differing settlement result on your backward/forward channels and offer to a routing node a delay to confirm the commitment and corresponding HTLC transaction before the incoming HTLC can be cancel by the backward counterparty.

What purpose does it serve to document CSV delay around CLTV delta ? It might confuse the read more than anything else, you can set up both of them quite independently. Though ideally both of them should account how much you "trust" your counterparty...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I'm open to better wording. The intent of the connection here is that both have similar "you must be online at least once per X" requirements, which I think is a useful connection for users. That said, when we handle #850 it becomes more complicated - "you must be online at least once per X, unless you have limited the htlc_in_flight amount to something you don't care about, in which case, we don't care about this".

Copy link

Choose a reason for hiding this comment

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

I think your onliness requirement can be reduced to the smaller one between soonest-HTLC expiration and CSV delay. You should care about the smaller one.

I don't think CLTV delta is the same than "soonest-HTLC expiration". What you should look for is the soonest to expire HTLC. If you're block 100, and expiration is 120, your "must-be-back-every X" dynamically decrease until reaching X - CLTV_CLAIM_BUFFER and you must go online.

I think a lightweight onliness requirement is achievable for non-forward node but in that case you don't care about CLTV_DELTA.

Copy link
Collaborator Author

@TheBlueMatt TheBlueMatt Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

DIscussed this in a long conversation on IRC, but I think we ultimately agreed that its ok as-is, plus mentioning rbf time.

IRC Convo Pasted Below 
<ariard> BlueMatt: i dunno about your "must-be-back-online-every-X" reasoning around https://github.com/rust-bitcoin/rust-lightning/pull/849#discussion_r597995251]
<BlueMatt> ariard: correct, but we don't expose the cltv time of all currently-in-flight htlcs to the user
<ariard> or you might allow to be back online every X but at some point you should be back lower than X
<ariard> otherwise you won't be on time for CLTV_CLAIM_BUFFER
<BlueMatt> and even if we did, it'd be race-y - you have to (a) disable forwarding, then (b) check the cltvs, then (c) shutdown, scheduling when to go back online
<ariard> like if it's block 100 and expiration=120, back every X=7 doesn't work
<BlueMatt> on the other hand, I think my statement is correct in that as long as you *are* online once every CLTV_DELTA blocks you're (mostly) ok
<BlueMatt> modulo desire to rbf, of course
<ariard> yeah modulo rbf
<BlueMatt> I could expland the comment and note that you should be online a ways more often to get rbf time, but I dont think trying to explain the full nuance of htlc timeouts is worth it
<ariard> hmmmm i think it depends when you start to go offline
<ariard> because if you go offline CLTV_DELTA + 10
<ariard> your model work if you assume you go offline as soon as you committed the htlc
<ariard> in practice you might be around for few blocks then disconnect
<BlueMatt> i think the solution to that is to have a forwarding_disabled flag
<BlueMatt> which we currently dont have (though you could set infinite fee, I think)
<BlueMatt> but, again, thats a lot of nuance that I dont think is worth communicating
<BlueMatt> if users really dont want to be online more often than once a week, they should set cltv_delta and csv_delay to 1 week and move on
<ariard> that's a lot of nuances for now
<BlueMatt> then they dont have to think about it
<ariard> they should set cltv_delta to a week for sure
<BlueMatt> if we try to explain how you have to track all your in-flight htlcs and think about when they time-out, then you have a lot of code for....nothing
<BlueMatt> when they should just turn up the delta and move on
<ariard> i think it's more about mobile-vs-routing-node
<ariard> like won't make sense to check your in-flight HTLCs for a mobile and decide your onliness requirement dynamically
<ariard> awful ux
<BlueMatt> somewhat, but if you're mobile, you can just jack up the cltv_delta
<BlueMatt> routing, the cltv_delta docs are correct now, cause you always have htlcs in flight
<BlueMatt> mobile, you dont care what it is cause you dont want to forward
<ariard> yeah that's my point
<BlueMatt> right, but I think in *either* case, the response you have to the docs is correct
<ariard> let's go forward with your reasoning
<BlueMatt> a mobile user reading it would increase the cltv_delta to something big (fine, they dont want to forward, fine outcome) and a routing node would read it and think about it the same as the csv_delay (which is correct)
<ariard> i'm still thinking if you're a big node, better to be always online and spot weird behaviors as soon as you can
<ariard> not exactly, depend risk of realization of each
<BlueMatt> yea, I mean I can also expand it to talk about rbf if you want
<ariard> account for timevalue
<ariard> fine for me as long as you mention the CLTV_CLAIM_BUFFER/rbf
<BlueMatt> ok, I'll note rbf
<BlueMatt> does the current wording not capture CLTV_CLAIM_BUFFER correctly?
<BlueMatt> ariard: ^?
<ariard> just add a Big Comment on #845
<ariard> let me see
<ariard> BlueMatt: "plus some margin to allow us enough time to broadcast and confirm a transaction" minus some margin ?
<BlueMatt> yea
<BlueMatt> well, yes, sorry
<ariard> otherwise looks good to mw
<BlueMatt> cool, thanks
<BlueMatt> ariard: can you ack 849?
<BlueMatt> I split the default/min and tweaked the wording

///
/// Thus, for HTLC-encumbered balances to be enforced on-chain when a channel is force-closed,
/// we (or one of our watchtowers) MUST be online to check for broadcast of the current
/// commitment transaction at least once per this many blocks (minus some margin to allow us
/// enough time to broadcast and confirm a transaction, possibly with time in between to RBF
/// the spending transaction).
///
/// Default value: 72 (12 hours at an average of 6 blocks/hour).
/// Minimum value: [`MIN_CLTV_EXPIRY_DELTA`], any values less than this will be treated as
/// [`MIN_CLTV_EXPIRY_DELTA`] instead.
///
/// [`MIN_CLTV_EXPIRY_DELTA`]: crate::ln::channelmanager::MIN_CLTV_EXPIRY_DELTA
pub cltv_expiry_delta: u16,
/// Set to announce the channel publicly and notify all nodes that they can route via this
/// channel.
///
Expand Down Expand Up @@ -192,15 +215,17 @@ impl Default for ChannelConfig {
fn default() -> Self {
ChannelConfig {
fee_proportional_millionths: 0,
cltv_expiry_delta: 6 * 12, // 6 blocks/hour * 12 hours
announced_channel: false,
commit_upfront_shutdown_pubkey: true,
}
}
}

//Add write and readable traits to channelconfig
impl_writeable!(ChannelConfig, 8+1+1, {
impl_writeable!(ChannelConfig, 8+2+1+1, {
fee_proportional_millionths,
cltv_expiry_delta,
announced_channel,
commit_upfront_shutdown_pubkey
});
Expand Down