Separate ChannelDetails' outbound capacity from the next HTLC max #1435

TheBlueMatt · 2022-04-20T14:46:12Z

ChannelDetails::outbound_capacity_msat describes the total amount
available for sending across several HTLCs, basically just our
balance minus the reserve value maintained by our counterparty.
However, when routing we use it to guess the maximum amount we can
send in a single additional HTLC, which it is not.

There are numerous reasons why our balance may not match the amount
we can send in a single HTLC, whether the HTLC in-flight limit, the
channe's HTLC maximum, or our feerate buffer.

This commit splits the outbound_capacity_msat field into two -
outbound_capacity_msat and outbound_htlc_limit_msat, setting us
up for correctly handling our next-HTLC-limit in the future.

This also addresses the first of the reasons why the values may
not match - the max-in-flight limit. The inaccuracy is ultimately
tracked as #1126.

This is pulled out of #1434 and is really just enough of #1126 fixed so that we can do MPP tests more easily - because we default to a max in-flight limit of 10% of the channel size we need to consider the in-flight limit when routing so that we can really simply just ask the router to give us a route of, eg, 15 sats over two 100 sats channels and get back a valid route (currently it'll give us a route over one channel which will fail).

Also threw in a bonus cause we all like bonuses.

codecov-commenter · 2022-04-20T15:18:52Z

Codecov Report

Merging #1435 (ad683d8) into main (637fb88) will increase coverage by 0.11%.
The diff coverage is 100.00%.

❗ Current head ad683d8 differs from pull request most recent head 61629bc. Consider uploading reports for the commit 61629bc to get more accurate results

@@            Coverage Diff             @@
##             main    #1435      +/-   ##
==========================================
+ Coverage   90.88%   91.00%   +0.11%     
==========================================
  Files          75       75              
  Lines       41474    42633    +1159     
  Branches    41474    42633    +1159     
==========================================
+ Hits        37695    38797    +1102     
- Misses       3779     3836      +57

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`88.42% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.45% <100.00%> (+0.72%)`	⬆️
lightning/src/routing/router.rs	`92.51% <100.00%> (-0.09%)`	⬇️
lightning/src/util/events.rs	`33.56% <100.00%> (ø)`
lightning/src/chain/mod.rs	`59.25% <0.00%> (-1.86%)`	⬇️
lightning/src/routing/scoring.rs	`94.00% <0.00%> (-0.36%)`	⬇️
lightning-invoice/src/utils.rs	`96.70% <0.00%> (-0.15%)`	⬇️
lightning-background-processor/src/lib.rs	`95.11% <0.00%> (-0.11%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.08% <0.00%> (-0.07%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 637fb88...61629bc. Read the comment docs.

valentinewallace · 2022-04-20T16:04:06Z

lightning/src/ln/channelmanager.rs

+	/// to use a limit as close as possible to the HTLC limit we can currently send.
+	///
+	/// See also [`ChannelDetails::balance_msat`] and [`ChannelDetails::outbound_capacity_msat`].
+	pub outbound_htlc_limit_msat: u64,


I think outbound_per_htlc_limit_msatwould be clearer, kinda reads as the limit for all htlcs otherwise

Hmmm, per_htlc reads like a static limit on any HTLC, though, which this is not - its the limit for the next HTLC. Maybe I should add the word "next" again?

True. I'd prefer re-adding next

valentinewallace · 2022-04-20T16:27:39Z

lightning/src/ln/channel.rs

+		let outbound_capacity_msat = cmp::max(self.value_to_self_msat as i64
+				- outbound_stats.pending_htlcs_value_msat as i64
+				- self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) as i64 * 1000,


Would this ever want to incorporate commit_tx_fee_msat (and/or the fee spike buffer)?

Yes, it totally should! I have a WIP commit to do that but haven't had a chance to work on the tests.

lightning/src/ln/channel.rs

lightning/src/routing/router.rs

TheBlueMatt · 2022-04-21T18:18:21Z

Rebased and squashed with only conflicts fixed.

valentinewallace · 2022-04-21T20:48:24Z

CI sad, will re-ack after that and another reviewer

`ChannelDetails::outbound_capacity_msat` describes the total amount available for sending across several HTLCs, basically just our balance minus the reserve value maintained by our counterparty. However, when routing we use it to guess the maximum amount we can send in a single additional HTLC, which it is not. There are numerous reasons why our balance may not match the amount we can send in a single HTLC, whether the HTLC in-flight limit, the channe's HTLC maximum, or our feerate buffer. This commit splits the `outbound_capacity_msat` field into two - `outbound_capacity_msat` and `outbound_htlc_limit_msat`, setting us up for correctly handling our next-HTLC-limit in the future. This also addresses the first of the reasons why the values may not match - the max-in-flight limit. The inaccuracy is ultimately tracked as lightningdevkit#1126.

arik-so · 2022-04-27T15:36:40Z

lightning/src/ln/channel.rs

 	/// Doesn't bother handling the
 	/// if-we-removed-it-already-but-haven't-fully-resolved-they-can-still-send-an-inbound-HTLC
 	/// corner case properly.
 	/// The channel reserve is subtracted from each balance.
 	/// See also [`Channel::get_balance_msat`]
-	pub fn get_inbound_outbound_available_balance_msat(&self) -> (u64, u64) {
+	pub fn get_inbound_outbound_available_balance_msat(&self) -> (u64, u64, u64) {


this is getting a bit unwieldy. Tuples are great, but I think we may wanna introduce a custom response struct here with field labels.

Good idea! Went ahead and moved get_balance_msat into the same function while we're at it (and left it as a separate commit for that reason).

valentinewallace · 2022-04-27T16:35:58Z

CI benchmark sad

arik-so · 2022-04-27T16:53:01Z

lightning/src/ln/channel.rs

@@ -62,6 +62,17 @@ pub struct ChannelValueStat {
 	pub counterparty_dust_limit_msat: u64,
 }

+pub struct AvailableBalance {


maybe AvailableBalances or AvailableBalanceBreakdown or something, because otherwise the naming is confusing.

Hmm, its not a breakdown, though, is it? They're separate "available balances" not really a breakdown of a "total available balance"

arik-so · 2022-04-27T16:53:12Z

lightning/src/ln/channel.rs

-	/// The channel reserve is subtracted from each balance.
-	/// See also [`Channel::get_balance_msat`]
-	pub fn get_inbound_outbound_available_balance_msat(&self) -> (u64, u64) {
+	pub fn get_available_balance_msat(&self) -> AvailableBalance {


same as above here

arik-so

Re-ACKing

TheBlueMatt · 2022-04-27T18:44:04Z

Squashed the fixup commit without changes.

lightning/src/ln/channel.rs

Some simple code motion to clean up how channel balances get fetched.

TheBlueMatt · 2022-04-27T20:22:11Z

Lol, sorry, the old function docs were stale/wrong, I fixed them now....had to change the docs on the capacity fields:

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 32478ee31..3a12a2c06 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -58,23 +58,23 @@ pub struct ChannelValueStat {
        pub pending_outbound_htlcs_amount_msat: u64,
        pub pending_inbound_htlcs_amount_msat: u64,
        pub holding_cell_outbound_amount_msat: u64,
        pub counterparty_max_htlc_value_in_flight_msat: u64, // outgoing
        pub counterparty_dust_limit_msat: u64,
 }
 
 pub struct AvailableBalances {
        /// The amount that would go to us if we close the channel, ignoring any on-chain fees.
        pub balance_msat: u64,
-       /// Total amount available for our counterparty to send to us, ignoring HTLCs.
+       /// Total amount available for our counterparty to send to us.
        pub inbound_capacity_msat: u64,
-       /// Total amount available for us to send to our counterparty, ignoring HTLCs.
+       /// Total amount available for us to send to our counterparty.
        pub outbound_capacity_msat: u64,
        /// The maximum value we can assign to the next outbound HTLC
        pub next_outbound_htlc_limit_msat: u64,
 }
 
 #[derive(Debug, Clone, Copy, PartialEq)]
 enum FeeUpdateState {
        // Inbound states mirroring InboundHTLCState
        RemoteAnnounced,
        AwaitingRemoteRevokeToAnnounce,

TheBlueMatt added the Seeking Code Review label Apr 20, 2022

TheBlueMatt mentioned this pull request Apr 20, 2022

Improve Robustness of Inbound MPP Claims Across Restart #1434

Merged

TheBlueMatt force-pushed the 2022-04-1126-first-step branch from a1db341 to fae8b63 Compare April 20, 2022 14:55

TheBlueMatt force-pushed the 2022-04-1126-first-step branch from fae8b63 to 1218a85 Compare April 20, 2022 15:44

valentinewallace reviewed Apr 20, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-04-1126-first-step branch from 1218a85 to ace4693 Compare April 20, 2022 17:22

valentinewallace reviewed Apr 20, 2022

View reviewed changes

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

lightning/src/routing/router.rs Show resolved Hide resolved

TheBlueMatt force-pushed the 2022-04-1126-first-step branch from ace4693 to 24150ee Compare April 20, 2022 19:21

valentinewallace previously approved these changes Apr 21, 2022

View reviewed changes

TheBlueMatt dismissed valentinewallace’s stale review via 2a13cbd April 21, 2022 18:18

TheBlueMatt force-pushed the 2022-04-1126-first-step branch from 24150ee to 2a13cbd Compare April 21, 2022 18:18

TheBlueMatt force-pushed the 2022-04-1126-first-step branch from 2a13cbd to 95b2048 Compare April 21, 2022 20:54

TheBlueMatt added 2 commits April 25, 2022 15:04

Sort Event variants somewhat more sensibly

68ee7ef

TheBlueMatt force-pushed the 2022-04-1126-first-step branch from 95b2048 to 68ee7ef Compare April 25, 2022 15:04

TheBlueMatt mentioned this pull request Apr 26, 2022

Add test coverage for failure of inconsistent MPP parts #1451

Merged

arik-so reviewed Apr 27, 2022

View reviewed changes

TheBlueMatt removed the Seeking Code Review label Apr 27, 2022

TheBlueMatt force-pushed the 2022-04-1126-first-step branch from 01f5c46 to ed06722 Compare April 27, 2022 16:12

TheBlueMatt assigned arik-so and valentinewallace Apr 27, 2022

arik-so reviewed Apr 27, 2022

View reviewed changes

TheBlueMatt force-pushed the 2022-04-1126-first-step branch from 68a08f7 to ad683d8 Compare April 27, 2022 17:07

TheBlueMatt mentioned this pull request Apr 27, 2022

Do not force-close channels when we cannot communicate with peers #1429

Merged

arik-so previously approved these changes Apr 27, 2022

View reviewed changes

TheBlueMatt dismissed arik-so’s stale review via a375f64 April 27, 2022 18:44

TheBlueMatt force-pushed the 2022-04-1126-first-step branch from ad683d8 to a375f64 Compare April 27, 2022 18:44

arik-so previously approved these changes Apr 27, 2022

View reviewed changes

valentinewallace previously approved these changes Apr 27, 2022

View reviewed changes

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

Consolidate Channel balance fetching into one fn returning struct

61629bc

Some simple code motion to clean up how channel balances get fetched.

TheBlueMatt dismissed stale reviews from valentinewallace and arik-so via 61629bc April 27, 2022 20:21

TheBlueMatt force-pushed the 2022-04-1126-first-step branch from a375f64 to 61629bc Compare April 27, 2022 20:21

valentinewallace approved these changes Apr 28, 2022

View reviewed changes

arik-so approved these changes Apr 28, 2022

View reviewed changes

TheBlueMatt merged commit 62edee5 into lightningdevkit:main Apr 28, 2022

jurvis mentioned this pull request Jun 6, 2022

ChannelDetails::outbound_capacity_msat needs to be more exact #1126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate ChannelDetails' outbound capacity from the next HTLC max #1435

Separate ChannelDetails' outbound capacity from the next HTLC max #1435

TheBlueMatt commented Apr 20, 2022 •

edited

codecov-commenter commented Apr 20, 2022 •

edited

valentinewallace Apr 20, 2022

TheBlueMatt Apr 20, 2022

valentinewallace Apr 20, 2022

valentinewallace Apr 20, 2022

TheBlueMatt Apr 20, 2022

TheBlueMatt commented Apr 21, 2022

valentinewallace commented Apr 21, 2022

arik-so Apr 27, 2022

TheBlueMatt Apr 27, 2022

valentinewallace commented Apr 27, 2022

arik-so Apr 27, 2022

TheBlueMatt Apr 27, 2022

arik-so Apr 27, 2022

arik-so left a comment

TheBlueMatt commented Apr 27, 2022

TheBlueMatt commented Apr 27, 2022

Separate ChannelDetails' outbound capacity from the next HTLC max #1435

Separate ChannelDetails' outbound capacity from the next HTLC max #1435

Conversation

TheBlueMatt commented Apr 20, 2022 • edited

codecov-commenter commented Apr 20, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 21, 2022

valentinewallace commented Apr 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arik-so left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 27, 2022

TheBlueMatt commented Apr 27, 2022

TheBlueMatt commented Apr 20, 2022 •

edited

codecov-commenter commented Apr 20, 2022 •

edited