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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

multi: make legacy feature bits compulsory #8275

Merged

Conversation

ProofOfKeags
Copy link
Collaborator

@ProofOfKeags ProofOfKeags commented Dec 14, 2023

Change Description

This PR makes some legacy feature bits compulsory. Closes #8048

  • var_onion_optin
  • gossip_queries
  • option_data_loss_protect
  • option_static_remote_key

Steps to Test

Steps for reviewers to follow to test the change.

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative (error paths) are included.
  • Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

馃摑 Please see our Contribution Guidelines for further guidance.

@ProofOfKeags ProofOfKeags changed the title multi: make tlv onion compulsory multi: make legacy feature bits compulsory Dec 14, 2023
@ProofOfKeags ProofOfKeags marked this pull request as ready for review December 14, 2023 01:59
@saubyk saubyk requested a review from guggero December 14, 2023 15:21
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Did a first pass, looks pretty good!
The linter/formatter CI step is failing due to a missing make fmt run and the itests fail because of an itest case that needs to be removed. Other than that I think this is quite close already.

invoices/invoiceregistry.go Show resolved Hide resolved
routing/integrated_routing_context_test.go Outdated Show resolved Hide resolved
feature/default_sets.go Show resolved Hide resolved
lncfg/protocol_legacy_on.go Outdated Show resolved Hide resolved
lncfg/protocol_legacy_on.go Outdated Show resolved Hide resolved
@lightninglabs-deploy
Copy link

@ProofOfKeags, remember to re-request review from reviewers when ready

@ProofOfKeags ProofOfKeags force-pushed the chore/enforce-feature-bits branch 2 times, most recently from 701f61d to 07103e1 Compare January 4, 2024 02:27
Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

Looks good so far!

My main comment is regarding removing any logic that switches on LegacyPayload.

Something I noticed in testing btw is the following:
If we have Alice running with a required bit and Bob with no knowledge of that bit, then if Alice tries to connect to Bob, the connection fails (which we expect) but if Bob tries to connect to Alice then that connection succeeds.... I think this might be incorrect behaviour. Not relevant to this PR though - just mentioning.

Just for future: it's generally good practice to make sure the CI is green before re-requesting review to help reduce the number of rounds :) There is currently a panic in the unit tests and the itests are all failing

@@ -24,7 +24,7 @@ var (
emptyFeatures = lnwire.NewFeatureVector(nil, lnwire.Features)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did some testing with Alice running this PR and Bob running master with --protocol.legacy.onion set and found that if Bob creates an invoice the Alice will refuse to pay it due to the face that Bob doesnt support the TLV onion option. This is expected but I think that means that we can remove a bunch of logic that switches on route.Hop.LegacyPayload:

lnd/routing/route/route.go

Lines 133 to 136 in 708bd05

// LegacyPayload if true, then this signals that this node doesn't
// understand the new TLV payload, so we must instead use the legacy
// payload.
LegacyPayload bool

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Paging @Roasbeef to weigh in on ripping this code out.

Copy link
Member

Choose a reason for hiding this comment

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

Have a slight pref for just kinda leaving it in, and also making sure the legacy protocol cfg stuff still works as well to enable, this way less sweeping itest changes are needed.

So I'm thinking:

  • flip to required
  • wait
  • observe
  • ???
  • "enough time passes"
  • rip out the code at the same time we start to repurpose the feature bits

@ProofOfKeags ProofOfKeags force-pushed the chore/enforce-feature-bits branch 3 times, most recently from 173f514 to 66e3f24 Compare January 6, 2024 01:09
@ProofOfKeags ProofOfKeags force-pushed the chore/enforce-feature-bits branch 3 times, most recently from 847b01b to 89afb29 Compare January 10, 2024 21:25
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Two questions around interpreting @Roasbeef's comment re leaving the command line option or not.

Other than that LGTM 馃帀

routing/pathfind_test.go Show resolved Hide resolved
feature/manager.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
@ProofOfKeags ProofOfKeags force-pushed the chore/enforce-feature-bits branch 4 times, most recently from 06cb65c to 5f50fe6 Compare January 11, 2024 17:41
@ProofOfKeags ProofOfKeags force-pushed the chore/enforce-feature-bits branch 2 times, most recently from 5b82ce4 to dccd743 Compare January 11, 2024 21:02
Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

LGTM! 馃敟

@ProofOfKeags
Copy link
Collaborator Author

SNED 馃殌

@saubyk saubyk added this to the v0.18.0 milestone Jan 18, 2024
@ellemouton ellemouton merged commit 08c18a3 into lightningnetwork:master Jan 18, 2024
24 of 25 checks passed
@RandyMcMillan
Copy link
Contributor

ACK 08c18a3

(post merge)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: begin flipping well deployed feature bits to the required bit
7 participants