Add a max_dust_htlc_exposure_msat

[ariard]

No description provided.

[niftynei]

This should be

Suggested change

	- if the `dust_balance_on_holder_tx` at the new `dust_buffer_feerate` is superior to the `max_dust_htlc_exposure_msat`:
	- if the `dust_balance_on_holder_tx` at the new `feerate_per_kw` is superior to the `max_dust_htlc_exposure_msat`:

The real question is if the new feerate pushes the balance over the exposure limit, not if the new feerate + buffer does. Pushing a feerate update where the new buffer ceiling is above the current should just fail to add any new dusty htlcs, but will leave the existing ones without a problem. Otherwise what was the point of having the buffer in the first place?

[ariard]

Yes you're right that's the spec doesn't match our LDK code on that: https://github.com/rust-bitcoin/rust-lightning/blob/7aa2caccd884cd7d40ee146323c2a65b7ea39407/lightning/src/ln/channel.rs#L3130

Pushing a feerate update where the new buffer ceiling is above the current should just fail to add any new dusty htlcs, but will leave the existing ones without a problem.

Yes that's right. If the inbound/outbound feerate is above the the dust buffer feerate ceiling, the dust exposure on counterparty and holder commitment transactions should be evaluated. If one of them is above the dust HTLC exposure the channel could be fail (I think a MAY is better).

We had multiple back-and-forth on the LDK-side about the design of update_fee reception mitigations.

Updating the PR.

[ariard]

Updated at 6a1cb21. If you think there are more corrections needed, let me know.

[niftynei]

imo this whole section should be moved into the update_add_htlc requirements, as they're actions that are triggered by the receipt of this message.

[ariard]

Noted, I'll update this PR in that sense.

[ariard]

Well, in fact it's not exactly clear that it should be in the update_add_htlc requirements as it's a relay requirement, to me in the same category of cltv_expiry_delta selection. At least was my thinking when I wrote those lines.

Though it can be a source of confusion and maybe we should even add a new BOLT4 message. Due to lack of that, I remember it was hard to assess that max_dust_htlc_exposure_msat was well in-place when I verified fixes against Eclair in a black-box way.

[niftynei]

yes but you don't enact these spec requirements until a update_add_htlc message is received.

[niftynei]

Suggested change

	- MAY fail the channel
	- SHOULD fail the channel

[ariard]

See the rational just under. That's a MAY as automatic-closure might bring newer issues. A third-party to a channel could relay and withhold HTLCs across the link to provoke a close.

See also Eclair new config settings close-on-update-fee-dust-exposure-overflow : https://github.com/ACINQ/eclair/pull/1985/files, which is false by default. Though in LDK we do auto-close by default.

As it's arbitrating between two safety risks, I think it's wiser to defer the choice to the implementator.

[niftynei]

Well 'SHOULD' is still a deferral to the implementor, just a more strongly worded form!

[niftynei]

Suggested change

	- MAY fail the channel
	- SHOULD fail the channel

[ariard]

See #919 (comment)

[TheBlueMatt]

I don't think the spec needs to be this prescriptive. This is what we do in LDK, but others can/should do other things. I think we the spec should just talk generally about feerate_per_kw and note that in calculating you SHOULD provide some buffer for future feerate increases.

[rustyrussell]

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 #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.

[niftynei]

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.

[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.

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.

[t-bast]

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 ™️ 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.

[Christewart]

i think this might be formatted wrong? Here is what it looks like to me, i think the FAIL should be be indented under this? Sorry if I'm totally misunderstanding

[ariard]

You're right, that's a formatting issue! Should be better now :)

[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.

[t-bast]

ACK 7705356

I do think there is value in merging this PR since:

• this is a non-trivial issue that implementers should know about: we help new protocol developers ramp up more quickly by mentioning this explicitly in the spec
• we've been able to reduce this to a small enough change that it doesn't add too much clutter to the existing spec

But I believe we should have an ACK from cln or lnd to move forward with adding that change.

[ariard]

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

[Roasbeef]

This was brought up in the latest spec meeting (re better go-to-chain heuristics factoring in chain unit economics): #1082

[ariard]

This was brought up in the latest spec meeting (re better go-to-chain heuristics factoring in chain unit economics): #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.

[niftynei]

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

[rustyrussell]

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 7705356

[ariard]

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.

[ProofOfKeags]

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.

[ariard]

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

[t-bast]

ACK 7705356