Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

BOLT #7: Make channel_update always have htlc_maximum_msat. #996

Closed
wants to merge 2 commits into from

Conversation

rustyrussell
Copy link
Collaborator

@rustyrussell rustyrussell commented May 31, 2022

I think everyone sets this. Vastly simplifies parsing.

Indeed, every single of the 149964 channel updates seen by my node has his set.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@cdecker
Copy link
Collaborator

cdecker commented May 31, 2022

ACK 466317a

Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🏂

Would be good to run some basic analysis just to check what % of channels aren't setting it.

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Concept ack for the "metrics"

Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Concept ACK, I think we can remove even more stuff

Suggested-by: @t-bast
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
t-bast
t-bast previously approved these changes Jun 1, 2022
Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK f99b549

@t-bast
Copy link
Collaborator

t-bast commented Jun 6, 2022

As noted in lightningdevkit/rust-lightning#1515 and discussed during the dev summit, it would also make sense to verify that every implementation sets this value to max_htlc_value_in_flight_msat instead of using the channel capacity.

@t-bast
Copy link
Collaborator

t-bast commented Jun 10, 2022

Retracting my ACK, I made a proposal at #999 that would benefit from message flags (but I did make htlc_maximum_msat mandatory, which was the goal of this PR).

@arik-so
Copy link

arik-so commented Jul 15, 2022

I have just downloaded a fresh batch of 134k channel updates from an LND peer; of those channel updates, 364 don't have an HTLC max set, which is 0.27%.

I have separately downloaded 161k updates from a different peer, and got 399 instances of an unset HTLC max, or 0.25%.

t-bast added a commit to ACINQ/eclair that referenced this pull request Aug 3, 2022
The specification is removing support for old channel updates that didn't
include an `htlc_maximum_msat` (lightning/bolts#996).

Every implementation has been generating updates containing this field for
years, so we can safely reject updates that don't contain it.
t-bast added a commit to ACINQ/eclair that referenced this pull request Aug 3, 2022
The specification is removing support for old channel updates that didn't
include an `htlc_maximum_msat` (lightning/bolts#996).

Every implementation has been generating updates containing this field for
years, so we can safely reject updates that don't contain it.
t-bast added a commit to ACINQ/eclair that referenced this pull request Aug 3, 2022
The specification is removing support for old channel updates that didn't
include an `htlc_maximum_msat` (lightning/bolts#996).

Every implementation has been generating updates containing this field for
years, so we can safely reject updates that don't contain it.
t-bast added a commit to ACINQ/eclair that referenced this pull request Aug 8, 2022
The specification is removing support for old channel updates that didn't
include an `htlc_maximum_msat` (lightning/bolts#996).

Every implementation has been generating updates containing this field for
years, so we can safely reject updates that don't contain it.
@Roasbeef
Copy link
Collaborator

Replaced by #999

@Roasbeef Roasbeef closed this Aug 15, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants