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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

multi: make legacy feature bits compulsory #8275

Merged
merged 5 commits into from Jan 18, 2024
Merged

multi: make legacy feature bits compulsory #8275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
239103c
multi: make tlv onion compulsory
ProofOfKeags Dec 13, 2023
717facc
feature: make gossip queries compulsory
ProofOfKeags Dec 14, 2023
d47338c
feature: fixup making data loss protection compulsory
ProofOfKeags Dec 14, 2023
109265c
multi: make static remote key compulsory
ProofOfKeags Dec 14, 2023
721463c
docs: update release notes
ProofOfKeags Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions channeldb/invoice_test.go
View file
Open in desktop
Expand Up @@ -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

ampFeatures = lnwire.NewFeatureVector(
lnwire.NewRawFeatureVector(
lnwire.TLVOnionPayloadOptional,
lnwire.TLVOnionPayloadRequired,
lnwire.PaymentAddrOptional,
lnwire.AMPRequired,
),
Expand Down Expand Up @@ -3158,7 +3158,7 @@ func TestAddInvoiceInvalidFeatureDeps(t *testing.T) {

invoice.Terms.Features = lnwire.NewFeatureVector(
lnwire.NewRawFeatureVector(
lnwire.TLVOnionPayloadOptional,
lnwire.TLVOnionPayloadRequired,
lnwire.MPPOptional,
),
lnwire.Features,
Expand Down
4 changes: 4 additions & 0 deletions docs/release-notes/release-notes-0.18.0.md
View file
Open in desktop
Expand Up @@ -209,6 +209,10 @@
with a lower min_final_cltv_expiry_delta value than 18 blocks since
LND 0.11.0.

* [Make Legacy Features Compulsory](https://github.com/lightningnetwork/lnd/pull/8275).
This change implements changes codified in [bolts#1092](https://github.com/lightning/bolts/pull/1092)
and makes TLV Onions, Static Remote Keys, Gossip Queries, compulsory features for
LND's peers. Data Loss Protection has been compulsory for years.

## Testing

Expand Down
4 changes: 2 additions & 2 deletions feature/default_sets.go
View file
Open in desktop
Expand Up @@ -14,11 +14,11 @@ var defaultSetDesc = setDesc{
SetInit: {}, // I
SetNodeAnn: {}, // N
},
lnwire.GossipQueriesOptional: {
lnwire.GossipQueriesRequired: {
ProofOfKeags marked this conversation as resolved.
Show resolved Hide resolved
SetInit: {}, // I
SetNodeAnn: {}, // N
},
lnwire.TLVOnionPayloadOptional: {
lnwire.TLVOnionPayloadRequired: {
SetInit: {}, // I
SetNodeAnn: {}, // N
SetInvoice: {}, // 9
Expand Down
10 changes: 5 additions & 5 deletions feature/deps_test.go
View file
Open in desktop
Expand Up @@ -33,7 +33,7 @@ var depTests = []depTest{
{
name: "one dep optional",
raw: lnwire.NewRawFeatureVector(
lnwire.TLVOnionPayloadOptional,
lnwire.TLVOnionPayloadRequired,
lnwire.PaymentAddrOptional,
),
},
Expand Down Expand Up @@ -61,7 +61,7 @@ var depTests = []depTest{
{
name: "two dep optional",
raw: lnwire.NewRawFeatureVector(
lnwire.TLVOnionPayloadOptional,
lnwire.TLVOnionPayloadRequired,
lnwire.PaymentAddrOptional,
lnwire.MPPOptional,
),
Expand Down Expand Up @@ -93,7 +93,7 @@ var depTests = []depTest{
{
name: "two dep first missing optional",
raw: lnwire.NewRawFeatureVector(
lnwire.TLVOnionPayloadOptional,
lnwire.TLVOnionPayloadRequired,
lnwire.MPPOptional,
),
expErr: ErrMissingFeatureDep{lnwire.PaymentAddrOptional},
Expand All @@ -110,7 +110,7 @@ var depTests = []depTest{
name: "forest optional",
raw: lnwire.NewRawFeatureVector(
lnwire.GossipQueriesOptional,
lnwire.TLVOnionPayloadOptional,
lnwire.TLVOnionPayloadRequired,
lnwire.PaymentAddrOptional,
lnwire.MPPOptional,
),
Expand All @@ -128,7 +128,7 @@ var depTests = []depTest{
name: "broken forest optional",
raw: lnwire.NewRawFeatureVector(
lnwire.GossipQueriesOptional,
lnwire.TLVOnionPayloadOptional,
lnwire.TLVOnionPayloadRequired,
lnwire.MPPOptional,
),
expErr: ErrMissingFeatureDep{lnwire.PaymentAddrOptional},
Expand Down
18 changes: 10 additions & 8 deletions feature/manager_internal_test.go
View file
Open in desktop
Expand Up @@ -19,11 +19,11 @@ var testSetDesc = setDesc{
lnwire.DataLossProtectRequired: {
SetNodeAnn: {}, // I
},
lnwire.TLVOnionPayloadOptional: {
lnwire.TLVOnionPayloadRequired: {
SetInit: {}, // I
SetNodeAnn: {}, // N
},
lnwire.StaticRemoteKeyOptional: {
lnwire.StaticRemoteKeyRequired: {
SetInit: {}, // I
SetNodeAnn: {}, // N
},
Expand Down Expand Up @@ -104,9 +104,11 @@ func testManager(t *testing.T, test managerTest) {
// Assert that the manager properly unset the configured feature
// bits from all sets.
if test.cfg.NoTLVOnion {
assertUnset(lnwire.TLVOnionPayloadRequired)
assertUnset(lnwire.TLVOnionPayloadOptional)
}
if test.cfg.NoStaticRemoteKey {
assertUnset(lnwire.StaticRemoteKeyRequired)
assertUnset(lnwire.StaticRemoteKeyOptional)
}
if test.cfg.NoAnchors {
Expand All @@ -127,12 +129,12 @@ func testManager(t *testing.T, test managerTest) {
}
}

assertSet(lnwire.DataLossProtectOptional)
assertSet(lnwire.DataLossProtectRequired)
if !test.cfg.NoTLVOnion {
assertSet(lnwire.TLVOnionPayloadRequired)
}
if !test.cfg.NoStaticRemoteKey {
assertSet(lnwire.StaticRemoteKeyOptional)
assertSet(lnwire.StaticRemoteKeyRequired)
}
}

Expand All @@ -149,7 +151,7 @@ func TestUpdateFeatureSets(t *testing.T) {
SetInit: {}, // I
SetNodeAnn: {}, // N
},
lnwire.GossipQueriesOptional: {
lnwire.GossipQueriesRequired: {
SetNodeAnn: {}, // N
},
}
Expand Down Expand Up @@ -199,7 +201,7 @@ func TestUpdateFeatureSets(t *testing.T) {
),
SetNodeAnn: lnwire.NewRawFeatureVector(
lnwire.DataLossProtectRequired,
lnwire.GossipQueriesOptional,
lnwire.GossipQueriesRequired,
lnwire.FeatureBit(1000),
),
},
Expand All @@ -220,7 +222,7 @@ func TestUpdateFeatureSets(t *testing.T) {
),
SetNodeAnn: lnwire.NewRawFeatureVector(
lnwire.DataLossProtectRequired,
lnwire.GossipQueriesOptional,
lnwire.GossipQueriesRequired,
),
},
config: Config{
Expand All @@ -240,7 +242,7 @@ func TestUpdateFeatureSets(t *testing.T) {
),
SetNodeAnn: lnwire.NewRawFeatureVector(
lnwire.DataLossProtectRequired,
lnwire.GossipQueriesOptional,
lnwire.GossipQueriesRequired,
lnwire.FeatureBit(500),
),
SetInvoice: lnwire.NewRawFeatureVector(
Expand Down
23 changes: 12 additions & 11 deletions funding/commitment_type_negotiation_test.go
View file
Open in desktop
Expand Up @@ -31,14 +31,15 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.AnchorsZeroFeeHtlcTxRequired,
),
localFeatures: lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyOptional,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxOptional,
lnwire.ExplicitChannelTypeOptional,
),
remoteFeatures: lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyOptional,
lnwire.AnchorsZeroFeeHtlcTxOptional,
),
//nolint:lll
expectsCommitType: lnwallet.CommitmentTypeAnchorsZeroFeeHtlcTx,
expectsChanType: nil,
expectsErr: nil,
Expand All @@ -50,7 +51,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.AnchorsZeroFeeHtlcTxRequired,
),
localFeatures: lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyOptional,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxOptional,
lnwire.ExplicitChannelTypeOptional,
),
Expand All @@ -70,7 +71,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
),
localFeatures: lnwire.NewRawFeatureVector(
lnwire.ZeroConfOptional,
lnwire.StaticRemoteKeyOptional,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxOptional,
lnwire.ScriptEnforcedLeaseOptional,
lnwire.ExplicitChannelTypeOptional,
Expand Down Expand Up @@ -103,7 +104,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
),
localFeatures: lnwire.NewRawFeatureVector(
lnwire.ZeroConfOptional,
lnwire.StaticRemoteKeyOptional,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxOptional,
lnwire.ExplicitChannelTypeOptional,
),
Expand Down Expand Up @@ -134,7 +135,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
),
localFeatures: lnwire.NewRawFeatureVector(
lnwire.ScidAliasOptional,
lnwire.StaticRemoteKeyOptional,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxOptional,
lnwire.ScriptEnforcedLeaseOptional,
lnwire.ExplicitChannelTypeOptional,
Expand Down Expand Up @@ -167,7 +168,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
),
localFeatures: lnwire.NewRawFeatureVector(
lnwire.ScidAliasOptional,
lnwire.StaticRemoteKeyOptional,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxOptional,
lnwire.ExplicitChannelTypeOptional,
),
Expand Down Expand Up @@ -195,7 +196,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.AnchorsZeroFeeHtlcTxRequired,
),
localFeatures: lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyOptional,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxOptional,
lnwire.ExplicitChannelTypeOptional,
),
Expand All @@ -219,7 +220,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
lnwire.StaticRemoteKeyRequired,
),
localFeatures: lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyOptional,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxOptional,
lnwire.ExplicitChannelTypeOptional,
),
Expand All @@ -240,7 +241,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
name: "explicit legacy",
channelFeatures: lnwire.NewRawFeatureVector(),
localFeatures: lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyOptional,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxOptional,
lnwire.ExplicitChannelTypeOptional,
),
Expand All @@ -262,7 +263,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
name: "default explicit anchors",
channelFeatures: nil,
localFeatures: lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyOptional,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxOptional,
lnwire.ExplicitChannelTypeOptional,
),
Expand All @@ -284,7 +285,7 @@ func TestCommitmentTypeNegotiation(t *testing.T) {
name: "implicit tweakless",
channelFeatures: nil,
localFeatures: lnwire.NewRawFeatureVector(
lnwire.StaticRemoteKeyOptional,
lnwire.StaticRemoteKeyRequired,
lnwire.AnchorsZeroFeeHtlcTxOptional,
),
remoteFeatures: lnwire.NewRawFeatureVector(
Expand Down
12 changes: 6 additions & 6 deletions invoices/invoiceregistry.go
View file
Open in desktop
Expand Up @@ -756,10 +756,10 @@ func (i *InvoiceRegistry) processKeySend(ctx invoiceUpdateCtx) error {
// Create an invoice for the htlc amount.
amt := ctx.amtPaid

// Set tlv optional feature vector on the invoice. Otherwise we wouldn't
// Set tlv required feature vector on the invoice. Otherwise we wouldn't
// be able to pay to it with keysend.
rawFeatures := lnwire.NewRawFeatureVector(
lnwire.TLVOnionPayloadOptional,
lnwire.TLVOnionPayloadRequired,
ProofOfKeags marked this conversation as resolved.
Show resolved Hide resolved
)
features := lnwire.NewFeatureVector(rawFeatures, lnwire.Features)

Expand Down Expand Up @@ -820,11 +820,11 @@ func (i *InvoiceRegistry) processAMP(ctx invoiceUpdateCtx) error {
// record.
amt := ctx.mpp.TotalMsat()

// Set the TLV and MPP optional features on the invoice. We'll also make
// the AMP features required so that it can't be paid by legacy or MPP
// htlcs.
// Set the TLV required and MPP optional features on the invoice. We'll
// also make the AMP features required so that it can't be paid by
// legacy or MPP htlcs.
rawFeatures := lnwire.NewRawFeatureVector(
lnwire.TLVOnionPayloadOptional,
lnwire.TLVOnionPayloadRequired,
lnwire.PaymentAddrOptional,
lnwire.AMPRequired,
)
Expand Down
4 changes: 2 additions & 2 deletions invoices/invoiceregistry_test.go
View file
Open in desktop
Expand Up @@ -1460,7 +1460,7 @@ func TestSettleInvoicePaymentAddrRequired(t *testing.T) {
Expiry: time.Hour,
Features: lnwire.NewFeatureVector(
lnwire.NewRawFeatureVector(
lnwire.TLVOnionPayloadOptional,
lnwire.TLVOnionPayloadRequired,
lnwire.PaymentAddrRequired,
),
lnwire.Features,
Expand Down Expand Up @@ -1549,7 +1549,7 @@ func TestSettleInvoicePaymentAddrRequiredOptionalGrace(t *testing.T) {
Expiry: time.Hour,
Features: lnwire.NewFeatureVector(
lnwire.NewRawFeatureVector(
lnwire.TLVOnionPayloadOptional,
lnwire.TLVOnionPayloadRequired,
lnwire.PaymentAddrOptional,
),
lnwire.Features,
Expand Down
1 change: 0 additions & 1 deletion itest/lnd_channel_force_close_test.go
View file
Open in desktop
Expand Up @@ -245,7 +245,6 @@ func testChannelForceClosure(ht *lntest.HarnessTest) {
// We'll test the scenario for some of the commitment types, to ensure
// outputs can be swept.
commitTypes := []lnrpc.CommitmentType{
lnrpc.CommitmentType_LEGACY,
lnrpc.CommitmentType_ANCHORS,
lnrpc.CommitmentType_SIMPLE_TAPROOT,
}
Expand Down
1 change: 0 additions & 1 deletion itest/lnd_funding_test.go
View file
Open in desktop
Expand Up @@ -32,7 +32,6 @@ func testBasicChannelFunding(ht *lntest.HarnessTest) {
// Run through the test with combinations of all the different
// commitment types.
allTypes := []lnrpc.CommitmentType{
lnrpc.CommitmentType_LEGACY,
lnrpc.CommitmentType_STATIC_REMOTE_KEY,
lnrpc.CommitmentType_ANCHORS,
lnrpc.CommitmentType_SIMPLE_TAPROOT,
Expand Down
4 changes: 0 additions & 4 deletions itest/lnd_multi-hop_test.go
View file
Open in desktop
Expand Up @@ -30,10 +30,6 @@ var commitWithZeroConf = []struct {
commitType lnrpc.CommitmentType
zeroConf bool
}{
{
commitType: lnrpc.CommitmentType_LEGACY,
zeroConf: false,
},
{
commitType: lnrpc.CommitmentType_ANCHORS,
zeroConf: false,
Expand Down
1 change: 0 additions & 1 deletion itest/lnd_payment_test.go
View file
Open in desktop
Expand Up @@ -26,7 +26,6 @@ func testSendDirectPayment(ht *lntest.HarnessTest) {

// Create a list of commitment types we want to test.
commitTyes := []lnrpc.CommitmentType{
lnrpc.CommitmentType_LEGACY,
lnrpc.CommitmentType_ANCHORS,
lnrpc.CommitmentType_SIMPLE_TAPROOT,
}
Expand Down
3 changes: 0 additions & 3 deletions itest/lnd_revocation_test.go
View file
Open in desktop
Expand Up @@ -195,7 +195,6 @@ func breachRetributionTestCase(ht *lntest.HarnessTest,
// the mempool.
func testRevokedCloseRetribution(ht *lntest.HarnessTest) {
for _, commitType := range []lnrpc.CommitmentType{
lnrpc.CommitmentType_LEGACY,
lnrpc.CommitmentType_SIMPLE_TAPROOT,
} {
testName := fmt.Sprintf("%v", commitType.String())
Expand Down Expand Up @@ -378,7 +377,6 @@ func revokedCloseRetributionZeroValueRemoteOutputCase(ht *lntest.HarnessTest,
// commitment output has zero-value.
func testRevokedCloseRetributionZeroValueRemoteOutput(ht *lntest.HarnessTest) {
for _, commitType := range []lnrpc.CommitmentType{
lnrpc.CommitmentType_LEGACY,
lnrpc.CommitmentType_SIMPLE_TAPROOT,
} {
testName := fmt.Sprintf("%v", commitType.String())
Expand Down Expand Up @@ -700,7 +698,6 @@ func revokedCloseRetributionRemoteHodlCase(ht *lntest.HarnessTest,
// remote party breaches before settling extended HTLCs.
func testRevokedCloseRetributionRemoteHodl(ht *lntest.HarnessTest) {
for _, commitType := range []lnrpc.CommitmentType{
lnrpc.CommitmentType_LEGACY,
lnrpc.CommitmentType_SIMPLE_TAPROOT,
} {
testName := fmt.Sprintf("%v", commitType.String())
Expand Down
7 changes: 7 additions & 0 deletions peer/brontide.go
View file
Open in desktop
Expand Up @@ -754,6 +754,13 @@ func (p *Brontide) initGossipSync() {
if p.remoteFeatures.HasFeature(lnwire.GossipQueriesOptional) {
p.log.Info("Negotiated chan series queries")

if p.cfg.AuthGossiper == nil {
// This should only ever be hit in the unit tests.
p.log.Warn("No AuthGossiper configured. Abandoning " +
"gossip sync.")
return
}

// Register the peer's gossip syncer with the gossiper.
// This blocks synchronously to ensure the gossip syncer is
// registered with the gossiper before attempting to read
Expand Down
1 change: 1 addition & 0 deletions peer/brontide_test.go
View file
Open in desktop
Expand Up @@ -1080,6 +1080,7 @@ func TestPeerCustomMessage(t *testing.T) {
initReplyMsg := lnwire.NewInitMessage(
lnwire.NewRawFeatureVector(
lnwire.DataLossProtectRequired,
lnwire.GossipQueriesOptional,
),
lnwire.NewRawFeatureVector(),
)
Expand Down