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

bitromortac · 2023-02-17T13:46:00Z

Change Description

Fixes #7290. We don't broadcast or forward any channel updates that are considered invalid by the spec (see change lightning/bolts@6fee63f), channel updates that don't set the must_be_one (or option_channel_htlc_max) flag. It may also fix #7273.

There are several places where we handle channel updates (indicated by @Crypt-iQ in #7290 (comment)).

Flows that are handled by this PR are:

channel funding: addToRouterGraph
receiving and broadcasting channel updates from a peer: ProcessRemoteAnnouncement
updating local channel policies: PropagateChanPolicyUpdate

The local graph may still contain older channel updates that don't set the flag. In order to not send those in a historical sync, we check them with ValidateOptionalFields. A migration to re-sign possible local legacy channel updates is not straightforward as it would require signing capabilities at migration time. Removing remote channel updates with a migration could be another way to fix this.

Should I rename the ChanUpdateOptionMaxHtlc field since it's required and is now named must_be_one in the spec?

Crypt-iQ · 2023-02-17T15:51:34Z

This is very close - not sure why CI isn't running. I think it makes sense to have a commit that renames ChanUpdateOptionMaxHtlc as well as ValidateOptionalFields since the bit is no longer optional.

The change to ValidateOptionalFields ensures LND doesn't relay updates without the bit because:

handleChanUpdate calls the function before adding the edge or returning it as a gossip message to relay:

lnd/discovery/gossiper.go

Line 2764 in 4d7ac82

err = routing.ValidateChannelUpdateAnn(pubKey, chanInfo.Capacity, upd)
UpdatesInHorizon now uses the function in the second commit
when processing an onion-wrapped error from a node in the route, we'll call ValidateChannelUpdateAnn before adding the edge to the graph:

lnd/routing/router.go

Line 2423 in 0eb07c9

err = ValidateChannelUpdateAnn(pubKey, ch.Capacity, msg)

The first two points mean that we won't relay these updates and will just drop them on the floor. The third point means that an onion-encrypted update won't get into our graph. Note that addToRouterGraph in current master will always set the bit when sending the update to the gossiper.

yyforyongyu · 2023-02-20T08:39:24Z

discovery/chan_series.go

+			// conform to the spec (anymore).
+			err := routing.ValidateChannelUpdateFields(0, edge1)
+			if err != nil {
+				log.Debugf("Invalid channel update %v: %v",


Maybe this should be an error log?

Changed it to an error log, I was not really sure here if we should surface it by default

bitromortac · 2023-02-20T11:01:13Z

I tested the following regtest setup: A (patched LND - sends invalid update) - B (LND - this PR) - C (core-lightning)
Node B detects the invalid update and does not relay it anymore.

ellemouton

Looking good! Nice and simple diff 🔥

ellemouton · 2023-02-20T13:37:38Z

channeldb/migration/lnwire21/channel_update.go

@@ -16,7 +16,7 @@ type ChanUpdateMsgFlags uint8
 const (
 	// ChanUpdateOptionMaxHtlc is a bit that indicates whether the


The comment no longer matches the variable name

ah yes, thank you!

ellemouton · 2023-02-20T13:40:06Z

lnwire/channel_update.go

-	// optional htlc_maximum_msat field is present in this ChannelUpdate.
-	ChanUpdateOptionMaxHtlc ChanUpdateMsgFlags = 1 << iota
+	// required htlc_maximum_msat field is present in this ChannelUpdate.
+	ChanUpdateRequiredMaxHtlc ChanUpdateMsgFlags = 1 << iota


same here re comment and var name

ellemouton · 2023-02-20T13:45:23Z

discovery/gossiper_test.go

+	case <-time.After(2 * time.Second):
+		t.Fatal("remote announcement not processed")
+	}
+	require.Error(t, err, "we expect the update to be invalid")


could be slightly stricter here and assert that the error contains max htlc flag not set for channel update

ellemouton · 2023-02-20T13:47:52Z

discovery/gossiper_test.go

@@ -2921,8 +2941,8 @@ 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 final update should not succeed, a channel update with no message


I think we should add this extra "should fail case" but then still have a final test that succeeds.

I fully agree, added 👍

ellemouton · 2023-02-20T13:49:02Z

routing/ann_validation.go

+	capacityMsat := lnwire.NewMSatFromSatoshis(capacity)
+	if capacityMsat != 0 && maxHtlc > capacityMsat {
+		return errors.Errorf("max_htlc(%v) for channel "+
+			"update greater than capacity(%v)", maxHtlc,


tiny nit: this error can now fit in 2 lines with the new indentation

ellemouton · 2023-02-20T13:51:01Z

discovery/chan_series.go

+			// conform to the spec (anymore).
+			err := routing.ValidateChannelUpdateFields(0, edge1)
+			if err != nil {
+				log.Errorf("invalid channel update %v: %v",


nit: Just to make the log a bit clearer, perhaps: "not sending invalid channel update..."

ellemouton · 2023-02-20T14:06:06Z

qq @Crypt-iQ:

when processing an onion-wrapped error from a node in the route, we'll call ValidateChannelUpdateAnn before adding the edge to the graph which means that an onion-encrypted update won't get into our graph.

I think there is a chance that we add an invalid update to our graph, then update LND to include this change and restart. Then there is a (small) chance that we call this with an invalid update. right?

Should we perhaps just validate the update there too?

Crypt-iQ · 2023-02-20T14:55:54Z

I think there is a chance that we add an invalid update to our graph, then update LND to include this change and restart.

For the onion-wrapped updates, those are an update for a node in the route (so not us). So if we add them to the graph and then restart, at least we won't relay them. We could have went with deleting local+remote updates in the graph that are missing the bit, but I was a bit concerned about edge cases.

Then there is a (small) chance that we call this with an invalid update. right?

This fetches our channel update from the graph. There is a chance that an old node has the bit un-set and upgrades and then fails backwards in the link/switch. In the linked issue (#7290 (comment)) I also pointed out a case where the gossiper's reliableSender could have an old update persisted, but it seemed like a super-edge case. I also realized later that it's possible for the switch to be restarted and call reforwardSettleFails and send a fail message backwards with an old-style update w/o the bit. This htlcswitch case seems more likely but I don't think it will cause a CLN node to disconnect an LND node because the failing node could be several hops away.

This is all a long-winded way of saying that the fix isn't perfect, but I think it's crucial to get it in 0.16.0. There is some risk that an old node tries to send an old-style update, but then they can just upgrade

We rename `ChanUpdateOptionMaxHtlc` to `ChanUpdateRequiredMaxHtlc` as with the latest changes it is now required. Similarly, rename `validateOptionalFields` to `ValidateChannelUpdateFields`, export it to use it in a later commit.

We require channel updates to have the max HTLC message flag set. Several flows need to pass that check before channel updates are forwarded to peers: * after channel funding: `addToRouterGraph` * after receiving channel updates from a peer: `ProcessRemoteAnnouncement` * after we update channel policies: `PropagateChanPolicyUpdate`

We skip syncing channel updates that don't conform to the spec.

ellemouton

Noice Noice Noice 🔥

bitromortac requested a review from Crypt-iQ February 17, 2023 13:46

saubyk requested a review from ellemouton February 17, 2023 17:18

bitromortac force-pushed the 2302-max-htlc branch from 8310b14 to 5666f6a Compare February 17, 2023 19:47

yyforyongyu reviewed Feb 20, 2023

View reviewed changes

bitromortac force-pushed the 2302-max-htlc branch from 5666f6a to 7995290 Compare February 20, 2023 10:58

Crypt-iQ approved these changes Feb 20, 2023

View reviewed changes

ellemouton reviewed Feb 20, 2023

View reviewed changes

bitromortac added 3 commits February 21, 2023 11:10

multi: rename due to required maxHTLC bit

dd5273c

We rename `ChanUpdateOptionMaxHtlc` to `ChanUpdateRequiredMaxHtlc` as with the latest changes it is now required. Similarly, rename `validateOptionalFields` to `ValidateChannelUpdateFields`, export it to use it in a later commit.

discovery: skip non-maxHTLC updates in gossip sync

6459385

We skip syncing channel updates that don't conform to the spec.

bitromortac force-pushed the 2302-max-htlc branch from 7995290 to 6459385 Compare February 21, 2023 13:23

bitromortac requested a review from ellemouton February 21, 2023 13:31

ellemouton approved these changes Feb 21, 2023

View reviewed changes

guggero merged commit b3e27f9 into lightningnetwork:master Feb 22, 2023

bitromortac deleted the 2302-max-htlc branch April 17, 2024 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bitromortac commented Feb 17, 2023

Crypt-iQ commented Feb 17, 2023

yyforyongyu Feb 20, 2023

bitromortac Feb 20, 2023

bitromortac commented Feb 20, 2023

ellemouton left a comment

ellemouton Feb 20, 2023

bitromortac Feb 21, 2023

ellemouton Feb 20, 2023

ellemouton Feb 20, 2023

ellemouton Feb 20, 2023

bitromortac Feb 21, 2023

ellemouton Feb 20, 2023

ellemouton Feb 20, 2023

ellemouton commented Feb 20, 2023

Crypt-iQ commented Feb 20, 2023 •

edited

Loading

ellemouton left a comment

		@@ -16,7 +16,7 @@ type ChanUpdateMsgFlags uint8
		const (
		// ChanUpdateOptionMaxHtlc is a bit that indicates whether the

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

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

Conversation

bitromortac commented Feb 17, 2023

Change Description

Crypt-iQ commented Feb 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac commented Feb 20, 2023

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton commented Feb 20, 2023

Crypt-iQ commented Feb 20, 2023 • edited Loading

ellemouton left a comment

Choose a reason for hiding this comment

Crypt-iQ commented Feb 20, 2023 •

edited

Loading