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

Consider anchor outputs value throughout balance checks and computations #2674

Conversation

wpaulino
Copy link
Contributor

Fixes #2672.

@wpaulino wpaulino added this to the 0.0.118 milestone Oct 19, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (be8797e) 88.80% compared to head (27fba2d) 88.74%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2674      +/-   ##
==========================================
- Coverage   88.80%   88.74%   -0.06%     
==========================================
  Files         112      112              
  Lines       88282    88377      +95     
  Branches    88282    88377      +95     
==========================================
+ Hits        78397    78431      +34     
- Misses       7636     7689      +53     
- Partials     2249     2257       +8     
Files Coverage Δ
lightning/src/ln/monitor_tests.rs 97.93% <100.00%> (-0.49%) ⬇️
lightning/src/ln/channel.rs 88.40% <95.55%> (+0.02%) ⬆️
lightning/src/ln/payment_tests.rs 98.16% <97.61%> (-0.05%) ⬇️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightning/src/ln/payment_tests.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
@TheBlueMatt TheBlueMatt self-assigned this Oct 19, 2023
@wpaulino wpaulino force-pushed the consider-anchor-outputs-value-balances branch from 90da6cc to 568f169 Compare October 19, 2023 20:43
@benthecarman
Copy link
Contributor

Would be nice if the anchor outputs value was publicly exposed for a channel as well

@wpaulino
Copy link
Contributor Author

wpaulino commented Oct 20, 2023

Would be nice if the anchor outputs value was publicly exposed for a channel as well

It's a static value of 330 sats per output. We have a constant for it but it's not public in the API. Is there some place you had in mind where we should expose it? get_claimable_balances does not include that or fees in its balances.

@TheBlueMatt
Copy link
Collaborator

Seems like a thing to do in #2618.

@TheBlueMatt
Copy link
Collaborator

Do you want to go ahead and rebase on #2660?

This could lead us to sending/forwarding HTLCs that would put us below
our reserve, forcing our counterparty to close the channel on us due to
an invalid update.
This could lead us to accept HTLCs that would put the sender below
their reserve, which must never happen.
We should make sure the funding amount of a channel can cover all its
associated costs, including the value of anchor outputs, to make sure
that it is actually usable once "opened".
Anchor outputs channels are no longer susceptible to fee spikes as they
now mostly target the dynamic minimum mempool fee and can contribute the
remainder of fees when closing.
@wpaulino wpaulino force-pushed the consider-anchor-outputs-value-balances branch from 568f169 to 27fba2d Compare October 20, 2023 18:04
lightning/src/ln/channel.rs Show resolved Hide resolved
lightning/src/ln/channel.rs Show resolved Hide resolved
lightning/src/ln/channel.rs Show resolved Hide resolved
};
let commitment_feerate = fee_estimator.bounded_sat_per_1000_weight(commitment_conf_target);

let value_to_self_msat = channel_value_satoshis * 1000 - push_msat;
let commitment_tx_fee = commit_tx_fee_msat(commitment_feerate, MIN_AFFORDABLE_HTLC_COUNT, &channel_type);
if value_to_self_msat < commitment_tx_fee {
if value_to_self_msat.saturating_sub(anchor_outputs_value_msat) < commitment_tx_fee {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not adjust for this in commit_tx_fee_msat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

lightning/src/ln/channel.rs Show resolved Hide resolved
@TheBlueMatt TheBlueMatt merged commit fba99b7 into lightningdevkit:main Oct 20, 2023
15 checks passed
@wpaulino wpaulino deleted the consider-anchor-outputs-value-balances branch October 20, 2023 23:25
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.

Consider value of anchor outputs when computing balances and enforcing balance checks
6 participants