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.

Sign up for GitHub

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

Jump to bottom

implementation HTLC Endorsement to Mitigate Channel Jamming #6714

Closed
wants to merge 4 commits into from
Closed

implementation HTLC Endorsement to Mitigate Channel Jamming #6714

wants to merge 4 commits into from

Conversation

vincenzopalazzo
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo commented Sep 22, 2023 •
edited
Loading

Implementation of lightning/bolts#1071

The only thing is that that I think that the hook should be under and
experimental flag

@vincenzopalazzo vincenzopalazzo force-pushed the macros/jamming-mitigation-change branch from ec3a3d2 to e399424 Compare September 22, 2023 19:12
@vincenzopalazzo vincenzopalazzo added the protocol These issues are protocol level issues that should be discussed on the protocol spec repo label Sep 29, 2023
@vincenzopalazzo vincenzopalazzo force-pushed the macros/jamming-mitigation-change branch 2 times, most recently from 1818598 to d15c4c8 Compare October 4, 2023 09:12
@vincenzopalazzo vincenzopalazzo force-pushed the macros/jamming-mitigation-change branch from d15c4c8 to 554241e Compare October 13, 2023 10:20
@vincenzopalazzo vincenzopalazzo added this to the v23.11 milestone Oct 13, 2023
@vincenzopalazzo vincenzopalazzo force-pushed the macros/jamming-mitigation-change branch from 0f5fb2b to a7eb48a Compare October 13, 2023 19:55
@vincenzopalazzo vincenzopalazzo marked this pull request as ready for review October 13, 2023 19:59
@vincenzopalazzo vincenzopalazzo marked this pull request as draft October 13, 2023 20:01
@vincenzopalazzo vincenzopalazzo force-pushed the macros/jamming-mitigation-change branch 5 times, most recently from bb2cbaf to 3cbaac7 Compare October 18, 2023 22:14
@vincenzopalazzo vincenzopalazzo force-pushed the macros/jamming-mitigation-change branch 2 times, most recently from 3982077 to ffa7580 Compare October 24, 2023 16:50
@vincenzopalazzo vincenzopalazzo marked this pull request as ready for review October 24, 2023 16:59
@vincenzopalazzo
Copy link
Collaborator Author

This is quite minimal, but it includes the essential work needed to start addressing the channel mitigation issue.

Additionally, I believe it should support modifying the value of htlc_accepted. However, I'm uncertain if we need a different type of hook to fully implement the channel jamming proposal. We'll address this as it arises in the future.

@vincenzopalazzo vincenzopalazzo requested a review from nepet October 24, 2023 17:04
@vincenzopalazzo vincenzopalazzo force-pushed the macros/jamming-mitigation-change branch from ffa7580 to 0178a29 Compare October 24, 2023 17:07
rustyrussell
rustyrussell reviewed Oct 25, 2023
View reviewed changes
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Looks intersesting, but not for this release, since it doesn't actually do any endorsement :)

wire/peer_wire.csv Show resolved Hide resolved
lightningd/pay.c Outdated Show resolved Hide resolved
lightningd/pay.c Outdated Show resolved Hide resolved
channeld/channeld_wire.csv Outdated Show resolved Hide resolved
@rustyrussell rustyrussell removed this from the v23.11 milestone Oct 25, 2023
@vincenzopalazzo
Copy link
Collaborator Author

Looks intersesting, but not for this release, since it doesn't actually do any endorsement :)

I agree that it does not make a lot of sense to put this inside this release, not because we do not have any endorsement, but because I do not have tested it and I do not know if we need another hook or the current one is fine.

For endorsement currently, we need to implement the reputation and look at it, so I am planning to share the implementation done by Carla inside the https://github.com/LNOpenMetrics/go-lnmetrics.reporter before implementing it inside the cln builtin plugins

I know that you have an opinion on the way that how the metrics should be, so I will move incremental step before putting an effort into implementing it in C

@vincenzopalazzo
wire: adds endorsed tlv type inside the update_add_htlc
4b6470e 
This commit introduce the wire change to the wire sysytem of core
lightning in order to implement the [1].

[1] lightning/bolts#1071

Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo vincenzopalazzo force-pushed the macros/jamming-mitigation-change branch 4 times, most recently from 7a75b2d to 4249cb9 Compare January 28, 2024 11:11
@vincenzopalazzo

This comment was marked as outdated.

Sign in to view
@vincenzopalazzo vincenzopalazzo requested review from cdecker and rustyrussell and removed request for nepet January 28, 2024 11:13
@vincenzopalazzo vincenzopalazzo force-pushed the macros/jamming-mitigation-change branch 3 times, most recently from 6edb5d5 to 5882f09 Compare January 28, 2024 13:33
@vincenzopalazzo
lightningd: handling the endorsed field
2b898d6 
Changelog-Experimental: minimal peer to peer support for the channel
jamming mitigation
Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo
hook: add endorsed value in the htlc_accepted
abb1cee 
Passing down to the plugins the endorsed value,
using the htlc_accepted hook.

Allow plugins to modify it, but I think
we should have a way to limit what kind of plugins
can modify the following value. This is
left as a open question.

Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo
experimental: hide the jamming_endorsement under an experimetal flag
42609ca 
Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo vincenzopalazzo force-pushed the macros/jamming-mitigation-change branch from 5882f09 to 42609ca Compare January 29, 2024 09:23
@vincenzopalazzo
Copy link
Collaborator Author

vincenzopalazzo commented Jan 29, 2024 •
edited
Loading

Ok this is better that will go in the next release there is something that I need to do

@vincenzopalazzo vincenzopalazzo marked this pull request as draft January 29, 2024 09:30
@vincenzopalazzo
Copy link
Collaborator Author

Ok i need to rework this, I would like to take another approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rustyrussell rustyrussell rustyrussell left review comments

Assignees
No one assigned
Labels
needs-rebase protocol These issues are protocol level issues that should be discussed on the protocol spec repo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vincenzopalazzo @rustyrussell @cdecker