diff --git a/channeldb/graph_test.go b/channeldb/graph_test.go index 35c7050eb..0afc9ea19 100644 --- a/channeldb/graph_test.go +++ b/channeldb/graph_test.go @@ -2924,7 +2924,7 @@ func TestEdgePolicyMissingMaxHtcl(t *testing.T) { // Set the max_htlc field. The extra bytes added to the serialization // will be the opaque data containing the serialized field. - edge1.MessageFlags = lnwire.ChanUpdateOptionMaxHtlc + edge1.MessageFlags = lnwire.ChanUpdateRequiredMaxHtlc edge1.MaxHTLC = 13928598 var b2 bytes.Buffer err = serializeChanEdgePolicy(&b2, edge1, to) diff --git a/channeldb/migration/lnwire21/channel_update.go b/channeldb/migration/lnwire21/channel_update.go index 037f3d556..6d19e3e4c 100644 --- a/channeldb/migration/lnwire21/channel_update.go +++ b/channeldb/migration/lnwire21/channel_update.go @@ -14,9 +14,9 @@ import ( type ChanUpdateMsgFlags uint8 const ( - // ChanUpdateOptionMaxHtlc is a bit that indicates whether the + // ChanUpdateRequiredMaxHtlc is a bit that indicates whether the // optional htlc_maximum_msat field is present in this ChannelUpdate. - ChanUpdateOptionMaxHtlc ChanUpdateMsgFlags = 1 << iota + ChanUpdateRequiredMaxHtlc ChanUpdateMsgFlags = 1 << iota ) // String returns the bitfield flags as a string. @@ -27,7 +27,7 @@ func (c ChanUpdateMsgFlags) String() string { // HasMaxHtlc returns true if the htlc_maximum_msat option bit is set in the // message flags. func (c ChanUpdateMsgFlags) HasMaxHtlc() bool { - return c&ChanUpdateOptionMaxHtlc != 0 + return c&ChanUpdateRequiredMaxHtlc != 0 } // ChanUpdateChanFlags is a bitfield that signals various options concerning a diff --git a/discovery/chan_series.go b/discovery/chan_series.go index 42ebe8889..0fe819d4e 100644 --- a/discovery/chan_series.go +++ b/discovery/chan_series.go @@ -7,6 +7,7 @@ import ( "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/lnwire" "github.com/lightningnetwork/lnd/netann" + "github.com/lightningnetwork/lnd/routing" "github.com/lightningnetwork/lnd/routing/route" ) @@ -131,10 +132,24 @@ func (c *ChanSeries) UpdatesInHorizon(chain chainhash.Hash, updates = append(updates, chanAnn) if edge1 != nil { - updates = append(updates, edge1) + // We don't want to send channel updates that don't + // conform to the spec (anymore). + err := routing.ValidateChannelUpdateFields(0, edge1) + if err != nil { + log.Errorf("not sending invalid channel "+ + "update %v: %v", edge1, err) + } else { + updates = append(updates, edge1) + } } if edge2 != nil { - updates = append(updates, edge2) + err := routing.ValidateChannelUpdateFields(0, edge2) + if err != nil { + log.Errorf("not sending invalid channel "+ + "update %v: %v", edge2, err) + } else { + updates = append(updates, edge2) + } } } diff --git a/discovery/gossiper.go b/discovery/gossiper.go index e3bca5e1f..9fce884f2 100644 --- a/discovery/gossiper.go +++ b/discovery/gossiper.go @@ -1533,7 +1533,7 @@ func (d *AuthenticatedGossiper) retransmitStaleAnns(now time.Time) error { if !edge.MessageFlags.HasMaxHtlc() { // We'll make sure we support the new max_htlc field if // not already present. - edge.MessageFlags |= lnwire.ChanUpdateOptionMaxHtlc + edge.MessageFlags |= lnwire.ChanUpdateRequiredMaxHtlc edge.MaxHTLC = lnwire.NewMSatFromSatoshis(info.Capacity) edgesToUpdate = append(edgesToUpdate, updateTuple{ diff --git a/discovery/gossiper_test.go b/discovery/gossiper_test.go index 322cd9564..f215b77ae 100644 --- a/discovery/gossiper_test.go +++ b/discovery/gossiper_test.go @@ -582,7 +582,7 @@ func createUpdateAnnouncement(blockHeight uint32, BlockHeight: blockHeight, }, Timestamp: timestamp, - MessageFlags: lnwire.ChanUpdateOptionMaxHtlc, + MessageFlags: lnwire.ChanUpdateRequiredMaxHtlc, ChannelFlags: flags, TimeLockDelta: uint16(prand.Int63()), HtlcMinimumMsat: htlcMinMsat, @@ -859,9 +859,30 @@ func TestProcessAnnouncement(t *testing.T) { t.Fatalf("edge wasn't added to router: %v", err) } + // We'll craft an invalid channel update, setting no message flags. + ua, err := createUpdateAnnouncement(0, 0, remoteKeyPriv1, timestamp) + require.NoError(t, err, "can't create update announcement") + ua.MessageFlags = 0 + + // We send an invalid channel update and expect it to fail. + select { + case err = <-ctx.gossiper.ProcessRemoteAnnouncement(ua, nodePeer): + case <-time.After(2 * time.Second): + t.Fatal("remote announcement not processed") + } + require.ErrorContains(t, err, "max htlc flag not set for channel "+ + "update") + + // We should not broadcast the channel update. + select { + case <-ctx.broadcastedMessage: + t.Fatal("gossiper should not have broadcast channel update") + case <-time.After(2 * trickleDelay): + } + // We'll then craft the channel policy of the remote party and also send // it to the gossiper. - ua, err := createUpdateAnnouncement(0, 0, remoteKeyPriv1, timestamp) + ua, err = createUpdateAnnouncement(0, 0, remoteKeyPriv1, timestamp) require.NoError(t, err, "can't create update announcement") select { @@ -2861,24 +2882,26 @@ func TestNodeAnnouncementNoChannels(t *testing.T) { } // TestOptionalFieldsChannelUpdateValidation tests that we're able to properly -// validate the msg flags and optional max HTLC field of a ChannelUpdate. +// validate the msg flags and max HTLC field of a ChannelUpdate. func TestOptionalFieldsChannelUpdateValidation(t *testing.T) { t.Parallel() ctx, err := createTestCtx(t, 0) require.NoError(t, err, "can't create context") + processRemoteAnnouncement := ctx.gossiper.ProcessRemoteAnnouncement + chanUpdateHeight := uint32(0) timestamp := uint32(123456) nodePeer := &mockPeer{remoteKeyPriv1.PubKey(), nil, nil} - // In this scenario, we'll test whether the message flags field in a channel - // update is properly handled. + // In this scenario, we'll test whether the message flags field in a + // channel update is properly handled. chanAnn, err := createRemoteChannelAnnouncement(chanUpdateHeight) require.NoError(t, err, "can't create channel announcement") select { - case err = <-ctx.gossiper.ProcessRemoteAnnouncement(chanAnn, nodePeer): + case err = <-processRemoteAnnouncement(chanAnn, nodePeer): case <-time.After(2 * time.Second): t.Fatal("did not process remote announcement") } @@ -2886,7 +2909,9 @@ func TestOptionalFieldsChannelUpdateValidation(t *testing.T) { // The first update should fail from an invalid max HTLC field, which is // less than the min HTLC. - chanUpdAnn, err := createUpdateAnnouncement(0, 0, remoteKeyPriv1, timestamp) + chanUpdAnn, err := createUpdateAnnouncement( + 0, 0, remoteKeyPriv1, timestamp, + ) require.NoError(t, err, "unable to create channel update") chanUpdAnn.HtlcMinimumMsat = 5000 @@ -2896,7 +2921,7 @@ func TestOptionalFieldsChannelUpdateValidation(t *testing.T) { } select { - case err = <-ctx.gossiper.ProcessRemoteAnnouncement(chanUpdAnn, nodePeer): + case err = <-processRemoteAnnouncement(chanUpdAnn, nodePeer): case <-time.After(2 * time.Second): t.Fatal("did not process remote announcement") } @@ -2913,7 +2938,7 @@ func TestOptionalFieldsChannelUpdateValidation(t *testing.T) { } select { - case err = <-ctx.gossiper.ProcessRemoteAnnouncement(chanUpdAnn, nodePeer): + case err = <-processRemoteAnnouncement(chanUpdAnn, nodePeer): case <-time.After(2 * time.Second): t.Fatal("did not process remote announcement") } @@ -2921,19 +2946,36 @@ func TestOptionalFieldsChannelUpdateValidation(t *testing.T) { t.Fatalf("expected chan update to error, instead got %v", err) } - // The final update should succeed, since setting the flag 0 means the - // nonsense max_htlc field will just be ignored. + // The third update should not succeed, a channel update with no message + // flag set is invalid. chanUpdAnn.MessageFlags = 0 if err := signUpdate(remoteKeyPriv1, chanUpdAnn); err != nil { t.Fatalf("unable to sign channel update: %v", err) } select { - case err = <-ctx.gossiper.ProcessRemoteAnnouncement(chanUpdAnn, nodePeer): + case err = <-processRemoteAnnouncement(chanUpdAnn, nodePeer): case <-time.After(2 * time.Second): t.Fatal("did not process remote announcement") } - require.NoError(t, err, "unable to process announcement") + require.ErrorContains(t, err, "max htlc flag not set") + + // The final update should succeed. + chanUpdAnn, err = createUpdateAnnouncement( + 0, 0, remoteKeyPriv1, timestamp, + ) + require.NoError(t, err, "unable to create channel update") + + if err := signUpdate(remoteKeyPriv1, chanUpdAnn); err != nil { + t.Fatalf("unable to sign channel update: %v", err) + } + + select { + case err = <-processRemoteAnnouncement(chanUpdAnn, nodePeer): + case <-time.After(2 * time.Second): + t.Fatal("did not process remote announcement") + } + require.NoError(t, err, "expected update to be processed") } // TestSendChannelUpdateReliably ensures that the latest channel update for a diff --git a/docs/release-notes/release-notes-0.16.0.md b/docs/release-notes/release-notes-0.16.0.md index bb2b9a9d7..c05238793 100644 --- a/docs/release-notes/release-notes-0.16.0.md +++ b/docs/release-notes/release-notes-0.16.0.md @@ -21,6 +21,9 @@ of order is also [fixed](https://github.com/lightningnetwork/lnd/pull/7264). bytes](https://github.com/lightningnetwork/lnd/pull/6913). This moves LND closer to being spec compliant. +* Channel updates without the maxHTLC message flag set are recognized as invalid + and are [not relayed](https://github.com/lightningnetwork/lnd/pull/7415). + ## RPC * The `RegisterConfirmationsNtfn` call of the `chainnotifier` RPC sub-server diff --git a/funding/manager.go b/funding/manager.go index 80be2a67d..0f65e920e 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -3637,7 +3637,7 @@ func (f *Manager) newChanAnnouncement(localPubKey, // Our channel update message flags will signal that we support the // max_htlc field. - msgFlags := lnwire.ChanUpdateOptionMaxHtlc + msgFlags := lnwire.ChanUpdateRequiredMaxHtlc // We announce the channel with the default values. Some of // these values can later be changed by crafting a new ChannelUpdate. diff --git a/lnrpc/devrpc/dev_server.go b/lnrpc/devrpc/dev_server.go index d6b2f1f68..462328ea3 100644 --- a/lnrpc/devrpc/dev_server.go +++ b/lnrpc/devrpc/dev_server.go @@ -311,7 +311,8 @@ func (s *Server) ImportGraph(ctx context.Context, policy.MaxHTLC = lnwire.MilliSatoshi( rpcPolicy.MaxHtlcMsat, ) - policy.MessageFlags |= lnwire.ChanUpdateOptionMaxHtlc + policy.MessageFlags |= + lnwire.ChanUpdateRequiredMaxHtlc } return policy diff --git a/lnwire/channel_update.go b/lnwire/channel_update.go index 7881f972f..7f42a58b4 100644 --- a/lnwire/channel_update.go +++ b/lnwire/channel_update.go @@ -13,9 +13,9 @@ import ( type ChanUpdateMsgFlags uint8 const ( - // ChanUpdateOptionMaxHtlc is a bit that indicates whether the - // optional htlc_maximum_msat field is present in this ChannelUpdate. - ChanUpdateOptionMaxHtlc ChanUpdateMsgFlags = 1 << iota + // ChanUpdateRequiredMaxHtlc is a bit that indicates whether the + // required htlc_maximum_msat field is present in this ChannelUpdate. + ChanUpdateRequiredMaxHtlc ChanUpdateMsgFlags = 1 << iota ) // String returns the bitfield flags as a string. @@ -26,7 +26,7 @@ func (c ChanUpdateMsgFlags) String() string { // HasMaxHtlc returns true if the htlc_maximum_msat option bit is set in the // message flags. func (c ChanUpdateMsgFlags) HasMaxHtlc() bool { - return c&ChanUpdateOptionMaxHtlc != 0 + return c&ChanUpdateRequiredMaxHtlc != 0 } // ChanUpdateChanFlags is a bitfield that signals various options concerning a diff --git a/lnwire/lnwire_test.go b/lnwire/lnwire_test.go index 44d6cfb9b..96d0c8ae5 100644 --- a/lnwire/lnwire_test.go +++ b/lnwire/lnwire_test.go @@ -772,7 +772,7 @@ func TestLightningWireProtocol(t *testing.T) { // as being part of the ChannelUpdate, to pass // serialization tests, as it will be ignored if the bit // is not set. - if msgFlags&ChanUpdateOptionMaxHtlc == 0 { + if msgFlags&ChanUpdateRequiredMaxHtlc == 0 { maxHtlc = 0 } diff --git a/lnwire/message_test.go b/lnwire/message_test.go index f2b0892d7..090978a74 100644 --- a/lnwire/message_test.go +++ b/lnwire/message_test.go @@ -690,7 +690,7 @@ func newMsgChannelUpdate(t testing.TB, r *rand.Rand) *lnwire.ChannelUpdate { // as being part of the ChannelUpdate, to pass // serialization tests, as it will be ignored if the bit // is not set. - if msgFlags&lnwire.ChanUpdateOptionMaxHtlc == 0 { + if msgFlags&lnwire.ChanUpdateRequiredMaxHtlc == 0 { maxHtlc = 0 } diff --git a/netann/channel_update_test.go b/netann/channel_update_test.go index 4d4a6d72b..e49e5c65e 100644 --- a/netann/channel_update_test.go +++ b/netann/channel_update_test.go @@ -182,8 +182,8 @@ func TestUpdateDisableFlag(t *testing.T) { // Finally, validate the signature using the router's // verification logic. - err = routing.ValidateChannelUpdateAnn( - pubKey, 0, newUpdate, + err = routing.VerifyChannelUpdateSignature( + newUpdate, pubKey, ) if err != nil { t.Fatalf("channel update failed to "+ diff --git a/routing/ann_validation.go b/routing/ann_validation.go index 6a84aa610..35f0afd4d 100644 --- a/routing/ann_validation.go +++ b/routing/ann_validation.go @@ -129,7 +129,7 @@ func ValidateNodeAnn(a *lnwire.NodeAnnouncement) error { func ValidateChannelUpdateAnn(pubKey *btcec.PublicKey, capacity btcutil.Amount, a *lnwire.ChannelUpdate) error { - if err := validateOptionalFields(capacity, a); err != nil { + if err := ValidateChannelUpdateFields(capacity, a); err != nil { return err } @@ -160,27 +160,30 @@ func VerifyChannelUpdateSignature(msg *lnwire.ChannelUpdate, return nil } -// validateOptionalFields validates a channel update's message flags and +// ValidateChannelUpdateFields validates a channel update's message flags and // corresponding update fields. -func validateOptionalFields(capacity btcutil.Amount, +func ValidateChannelUpdateFields(capacity btcutil.Amount, msg *lnwire.ChannelUpdate) error { - if msg.MessageFlags.HasMaxHtlc() { - maxHtlc := msg.HtlcMaximumMsat - if maxHtlc == 0 || maxHtlc < msg.HtlcMinimumMsat { - return errors.Errorf("invalid max htlc for channel "+ - "update %v", spew.Sdump(msg)) - } + // The maxHTLC flag is mandatory. + if !msg.MessageFlags.HasMaxHtlc() { + return errors.Errorf("max htlc flag not set for channel "+ + "update %v", spew.Sdump(msg)) + } - // For light clients, the capacity will not be set so we'll skip - // checking whether the MaxHTLC value respects the channel's - // capacity. - capacityMsat := lnwire.NewMSatFromSatoshis(capacity) - if capacityMsat != 0 && maxHtlc > capacityMsat { - return errors.Errorf("max_htlc(%v) for channel "+ - "update greater than capacity(%v)", maxHtlc, - capacityMsat) - } + maxHtlc := msg.HtlcMaximumMsat + if maxHtlc == 0 || maxHtlc < msg.HtlcMinimumMsat { + return errors.Errorf("invalid max htlc for channel "+ + "update %v", spew.Sdump(msg)) + } + + // For light clients, the capacity will not be set so we'll skip + // checking whether the MaxHTLC value respects the channel's + // capacity. + capacityMsat := lnwire.NewMSatFromSatoshis(capacity) + if capacityMsat != 0 && maxHtlc > capacityMsat { + return errors.Errorf("max_htlc (%v) for channel update "+ + "greater than capacity (%v)", maxHtlc, capacityMsat) } return nil diff --git a/routing/localchans/manager.go b/routing/localchans/manager.go index 4484d5f62..4cec09354 100644 --- a/routing/localchans/manager.go +++ b/routing/localchans/manager.go @@ -213,7 +213,7 @@ func (r *Manager) updateEdge(tx kvdb.RTx, chanPoint wire.OutPoint, } // If the MaxHtlc flag wasn't already set, we can set it now. - edge.MessageFlags |= lnwire.ChanUpdateOptionMaxHtlc + edge.MessageFlags |= lnwire.ChanUpdateRequiredMaxHtlc // Validate htlc amount constraints. switch { diff --git a/routing/localchans/manager_test.go b/routing/localchans/manager_test.go index 55bf97e71..94f4255ce 100644 --- a/routing/localchans/manager_test.go +++ b/routing/localchans/manager_test.go @@ -46,7 +46,7 @@ func TestManager(t *testing.T) { currentPolicy := channeldb.ChannelEdgePolicy{ MinHTLC: minHTLC, - MessageFlags: lnwire.ChanUpdateOptionMaxHtlc, + MessageFlags: lnwire.ChanUpdateRequiredMaxHtlc, } updateForwardingPolicies := func( diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go index 3fb0dc9ce..4224a4f63 100644 --- a/routing/pathfind_test.go +++ b/routing/pathfind_test.go @@ -676,7 +676,7 @@ func createTestGraphFromChannels(t *testing.T, useCache bool, if node1.testChannelPolicy != nil { var msgFlags lnwire.ChanUpdateMsgFlags if node1.MaxHTLC != 0 { - msgFlags |= lnwire.ChanUpdateOptionMaxHtlc + msgFlags |= lnwire.ChanUpdateRequiredMaxHtlc } var channelFlags lnwire.ChanUpdateChanFlags if node1.Disabled { @@ -713,7 +713,7 @@ func createTestGraphFromChannels(t *testing.T, useCache bool, if node2.testChannelPolicy != nil { var msgFlags lnwire.ChanUpdateMsgFlags if node2.MaxHTLC != 0 { - msgFlags |= lnwire.ChanUpdateOptionMaxHtlc + msgFlags |= lnwire.ChanUpdateRequiredMaxHtlc } var channelFlags lnwire.ChanUpdateChanFlags if node2.Disabled { diff --git a/routing/unified_edges_test.go b/routing/unified_edges_test.go index 00a539034..752a79718 100644 --- a/routing/unified_edges_test.go +++ b/routing/unified_edges_test.go @@ -25,7 +25,7 @@ func TestNodeEdgeUnifier(t *testing.T) { FeeProportionalMillionths: 100000, FeeBaseMSat: 30, TimeLockDelta: 60, - MessageFlags: lnwire.ChanUpdateOptionMaxHtlc, + MessageFlags: lnwire.ChanUpdateRequiredMaxHtlc, MaxHTLC: 5000, MinHTLC: 100, } @@ -33,7 +33,7 @@ func TestNodeEdgeUnifier(t *testing.T) { FeeProportionalMillionths: 190000, FeeBaseMSat: 10, TimeLockDelta: 40, - MessageFlags: lnwire.ChanUpdateOptionMaxHtlc, + MessageFlags: lnwire.ChanUpdateRequiredMaxHtlc, MaxHTLC: 4000, MinHTLC: 100, }