Merge pull request #9884 from ellemouton/relaxFeatureBitCheck

multi: use relaxed feature bit `Set` method for peer features

Author: yyforyongyu

docs/release-notes/release-notes-0.20.0.md

@@ -223,6 +223,9 @@ reader of a payment request.
   record](https://github.com/lightningnetwork/lnd/pull/9897) on the 
   `channel_update` message and handle it explicitly throughout the code base 
   instead of extracting it from the TLV stream at various call-sites.
+* [Don't error out](https://github.com/lightningnetwork/lnd/pull/9884) if an 
+  invoice's feature vector contain both the required and optional versions of a 
+  feature bit. In those cases, just treat the feature as mandatory. 
 
 * [Require invoices to include a payment address or blinded paths](https://github.com/lightningnetwork/lnd/pull/9752) 
   to comply with updated BOLT 11 specifications before sending payments.

lnrpc/routerrpc/router_backend.go

@@ -365,10 +365,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
@@ -510,10 +507,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,
@@ -1148,10 +1142,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 {
@@ -1173,10 +1164,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.
@@ -1372,24 +1360,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

@@ -881,6 +881,27 @@ func TestExtractIntentFromSendRequest(t *testing.T) {
 			valid:            false,
 			expectedErrorMsg: "self-payments not allowed",
 		},
+		{
+			name: "Required and optional feature bits set",
+			backend: &RouterBackend{
+				MaxTotalTimelock: 1000,
+				ShouldSetExpEndorsement: func() bool {
+					return false
+				},
+			},
+			sendReq: &SendPaymentRequest{
+				Dest:             destNodeBytes,
+				Amt:              int64(paymentAmount),
+				PaymentHash:      make([]byte, 32),
+				MaxParts:         10,
+				MaxShardSizeMsat: 30_000_000,
+				DestFeatures: []lnrpc.FeatureBit{
+					lnrpc.FeatureBit_GOSSIP_QUERIES_OPT,
+					lnrpc.FeatureBit_GOSSIP_QUERIES_REQ,
+				},
+			},
+			valid: true,
+		},
 		{
 			name: "Valid send req parameters, payment settled",
 			backend: &RouterBackend{

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

@@ -5827,12 +5827,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
 }