Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

discovery+switch: apply zero forwarding policy updates in-memory as w… #3439

Conversation

valentinewallace
Copy link
Contributor

…ell as on disk

In this commit, we fix a bug where if a user updates a forwarding policy to be
zero, the update will be applied to the policy correctly on-disk, but not
in-memory.

We solve this issue by having the gossiper return the list of on-disk updated
policies and passing these policies to the switch for updates, so the switch can assume
that zero-valued fields are intentional and not just uninitialized.

discovery/gossiper.go Show resolved Hide resolved
discovery/gossiper.go Outdated Show resolved Hide resolved
discovery/gossiper_test.go Outdated Show resolved Hide resolved
htlcswitch/link.go Outdated Show resolved Hide resolved
htlcswitch/link_test.go Show resolved Hide resolved
htlcswitch/switch.go Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
}
// Finally, we'll apply the set of channel policies to the target
// channels' links.
r.server.htlcSwitch.UpdateForwardingPolicies(chanPolicies)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, the gossiper becomes even more a requirement to update a local link policy. In my model of how this should work, it should be the other around. The link primarly determines what the policy is and gossiper just gossips. Maybe local channel parameter validation should happen in the link and when all is checked and updated, we just extract the current settings from the link and gossip it around.

It seems that something isn't right in the separation of concerns. Perhaps the part of gossiper that handles validation and storage for local channels should be extracted to a new object.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to it, but going this route seems like a much bigger change for a bug that's causing routing failures at the moment. This change is pretty small in that it's just exposing the state the gossiper is already acting on. I understand that doing so will just make the separation of concerns worse though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I also realize now that links are not always up, so there must be an independent data store somewhere that stores the local policy.

The only thing I am still thinking about: if we'd just make all policy parameters mandatory for the rpc call (including minhtlc and maxhtlc), would this feedback path of final policies from gossiper still be necessary? If the policy is complete, the links (the ones that are up) and gossiper can be updated independently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, but I think it would be best if none of them were mandatory, meaning a user could only specify the ones they want to be updated. That makes me want to make max htlc non-mandatory and lay groundwork for making the other fields non-mandatory as well.

This approach also has the advantage of not breaking the RPC interface for existing users of UpdateChannelPolicy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline. Still not convinced about the business case for adding this granularity on the rpc level. Because the UpdateChannelPolicy is currently buggy with zero values, I think there is an opportunity to make the fields mandatory now without it being considered as really breaking (it was broken already). But as discussed, leaving further development out of this pr's scope.

discovery/gossiper.go Show resolved Hide resolved
Copy link
Collaborator

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🔍

htlcswitch/switch.go Outdated Show resolved Hide resolved
htlcswitch/switch.go Show resolved Hide resolved
discovery/gossiper.go Outdated Show resolved Hide resolved
discovery/gossiper.go Outdated Show resolved Hide resolved
discovery/gossiper_test.go Outdated Show resolved Hide resolved
htlcswitch/switch.go Show resolved Hide resolved
}
// Finally, we'll apply the set of channel policies to the target
// channels' links.
r.server.htlcSwitch.UpdateForwardingPolicies(chanPolicies)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline. Still not convinced about the business case for adding this granularity on the rpc level. Because the UpdateChannelPolicy is currently buggy with zero values, I think there is an opportunity to make the fields mandatory now without it being considered as really breaking (it was broken already). But as discussed, leaving further development out of this pr's scope.

@valentinewallace valentinewallace force-pushed the fix-zero-fwding-policy-updates branch 2 times, most recently from 3b84b5c to 7a3341b Compare September 7, 2019 00:39
Copy link
Collaborator

@joostjager joostjager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. One outstanding comment about concurrency.

rpcserver.go Show resolved Hide resolved
@valentinewallace valentinewallace force-pushed the fix-zero-fwding-policy-updates branch 2 times, most recently from 9f27767 to 9423045 Compare September 9, 2019 20:29
…ell as on disk

In this commit, we fix a bug where if a user updates a forwarding policy to be
zero, the update will be applied to the policy correctly on-disk, but not
in-memory.

We solve this issue by having the gossiper return the list of on-disk updated
policies and passing these policies to the switch, so the switch can assume
that zero-valued fields are intentional and not just uninitialized.
Copy link
Collaborator

@joostjager joostjager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wpaulino wpaulino merged commit 5d016f8 into lightningnetwork:master Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants