Skip to content
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 a max_dust_htlc_exposure_msat #919

Merged
merged 4 commits into from
Aug 14, 2023

Conversation

ariard
Copy link

@ariard ariard commented Oct 4, 2021

No description provided.

- if the `dust_balance_on_counterparty_tx` at the new `dust_buffer_feerate` is superior to `max_dust_htlc_exposure_msat`:
- MAY NOT send `update_fee`
- MAY fail the channel
- if the `dust_balance_on_holder_tx` at the new `dust_buffer_feerate` is superior to the `max_dust_htlc_exposure_msat`:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Author

@ariard ariard Oct 4, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

To mitigate this scenario, a `max_dust_htlc_exposure_msat` must be apply at
HTLC sending, forwarding and receiving.

A node:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@@ -1127,6 +1190,17 @@ A receiving node:
current commitment transaction:
- SHOULD fail the channel,
- but MAY delay this check until the `update_fee` is committed.
- if the `dust_balance_on_counterparty_tx` at the new `dust_buffer_feerate` is superior to `max_dust_htlc_exposure_msat`:
- MAY fail the channel
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- MAY fail the channel
- SHOULD fail the channel

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

- if the `dust_balance_on_counterparty_tx` at the new `dust_buffer_feerate` is superior to `max_dust_htlc_exposure_msat`:
- MAY fail the channel
- if the `dust_balance_on_holder_tx` at the new `dust_buffer_feerate` is superior to the `max_dust_htlc_exposure_msat`:
- MAY fail the channel
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- MAY fail the channel
- SHOULD fail the channel

Copy link
Author

Choose a reason for hiding this comment

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

niftynei added a commit to niftynei/lightning that referenced this pull request Oct 4, 2021
And update some behavior to check both sides on receipt of a
update_fee, as per the proposed spec.

lightning/bolts#919
niftynei added a commit to niftynei/lightning that referenced this pull request Oct 4, 2021
And update some behavior to check both sides on receipt of a
update_fee, as per the proposed spec.

lightning/bolts#919
@ariard ariard force-pushed the 2021-08-max-dust-exposure branch from c5b7b89 to 6a1cb21 Compare October 4, 2021 23:49
- SHOULD NOT send this HTLC
- SHOULD fail this HTLC if it's forwarded

`dust_buffer_feerate` is defined as the maximum of either 2530 sats per kWU or
Copy link
Collaborator

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

niftynei added a commit to niftynei/lightning that referenced this pull request Oct 5, 2021
And update some behavior to check both sides on receipt of a
update_fee, as per the proposed spec.

lightning/bolts#919
niftynei added a commit to niftynei/lightning that referenced this pull request Oct 6, 2021
And update some behavior to check both sides on receipt of a
update_fee, as per the proposed spec.

lightning/bolts#919
niftynei added a commit to niftynei/lightning that referenced this pull request Oct 7, 2021
And update some behavior to check both sides on receipt of a
update_fee, as per the proposed spec.

lightning/bolts#919
@rustyrussell
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

ariard commented Oct 20, 2021

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
Copy link
Collaborator

t-bast commented Oct 20, 2021

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.


A node:
- upon an incoming HTLC:
- if a HTLC's `amount_msat` is inferior to the counterparty's `dust_limit_satoshis` plus the HTLC-timeout fee at the `dust_buffer_feerate`:
Copy link
Contributor

@Christewart Christewart Oct 21, 2021

Choose a reason for hiding this comment

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

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

Screen Shot 2021-10-21 at 3 55 29 PM

Copy link
Author

Choose a reason for hiding this comment

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

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

cdecker pushed a commit to cdecker/lightning that referenced this pull request Oct 22, 2021
And update some behavior to check both sides on receipt of a
update_fee, as per the proposed spec.

lightning/bolts#919
cdecker pushed a commit to cdecker/lightning that referenced this pull request Oct 22, 2021
And update some behavior to check both sides on receipt of a
update_fee, as per the proposed spec.

lightning/bolts#919
cdecker pushed a commit to ElementsProject/lightning that referenced this pull request Oct 23, 2021
And update some behavior to check both sides on receipt of a
update_fee, as per the proposed spec.

lightning/bolts#919
@ariard ariard force-pushed the 2021-08-max-dust-exposure branch from 6a1cb21 to 37f1540 Compare October 25, 2021 19:09
@ariard ariard force-pushed the 2021-08-max-dust-exposure branch from 219adca to 7705356 Compare February 13, 2023 02:41
@ariard
Copy link
Author

ariard commented Feb 13, 2023

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.

Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

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
Copy link
Author

ariard commented May 29, 2023

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

@ariard ariard closed this May 29, 2023
@Roasbeef
Copy link
Collaborator

Roasbeef commented Jun 1, 2023

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

@ariard
Copy link
Author

ariard commented Jun 5, 2023

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
Copy link
Collaborator

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

@rustyrussell
Copy link
Collaborator

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
Copy link
Author

ariard commented Aug 2, 2023

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
Copy link
Contributor

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
Copy link
Author

ariard commented Aug 8, 2023

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

Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK 7705356

@t-bast t-bast merged commit a1870e1 into lightning:master Aug 14, 2023
adi2011 pushed a commit to adi2011/bolts that referenced this pull request Sep 18, 2023
* Bound exposure to trimmed in-flight HTLCs
* Reject update_fee beyond max_dust_htlc_exposure_msat

Co-authored-by: t-bast <[email protected]>
adi2011 pushed a commit to adi2011/bolts that referenced this pull request Sep 18, 2023
* Bound exposure to trimmed in-flight HTLCs
* Reject update_fee beyond max_dust_htlc_exposure_msat

Co-authored-by: t-bast <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants