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

[autopilot] distribute available funds among channels #2633

Merged

Conversation

Projects
None yet
3 participants
@halseth
Copy link
Collaborator

halseth commented Feb 12, 2019

This commit fixes a regression in how we allocate funds to attempted
channels. We would earlier stay within the channel size limits, but we
wouldn't account for funds consumed by other channels being opened in
parallel.

We fix this by introducing a loop which greadily tries to distribute the
funds among the channels to open, and reduces the number of channels to
open in case not enough funds are available to satisfy the channel size
limits.

Fixes #2609

@halseth halseth added this to the 0.6 milestone Feb 12, 2019

@@ -799,6 +817,9 @@ func (a *Agent) executeDirective(directive AttachmentDirective) {

// Since the channel open was successful and is currently pending,
// we'll trigger the autopilot agent to query for more peers.
// TODO(halseth): this triggers a new loop before all the new channels

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Feb 13, 2019

Member

Does it? The pending opens are added above.

This comment has been minimized.

Copy link
@halseth

halseth Feb 20, 2019

Author Collaborator

Yes, but this happens in a goroutine, and they all get spawned in parallel, so we have no guarantee that they are all added to the map before the first one finishes.

Show resolved Hide resolved autopilot/agent.go Outdated

@halseth halseth force-pushed the halseth:autpilot-chansize-allocation branch from cd64c95 to 322bee0 Feb 19, 2019

@halseth

This comment has been minimized.

Copy link
Collaborator Author

halseth commented Feb 19, 2019

PTAL @Roasbeef

t.Fatalf("heuristic wasn't queried in time")
}

// Since the maximum channel size is 1 BTC, the agent won't have enough

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Feb 22, 2019

Member

I find these tests hard to follow at times in general. I think we can reduce some vertical space here with usage of some a helper functions to handle sending/receiving messages to/from the various mocked interfaces. I'd expect the logic to go "ok I have 1.5 BTC and the max size is 1 BTC, so I'll open to channels here for 1 and 0.5 BTC". Instead, as is, it'll instead execute the 1 BTC and the 0.5 BTC during distinct iterations.

This is a regression compared to the behavior of the prior system, that would attempt to pipeline open as many channels as possible.

This comment has been minimized.

Copy link
@halseth

halseth Feb 22, 2019

Author Collaborator

The rationale behind doing it this way was that you don't know if an opening attempt will always succeed, so I would wait until the next iteration to have more information about which ones succeed and potentially open a bigger channel.

Looking at it now though, I see that it won't matter much, since we will eventually end up with the smaller channel anyway, so whether it is being opened last or not makes no difference. Will fix!

Also agree on the verbosity of the tests, will clean they up a bit.

@Roasbeef

This comment has been minimized.

Copy link
Member

Roasbeef commented Feb 25, 2019

Pong.

@halseth halseth force-pushed the halseth:autpilot-chansize-allocation branch 3 times, most recently from 8a5498b to 40fbe57 Mar 1, 2019

@halseth

This comment has been minimized.

Copy link
Collaborator Author

halseth commented Mar 5, 2019

Comments addressed. PTAL @Roasbeef

Show resolved Hide resolved autopilot/agent.go
Show resolved Hide resolved autopilot/agent_test.go
Show resolved Hide resolved autopilot/agent_test.go Outdated
Show resolved Hide resolved autopilot/agent_test.go
Show resolved Hide resolved autopilot/agent_test.go Outdated
Show resolved Hide resolved autopilot/agent_test.go Outdated

@halseth halseth force-pushed the halseth:autpilot-chansize-allocation branch from 38cb99f to 30bf23d Mar 13, 2019

@halseth

This comment has been minimized.

Copy link
Collaborator Author

halseth commented Mar 13, 2019

Comments addressed and rebase PTAL @wpaulino @Roasbeef

@Roasbeef
Copy link
Member

Roasbeef left a comment

LGTM 💎

I dig the overhaul of the unit tests in the package, should prove useful in the future once we start to explore more complex scenarios.

Ready to land after rebase!

Show resolved Hide resolved autopilot/agent_test.go

halseth added some commits Feb 12, 2019

autopilot/agent: distribute available funds among channels
This commit fixes a regression in how we allocate funds to attempted
channels. We would earlier stay within the channel size limits, but we
wouldn't account for funds consumed by other channels being opened in
parallel.

We fix this by introducing a loop which greadily tries to distribute the
funds among the channels to open, and reduces the number of channels to
open in case not enough funds are available to satisfy the channel size
limits.
autopilot/agent_test: define testCtx, setup, other helpers
This commit defines a set of helper methods that are used by many of the existing tests.
autopilot/agent_test: add TestAgentChannelSizeAllocation
TestAgentChannelSizeAllocation tests that the autopilot agent opens
channel of size that stays within the channel budget and size
restrictions.

@halseth halseth force-pushed the halseth:autpilot-chansize-allocation branch from 30bf23d to fb5b6ff Mar 14, 2019

@halseth

This comment has been minimized.

Copy link
Collaborator Author

halseth commented Mar 14, 2019

Rebased.

@Roasbeef Roasbeef merged commit 36cc1da into lightningnetwork:master Mar 14, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
coverage/coveralls Coverage decreased (-0.02%) to 60.213%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.