Merge pull request #7415 from bitromortac/2302-max-htlc

routing+discovery: fail non-maxHTLC channel updates in validation

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)

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

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)
+			}
 		}
 	}
 

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{

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

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 

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.

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

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

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
 			}
 

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
 	}
 

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 "+

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

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 {

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(

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 {

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,
 	}