[bug]: htlc_maximum_msat is not optional anymore in channel updates

[yaslama]

Background

lnd forwards channel updates without htlc_maximum_msat which is now forbidden after lightning/bolts#999 was merged.
The problem is that in this case, other nodes (cln for instance) send back a warning message which triggers a disconnect.

Your environment

lnd v0.15.5-beta or master

Steps to reproduce

Run lnd and create a channel to a node running cln.

Expected behaviour

No channel update messages without htlc_maximum_msat

Actual behaviour

Some channel update messages without htlc_maximum_msat. Then a warning is issued by the other node (cln for instance). Then the peer is disconnected in lnd v0.15.5 because the warning message is "unknown" and in master because a warning triggers a disconnect for an active channel.

[yaslama]

BTW the logs were the same as #7273 and we backported the warning message handling from master to see that the problem was the issue described here

[kingonly]

@saubyk this is spec incompatibly issue that has some nasty side effects. Can you please consider prioritizing to the next update?

[Roasbeef]

Interesting, I think another operative question is: what software is sending out update w/o max sat set? FWIW, the spec doesn't say precisely what to do if you encounter such an update. A conservative thing to do would just be to forward it anyway, since it's older software that may not be keeping up lockstep with the frequent spec changes. If we started to reject this wholesale, this could lead to other unintended propagation issues (we've fixed a number of them in the upcoming 0.16 release).

in master because a warning triggers a disconnect for an active channel.

This should have been resolved with this PR: #6840 -- are you able to repro?

[kingonly]

We don't know what software is sending these updates, but we're seeing many.

If we started to reject this wholesale, this could lead to other unintended propagation issues (we've fixed a number of them in the upcoming 0.16 release).

I think messages that don't comply with the spec shouldn't be propagated. Are there any more that are being propagated?

[Roasbeef]

I think this is the CLN commit that starts to reject (parsed as zero so fails validation?): ElementsProject/lightning@5553b85

I looked at this Eclair PR, but can't tell for sure if they're rejecting these updates themselves: https://github.com/ACINQ/eclair/pull/2299/commits

Looks like RL has been doing this since July or so (before that spec PR above was merged): https://github.com/lightningdevkit/rust-lightning/pull/1519/files

---

We don't know what software is sending these updates, but we're seeing many.

Gotcha, that's interesting, since the original motive for this was that from observations pretty much no updates were setting the value. If we're still seeing a lot, then I think that argues for pushing out the deployment of the spec modification.

From the original spec PR:

I have just downloaded a fresh batch of 134k channel updates from an LND peer; of those channel updates, 364 don't have an HTLC max set, which is 0.27%.
I have separately downloaded 161k updates from a different peer, and got 399 instances of an unset HTLC max, or 0.25%.
Indeed, every single of the 149964 channel updates seen by my node has his set.

---

I think messages that don't comply with the spec shouldn't be propagated. Are there any more that are being propagated?

Generally I'd agree, but this is a situation where something in the spec was merged that affects gossip, but not everyone has started to observe the new rules. If there're a lot of updates from routing nodes that would no longer propagate with strict enforcement, then we might want to consider holding off if the old assumptions weren't correct (tho ofc the change is trivial for the most part).

I guess w/e unknown impl/version is sending update w/o it set will just be slowly cut off from the network 🤷