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?

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

[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

[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

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

[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

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

[Roasbeef]

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

[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

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

[t-bast]

ACK 7705356