bolts icon indicating copy to clipboard operation
bolts copied to clipboard

Add a `max_dust_htlc_exposure_msat`

Open ariard opened this issue 2 years ago • 6 comments

ariard avatar Oct 04 '21 15:10 ariard

I don't think any implementer reading this will know how to implement it; it is too loose on requirements, and risks channel breakage as a result.

Ideally, Alice wants to set max_htlc_dust_exposure_msat and never have dust greater than that. Unfortunately, this isn't possible due to the async nature of updates (see https://github.com/lightningnetwork/lightning-rfc/pull/867): they can both add dust at once.

She can, however, limit how much Bob can send.

So this should simply say:

  1. (On sending update_add_htlc): You MUST NOT send an HTLC to a peer which would increase the total dust on sent HTLCs in the peer's commitment transaction above max_received_dust_msat.
  2. (On receiving update_add_htlc): If the HTLC would increase the total dust on received HTLCs in the node's commitment transaction above max_received_dust_msat: MAY send a warning and close the connection, otherwise MUST fail the channel.
  3. (On sending update_fee): MUST NOT send an update_fee which would increase the total dust on sent HTLCs in the peer's commitment transaction above max_received_dust_msat. MAY send an update-fee which will increase the total dust on its own commitment transaction above max_received_dust_msat.
  4. (On receiving update_fee): if the update_fee would increase the total dust on received HTLCs in the node's commitment transaction above max_received_dust_msat: MAY send a warning and close the connection, otherwise MUST fail the channel.

Now, this amount has not been defined anywhere! I am reluctant to add YA random value send on connection (and it won't help existing deployments), so perhaps we define it as 0.5% of the channel capacity, and gate it on a feature bit? This may make tiny channels unusable: if we care, the language above should be changed to always allow a single dust HTLC.

rustyrussell avatar Oct 11 '21 19:10 rustyrussell

Having a uniform dust threshold for both peers seems strictly nicer imo than having it be arbitrarily selected per peer; in fact the newly proposed set of rules from @rustyrussell makes it such that you'd need to know what the peer's dust threshold is so as to not exceed it when deciding whether to send an update_add_htlc message. Using different numbers here would result in a channel closure.

This points to the fact that interop is a bit problematic here given that ariard’s rules have already shipped in a few clients; if our peer is using ariard’s rules and has chosen a dust theshold that’s higher than ours, we’ll fail the channel/close the connection when they send an “ok for their dust threshold, but not for ours” htlc; this value is currently arbitrarily set by the peer.

Given that Rusty's proposal uses an implicit (0.5% of chan balance or thereabouts) variable for 'consensus' so to speak, am I right in thinking we'd need a feature flag and some way to migrate existing channels over to this policy? It's going to be a lot slower to roll out if that's the case.

niftynei avatar Oct 18 '21 14:10 niftynei

I don't think any implementer reading this will know how to implement it; it is too loose on requirements, and risks channel breakage as a result.

Yes having to arbiter between "channel breakage" and "balance burning" risks have been laid out in the update_fee reception section at least.

Ideally, Alice wants to set max_htlc_dust_exposure_msat and never have dust greater than that. Unfortunately, this isn't possible due to the async nature of updates (see #867): they can both add dust at once.

Yes, also a peer can batch dust add. As such, the max_htlc_dust_exposure_msat should be evaluated against the sum of both announced/committed dust HTLCs.

(On sending update_add_htlc): You MUST NOT send an HTLC to a peer which would increase the total dust on sent HTLCs in the peer's commitment transaction above max_received_dust_msat. (On receiving update_add_htlc): If the HTLC would increase the total dust on received HTLCs in the node's commitment transaction above max_received_dust_msat: MAY send a warning and close the connection, otherwise MUST fail the channel.

I think the checks should be done on both peer and node's commitment transactions ? If the peer's dust_limit_satoshis is lower than the node's one, the max_received_dust_msat could be reached on their side but not on our. If we're forwarding those "dust-on-peer's commitment" HTLCs we're at risk of a forwarded HTLC balance loss.

Now, this amount has not been defined anywhere! I am reluctant to add YA random value send on connection (and it won't help existing deployments), so perhaps we define it as 0.5% of the channel capacity, and gate it on a feature bit?

As a downside, a "consenus" value doesn't encompass node operators subjective loss threshold and the node's channels topology. E.g, if you have a high-degree of confidence in the peer's reliability and trustworthiness you might accept double-digits of the channel capacity as a max_received_dust_msat.

If we want to account node operators risk preferences better, of course we can announce this new value at channel opening ? Though, only work for future deployment, in the lack of dynamic upgrades.

ariard avatar Oct 20 '21 14:10 ariard

Now, this amount has not been defined anywhere! I am reluctant to add YA random value send on connection (and it won't help existing deployments), so perhaps we define it as 0.5% of the channel capacity, and gate it on a feature bit? This may make tiny channels unusable: if we care, the language above should be changed to always allow a single dust HTLC.

It's not only tiny channels, if the feerate rises too much it becomes a real issue for a lot of channels. At 50 sat/byte the trim threshold is around 9 000 sats, so any channel below 1 800 000 sat won't accept any HTLC. At 75 sat/byte the treshold is around 13 000 sats, so any channel below 2 600 000 sat won't accept any HTLC. And we've seen sustained feerates as high as that less than a year ago...

Unless we add a new rule as you suggest to allow at least one HTLC.

However, since we're all migrating to anchor_outputs_zero_fee_htlcs soon :tm: I'm not sure it's worth bike-shedding this too much. We won't have this issue since dust thresholds won't depend on the feerate anymore. It also removes the update_fee issue completely (and it is IMO the most problematic one because it leads to channels closing). At that point we only have to sometimes fail incoming htlcs instead of relaying them, which is a useful mechanism to implement anyway for throttling / mitigating channel jamming.

I think this parameter really should be configurable by each node operator based on their risk aversion and their channel size. If I have a 10 BTC channel, I really don't want to risk burning 50 mbtc in dust (0.5%), I want to be able to set it to a much lower value.

I don't think it's worth communicating to your peers either, since it's harmless when they send you HTLCs that overflow your tolerance, they simply end up being failed. And I do see node operators updating this value over time (maybe as they build trust with their peer) which means it would need to allow updates, which is a pain.

t-bast avatar Oct 20 '21 15:10 t-bast

Finally updated at 0ed8805.

The level of dust exposure is recommended to be evaluated against a node-setting max_dust_htlc_exposure_msat, instead to introduce a new channel parameter. The prescriptive description of dust_buffer_feerate has been removed, as commented before and is deferred to implementors.

I think few open review questions :

  • describe more precisely the dust balance on holder/counterparty transactions ?
  • make it more clear the mitigation distinction between pre/post-option_anchors_zero_htlc channels ?

ariard avatar Jun 20 '22 14:06 ariard

Thanks for the review. Fixed all comments, if you have suggestions for the wording happy to take them!

ariard avatar Sep 12 '22 22:09 ariard

I created a commit here with many cosmetic changes (use same terminology as the existing spec, rephrase some sentences that were unclear to me, etc). Most of those are very subjective, so feel free to judge whether it's better or not!

t-bast avatar Oct 10 '22 16:10 t-bast

Many thanks for the edit. Updated the local branch with it at 5fb0b5c. Still happy if we can get in, as we might have to reconsider our per-implementation max_dust_htlc_exposure_msat if something like "ephemeral anchors" gets in during the coming years: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/021046.html It might change the attacker set of the dust exposure.

ariard avatar Oct 24 '22 17:10 ariard

Updated at 219adca

Reviewed back the recommendations against LDK logic as part of our option_anchors_zero_fee_htlc_tx output support. Added new precision in consequence, especially when receiving update_fee, there is no need to reject the message if option_anchors_zero_fee_htlc_tx has been negotiated.

ariard avatar Dec 09 '22 02:12 ariard

Updated at 7705356.

If we think there is no more value in adding a clarification in the specification about dust exposure, I can close it. On the other-side, while looking on the validating lightning signer code recently, it turns out to be affected by the same security issue (https://lists.linuxfoundation.org/pipermail/lightning-dev/2023-January/003829.html) so I wouldn't be surprised people writing that type of auxiliary lightning softwares to screw up again, if not warned.

ariard avatar Feb 13 '23 02:02 ariard

I’ll close this PR. Like said I know where are the flaws, not my issue if the wider ecosystem forgets about them.

ariard avatar May 29 '23 23:05 ariard

This was brought up in the latest spec meeting (re better go-to-chain heuristics factoring in chain unit economics): https://github.com/lightning/bolts/issues/1082

Roasbeef avatar Jun 01 '23 22:06 Roasbeef

This was brought up in the latest spec meeting (re better go-to-chain heuristics factoring in chain unit economics): https://github.com/lightning/bolts/issues/1082

Good I think this is just missing one more review to be merged on top of the one of t-bast. On the long-term, I think transaction-relay policy rules could be relaxed to have dust outputs being spend in a single package a la ephemeral anchor.

ariard avatar Jun 05 '23 00:06 ariard

Spec meeting notes (#1098): pending review from @rustyrussell

niftynei avatar Jul 31 '23 20:07 niftynei

There are some nits I could pick (the naming makes it sound like a field on the wire, rather than an internal threshold), but it's basically sound. I like that it has fewer requirements with zero-fee-anchors, too.

Ack 77053564e66f83d47813d82de39fa3eef1824b1a

rustyrussell avatar Jul 31 '23 21:07 rustyrussell

If you have specific nits feel free to let them, I’ll address them or please add them with a follow-up PR.

Happy to add the recommendations we’re working for fee-bumping reserves management post-anchor we’re working for LDK as other spec recommendations, once we’re good with dust exposure here.

ariard avatar Aug 02 '23 03:08 ariard

Forgive me if I'm missing something, but this doesn't seem like a protocol change so much as a policy recommendation? The only thing I can imagine leaking into the protocol would be an error message that basically said it was overallocated on htlc exposure but I would imagine that would be (and should be) classified as a temp channel failure.

If it is a policy rec, I would like to second what @rustyrussell said and at least "unquote" the term, if not move it to a different section. That said, I do see how we already have precedence for this with the "cltv_expiry_delta selection" section.

ProofOfKeags avatar Aug 04 '23 22:08 ProofOfKeags

That said, I do see how we already have precedence for this with the "cltv_expiry_delta selection" section.

Note here the distinction between mechanism vs policy is maintained, as there is no mandatory selection of the value of max_dust_htlc_exposure, only laying out the “authorization” mechanism by which the per-channel dust exposure can be controlled.

Such authorization mechanism deployment alters the protocol behavior, and therefore qualifies as protocol change from my viewpoint. This is unclear to me what you qualify as a “pure” protocol change, as you note historically in the BOLT we have documented some variables selection, e.g you have other example such as fragmentation of punishment transaction post-anchor: https://github.com/lightning/bolts/blob/master/05-onchain.md#rationale-6

ariard avatar Aug 08 '23 00:08 ariard