LND should fail connection upon receipt of unknown, evenly-typed message

[Crypt-iQ]

From BOLT#01

upon receiving a message of even, unknown type:
    MUST close the connection.
    MAY fail the channels.

However, from my reading of the code, LND will just return UnknownMessage from makeEmptyMessage and continue to read the next message in the readHandler:

lnd/peer/brontide.go

Lines 1464 to 1468 in 143eba8

	// compatible manner.
	case *lnwire.UnknownMessage:
	p.storeError(e)
	idleTimer.Reset(idleTimeout)
	continue

This is fine for unknown, odd messages but apparently not for unknown, even messages

[lightningorb]

#7273 might be related.

[lightningorb]

Something like this then maybe.. i don't know. This is a draft contribution.

			// If this is an odd message we don't yet recognize,
			// we'll continue processing as normal as this allows
			// us to introduce new messages in a forwards
			// compatible manner. However if it's even we must close
			// the connection
			case *lnwire.UnknownMessage:
				msgType := nextMsg.MsgType()
				error := fmt.Sprintf("Received unknown even message type: %v", uint16(msgType))
				p.storeError(errors.New(error))
				if nextMsg.MsgType()%2 == 0 {
					p.Disconnect(errors.New(error))
				} else {
					idleTimer.Reset(idleTimeout)
				}
				continue

This builds, runs and the tests pass but ideally this would require a tailored test for it before creating a PR.

[Crypt-iQ]

I think a new error should be returned from lnwire.ReadMessage. Then there would be another switch case in the section you referenced that will disconnect from the peer. This keeps the wire-parsing logic nicely separated from the peer/brontide.go code

[lightninglabs-deploy]

Closing as duplicate of #10311 which tracks the same class of spec non-compliance: LND not properly rejecting unknown even (required) types. Unique details have been summarized there.