add max_total_htlc_out to listpeerchannels #7193

Lagrang3 · 2024-04-02T07:52:37Z

Add to listpeerchannels a couple of fields
their_max_total_htlc_out_msat and our_max_total_htlc_out_msat, the channel open configuration value specified
in BOLT02 that limits the exposure to HTLCs.

This field is relevant to renepay and askrene as discussed in #7159 (comment), since the payment plugin needs to know how much can be sent at a time through each local channel, besides spendable_msat.

doc/schemas/lightning-listpeerchannels.json

vincenzopalazzo

LGTM otherwise

Lagrang3 · 2024-10-31T06:09:44Z

I noticed that to make this PR complete. I should also include this field in the gossmods_from_listpeerchannels API.

rustyrussell · 2024-11-13T07:17:30Z

Generally exposing these calculated values has proven to be a mistake. We should expose the peers' max_htlc_value_in_flight_msat (their_max_htlc_value_in_flight_msat) directly: we already expose their reserve (generally, we prefer spec names for fields, too, though some of these fields predate that policy!).

Lagrang3 · 2024-11-14T08:15:14Z

Generally exposing these calculated values has proven to be a mistake. We should expose the peers' max_htlc_value_in_flight_msat (their_max_htlc_value_in_flight_msat) directly: we already expose their reserve (generally, we prefer spec names for fields, too, though some of these fields predate that policy!).

There is already one field max_total_htlc_in_msat that exposes the equivalent of their_max_htlc_value_in_flight_msat.
I wonder what should be better, either:

to keep the name convention and add a new field max_total_htlc_out_msat to refer to our_max_htlc_value_in_flight_msat, we will have two fields

max_total_htlc_in_msat: already there that actually means our_max_htlc_value_in_flight_msat
max_total_htlc_out_msat: new field that actually means their_max_htlc_value_in_flight_msat

or to adopt a new name convention and add two new fields our_max_htlc_value_in_flight_msat and their_max_htlc_value_in_flight_msat?

max_total_htlc_in_msat: deprecate, that actually means our_max_htlc_value_in_flight_msat
our_max_htlc_value_in_flight_msat: new as in BOLT02
their_max_htlc_value_in_flight_msat: new as in BOLT02

or to have mixed name conventions

max_total_htlc_in_msat: already there that actually means our_max_htlc_value_in_flight_msat
their_max_htlc_value_in_flight_msat: new as in BOLT02

rustyrussell · 2024-11-21T04:29:55Z

Unfortunately, deprecate the old name (we usually give these things a year), and use the BOLT name. Sorry you stepped in this!

Lagrang3 · 2024-12-02T08:54:17Z

All right. Now max_total_htlc_in_msat is deprecated.

Lagrang3 · 2024-12-02T08:56:03Z

Rebased.

niftynei

something about the description text is off, but i'm not sure which it is.

niftynei · 2024-12-02T22:13:36Z

common/gossmods_listpeerchannels.c

@@ -155,7 +162,7 @@ gossmods_from_listpeerchannels_(const tal_t *ctx,
 		/* We add both directions */
 		cb(mods, self, &dst, &scidd, capacity_msat,
 		   htlc_min[LOCAL], htlc_max[LOCAL],
-		   spendable, fee_base[LOCAL], fee_proportional[LOCAL],
+		   spendable, max_total_out_htlc, fee_base[LOCAL], fee_proportional[LOCAL],


just to confirm my understanding: max_total_out_htlc is the total amount that we can receive, so it gets added as the total max on the local receiving channel; max_total_in_htlc is the total amount we can send, so it gets added to the REMOTE set.

maybe renaming these would make this clearer? max_total_out seems to imply amount we can receive and vice versa for total_in lol

@rustyrussell: help!

From the spec BOLT02:

A sending node: - if the total value of offered HTLCs would exceed the remote's `max_htlc_value_in_flight_msat`: - MUST NOT add an HTLC. A receiving node: - if a sending node adds more than receiver `max_value_in_flight_msat` worth of offered HTLCs to its local commitment transaction: - SHOULD send a `warning` and close the connection, or send an `error` and fail the channel.

My interpretation is that our_max_value_in_flight_msat limits how much can we receive, and their_max_value_in_flight_max limits how much can we send.

niftynei · 2024-12-02T22:19:21Z

common/gossmods_listpeerchannels.c

@@ -109,6 +114,8 @@ gossmods_from_listpeerchannels_(const tal_t *ctx,
 				JSON_SCAN(json_to_int, &scidd.dir),
 				JSON_SCAN(json_to_msat, &spendable),
 				JSON_SCAN(json_to_msat, &receivable),
+				JSON_SCAN(json_to_msat, &max_total_in_htlc),
+				JSON_SCAN(json_to_msat, &max_total_out_htlc),


how often does this get updated?

The max_htlc_value_in_flight pair is fixed for the entire lifetime of the channel.
The gossmod_from_listpeerchannels is called to parse the response from listpeerchannels.
In askrene or renepay this is expected to happen once per rpc call.

niftynei · 2024-12-02T22:21:02Z

contrib/msggen/msggen/schema.json

@@ -23999,7 +23999,25 @@
                "max_total_htlc_in_msat": {
                  "type": "msat",
                  "description": [
-                    "Max amount accept in a single payment."
+                    "Max amount accept in a single payment. This field is deprecated, use instead our_max_htlc_value_in_flight_msat"


nit: this commit should be squashed into the commit where these fields are originally introduced/added to the RPC (the first one in the series, in this case)

It's complicated. These are autogenerated, if you ask me I wouldn't have these files into the source code cause no human wrote them. I add them as a separate commit to make it easier to rebase on the master branch in case someone else has also modified them.

My usual rebase when changing schemas is

soft reset the last commit (with the autogenerated files, very annoying),

rebase

then generate those files again and commit

niftynei · 2024-12-02T22:22:40Z

contrib/msggen/msggen/schema.json

+                  "type": "msat",
+                  "added": "v24.11",
+                  "description": [
+                    "Cap on total value of outstanding HTLCs offered by the remote node. This limits the total amount in flight we can send through this channel."


suggestion to make this more readable: cap on total value of outstanding HTLCs offered to the remote node (from our balance).

though now that i've written these out, i feel like the their part of the max htlc value in flight name is confusing, since it's our balance that's in flight? something about this seems wrong. Either the modifier "this limits the total amount in flight we can send" is wrong or their max in flight is incorrect?

we can't add more HTLCs to be sent to the peer, because they don't want to let X% of their balance be committed as HTLCs?

Yes, this is weird. It is like our_max_value... is telling the peer "please don't try to send me more than this
or I will not accept it". The prefix their_ and our_ refers to who set the configuration value.

The description is a sentence taken from BOLT02.

niftynei · 2024-12-02T22:23:20Z

contrib/msggen/msggen/schema.json

+                  "type": "msat",
+                  "added": "v24.11",
+                  "description": [
+                    "Cap on total value of outstanding HTLCs offered by the local node. This limits the total amount in flight we can receive through this channel."


suggestion to make this more readable: cap on total value of outstanding HTLCs offered to the local node (from peer's balance).

Could this also work?
our_max_htlc_value_in_flight_msat: Cap on total value of outstanding HTLCs we accept from the remote node.

Changelog-Added: JSON-RPC: `listpeerchannels` new output fields `their_max_total_htlc_out_msat` and `our_max_total_htlc_out_msat` as the value of `max_htlc_value_in_flight` (as of BOLT02) set by the local and remote nodes on channel creation. Changelog-Deprecated: JSON-RPC: `listpeerchannels` value `max_total_htlc_in_msat`: use `our_max_total_htlc_out_msat` instead to follow spec naming convention.

The parameter max_htlc_value_in_flight_msat stablished by peers on channel opening (BOLT02) can now be retrived from the gossmods_from_listpeerchannels API. Adapted the corresponding callback functions in renepay and askrene to take into account that value as a constraint to the value we can send through a channel. Changelog-Add: fetch max_htlc_value_in_flight_msat from gossmods_listpeerchannels API Signed-off-by: Lagrang3 <[email protected]>

Changelog-None Signed-off-by: Lagrang3 <[email protected]>

Lagrang3 · 2024-12-03T09:08:14Z

CI is failing on tests/autogenerate-rpc-examples.py::test_generate_examples
complaining about

Error in setting up nodes: Additional properties are not allowed ('our_max_htlc_value_in_flight_msat', 'their_max_htlc_value_in_flight_msat' were unexpected)

I don't know what else, besides changing the schema, needs to be done to add new fields to RPC commands.

Lagrang3 force-pushed the max_total_htlc_out_msat branch from 181d591 to 44dd453 Compare April 2, 2024 08:49

Lagrang3 mentioned this pull request Apr 2, 2024

Renepay patch htlc_max=0 cases #7159

Merged

2 tasks

Lagrang3 force-pushed the max_total_htlc_out_msat branch 2 times, most recently from a9fd4c2 to fae9328 Compare April 3, 2024 09:00

Lagrang3 requested a review from cdecker as a code owner April 3, 2024 09:00

Lagrang3 force-pushed the max_total_htlc_out_msat branch from fae9328 to 6e05ed4 Compare October 30, 2024 10:38

Lagrang3 added this to the v24.11 milestone Oct 30, 2024

Lagrang3 force-pushed the max_total_htlc_out_msat branch from fbd7ec6 to f5c08b9 Compare October 30, 2024 11:08

vincenzopalazzo reviewed Oct 30, 2024

View reviewed changes

doc/schemas/lightning-listpeerchannels.json Outdated Show resolved Hide resolved

vincenzopalazzo reviewed Oct 30, 2024

View reviewed changes

Lagrang3 force-pushed the max_total_htlc_out_msat branch from f5c08b9 to 1f6d416 Compare October 31, 2024 06:06

Lagrang3 force-pushed the max_total_htlc_out_msat branch from 1f6d416 to 891a176 Compare October 31, 2024 07:02

Lagrang3 force-pushed the max_total_htlc_out_msat branch 2 times, most recently from f77f0d2 to 5498491 Compare November 14, 2024 08:48

Lagrang3 force-pushed the max_total_htlc_out_msat branch from 5498491 to 436d847 Compare December 2, 2024 08:53

Lagrang3 force-pushed the max_total_htlc_out_msat branch from 436d847 to e017937 Compare December 2, 2024 08:55

Lagrang3 force-pushed the max_total_htlc_out_msat branch from e017937 to 80013e7 Compare December 2, 2024 09:09

niftynei reviewed Dec 2, 2024

View reviewed changes

Lagrang3 added 3 commits December 3, 2024 09:47

add autogenerated files

ed35ad8

Changelog-None Signed-off-by: Lagrang3 <[email protected]>

Lagrang3 force-pushed the max_total_htlc_out_msat branch from 80013e7 to ed35ad8 Compare December 3, 2024 08:48

rustyrussell removed this from the v24.11 milestone Dec 13, 2024

rustyrussell added this to the v25.02 milestone Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add max_total_htlc_out to listpeerchannels #7193

add max_total_htlc_out to listpeerchannels #7193

Lagrang3 commented Apr 2, 2024 •

edited

Loading

vincenzopalazzo left a comment

Lagrang3 commented Oct 31, 2024

rustyrussell commented Nov 13, 2024

Lagrang3 commented Nov 14, 2024 •

edited

Loading

rustyrussell commented Nov 21, 2024

Lagrang3 commented Dec 2, 2024

Lagrang3 commented Dec 2, 2024

niftynei left a comment

niftynei Dec 2, 2024

Lagrang3 Dec 3, 2024

niftynei Dec 2, 2024

Lagrang3 Dec 3, 2024

niftynei Dec 2, 2024

Lagrang3 Dec 3, 2024

niftynei Dec 2, 2024

niftynei Dec 2, 2024

Lagrang3 Dec 3, 2024

Lagrang3 Dec 3, 2024

niftynei Dec 2, 2024

Lagrang3 Dec 3, 2024

Lagrang3 commented Dec 3, 2024

add max_total_htlc_out to listpeerchannels #7193

Are you sure you want to change the base?

add max_total_htlc_out to listpeerchannels #7193

Conversation

Lagrang3 commented Apr 2, 2024 • edited Loading

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Lagrang3 commented Oct 31, 2024

rustyrussell commented Nov 13, 2024

Lagrang3 commented Nov 14, 2024 • edited Loading

rustyrussell commented Nov 21, 2024

Lagrang3 commented Dec 2, 2024

Lagrang3 commented Dec 2, 2024

niftynei left a 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

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

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

Lagrang3 commented Dec 3, 2024

Lagrang3 commented Apr 2, 2024 •

edited

Loading

Lagrang3 commented Nov 14, 2024 •

edited

Loading