-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add Experimental Endorsement Signalling #8390
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.
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
Add Experimental Endorsement Signalling #8390
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
294acee to
2e980c9
Compare
|
Updated to depend on some of the new APIS, depends on #8660 so only the last 4 commits are relevant. |
d07cd38 to
d1bc9f7
Compare
|
Opening this up for a high level conceptual review - code now matches what's outlined in the blip. |
d1bc9f7 to
0cbce93
Compare
|
Updated feature bit values to match update to blip. |
0cbce93 to
21af106
Compare
21af106 to
ab99ae1
Compare
748cb42 to
fdf9566
Compare
|
Rebased on master! |
ellemouton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the testForwardInterceptorWireRecords itest which tests that custom records are set just needs to be adjusted to account for the new endorsed bit. Either explicitly checking that it is set too or just starting those itest nodes with the protocol.no-experimental-endorsement=false flag.
Other than that, this is g2g!
eff8ab5 to
e21cdd0
Compare
e21cdd0 to
36b5313
Compare
|
needs rebase and then we g2g I think |
36b5313 to
b80cdba
Compare
|
Haven't abandoned this - just ran into some itest issues, pushed with existing failures We don't resolve incoming HTLCs if they have any custom records present, which means that on-chain resolution breaks for any HTLC which has an Two possible approaches here:
If (1) will be included in 19, I think it's slightly preferable to having to make changes to totally unrelated itests but open to opinions! |
|
Circling back on this, you can get the CI to pass if you change that check to instead checking for the |
b80cdba to
50a8dda
Compare
Great! By this, do you mean:
Either option works for me! |
50a8dda to
9de1365
Compare
|
Rebased on new dependent PRs! Depends on #9240: with this change we need fewer workarounds in itests for the API peculiarity (described in #9166), and the tests that include endorsement give us coverage for testing merging of existing custom records and Depends on #9208: workaround to make itests run for channels that have custom records that aren't |
ellemouton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice - g2g once the dependant PRs are in. Also just one question about a case in the link where maybe we need to set the endorsement field (could be a misunderstanding on my part though)
9de1365 to
ef51601
Compare
ellemouton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once dependent PR is in! 🚢
|
@carlaKC, remember to re-request review from reviewers when ready |
Before we have sufficient signaling in the network to relay this signal, set a zero value experimental endorsement value on the sender's outgoing htlc. Once the network is relaying this signal and a flag day has been set, we'll be able to set a non-zero value here.
ef51601 to
a8c159b
Compare
|
Rebased on dependent PRs / itest failures unrelated (listsweeps x2 + windows borked) - should be good to go! |
This PR adds the ability for LND to set and relay an experimental range endorsement field with
UpdateAddHTLC.Fixes #7883.
Depends on #9049, uses custom records added to payload descriptor!