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

multi: enable max htlc update #3523

Merged
merged 7 commits into from Sep 24, 2019

Conversation

@joostjager
Copy link
Collaborator

commented Sep 19, 2019

In this PR, we enable users to update the max HTLC for a given channel or channels.

Rewrite of #2438

@joostjager joostjager force-pushed the joostjager:enable-update-max-htlc branch 4 times, most recently from f29e2e1 to e1d86d0 Sep 19, 2019
@joostjager joostjager removed request for Roasbeef and cfromknecht Sep 19, 2019
@joostjager joostjager added this to the 0.8.0 milestone Sep 19, 2019
@joostjager joostjager force-pushed the joostjager:enable-update-max-htlc branch 2 times, most recently from 8b0c83c to 494bd2b Sep 19, 2019
if !edge.MessageFlags.HasMaxHtlc() {
return fmt.Errorf("channel %v has no max htlc",
info.ChannelPoint)
}

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 19, 2019

Author Collaborator

Not sure about this sanity check. I actually don't like sanity checks.

This comment has been minimized.

Copy link
@halseth

halseth Sep 19, 2019

Collaborator

This assumes an order that the gossiper always runs before this is called, so I think it is safest to keep the check?

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 19, 2019

Author Collaborator

Removed those changes from the pr. It is more complicated, because the gossiper migrates the updates in the background.

@joostjager joostjager force-pushed the joostjager:enable-update-max-htlc branch from 494bd2b to 0d5a2c5 Sep 19, 2019
Copy link
Collaborator

left a comment

First impression: good change :)

discovery/gossiper.go Outdated Show resolved Hide resolved
if !edge.MessageFlags.HasMaxHtlc() {
return fmt.Errorf("channel %v has no max htlc",
info.ChannelPoint)
}

This comment has been minimized.

Copy link
@halseth

halseth Sep 19, 2019

Collaborator

This assumes an order that the gossiper always runs before this is called, so I think it is safest to keep the check?

@@ -1211,6 +1218,14 @@ func (d *AuthenticatedGossiper) retransmitStaleAnns(now time.Time) error {
// introduction of the MaxHTLC field, then we'll update this
// edge to propagate this information in the network.
if !edge.MessageFlags.HasMaxHtlc() {
// We'll make sure we support the new max_htlc field if

This comment has been minimized.

Copy link
@halseth

halseth Sep 19, 2019

Collaborator

maybe we should move this check/update to the local manager instead? Making gossiper as unaware of local changes as possible,

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 19, 2019

Author Collaborator

I wanted to keep the change set small. Moving retransmission of stale channels to the local chan mgr is a much larger change. Also not sure if that is the place where it belongs.

This comment has been minimized.

Copy link
@halseth

halseth Sep 23, 2019

Collaborator

I think that's the direction we should move in, but agree that's a much larger change that shouldn't be done in this PR.

@joostjager joostjager force-pushed the joostjager:enable-update-max-htlc branch 3 times, most recently from 6858af0 to 3be4344 Sep 19, 2019
discovery/gossiper.go Outdated Show resolved Hide resolved
discovery/gossiper.go Outdated Show resolved Hide resolved
routing/localchans/manager.go Show resolved Hide resolved
routing/localchans/manager.go Outdated Show resolved Hide resolved
routing/localchans/manager.go Show resolved Hide resolved
discovery/gossiper_test.go Outdated Show resolved Hide resolved
routing/localchans/manager.go Outdated Show resolved Hide resolved
routing/localchans/manager.go Outdated Show resolved Hide resolved
routing/localchans/manager.go Outdated Show resolved Hide resolved
@joostjager joostjager force-pushed the joostjager:enable-update-max-htlc branch 3 times, most recently from 35b3338 to 6102618 Sep 20, 2019
@joostjager joostjager force-pushed the joostjager:enable-update-max-htlc branch 4 times, most recently from 15e88aa to 1e68d76 Sep 20, 2019
Copy link
Collaborator

left a comment

LGTM

@@ -1211,6 +1218,14 @@ func (d *AuthenticatedGossiper) retransmitStaleAnns(now time.Time) error {
// introduction of the MaxHTLC field, then we'll update this
// edge to propagate this information in the network.
if !edge.MessageFlags.HasMaxHtlc() {
// We'll make sure we support the new max_htlc field if

This comment has been minimized.

Copy link
@halseth

halseth Sep 23, 2019

Collaborator

I think that's the direction we should move in, but agree that's a much larger change that shouldn't be done in this PR.

discovery/gossiper.go Outdated Show resolved Hide resolved
}

// Apply the new policy to the edge.
err := r.updateEdge(info.ChannelPoint, edge, newSchema)

This comment has been minimized.

Copy link
@halseth

halseth Sep 23, 2019

Collaborator

This is a bit strange, since the updated edge policy is invalid after this call (it hasn't been re-signed). A more natural split would be for the localChanManager to fully update and re-sign the policies, and then for the gossiper to just forward them around as any other network update. but maybe that's a change too big for this PR.

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 23, 2019

Author Collaborator

Summary from offline discussion:

  • Ideally gossiper would just gossip and not sign.
  • In an lnd mode of operation where there is no gossip (all is private), there wouldn't be a need for signatures on local updates. Signing in localchans.Manager wouldn't be necessary either.
  • An interconnecting component that takes policies from localchans.Manager and resigns them when they change or become stale could be a solution.

To keep the scope of this pr under control, I only added a call to clear the signature in localchans.Manager.

joostjager and others added 7 commits Sep 23, 2019
The signature is retrieved, not used and overwritten with a
new signature.
As a preparation for making the gossiper less responsible for validating
and supplementing local channel policy updates, this commits moves the
on-the-fly max htlc migration up the call tree. The plan for a follow up
commit is to move it out of the gossiper completely for local channel
updates, so that we don't need to return a list of final applied policies
anymore.
The policy update logic that resided part in the gossiper and
part in the rpc server is extracted into its own object.

This prepares for additional validation logic to be added for policy
updates that would otherwise make the gossiper heavier.

It is also a small first step towards separation of our own channel data
from the rest of the graph.
In this commit, we update the router and link to support users
updating the max HTLC policy for their channels. By updating these internal
systems before updating the RPC server and lncli, we protect users from
being shown an option that doesn't actually work.
In this commit, we enable callers of UpdateChannelPolicy to
specify their desired max HTLC forwarding policy for one or
multiple channels.
In this commit, we enable callers of UpdateChannelPolicy to
specify their desired max HTLC forwarding policy for one or
multiple channels over lncli.
@joostjager joostjager force-pushed the joostjager:enable-update-max-htlc branch from 1e68d76 to 5aefe8b Sep 23, 2019
@joostjager joostjager requested a review from Roasbeef Sep 23, 2019
@wpaulino wpaulino requested review from Roasbeef and removed request for Roasbeef and wpaulino Sep 23, 2019
Copy link
Member

left a comment

LGTM 🚁

@Roasbeef Roasbeef merged commit 9da8951 into lightningnetwork:master Sep 24, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.08%) to 63.005%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.