multi: use relaxed feature bit Set method for peer features

The spec says: `The origin node MUST NOT set both the optional and
mandatory bits`. and so this is why we have the SafeSet method which
prevents us from accidentally setting both optional and required bits
for any of our own feature bits. But the spec then also says `if both
the optional and the mandatory feature bits in a pair are set, the
feature should be treated as mandatory.` which means that when we read
the feature vectors of our peers or from a payment request, we should be
a bit less strict and not error out. We should just set both bits which
will result in "IsRequired" returning true.

Update the `TestExtractIntentFromSendRequest` test to show the new
behaviour.

Author: ellemouton

lnrpc/routerrpc/router_backend.go

@@ -360,10 +360,7 @@ func (r *RouterBackend) parseQueryRoutesRequest(in *lnrpc.QueryRoutesRequest) (
 		}
 
 		// Parse destination feature bits.
-		destinationFeatures, err = UnmarshalFeatures(in.DestFeatures)
-		if err != nil {
-			return nil, err
-		}
+		destinationFeatures = UnmarshalFeatures(in.DestFeatures)
 	}
 
 	// We need to subtract the final delta before passing it into path
@@ -495,10 +492,7 @@ func unmarshalBlindedPayment(rpcPayment *lnrpc.BlindedPaymentPath) (
 		return nil, err
 	}
 
-	features, err := UnmarshalFeatures(rpcPayment.Features)
-	if err != nil {
-		return nil, err
-	}
+	features := UnmarshalFeatures(rpcPayment.Features)
 
 	return &routing.BlindedPayment{
 		BlindedPath:         path,
@@ -1124,10 +1118,7 @@ func (r *RouterBackend) extractIntentFromSendRequest(
 		payIntent.Amount = reqAmt
 
 		// Parse destination feature bits.
-		features, err := UnmarshalFeatures(rpcPayReq.DestFeatures)
-		if err != nil {
-			return nil, err
-		}
+		features := UnmarshalFeatures(rpcPayReq.DestFeatures)
 
 		// Validate the features if any was specified.
 		if features != nil {
@@ -1149,10 +1140,7 @@ func (r *RouterBackend) extractIntentFromSendRequest(
 					lnrpc.FeatureBit_AMP_OPT,
 				}
 
-				features, err = UnmarshalFeatures(ampFeatures)
-				if err != nil {
-					return nil, err
-				}
+				features = UnmarshalFeatures(ampFeatures)
 			}
 
 			// First make sure the destination supports AMP.
@@ -1348,24 +1336,26 @@ func MarshalFeatures(feats *lnwire.FeatureVector) []lnrpc.FeatureBit {
 // UnmarshalFeatures converts a list of uint32's into a valid feature vector.
 // This method checks that feature bit pairs aren't assigned together, and
 // validates transitive dependencies.
-func UnmarshalFeatures(
-	rpcFeatures []lnrpc.FeatureBit) (*lnwire.FeatureVector, error) {
-
+func UnmarshalFeatures(rpcFeatures []lnrpc.FeatureBit) *lnwire.FeatureVector {
 	// If no destination features are specified we'll return nil to signal
 	// that the router should try to use the graph as a fallback.
 	if rpcFeatures == nil {
-		return nil, nil
+		return nil
 	}
 
 	raw := lnwire.NewRawFeatureVector()
 	for _, bit := range rpcFeatures {
-		err := raw.SafeSet(lnwire.FeatureBit(bit))
-		if err != nil {
-			return nil, err
-		}
+		// Even though the spec says that the writer of a feature vector
+		// should never set both the required and optional bits of a
+		// feature, it also says that if we receive a vector with both
+		// bits set, then we should just treat the feature as required.
+		// Therefore, we don't use SafeSet here when parsing a peer's
+		// feature bits and just set the feature no matter what so that
+		// if both are set then IsRequired returns true.
+		raw.Set(lnwire.FeatureBit(bit))
 	}
 
-	return lnwire.NewFeatureVector(raw, lnwire.Features), nil
+	return lnwire.NewFeatureVector(raw, lnwire.Features)
 }
 
 // ValidatePayReqExpiry checks if the passed payment request has expired. In

lnrpc/routerrpc/router_backend_test.go

@@ -828,8 +828,7 @@ func TestExtractIntentFromSendRequest(t *testing.T) {
 					lnrpc.FeatureBit_GOSSIP_QUERIES_REQ,
 				},
 			},
-			valid:            false,
-			expectedErrorMsg: "feature pair exists",
+			valid: true,
 		},
 		{
 			name: "Valid send req parameters, payment settled",

lnwire/features.go

@@ -446,7 +446,7 @@ func (fv RawFeatureVector) Equals(other *RawFeatureVector) bool {
 	return true
 }
 
-// Merges sets all feature bits in other on the receiver's feature vector.
+// Merge sets all feature bits in other on the receiver's feature vector.
 func (fv *RawFeatureVector) Merge(other *RawFeatureVector) error {
 	for bit := range other.features {
 		err := fv.SafeSet(bit)

rpcserver.go

@@ -5782,12 +5782,9 @@ func (r *rpcServer) extractPaymentIntent(rpcPayReq *rpcPaymentRequest) (rpcPayme
 	}
 
 	// Unmarshal any custom destination features.
-	payIntent.destFeatures, err = routerrpc.UnmarshalFeatures(
+	payIntent.destFeatures = routerrpc.UnmarshalFeatures(
 		rpcPayReq.DestFeatures,
 	)
-	if err != nil {
-		return payIntent, err
-	}
 
 	return payIntent, nil
 }