funding: use default forwarding policy if persisted values not found #7613
Conversation
ellemouton
force-pushed
the
fndMgrDefaultPolicyValuesFix
branch
from
ae83658
to
72000db
Compare
ellemouton
force-pushed
the
fndMgrDefaultPolicyValuesFix
branch
from
72000db
to
4726666
Compare
|
Quick Fix 🥳, wondering why the initial fwd-policy was not populated, isn't is set in the Funding Workflow and also persist in the database before 0.16 ? |
There was a problem hiding this comment.
Very quick fix🤙 While this works, my main question is, how can we distinguish the two cases,
- the policy is not found due to the upgrade,
- the policy is not found due to other bugs
In a hindsight, I think #6753 is missing a migration, which is the proper way to fix this.
funding/manager.go
Outdated
| storedFwdingPolicy, err := f.getInitialFwdingPolicy(chanID) | ||
| if err != nil { | ||
| return nil, errors.Errorf("unable to generate channel "+ | ||
| "update announcement: %v", err) | ||
| log.Errorf("No initial forwarding policy found for "+ |
There was a problem hiding this comment.
I think we need to do a more strict check against ErrChannelNotFound to make sure it's a not found error, tho that's the only error that could ever be returned from getInitialFwdingPolicy.
There was a problem hiding this comment.
hmm still have the question re how to distinguish the two cases tho. Is it too late to add a migration?
There was a problem hiding this comment.
ok gotcha - I understand now.
The tricky part is passing the Default Forwarding Policy into the migration... gotta do some weird config passing again... will try 👍
There was a problem hiding this comment.
ok mostly there - just need to touch up a few things and clean it up . Will do it first thing tomorrow. Worth noting that it is quite more involved than this approach but agreed that it is better 👍
There was a problem hiding this comment.
But what exactly would we migrate? Before 0.16 you cannot set custom forwarding policies, so there is no data to migrate. All we have to make sure is that the parent bucket for these keys exist (which is probably already done) and that we fall back to a default policy if we don't have custom forwarding policies stored.
|
ohh you cleared it up for me @yyforyongyu, my vscode was kind of showing that this part was 12 months old commit lol, that's why I thought the fwd-history bucket was also there before 0.16, but it was not. |
In this commit, a bug is fixed in the funding manager that could result in the funding process erroring out if the persisted initial forwarding policy is not found. This could occur if a node restarts after opening a channel that is not yet fully confirmed and also upgrades their node from a pre-0.16 version to 0.16 since the values are only expected to be persisted after 0.16.
ellemouton
force-pushed
the
fndMgrDefaultPolicyValuesFix
branch
from
4726666
to
784dc8d
Compare
|
Thank you for fixing this @ellemouton My fault, I did mention that we needed to take care of this edge case in the PR and then we added the default entry in the branch but I did not noticed that it would return an error and never hit the switch 👎 We also need to also catch the same error when deleting the |
In lightningnetwork#6753 we added support to custom base and fee rate fees when opening a channel. Since v0.16.0 all channels store their initial fwdPolicy even if they are using the default values. However, there is an edge case where a channel is open before migrating to >v0.16.0 but updating to +v0.16.0 before that channel gets confirmed. For those cases we won't be able to retrieve/delete the initial fwdPolicy. In that case we need to use the default values but we also need to ensure that we do not fail when trying to read/delete the initial fwdPolicy. lightningnetwork#7613 fixed the problem for reading, here we fix it for deleting.
In this commit, a bug is fixed in the funding manager that could result in the funding process erroring out if the persisted initial forwarding policy is not found. This could occur if a node restarts after opening a channel that is not yet fully confirmed and also upgrades their node from a pre-0.16 version to 0.16 since the values are only expected to be persisted after 0.16 (which includes the changes made in #6753).
This addresses the issue reported in this comment