From 9ee3bebc6ad15633573be7506da6f94b31a166c6 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 25 Jun 2025 13:10:48 +0200 Subject: [PATCH 1/3] lnrpc: add test case to TestExtractIntentFromSendRequest Here we add a test case to TestExtractIntentFromSendRequest which shows that an error is returned if the destination feature bit vector of a payment request contains both the required and optional bits for a given feature. This will be updated in an upcoming commit to be less strict. --- lnrpc/routerrpc/router_backend_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/lnrpc/routerrpc/router_backend_test.go b/lnrpc/routerrpc/router_backend_test.go index 5a633d9e6..b1b8a2471 100644 --- a/lnrpc/routerrpc/router_backend_test.go +++ b/lnrpc/routerrpc/router_backend_test.go @@ -809,6 +809,28 @@ 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: false, + expectedErrorMsg: "feature pair exists", + }, { name: "Valid send req parameters, payment settled", backend: &RouterBackend{ From b41a0799018c0780e0179906221dad59cf4aba10 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Mon, 2 Jun 2025 10:20:15 +0200 Subject: [PATCH 2/3] 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. --- lnrpc/routerrpc/router_backend.go | 40 ++++++++++---------------- lnrpc/routerrpc/router_backend_test.go | 3 +- lnwire/features.go | 2 +- rpcserver.go | 5 +--- 4 files changed, 18 insertions(+), 32 deletions(-) diff --git a/lnrpc/routerrpc/router_backend.go b/lnrpc/routerrpc/router_backend.go index b74484d5c..81c6dde14 100644 --- a/lnrpc/routerrpc/router_backend.go +++ b/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 diff --git a/lnrpc/routerrpc/router_backend_test.go b/lnrpc/routerrpc/router_backend_test.go index b1b8a2471..8da0af682 100644 --- a/lnrpc/routerrpc/router_backend_test.go +++ b/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", diff --git a/lnwire/features.go b/lnwire/features.go index 31e750055..107828ee2 100644 --- a/lnwire/features.go +++ b/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) diff --git a/rpcserver.go b/rpcserver.go index 124f7e9dd..cc000ed31 100644 --- a/rpcserver.go +++ b/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 } From e68b882f7bb23e8246227914c27d6008793b5a62 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Mon, 2 Jun 2025 10:52:58 +0200 Subject: [PATCH 3/3] docs: update release notes --- docs/release-notes/release-notes-0.20.0.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/release-notes/release-notes-0.20.0.md b/docs/release-notes/release-notes-0.20.0.md index 040290f33..ebfc453b5 100644 --- a/docs/release-notes/release-notes-0.20.0.md +++ b/docs/release-notes/release-notes-0.20.0.md @@ -125,6 +125,9 @@ circuit. The indices are only available for forwarding events saved after v0.20. 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. ## Testing