multi: Inbound fees are retained when not provided #8758

feelancer21 · 2024-05-14T21:37:41Z

Fixes the problem that inbound base fee and fee rate are overwritten with 0 if they are not specified in PolicyUpdateRequest. This ensures backward compatibility with older rpc clients that do not yet support the inbound feature.

Change Description

Fixes #8614

I had initially considered working with a boolean. However, as the base fee and fee rate are two values that are updated and not just one, I found it more elegant to outsource them to an extra message, which can also be nil. Both implementation variants mean that rpc clients that already implement the inbound fees in the current variant have to update their protos again.

The idea with the fn package comes from @ziggie1984 . Thanks for that .

Steps to Test

Tested with lncli as described in #8614.

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

coderabbitai · 2024-05-14T21:37:46Z

Important

Review Skipped

Auto reviews are disabled on this repository.

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.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

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>.
- Generate unit testing code for this file.
- 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 generate unit testing code for this file.
- @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 generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration 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.

yyforyongyu

Thanks for the PR! Left a few comments, and could you test the new behavior in itest/lnd_channel_policy_test.go?

yyforyongyu · 2024-05-16T18:08:20Z

cmd/lncli/commands.go

+	// Inbound fees are optional. However, if an update is required,
+	// both the base fee and the fee rate must be provided.
+	var inboundFee *lnrpc.InboundFee
+	switch {


I'd remove the switch case and do the following instead, easier to understand,

if ctx.IsSet("inbound_base_fee_msat") != ctx.IsSet("inbound_fee_rate_ppm") { return errors.New("both parameters must be provided: " + "inbound_base_fee_msat and inbound_fee_rate_ppm") } inboundFee := &lnrpc.InboundFee{ BaseFeeMsat: int32(inboundBaseFeeMsat), FeeRatePpm: int32(inboundFeeRatePpm), }

Thank you very much for your comments. I'll take a detailed look tomorrow.
At this point I don't quite understand what you mean. We need to catch the case where both lncli are not set and then pass nil to the rpc. Or am I missing something?

nvm, it seems this PR requires the user to set both inbound_fee_rate_ppm and inbound_base_fee_msat at the same time, but I think we want to allow updating a single field, so either inbound_base_fee_msat or inbound_fee_rate_ppm?

lnrpc/lightning.proto

yyforyongyu · 2024-05-16T18:19:47Z

routing/localchans/manager.go

-	inboundFee := newSchema.InboundFee.ToWire()
-	if err := edge.ExtraOpaqueData.PackRecords(&inboundFee); err != nil {
-		return err
+	if !newSchema.InboundFee.IsNone() {


Should use WhenSome instead

Yeah we want to remove UnsafeFromSome.

You can also write it like this (to catch that error):

errr := fn.MapOptionZ(newSchema.InboundFee, func(f models.InboundFee) error { return edge.ExtraOpaqueData.PackRecords(&inboundWireFee) })

But see my other comment, I think we should just have the new fields be a direct part of the struct instead of encoding into the extra data. Though I can understand if you leave to wish it like this, as your main goal was resolving the bug re default values.

yyforyongyu · 2024-05-16T18:27:48Z

routing/localchans/manager.go

@@ -112,7 +122,7 @@ func (r *Manager) UpdatePolicy(newSchema routing.ChannelPolicy,
 			TimeLockDelta: uint32(edge.TimeLockDelta),
 			MinHTLCOut:    edge.MinHTLC,
 			MaxHTLC:       edge.MaxHTLC,
-			InboundFee:    newSchema.InboundFee,
+			InboundFee:    models.NewInboundFeeFromWire(inboundFee),


Think we can just do newSchema.InboundFee.UnwrapOr(models.InboundFee{}) here to avoid the ExtractRecords above?

yyforyongyu · 2024-05-16T18:29:38Z

routing/localchans/manager.go

@@ -182,9 +192,14 @@ func (r *Manager) updateEdge(tx kvdb.RTx, chanPoint wire.OutPoint,
 		newSchema.FeeRate,
 	)

-	inboundFee := newSchema.InboundFee.ToWire()
-	if err := edge.ExtraOpaqueData.PackRecords(&inboundFee); err != nil {


Do we know what else has been saved in this ExtraOpaqueData field? I think PackRecords will overwrite it - if we have other fields they will be lost.

There probably should be an UpdateRecords method on ExtraOpaqueData, it first reads the existing records, then updates the specified records.

I think we should just lift the field directly into the struct, rather than making a callers aways encode it and decode it. Not sure the rational of implementing it like this in the first place...

If we just set the field, then we also ensure that nothing else gets clobbered.

Yeah that'd be even better if we could just add these two fields on edge.

I can add the fields to the edge. perhaps the reason was to avoid redundant data storage

yyforyongyu · 2024-05-16T18:33:27Z

rpcserver.go

 		}
 	}

+	// In case inbound fees are not specified no inbound fees will be
+	// applied but the previous value will be retained.


How is the previous value being retained?

updateEdge has been modified to check the option value, only setting the value if Some.

Roasbeef · 2024-05-16T23:06:31Z

cmd/lncli/commands.go

+	case ctx.IsSet("inbound_fee_rate_ppm"):
+		inboundFee = &lnrpc.InboundFee{
+			BaseFeeMsat: int32(inboundBaseFeeMsat),
+			FeeRatePpm:  int32(inboundFeeRatePpm)}


Close bracket should be on a new line.

I see how using the sub-proto here helps us check for nil-ness. But don't we need to use the approach for the individual fields? Otherwise if the the base fee value is set, but the ppm value isn't, then the msat value will get reset to zero.

In the future we could deprecate the other fields (e.g. outbound fees) and make them optional as well, as it seems to lead to a non-intuitive way to update values where one has to echo the non-updated values.

We could make BaseFeeMsat and FeeRatePpm using the optional flag in proto3 and check their nil-ness - we need to update falafel first tho, as last time I tried to use it, make rpc would fail with,

walletkit.proto: is a proto3 file that contains optional fields, but code generator protoc-gen-custom hasn't been updated to support optional fields in proto3. Please ask the owner of this code generator to support proto3 optional.--custom_out:

My primary goal was to ensure backwards compatibility with older RPC clients, so that a PolicyUpdate from clients that do not recognize inbound fees does not overwrite the InboundFees with 0. Many noderunners use several clients. However, clients that use InboundFees must set both fields.

But I would also prefer an optional flag. Then you could also do without the submessage, which is more of a workaround.

Who can customize Falafel, if optionality is desired?

Roasbeef · 2024-05-16T23:07:15Z

itest/lnd_multi-hop-payments_test.go

+		MaxHtlcMsat: maxHtlc,
+		InboundFee: &lnrpc.InboundFee{
+			BaseFeeMsat: inboundBaseFee,
+			FeeRatePpm:  inboundFeeRate},


Close bracket should be on a new line.

Roasbeef · 2024-05-16T23:08:22Z

lnrpc/lightning.proto

-    int32 inbound_fee_rate_ppm = 11;
+    // Optional inbound fee. If unset, the previously set value will be
+    // retained [EXPERIMENTAL].
+    InboundFee inbound_fee = 10;


Good move to make it into a sub-proto, this way we can check for nil-ness 👍

Roasbeef · 2024-05-16T23:09:59Z

routing/localchans/manager.go

+		// or without specified inbound fees. So having a decoding error
+		// reveils another problem.
+		var inboundFee lnwire.Fee
+		_, err = edge.ExtraOpaqueData.ExtractRecords(&inboundFee)


Why's parsing it again require here, whereas it wasn't before this diff?

Roasbeef · 2024-05-16T23:12:47Z

routing/router.go

@@ -310,7 +311,7 @@ type ChannelPolicy struct {

 	// MinHTLC is the minimum HTLC size including fees we are allowed to
 	// forward over this channel.
-	MinHTLC *lnwire.MilliSatoshi
+	MinHTLC fn.Option[lnwire.MilliSatoshi]


Seems like a distinct change.

Roasbeef · 2024-05-16T23:14:16Z

routing/localchans/manager.go

@@ -182,9 +192,14 @@ func (r *Manager) updateEdge(tx kvdb.RTx, chanPoint wire.OutPoint,
 		newSchema.FeeRate,
 	)

-	inboundFee := newSchema.InboundFee.ToWire()
-	if err := edge.ExtraOpaqueData.PackRecords(&inboundFee); err != nil {


I think we should just lift the field directly into the struct, rather than making a callers aways encode it and decode it. Not sure the rational of implementing it like this in the first place...

If we just set the field, then we also ensure that nothing else gets clobbered.

Roasbeef · 2024-05-16T23:15:07Z

routing/localchans/manager.go

-	inboundFee := newSchema.InboundFee.ToWire()
-	if err := edge.ExtraOpaqueData.PackRecords(&inboundFee); err != nil {
-		return err
+	if !newSchema.InboundFee.IsNone() {


Yeah we want to remove UnsafeFromSome.

You can also write it like this (to catch that error):

errr := fn.MapOptionZ(newSchema.InboundFee, func(f models.InboundFee) error { return edge.ExtraOpaqueData.PackRecords(&inboundWireFee) })

But see my other comment, I think we should just have the new fields be a direct part of the struct instead of encoding into the extra data. Though I can understand if you leave to wish it like this, as your main goal was resolving the bug re default values.

Roasbeef · 2024-05-16T23:15:49Z

rpcserver.go

 		}
 	}

+	// In case inbound fees are not specified no inbound fees will be
+	// applied but the previous value will be retained.


updateEdge has been modified to check the option value, only setting the value if Some.

saubyk · 2024-05-21T03:48:52Z

Hi @feelancer21 we'd like to get this pr merged into the next rc asap.
Please let us know, if you can turn this around quickly. If not, we can take this over. Thanks

feelancer21 · 2024-05-21T06:51:51Z

Hello guys,

have made an update. Here are a few comments:

My primary goal was to ensure backwards compatibility with older RPC clients, so that a PolicyUpdate from clients that do not recognize inbound fees does not overwrite the InboundFees with 0. Many noderunners use several clients. However, clients that use InboundFees must set both fields. Optionality at field level should be addressed separately in the future. Maybe we need an extra rpc call then.

2 I have now included the inbound fees in the edge. However, I did not split up the individual fields because they would have been merged again in UpdatePolicy anyway. But I can still change this if desired.

extract and pack the ExtraOpaqueData I have not outsourced to a separate method. Since policies have no other ExtraOpaqueData, this should not lead to problems.
I had to integrate itests for the inbound fees into the lnd_channel_policy_test.go. As part of this, I had to fix another issue with the assert of the policies. The nil case is also tested.

Regards

routing/localchans/manager.go

bitromortac · 2024-05-21T08:58:52Z

channeldb/models/channel_edge_policy.go

@@ -60,6 +60,9 @@ type ChannelEdgePolicy struct {
 	// HTLCs for each millionth of a satoshi forwarded.
 	FeeProportionalMillionths lnwire.MilliSatoshi

+	// InboundFee that will be charged for incoming HTLCs.
+	InboundFee


Not sure if adding this is the best approach (in this PR) if we also expose ExtraOpaqueData because those two values could become out of sync? We should probably decide on whether we want to use the fields or the ExtraOpaqueData as source of truth and then also take care about de/serialization and setting/getting inbound fees from it.

I had that thought too. An alternative I had in mind was to define an InboundFee in UpdatePolicy and pass a pointer to it to UpdateEdge. This would be quickly adjusted. I would just need to know in which direction we want to go.

Have created an alternative. Preview can be found here: https://github.com/feelancer21/lnd/compare/e080792451184cf0810831230cf8ef8609a4cc51..9b17806

I'd probably have it this way (I think you had a similar version before): b8b2a05, but not sure about the other reviewer's opinions. We could deal with adding the inbound fees field in a later PR. I think technically htlc_maximum_msat was also part of the extra data, because it was optional, but got then promoted to models.UpdateEdgePolicy and lnwire.ChannelUpdate. We could do a similar transition by removing inbound fees from the extra data to promote it to models.UpdateEdgePolicy. We could then leave any non-interpreted extra data untouched, for others to use via APIs.

I'd rather do InboundFee *InboundFee than embed this, tho I think we should do it in another PR.

Ok. Have integrated the version of @bitromortac.

One problem I wanted to note here (not an issue with this PR) with extracting the inbound fees (or other data later) from the extra data may be that the order of TLVs could change if we re-serialize to lnwire.ChannelUpdate, maybe leading to signature invalidation, when we try to relay the message.

lntest/node/watcher.go

yyforyongyu

I think this is very close! As mentioned by @bitromortac, there's this mapping from a ChannelUpdate to a ChannelEdgePolicy, in which we also map ChannelUpdate.ExtraOpaqueData to ChannelEdgePolicy.ExtraOpaqueData. Ideally we'll want to deserialize the bytes in ChannelUpdate.ExtraOpaqueData, and extract the inbound fee info into a new field ChannelEdgePolicy.InboundFee since we cannot be sure in the future whether there would be more data being saved in ChannelUpdate.ExtraOpaqueData and transferred over the wire. This means we need to also change the existing mapping, which means more work.

The second approach is to stick to the ChannelEdgePolicy.ExtraOpaqueData as suggested by @bitromortac - we certainly don't wanna two places to hold the same data as it'd soon become a question about who holds the truth.

I'd prefer the second approach as it's almost already done here and we want the release soon.

yyforyongyu · 2024-05-22T13:43:15Z

channeldb/models/channel_edge_policy.go

@@ -60,6 +60,9 @@ type ChannelEdgePolicy struct {
 	// HTLCs for each millionth of a satoshi forwarded.
 	FeeProportionalMillionths lnwire.MilliSatoshi

+	// InboundFee that will be charged for incoming HTLCs.
+	InboundFee


I'd rather do InboundFee *InboundFee than embed this, tho I think we should do it in another PR.

yyforyongyu · 2024-05-22T13:44:37Z

cmd/lncli/commands.go

+	// Inbound fees are optional. However, if an update is required,
+	// both the base fee and the fee rate must be provided.
+	var inboundFee *lnrpc.InboundFee
+	if ctx.IsSet("inbound_base_fee_msat") !=


Q: so these two fields must be set together?

Yes, Base and Feerate must be set, analogous to the outbound fees. Optionality at field level should be addressed separately for all fields of the update request.

yyforyongyu · 2024-05-22T13:47:54Z

itest/lnd_multi-hop-payments_test.go

-	chanPoint *lnrpc.ChannelPoint, baseFee int64,
-	feeRate int64, inboundBaseFee, inboundFeeRate int32,
+	chanPoint *lnrpc.ChannelPoint, baseFee int64, feeRate int64,
+	inboundBaseFee, inboundFeeRate int32, updateInboundFee bool,


think we can pass a param inboundFee *lnrpc.InboundFee here instead of using three params - we can then simply assign it to PolicyUpdateRequest therefore no need to have updateInboundFee bool.

We would also need the actual inbound fees within the function in the nil case. If you work with 0 in this case, the assert of the RoutingPolicy will fail. That's why I introduced the update boolen. Then you can pass the inbound fees for the assert without updating the fees.

I see what you doing now - updateChannelPolicy is just a helper method that updates the policy and asserts the update has taken effect - this function should probably be removed in the future as it's not just the inbound fees that have this problem, all other params passed here share this issue. For instance, we always copy the same maxHtlc over and over in this test, as otherwise, the assertion would fail.

As for now, we can just keep it as it is.

saubyk · 2024-05-22T15:30:21Z

Hi @feelancer21 would you be able to address the feedback comments today?

feelancer21 · 2024-05-22T18:20:38Z

Hi @feelancer21 would you be able to address the feedback comments today?

that's the plan

Fixes the problem that inbound base fee and fee rate are overwritten with 0 if they are not specified in PolicyUpdateRequest. This ensures backward compatibility with older rpc clients that do not yet support the inbound feature.

Add tests for setting inbound fees in channel policies, including tests where no inbound fees are set in the PolicyUpdateRequest.

yyforyongyu

Thanks for the fix 🙏

yyforyongyu · 2024-05-23T09:56:33Z

itest/lnd_multi-hop-payments_test.go

-	chanPoint *lnrpc.ChannelPoint, baseFee int64,
-	feeRate int64, inboundBaseFee, inboundFeeRate int32,
+	chanPoint *lnrpc.ChannelPoint, baseFee int64, feeRate int64,
+	inboundBaseFee, inboundFeeRate int32, updateInboundFee bool,


I see what you doing now - updateChannelPolicy is just a helper method that updates the policy and asserts the update has taken effect - this function should probably be removed in the future as it's not just the inbound fees that have this problem, all other params passed here share this issue. For instance, we always copy the same maxHtlc over and over in this test, as otherwise, the assertion would fail.

As for now, we can just keep it as it is.

yyforyongyu · 2024-05-23T10:00:20Z

itest/lnd_multi-hop-payments_test.go

@@ -85,9 +85,19 @@ func testMultiHopPayments(ht *lntest.HarnessTest) {
 		aliceInboundFeeRate     = -50000 // 5%
 	)

+	// We update the channel twice. The first time we set the inbound fee,


I think this behavior is already tested in req.InboundFee = nil, plus this is an MPP-related test and should not test policy updates.

bitromortac

LGTM 🎉, I tested also via cli.

saubyk requested a review from bitromortac May 16, 2024 15:25

saubyk assigned feelancer21 May 16, 2024

saubyk requested a review from yyforyongyu May 16, 2024 15:25

saubyk added this to the v0.18.0 milestone May 16, 2024

yyforyongyu requested changes May 16, 2024

View reviewed changes

Roasbeef requested changes May 16, 2024

View reviewed changes

feelancer21 force-pushed the preserve-inbound-fees branch from b4bfab5 to e080792 Compare May 21, 2024 06:44

bitromortac reviewed May 21, 2024

View reviewed changes

yyforyongyu requested changes May 22, 2024

View reviewed changes

feelancer21 added 2 commits May 22, 2024 20:48

multi: Inbound fees are retained when not provided

f62c00f

Fixes the problem that inbound base fee and fee rate are overwritten with 0 if they are not specified in PolicyUpdateRequest. This ensures backward compatibility with older rpc clients that do not yet support the inbound feature.

itest: add more tests related to inbound fees

dc4ec45

Add tests for setting inbound fees in channel policies, including tests where no inbound fees are set in the PolicyUpdateRequest.

feelancer21 force-pushed the preserve-inbound-fees branch from e080792 to dc4ec45 Compare May 22, 2024 20:00

saubyk requested review from Roasbeef, yyforyongyu and bitromortac May 23, 2024 02:15

yyforyongyu added the no-changelog label May 23, 2024

yyforyongyu approved these changes May 23, 2024

View reviewed changes

bitromortac approved these changes May 23, 2024

View reviewed changes

Roasbeef merged commit bc6292f into lightningnetwork:master May 23, 2024
31 of 34 checks passed

This was referenced May 23, 2024

inbound fees cryptosharks131/lndg#391

Open

update protos to current lnd master accumulator/charge-lnd#112

Merged

multi: Inbound fees are retained when not provided #8758

multi: Inbound fees are retained when not provided #8758

Conversation

feelancer21 commented May 14, 2024

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

coderabbitai bot commented May 14, 2024 • edited

Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feelancer21 May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saubyk commented May 21, 2024

feelancer21 commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feelancer21 May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saubyk commented May 22, 2024

feelancer21 commented May 22, 2024

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitromortac left a comment

Choose a reason for hiding this comment

coderabbitai bot commented May 14, 2024 •

edited

CodeRabbit Configration File (`.coderabbit.yaml`)

feelancer21 May 20, 2024 •

edited

bitromortac May 17, 2024 •

edited

bitromortac May 22, 2024 •

edited

feelancer21 May 22, 2024 •

edited