htlcswitch/hop/payload: apply known constraints if required failure

This commit modifies the NewPayloadFromReader to apply known
presence/omission contraints in the event that the tlv parser returns an
unknown required type failure.

Now that the parser has been modified to finished parsing the stream to
obtain a proper parsed type set, we can accurately apply these higher
level validation checks. This overrides required type failures, such
that they are only returned if the sender properly abided by the
constraints on fields for which we know.

The unit tests are updated to create otherwise valid payloads that then
return unknown required type failures. In one case, a test which
previously returned an unknown required type failure is made to return
an included failure for the sid, indicating the unknown required type 0
is being overruled.

htlcswitch/hop/payload.go

@@ -5,7 +5,7 @@ import (
 	"fmt"
 	"io"
 
-	"github.com/lightningnetwork/lightning-onion"
+	sphinx "github.com/lightningnetwork/lightning-onion"
 	"github.com/lightningnetwork/lnd/lnwire"
 	"github.com/lightningnetwork/lnd/record"
 	"github.com/lightningnetwork/lnd/tlv"
@@ -120,34 +120,32 @@ func NewPayloadFromReader(r io.Reader) (*Payload, error) {
 	if err != nil {
 		// Promote any required type failures into ErrInvalidPayload.
 		if e, required := err.(tlv.ErrUnknownRequiredType); required {
-			// NOTE: Sigh. If the sender included a next hop whose
+			// If the parser returned an unknown required type
-			// value is zero, this would be considered invalid by
+			// failure, we'll first check that the payload is
-			// our validation rules below. It's not totally clear
+			// properly formed according to our known set of
-			// whether this required failure should take precedence
+			// constraints. If an error is discovered, this
-			// over the constraints applied by known types.
+			// overrides the required type failure.
-			// Unfortunately this is an artifact of the layering
+			nextHop := lnwire.NewShortChanIDFromInt(cid)
-			// violation in placing the even/odd rule in the parsing
+			err = ValidateParsedPayloadTypes(parsedTypes, nextHop)
-			// logic and not at a higher level of validation like
+			if err != nil {
-			// the other presence/omission checks.
+				return nil, err
-			//
+			}
-			// As a result, this may need to be revisted if it is
+
-			// decided that the checks below overrule an unknown
+			// Otherwise the known constraints were applied
-			// required type failure, in which case an
+			// successfully, report the invalid type failure
-			// IncludedViolation should be returned instead of the
+			// returned by the parser.
-			// RequiredViolation.
 			return nil, ErrInvalidPayload{
 				Type:      tlv.Type(e),
 				Violation: RequiredViolation,
-				FinalHop:  cid == 0,
+				FinalHop:  nextHop == Exit,
 			}
 		}
 		return nil, err
 	}
 
-	nextHop := lnwire.NewShortChanIDFromInt(cid)
-
 	// Validate whether the sender properly included or omitted tlv records
 	// in accordance with BOLT 04.
+	nextHop := lnwire.NewShortChanIDFromInt(cid)
 	err = ValidateParsedPayloadTypes(parsedTypes, nextHop)
 	if err != nil {
 		return nil, err

htlcswitch/hop/payload_test.go

@@ -79,7 +79,7 @@ var decodePayloadTests = []decodePayloadTest{
 	},
 	{
 		name:    "required type after omitted hop id",
-		payload: []byte{0x08, 0x00},
+		payload: []byte{0x02, 0x00, 0x04, 0x00, 0x08, 0x00},
 		expErr: hop.ErrInvalidPayload{
 			Type:      8,
 			Violation: hop.RequiredViolation,
@@ -88,8 +88,8 @@ var decodePayloadTests = []decodePayloadTest{
 	},
 	{
 		name: "required type after included hop id",
-		payload: []byte{0x06, 0x08, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00,
+		payload: []byte{0x02, 0x00, 0x04, 0x00, 0x06, 0x08, 0x01, 0x00,
-			0x00, 0x00, 0x08, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00,
 		},
 		expErr: hop.ErrInvalidPayload{
 			Type:      8,
@@ -99,7 +99,7 @@ var decodePayloadTests = []decodePayloadTest{
 	},
 	{
 		name:    "required type zero final hop",
-		payload: []byte{0x00, 0x00},
+		payload: []byte{0x00, 0x00, 0x02, 0x00, 0x04, 0x00},
 		expErr: hop.ErrInvalidPayload{
 			Type:      0,
 			Violation: hop.RequiredViolation,
@@ -108,19 +108,19 @@ var decodePayloadTests = []decodePayloadTest{
 	},
 	{
 		name: "required type zero final hop zero sid",
-		payload: []byte{0x00, 0x00, 0x06, 0x08, 0x00, 0x00, 0x00, 0x00,
+		payload: []byte{0x00, 0x00, 0x02, 0x00, 0x04, 0x00, 0x06, 0x08,
-			0x00, 0x00, 0x00, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 		},
 		expErr: hop.ErrInvalidPayload{
-			Type:      0,
+			Type:      6,
-			Violation: hop.RequiredViolation,
+			Violation: hop.IncludedViolation,
 			FinalHop:  true,
 		},
 	},
 	{
 		name: "required type zero intermediate hop",
-		payload: []byte{0x00, 0x00, 0x06, 0x08, 0x01, 0x00, 0x00, 0x00,
+		payload: []byte{0x00, 0x00, 0x02, 0x00, 0x04, 0x00, 0x06, 0x08,
-			0x00, 0x00, 0x00, 0x00,
+			0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 		},
 		expErr: hop.ErrInvalidPayload{
 			Type:      0,