-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
multi: Inbound fees are retained when not provided #8758
multi: Inbound fees are retained when not provided #8758
Conversation
Important Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Left a few comments, and could you test the new behavior in itest/lnd_channel_policy_test.go
?
cmd/lncli/commands.go
Outdated
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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),
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
routing/localchans/manager.go
Outdated
inboundFee := newSchema.InboundFee.ToWire() | ||
if err := edge.ExtraOpaqueData.PackRecords(&inboundFee); err != nil { | ||
return err | ||
if !newSchema.InboundFee.IsNone() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use WhenSome
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
routing/localchans/manager.go
Outdated
@@ -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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think we can just do newSchema.InboundFee.UnwrapOr(models.InboundFee{})
here to avoid the ExtractRecords
above?
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There probably should be an UpdateRecords
method on ExtraOpaqueData
, it first reads the existing records, then updates the specified records.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that'd be even better if we could just add these two fields on edge
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add the fields to the edge. perhaps the reason was to avoid redundant data storage
rpcserver.go
Outdated
} | ||
} | ||
|
||
// In case inbound fees are not specified no inbound fees will be | ||
// applied but the previous value will be retained. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the previous value being retained?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateEdge
has been modified to check the option value, only setting the value if Some
.
cmd/lncli/commands.go
Outdated
case ctx.IsSet("inbound_fee_rate_ppm"): | ||
inboundFee = &lnrpc.InboundFee{ | ||
BaseFeeMsat: int32(inboundBaseFeeMsat), | ||
FeeRatePpm: int32(inboundFeeRatePpm)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close bracket should be on a new line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
itest/lnd_multi-hop-payments_test.go
Outdated
MaxHtlcMsat: maxHtlc, | ||
InboundFee: &lnrpc.InboundFee{ | ||
BaseFeeMsat: inboundBaseFee, | ||
FeeRatePpm: inboundFeeRate}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close bracket should be on a new line.
int32 inbound_fee_rate_ppm = 11; | ||
// Optional inbound fee. If unset, the previously set value will be | ||
// retained [EXPERIMENTAL]. | ||
InboundFee inbound_fee = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good move to make it into a sub-proto, this way we can check for nil
-ness 馃憤
routing/localchans/manager.go
Outdated
// or without specified inbound fees. So having a decoding error | ||
// reveils another problem. | ||
var inboundFee lnwire.Fee | ||
_, err = edge.ExtraOpaqueData.ExtractRecords(&inboundFee) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why's parsing it again require here, whereas it wasn't before this diff?
routing/router.go
Outdated
@@ -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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a distinct change.
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
routing/localchans/manager.go
Outdated
inboundFee := newSchema.InboundFee.ToWire() | ||
if err := edge.ExtraOpaqueData.PackRecords(&inboundFee); err != nil { | ||
return err | ||
if !newSchema.InboundFee.IsNone() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
rpcserver.go
Outdated
} | ||
} | ||
|
||
// In case inbound fees are not specified no inbound fees will be | ||
// applied but the previous value will be retained. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateEdge
has been modified to check the option value, only setting the value if Some
.
Hi @feelancer21 we'd like to get this pr merged into the next rc asap. |
b4bfab5
to
e080792
Compare
Hello guys, have made an update. Here are a few comments:
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.
Regards |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have created an alternative. Preview can be found here: https://github.com/feelancer21/lnd/compare/e080792451184cf0810831230cf8ef8609a4cc51..9b17806
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather do InboundFee *InboundFee
than embed this, tho I think we should do it in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Have integrated the version of @bitromortac.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather do InboundFee *InboundFee
than embed this, tho I think we should do it in another PR.
// 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") != |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: so these two fields must be set together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
chanPoint *lnrpc.ChannelPoint, baseFee int64, | ||
feeRate int64, inboundBaseFee, inboundFeeRate int32, | ||
chanPoint *lnrpc.ChannelPoint, baseFee int64, feeRate int64, | ||
inboundBaseFee, inboundFeeRate int32, updateInboundFee bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
e080792
to
dc4ec45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix 馃檹
chanPoint *lnrpc.ChannelPoint, baseFee int64, | ||
feeRate int64, inboundBaseFee, inboundFeeRate int32, | ||
chanPoint *lnrpc.ChannelPoint, baseFee int64, feeRate int64, | ||
inboundBaseFee, inboundFeeRate int32, updateInboundFee bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this behavior is already tested in req.InboundFee = nil
, plus this is an MPP-related test and should not test policy updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 馃帀, I tested also via cli.
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
Code Style and Documentation
[skip ci]
in the commit message for small changes.馃摑 Please see our Contribution Guidelines for further guidance.