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

ellemouton · 2025-06-02T08:55:38Z

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.

Fixes #9855

The error in #9855 could appear if the user was using estimateroutefee with an
invoice containing both required&optional bits for a feature or if the SendPaymentV2 method
was used via the proto fields (ie, not setting the PayReq field).

coderabbitai · 2025-06-02T08:55:44Z

Important

Review skipped

Auto reviews are limited to specific labels.

🏷️ Labels to auto review (1)

llm-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

ellemouton · 2025-06-09T06:22:03Z

@saubyk for reviewers 🙏

yyforyongyu · 2025-06-10T17:52:58Z

lnrpc/routerrpc/router_backend.go

+		// 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


yeah the spec is confusing - but based on the description, when we are setting the bits we should always make sure not to set both, and only when we are receiving the bits we can then treat the feature as mandatory? so we should still have this check when we are parseQueryRoutesRequest or extractIntentFromSendRequest since we are creating the bits?

but we are not creating the bits in parseQueryRoutesRequest and extractIntentFromSendRequest. For those we are reading bits we got from the peer in an invoice. When we set our own bits, we do only set one.

I think for sending payments that's only true when it's using PaymentRequest aka the invoice, as the caller can set DestFeatures.

sure but i think it's ok for us to be lenient for any externally created feature vector. ie, including those set in DestFeatures - as perhaps they were extracted from a payReq by the caller.

Ie, i think we only need to be strict with the ones that we ourselves are generating from within LND and including in invoices/feature vectors we create.

Thoughts?

I would rather keep the safeSet method and also log a warning and then proceed, this makes us at least aware that something is wrong on the invoice side for example.

sure but i think it's ok for us to be lenient for any externally created feature vector. ie, including those set in DestFeatures - as perhaps they were extracted from a payReq by the caller.

My assumption is, when the user is using the payReq, it's from an invoice outside of lnd's control, so we skip the check, which is most of the case. When the user is touching the custom DestFeatures, it's another level of usage - and since it's customizing, it won't be difficult to remove the wrong feature param. Based on the specs we should not set both.

BUT, the spec is clearly broken, as sender is using receiver's data, and apparently the MUST NOT set both the optional and mandatory bits clearly cannot be enforced as the receiver doesn't care. So I think this approach works.

I think I agree with @ziggie1984. Why not keep the SafeSet, and then check for ErrFeaturePairExists error in places where we're reading the features? This way, it doesn't seem like we're normalizing this behavior since we all agree the spec isn't clear on this.

yyforyongyu

Thanks for the fix! We should update the test here tho,

lnd/lnrpc/routerrpc/router_backend_test.go

Line 497 in c6d6d4c

func TestExtractIntentFromSendRequest(t *testing.T) {

yyforyongyu · 2025-06-10T18:41:45Z

lnrpc/routerrpc/router_backend.go

+		// 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


I think for sending payments that's only true when it's using PaymentRequest aka the invoice, as the caller can set DestFeatures.

yyforyongyu · 2025-06-10T18:57:08Z

lnrpc/routerrpc/router_backend.go

-		if err != nil {
-			return nil, err
-		}
+		features := UnmarshalFeatures(rpcPayReq.DestFeatures)


The sender is setting the field, so I think we should error out in this case? payReq.Features is what's extracted from the invoice at L1067, and it was not validated I think.

see comment below

and it was not validated I think.

i think this is the point though right? ie, not to be too strict with feature vectors we are parsing. As long as we then read it as "required", things are fine

As long as we then read it as "required", things are fine

I think so, tho not sure it was skipped intentionally or accidentally, but yeah what i meant was the features parsed from an outside source has already been skipped so no need to worry about that.

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.

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.

ellemouton

thanks for the review @yyforyongyu 🙏
updated that test as per suggestion.
Let me know your thoughts on the discussion

ellemouton · 2025-06-25T11:03:48Z

lnrpc/routerrpc/router_backend.go

+		// 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


sure but i think it's ok for us to be lenient for any externally created feature vector. ie, including those set in DestFeatures - as perhaps they were extracted from a payReq by the caller.

Ie, i think we only need to be strict with the ones that we ourselves are generating from within LND and including in invoices/feature vectors we create.

Thoughts?

ellemouton · 2025-06-25T11:04:14Z

lnrpc/routerrpc/router_backend.go

-		if err != nil {
-			return nil, err
-		}
+		features := UnmarshalFeatures(rpcPayReq.DestFeatures)


see comment below

ellemouton · 2025-06-25T11:14:15Z

lnrpc/routerrpc/router_backend.go

-		if err != nil {
-			return nil, err
-		}
+		features := UnmarshalFeatures(rpcPayReq.DestFeatures)


and it was not validated I think.

i think this is the point though right? ie, not to be too strict with feature vectors we are parsing. As long as we then read it as "required", things are fine

yyforyongyu

LGTM🦾

As long as every implementation is following the spec there's no issue relaxing setting the feature flags,

The receiving node:

if both the optional and the mandatory feature bits in a pair are set, the feature should be treated as mandatory.

yyforyongyu · 2025-06-27T10:31:01Z

lnrpc/routerrpc/router_backend_test.go

@@ -809,6 +809,27 @@ func TestExtractIntentFromSendRequest(t *testing.T) {
 			valid:            false,
 			expectedErrorMsg: "self-payments not allowed",
 		},
+		{
+			name: "Required and optional feature bits set",


Abdulkbk

Nice one!, left a feedback.

Abdulkbk · 2025-07-28T22:15:32Z

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,


Abdulkbk · 2025-07-28T22:23:25Z

lnrpc/routerrpc/router_backend.go

@@ -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


Need to update this comment then because it says ...checks that feature bit pairs aren't assigned together?

Abdulkbk · 2025-07-28T22:59:36Z

lnrpc/routerrpc/router_backend.go

+		// 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


I think I agree with @ziggie1984. Why not keep the SafeSet, and then check for ErrFeaturePairExists error in places where we're reading the features? This way, it doesn't seem like we're normalizing this behavior since we all agree the spec isn't clear on this.

lightninglabs-deploy · 2025-09-02T02:33:44Z

@ziggie1984: review reminder
@ellemouton, remember to re-request review from reviewers when ready

ellemouton force-pushed the relaxFeatureBitCheck branch from b761069 to 80db68a Compare June 2, 2025 08:56

ellemouton self-assigned this Jun 2, 2025

ellemouton added this to lnd v0.20 Jun 2, 2025

ellemouton removed this from lnd v0.20 Jun 2, 2025

ellemouton added this to the v0.20.0 milestone Jun 2, 2025

ellemouton added spec feature bits labels Jun 2, 2025

ellemouton requested review from ziggie1984 and yyforyongyu June 10, 2025 16:15

yyforyongyu reviewed Jun 10, 2025

View reviewed changes

ellemouton added 3 commits June 25, 2025 13:10

docs: update release notes

e68b882

ellemouton force-pushed the relaxFeatureBitCheck branch from 80db68a to e68b882 Compare June 25, 2025 11:15

ellemouton commented Jun 25, 2025

View reviewed changes

yyforyongyu approved these changes Jun 27, 2025

View reviewed changes

Abdulkbk reviewed Jul 28, 2025

View reviewed changes

		@@ -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

multi: use relaxed feature bit Set method for peer features #9884

Are you sure you want to change the base?

multi: use relaxed feature bit Set method for peer features #9884

Uh oh!

Conversation

ellemouton commented Jun 2, 2025

Uh oh!

coderabbitai bot commented Jun 2, 2025

Review skipped

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

ellemouton commented Jun 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Abdulkbk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented Sep 2, 2025

Uh oh!

Uh oh!

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

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

CodeRabbit Configuration File (`.coderabbit.yaml`)