-
Notifications
You must be signed in to change notification settings - Fork 376
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.
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 new config setting max_balance_dust_htlc_msat
#1009
Add new config setting max_balance_dust_htlc_msat
#1009
Conversation
lightning/src/util/config.rs
Outdated
@@ -228,6 +236,7 @@ impl_writeable_tlv_based!(ChannelConfig, { | |||
(4, announced_channel, required), | |||
(6, commit_upfront_shutdown_pubkey, required), | |||
(8, forwarding_fee_base_msat, required), | |||
(10, max_outbound_dusted_htlc_msat, required), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this required
means we cannot read serialized configs from old versions. Can you take https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commit;h=fcf9ac5ca6055254955fca9a1402b24168aeab14 and use (default_value, 25_350_000)
instead of required
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it present another burden for the user, namely to override the default value after deser if user is willingly to enforce its own setting ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we'll need them to update the field after loading. Sadly we currently don't have any mechanism to update channel config after load at all, but it's something we'll need to as soon to allow updating channel channel relay fees soon.
lightning/src/util/config.rs
Outdated
/// than counterparty's `dust_limit_satoshis` sumed up with their expexted onchain claim | ||
/// cost at minimal mempools congestion. | ||
/// | ||
/// Default value: 25_350_000 (330 sat + `min_feerate_per_kw` * HTLC_SUCCESS_TX_WEIGHT / 1000) * 50 * 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any reason to keep the current default here. Maybe 10k sats for max exposure of $5? Maybe 5k if we want to go a little lower?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picked up 5k as we might expect this threshold to become more sensible with time for the average node operator.
44e3137
to
cf0c5c9
Compare
New default is breaking tests, gonna fix that. |
You can also simply override the default to a higher value in the default config constructor in functional_test_utils. |
Beyond fixes, add a supplemental commit to generalize the new setting on incoming dust HTLC on counterparty commitment transaction. I'm still toying with the idea of introducing a new |
Codecov Report
@@ Coverage Diff @@
## main #1009 +/- ##
==========================================
+ Coverage 90.79% 92.50% +1.71%
==========================================
Files 61 63 +2
Lines 31578 37857 +6279
==========================================
+ Hits 28672 35021 +6349
+ Misses 2906 2836 -70
Continue to review full report at Codecov.
|
lightning/src/ln/channel.rs
Outdated
@@ -2032,6 +2047,11 @@ impl<Signer: Sign> Channel<Signer> { | |||
} | |||
} | |||
|
|||
if inbound_dusted_htlc_msat + msg.amount_msat > self.get_max_balance_dust_htlc_msat() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand why we need to enforce this on the inbound leg. It should be sufficient to enforce it only on outbound HTLCs - ensuring our balance isn't dusted heavily. If our counterparty wants to burn all their balance to dust, that's on them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ariard pointed out that this matters if we have to go claim the funds from this HTLC after it was forwarded onwards. The funds may be our counterparty's balance, but once we receive the preimage from the place we forwarded the HTLC to, the funds are now "ours" and may not be in the commitment transaction. Thus, this does need to be here.
lightning/src/util/config.rs
Outdated
/// Applied on outgoing dust HTLCs and incoming dust HTLCs committed on the counterparty | ||
/// commitment transaction. | ||
/// | ||
/// Dust HTLCs are defined as any HTLC lower than counterparty's `dust_limit_satoshis` summed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users have no idea what dust_limit_satoshis
is, or why it matters at all what the sum of dust HTLCs is. We need to say something that describes, without assuming any significant knowledge of lightning, why we limit the dust balance to some low value (that those funds go to miners, and that a malicious miner could forward lots of small payments through us that miners claim as additional fee revenue if we force-close).
Oops, the "approve" check shouldn't have been marked on that one. I mean Concept ACK, but I left comments :). |
0db41f0
to
3faf229
Compare
max_outbound_dusted_htlc_msat
max_balance_dust_htlc_msat
lightning/src/ln/channel.rs
Outdated
} | ||
} | ||
|
||
if msg.amount_msat <= (self.feerate_per_kw as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) * 1000 + self.holder_dust_limit_satoshis { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through a lot of back-and-forth on the utility to also check the dust balance on the holder transaction as it's implicitly protected by the check on the commitment transaction, with our current assumption that holder's dust_limit_satoshis
is always under counterparty's one.
Though finally I left it, as it gives us more belt-and-suspender if we touch our config settings/implementation default in the future.
Without, as of today, the worst-fee discrepancy would be 344_000 msat as explained in this Big Comment(tm): https://github.com/ariard/rust-lightning/blob/0db41f057ac4967f72646cc1e77ee59280fbf07c/lightning/src/ln/channel.rs#L2066
See lastest squashed branch at 3faf229, addressing review comment and hopefully exhaustive test coverage! |
3faf229
to
5158a1c
Compare
Needs rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Few doc comments and one actual code comment, haven't reviewed the test yet.
lightning/src/ln/channel.rs
Outdated
@@ -2032,6 +2061,20 @@ impl<Signer: Sign> Channel<Signer> { | |||
} | |||
} | |||
|
|||
if msg.amount_msat / 1000 < (self.feerate_per_kw as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we agree to a multiplier here? Ie instead of checking with the current feerate, check with the current feerate times 2/4/whatever to prevent feerate increases from blowing up our dust? I think we should set the multiplier pretty high so that we can remove the counterparty feerate check entirely and only ever check the dust threshold.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit: I think we agree in principle, but not on the heuristics to pick up an adequate feerate. In fact I'm not sure it makes sense to take a multiplier against the current feerate as this one might be already in the higher percentiles of the mempool feerate groups, thus allowing this HTLC to consume all our dust bandwidth.
Maybe we should instead give us a multiplier of the dust balance against a fixed point of 253 sat/kWU ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the reason I like a multiplier is that, without any other information, if the feerate is high, it seems likely the feerate may increase significantly. If the feerate is low, probably it will only increase by 10sat/vb or so.
We could do something nonlinear, but I'm not a fan of a straight constant. Maybe we could always increase by max(10 sat/vb, 0.25x current feerate)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so implemented the evaluation against a preventive high-feerate, note tests coverage has been modified in consequence to avoid the newly inflated check of counterparty dust limit satoshi blocking failure against the holder limit (508_000 -> 2_300_000)
let pending_remote_value_msat =
@@ -3482,6 +3496,10 @@ impl<Signer: Sign> Channel<Signer> {
self.feerate_per_kw
}
+ pub fn get_dust_buffer_feerate(&self) -> u32 {
+ cmp::max(2530, self.feerate_per_kw * 1250 / 1000)
+ }
+
lightning/src/util/config.rs
Outdated
/// Limit holder balance exposure to dusted HTLCs at any given time. | ||
/// | ||
/// Applied on at HTLC reception/forwarding, *each* on both-sides commitment transactions. | ||
/// | ||
/// Dust HTLCs are defined as any HTLC lower than counterparty's `dust_limit_satoshis` or | ||
/// holder's `dust_limit_satoshis` summed up with their expected onchain claim cost at | ||
/// negotiated feerate (`feerate_per_kw` * HTLC_*_TX_WEIGHT * 1000 / 1000). | ||
/// | ||
/// This limit does exist to prevent holder balance burnt to miners fees if the link is | ||
/// broken, either due to a accidental force-closure or a counterparty maliciously | ||
/// cooperating with miners. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This includes way too much technical detail about the implementation and not enough detail about what this means for users.
/// Limit holder balance exposure to dusted HTLCs at any given time. | |
/// | |
/// Applied on at HTLC reception/forwarding, *each* on both-sides commitment transactions. | |
/// | |
/// Dust HTLCs are defined as any HTLC lower than counterparty's `dust_limit_satoshis` or | |
/// holder's `dust_limit_satoshis` summed up with their expected onchain claim cost at | |
/// negotiated feerate (`feerate_per_kw` * HTLC_*_TX_WEIGHT * 1000 / 1000). | |
/// | |
/// This limit does exist to prevent holder balance burnt to miners fees if the link is | |
/// broken, either due to a accidental force-closure or a counterparty maliciously | |
/// cooperating with miners. | |
/// Limit our total exposure to in-flight HTLCs which are burned to fees as they are too small. | |
/// to claim on-chain. | |
/// | |
/// When an HTLC present in one of our channels is below a "dust" threshold, the HTLC will not | |
/// be claimable on-chain, instead being turned into additional miner fees if either party | |
/// force-closes the channel. Because the threshold is per-HTLC, our total exposure to such | |
/// payments may be substantial if there are many dust HTLCs present when the channel is | |
/// force-closed. | |
/// | |
/// This limit is applied for send, forwarded, and received HTLCs and limits the total exposure | |
/// across all three types per-channel. Setting this too low may prevent the sending or receipt | |
/// of low-value HTLCs on high-traffic nodes, and is very important if your counterparty is a | |
/// miner (as they could benefit from such dust HTLCs becoming miner fees). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better, though I would recall the last part about counterparty=miner being a worrying concern is kind of helpless. Even assuming you can effectively identify your LN counterparty, you can't exclude unseen out-of-band negotiation with a miner pool to introduce such non-propagated "dust-stealing tx".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I think you're right. It would give users too much of a false sense of security.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep modified a bit.
ec96b78
to
ad611ab
Compare
This allows TLV serialization macros to read non-Option-wrapped types but allow them to be missing, filling them in with the provided default value as needed.
ad611ab
to
6876991
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No functional issues, only docs/logs/git issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits, overall, this makes a lot of sense to me as a precursor to lightning/bolts#873.
Still grokking but I do wonder if the dust_buffer_feerate
deserves more thought/is more suited to its own PR, like should we also use it in dust calculations like in next_remote_commit_tx_fee_msat
?
/// An enum gathering stats on pending HTLCs, either inbound or outbound side. | ||
struct HTLCStats { | ||
pending_htlcs: u32, | ||
pending_htlcs_value_msat: u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggested nit: s/value/balance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think balance includes either the to_local
or to_remote
output so would like to avoid blurring meanings here.
lightning/src/ln/channel.rs
Outdated
on_counterparty_tx_dust_balance: u64, | ||
on_holder_tx_dust_balance: u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: get rid of the on_
prefixes, and add _msat
at the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to on_holder_tx_dust_exposure_msat
/on_counterparty_tx_dust_exposure_msat
. I concede the on_
prefix can be redundant but it's easy for a reviewer to interpret dust_balance
as coming from holder channel policy imo
lightning/src/util/config.rs
Outdated
@@ -218,6 +234,7 @@ impl Default for ChannelConfig { | |||
cltv_expiry_delta: 6 * 12, // 6 blocks/hour * 12 hours | |||
announced_channel: false, | |||
commit_upfront_shutdown_pubkey: true, | |||
max_balance_dust_htlc_msat: 5_000_000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make a const
of this number and use it everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the interest of a configuration setting, if a channel is opened with a trusted/highly-reliable peer, this number could be considerably lifted up and as such increase the dust bandwidth available on this link. The reasoning holds also in the other sense, a LDK node operator could consider the number picked up by us too high in function of their economic capabilities and would like to reduce.
lightning/src/ln/channel.rs
Outdated
@@ -2139,6 +2139,20 @@ impl<Signer: Sign> Channel<Signer> { | |||
} | |||
} | |||
|
|||
if msg.amount_msat / 1000 < (self.get_dust_buffer_feerate() as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: prefer making (self.get_dust_buffer_feerate() as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis
into a variable and below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean in this function call ? I think it's used only once, as the following check is based on HTLC_SUCCESS_TX_WEIGHT
?
lightning/src/ln/channel.rs
Outdated
@@ -2098,8 +2139,22 @@ impl<Signer: Sign> Channel<Signer> { | |||
} | |||
} | |||
|
|||
if msg.amount_msat / 1000 < (self.get_dust_buffer_feerate() as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis { | |||
if inbound_stats.on_counterparty_tx_dust_balance + outbound_stats.on_counterparty_tx_dust_balance + msg.amount_msat > self.get_max_balance_dust_htlc_msat() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why not make get_max_balance_dust_htlc_msat
include transaction fees for paying for said HTLCs, and instead check that the total dust HTLC values + their fees don't violate max_balance_dust_htlc_msat
? Not fully thought out but to me that seems like it would have more protection against burning $ to miners
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if understand well your point but this check does include the transaction fees potentially paying for said HTLCs with the following equation component : (self.get_dust_buffer_feerate() as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000)
? It's just we use a preventive higher-feerate to anticipate fees spikes during the lifetime of the HTLC on the commitment transactions.
6876991
to
5eb6cce
Compare
Updated at 5eb6cce, incorporating lastest reviews. |
I don't think so, this is a newer "dust" definition whereas the one in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically LGTM. Would be nice to fix fuzz
/// This limit is applied for sent, forwarded, and received HTLCs and limits the total | ||
/// exposure across all three types per-channel. Setting this too low may prevent the | ||
/// sending or receipt of low-value HTLCs on high-traffic nodes, and this limit is very | ||
/// important to prevent stealing of dust HTLCs by miners. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Stealing" might sound a little intense to users. Is there another way of phrasing it? lol
This should fix the fuzz test:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from one comment and the fuzz failure I pasted a patch for.
Trimmed-to-dust HTLCs are at risk of being burnt as miner fees at anytime during their lifetime due to the broadcast of either holder commitment transaction or counterparty's one. To hedge against this risk, we introduce a new config setting `max_balance_dust_htlc_msat`, with the initial value of 5_000_000 msat.
5eb6cce
to
d282586
Compare
At `update_add_htlc()`/`send_htlc()`, we verify that the inbound/ outbound dust or the sum of both, on either sides of the link isn't above new config setting `max_balance_dust_htlc_msat`. A dust HTLC is hence defined as a trimmed-to-dust one, i.e including the fee cost to publish its claiming transaction.
d282586
to
730f6f3
Compare
First step to support lightning/bolts#873