Update channelLink policy correctly when opening channel with custom fee parameters#7597
Conversation
guggero
added
funding
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
LGTM 🥇. Perfect work, also checked that the itest catches the failure.
itest/lnd_open_channel_test.go
Outdated
There was a problem hiding this comment.
Is this needed because Carol is shut down after each test?
There was a problem hiding this comment.
This is a consequence of how the new itest framework works. When you have a long-running node and are using ht.Subtest(), you need to restart those nodes in order for their context to be refreshed. I added a comment.
funding/manager.go
Outdated
There was a problem hiding this comment.
can we not change the link creation so that it takes these non-static params too? Just so that we never have to worry about the link being started with the incorrect forwarding policy
positiveblue
left a comment
positiveblue
left a comment
There was a problem hiding this comment.
I agree with @ellemouton that it would be better if we used the forward policy with the right values when we add the link instead of creating it using the default values and then make a call to UpdateForwardingPolicies to trigger a "fake" update.
Unfortunately, we do not have direct access to the channel fees in channeldb.OpenChannel. Ofc we can always add those fields to the struct or use a custom struct that includes them in the flow initiated by the funding manager but both options require quite a lot of rework to accommodate the changes.
This change look like a quick fix that make all subsystem be in sync so I am happy with it. Thank you for finding a way to make it work @guggero 💯
It may be worth adding a TODO here so we remember that this values will be overwritten by another subsystem as soon as the switch is ready. We can always think about how to use it in the right values since the beginning in the future.
With this commit we give the funding manager the ability to inform the switch about custom channel policies, right after we've announced the channel to the network. This change is necessary because before lightningnetwork#6753 a channel could only be opened with the default forwarding policies, so the switch automatically had the "correct" default values. Since lightningnetwork#6753 added the ability to specify forwarding policies at channel open time, we announced those policies to the network but never updated the switch to inform it about the changed policies (previously changing the policies was only possible through the UpdateChannelPolicy RPC which did call the switch).
This recently added file didn't follow the golang naming convention for tests.
This commit enhances the custom fee policy channel open test by adding a second channel and testing forwarding payments through the channel with the custom forwarding policies. This was able to reproduce the bug reported in lightningnetwork#5796 which was fixed in a previous commit of this PR.
guggero
force-pushed
the
persistent-fee-params
branch
from
79b0a69 to
0baa953
Compare
The right way to solve the problem of the link not being up to date with custom user set forwarding policies once the channel is announced would be to pass in those custom values when the link is created initially. This requires a bit more of a refactor and is not addressed in this bug fix.
guggero
force-pushed
the
persistent-fee-params
branch
from
0baa953 to
d54bc89
Compare
|
@ellemouton & @positiveblue thank you for your feedback and review! I agree that it would be nicer to set the initial fee policy correctly. BUT, it looks like there is some interplay with zero conf channels, where the actual policy we send out is only determined after the link was already created. So I opted to go with the TODO instead. |
ellemouton
left a comment
ellemouton
left a comment
There was a problem hiding this comment.
Sounds good, Looks good 🥇
5 participants
Fixes #7596.
With this PR we give the funding manager the ability to inform the
switch about custom channel policies, right after we've announced the
channel to the network.
This change is necessary because before #6753 a channel could only be
opened with the default forwarding policies, so the switch automatically
had the "correct" default values. Since #6753 added the ability to
specify forwarding policies at channel open time, we announced those
policies to the network but never updated the switch to inform it about
the changed policies (previously changing the policies was only possible
through the
UpdateChannelPolicyRPC which did call the switch).