Consider anchor outputs value throughout balance checks and computations #2674

wpaulino · 2023-10-19T17:59:38Z

codecov-commenter · 2023-10-19T18:11:03Z

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.

fuzz/src/chanmon_consistency.rs

lightning/src/ln/payment_tests.rs

lightning/src/ln/channel.rs

benthecarman · 2023-10-20T16:30:41Z

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

wpaulino · 2023-10-20T16:56:43Z

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 · 2023-10-20T17:07:57Z

Seems like a thing to do in #2618.

TheBlueMatt · 2023-10-20T17:19:48Z

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.

lightning/src/ln/channel.rs

jkczyz · 2023-10-20T21:02:44Z

lightning/src/ln/channel.rs

 		};
 		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 {


Why not adjust for this in commit_tx_fee_msat?

Same as above

lightning/src/ln/channel.rs

0.0.118 - Oct 23, 2023 - "Just the Twelve Sinks" API Updates =========== * BOLT12 sending and receiving is now supported as an alpha feature. You may run into unexpected issues and will need to have a direct connection with the offer's blinded path introduction points as messages are not yet routed. We are seeking feedback from early testers (lightningdevkit#2578, lightningdevkit#2039). * `ConfirmationTarget` has been rewritten to provide information about the specific use LDK needs the feerate estimate for, rather than the generic low-, medium-, and high-priority estimates. This allows LDK users to more accurately target their feerate estimates (lightningdevkit#2660). For those wishing to retain their existing behavior, see the table below for conversion. * `ChainHash` is now used in place of `BlockHash` where it represents the genesis block (lightningdevkit#2662). * `lightning-invoice` payment utilities now take a `Deref` to `AChannelManager` (lightningdevkit#2652). * `peel_onion` is provided to statelessly decode an `OnionMessage` (lightningdevkit#2599). * `ToSocketAddrs` + `Display` are now impl'd for `SocketAddress` (lightningdevkit#2636, lightningdevkit#2670) * `Display` is now implemented for `OutPoint` (lightningdevkit#2649). * `Features::from_be_bytes` is now provided (lightningdevkit#2640). For those moving to the new `ConfirmationTarget`, the new variants in terms of the old mempool/low/medium/high priorities are as follows: * `OnChainSweep` = `HighPriority` * `MaxAllowedNonAnchorChannelRemoteFee` = `max(25 * 250, HighPriority * 10)` * `MinAllowedAnchorChannelRemoteFee` = `MempoolMinimum` * `MinAllowedNonAnchorChannelRemoteFee` = `Background - 250` * `AnchorChannelFee` = `Background` * `NonAnchorChannelFee` = `Normal` * `ChannelCloseMinimum` = `Background` Bug Fixes ========= * Calling `ChannelManager::close_channel[_with_feerate_and_script]` on a channel which did not exist would immediately hang holding several key `ChannelManager`-internal locks (lightningdevkit#2657). * Channel information updates received from a failing HTLC are no longer applied to our `NetworkGraph`. This prevents a node which we attempted to route a payment through from being able to learn the sender of the payment. In some rare cases, this may result in marginally reduced payment success rates (lightningdevkit#2666). * Anchor outputs are now properly considered when calculating the amount available to send in HTLCs. This can prevent force-closes in anchor channels when sending payments which overflow the available balance (lightningdevkit#2674). * A peer that sends an `update_fulfill_htlc` message for a forwarded HTLC, then reconnects prior to sending a `commitment_signed` (thus retransmitting their `update_fulfill_htlc`) may result in the channel stalling and being unable to make progress (lightningdevkit#2661). * In exceedingly rare circumstances, messages intended to be sent to a peer prior to reconnection can be sent after reconnection. This could result in undefined channel state and force-closes (lightningdevkit#2663). Backwards Compatibility ======================= * Creating a blinded path to receive a payment then downgrading to LDK prior to 0.0.117 may result in failure to receive the payment (lightningdevkit#2413). * Calling `ChannelManager::pay_for_offer` or `ChannelManager::create_refund_builder` may prevent downgrading to LDK prior to 0.0.118 until the payment times out and has been removed (lightningdevkit#2039). Node Compatibility ================== * LDK now sends a bogus `channel_reestablish` message to peers when they ask to resume an unknown channel. This should cause LND nodes to force-close and broadcast the latest channel state to the chain. In order to trigger this when we wish to force-close a channel, LDK now disconnects immediately after sending a channel-closing `error` message. This should result in cooperative peers also working to confirm the latest commitment transaction when we wish to force-close (lightningdevkit#2658). Security ======== 0.0.118 expands mitigations against transaction cycling attacks to non-anchor channels, though note that no mitigations which exist today are considered robust to prevent the class of attacks. * In order to mitigate against transaction cycling attacks, non-anchor HTLC transactions are now properly re-signed before broadcasting (lightningdevkit#2667). In total, this release features 61 files changed, 3470 insertions, 1503 deletions in 85 commits from 12 authors, in alphabetical order: * Antonio Yang * Elias Rohrer * Evan Feenstra * Fedeparma74 * Gursharan Singh * Jeffrey Czyz * Matt Corallo * Sergi Delgado Segura * Vladimir Fomene * Wilmer Paulino * benthecarman * slanesuke

wpaulino added this to the 0.0.118 milestone Oct 19, 2023

TheBlueMatt reviewed Oct 19, 2023

View reviewed changes

fuzz/src/chanmon_consistency.rs Show resolved Hide resolved

TheBlueMatt reviewed Oct 19, 2023

View reviewed changes

lightning/src/ln/payment_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

TheBlueMatt self-assigned this Oct 19, 2023

wpaulino force-pushed the consider-anchor-outputs-value-balances branch from 90da6cc to 568f169 Compare October 19, 2023 20:43

wpaulino added 5 commits October 20, 2023 11:04

Run chanmon_consistency_test with anchor outputs channels

37a3a03

Consider anchor outputs value in get_available_balances

d7672d4

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.

Consider anchor outputs value on inbound HTLCs

297390a

This could lead us to accept HTLCs that would put the sender below their reserve, which must never happen.

Consider anchor outputs value on channel open

834f4d7

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

Only account for fee spike buffer multiple on non-anchor channels

27fba2d

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 force-pushed the consider-anchor-outputs-value-balances branch from 568f169 to 27fba2d Compare October 20, 2023 18:04

arik-so reviewed Oct 20, 2023

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

lightning/src/ln/channel.rs Show resolved Hide resolved

TheBlueMatt approved these changes Oct 20, 2023

View reviewed changes

jkczyz reviewed Oct 20, 2023

View reviewed changes

jkczyz approved these changes Oct 20, 2023

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

TheBlueMatt merged commit fba99b7 into lightningdevkit:main Oct 20, 2023

wpaulino deleted the consider-anchor-outputs-value-balances branch October 20, 2023 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider anchor outputs value throughout balance checks and computations #2674

Consider anchor outputs value throughout balance checks and computations #2674

wpaulino commented Oct 19, 2023

codecov-commenter commented Oct 19, 2023 •

edited

Loading

benthecarman commented Oct 20, 2023

wpaulino commented Oct 20, 2023 •

edited

Loading

TheBlueMatt commented Oct 20, 2023

TheBlueMatt commented Oct 20, 2023

jkczyz Oct 20, 2023

wpaulino Oct 20, 2023

Consider anchor outputs value throughout balance checks and computations #2674

Consider anchor outputs value throughout balance checks and computations #2674

Conversation

wpaulino commented Oct 19, 2023

codecov-commenter commented Oct 19, 2023 • edited Loading

Codecov Report

benthecarman commented Oct 20, 2023

wpaulino commented Oct 20, 2023 • edited Loading

TheBlueMatt commented Oct 20, 2023

TheBlueMatt commented Oct 20, 2023

jkczyz Oct 20, 2023

Choose a reason for hiding this comment

wpaulino Oct 20, 2023

Choose a reason for hiding this comment

codecov-commenter commented Oct 19, 2023 •

edited

Loading

wpaulino commented Oct 20, 2023 •

edited

Loading