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

Tweak htlc dust exposure due to excess fees #3572

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

tankyleo
Copy link
Contributor

@tankyleo tankyleo commented Jan 29, 2025

    Remove extra addition of dust due to excess counterparty htlc tx fees
    
    In `can_accept_incoming_htlc`, `get_pending_htlc_stats`
    accounts for the incoming htlc. Therefore,
    `on_counterparty_tx_dust_exposure_msat` already includes
    the excess tx fees of the incoming non-dust htlc.

and

    Correct `per_outbound_htlc_counterparty_commit_tx_fee_msat`
    
    We account for the fees of the htlc transaction only in case the channel
    does _not_ support zero fee htlc transactions.

We account for the fees of the htlc transaction only in case the channel
does _not_ support zero fee htlc transactions.
@tankyleo tankyleo changed the title Tweak htlc dust exposure due to exceess fees Tweak htlc dust exposure due to excess fees Jan 29, 2025
@tankyleo
Copy link
Contributor Author

See commit 51bf78d for further background.

let htlc_dust_exposure_msat =
per_outbound_htlc_counterparty_commit_tx_fee_msat(excess_feerate, &self.context.channel_type);
let counterparty_tx_dust_exposure =
htlc_stats.on_counterparty_tx_dust_exposure_msat.saturating_add(htlc_dust_exposure_msat);
Copy link
Contributor

@wpaulino wpaulino Jan 30, 2025

Choose a reason for hiding this comment

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

IIUC, since can_accept_incoming_htlc is now called once it's been irrevocably committed on both sides, I think the dust exposure for this HTLC is already being accounted for in get_pending_htlc_stats. I think we just need to enforce the check below? Having a test would be helpful here to confirm the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the review. I mistakenly assumed can_accept_incoming_htlc == "can we add this htlc to the commit tx?"

But at this point in the flow, the htlc is already on the commit tx !

Now I struggle to understand the point of checking here whether that htlc pushes us over the dust limit if it has already been added to the commit tx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as i understand now, this is a limitation of the spec - you cannot really complain about dust limits in direct response to an update_add_htlc msg.

@tankyleo tankyleo force-pushed the 25-01-htlc-dust-exposure branch from 133e661 to 188f299 Compare January 31, 2025 21:30
@wpaulino
Copy link
Contributor

Thanks! Some test coverage here would still be nice if you're keen and it's not too much trouble.

@tankyleo tankyleo force-pushed the 25-01-htlc-dust-exposure branch from 907c100 to 188f299 Compare February 6, 2025 19:23
@tankyleo tankyleo requested a review from wpaulino February 8, 2025 00:56
@tankyleo tankyleo force-pushed the 25-01-htlc-dust-exposure branch 2 times, most recently from a80a769 to 41a54dc Compare February 9, 2025 17:26
In `can_accept_incoming_htlc`, `get_pending_htlc_stats` accounts for the
incoming htlc. Therefore, `on_counterparty_tx_dust_exposure_msat`
already includes the excess tx fees of the incoming non-dust htlc.
The widely used sats/kWU ratio is equivalent to msats/WU
The goal is to align `per_outbound_htlc_counterparty_commit_tx_fee_msat`
with the slope `commit_tx_fee_sat` / `num_htlcs`.

This is a minor detail - no existing LDK code multiplies the value
returned by `per_outbound_htlc_counterparty_commit_tx_fee_msat` with
`num_htlcs`.
Specifically, we check that the slope `commit_tx_fee_sat` / `num_htlcs`
is consistent with `per_outbound_htlc_counterparty_commit_tx_fee_msat`.
@tankyleo tankyleo force-pushed the 25-01-htlc-dust-exposure branch from 41a54dc to eae77c0 Compare February 9, 2025 20:59
@tankyleo tankyleo force-pushed the 25-01-htlc-dust-exposure branch from eae77c0 to b63b336 Compare February 9, 2025 21:02
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.

2 participants