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

routing: routing may come up with suboptimal routes (weight function) #1358

Merged
merged 1 commit into from Jun 29, 2018

Conversation

Projects
None yet
3 participants
@joostjager
Collaborator

joostjager commented Jun 10, 2018

Fixes #1346

This PR:

  • implements a new weight function
  • contains a unit test that failed with the previous weight function and succeeds after this change
  • alternative way to build a channel test graph in code

The reason for creating an alternative method to build the test graph is that the current json file contains info that is not relevant for the unit test (pubkeys, channel id, channel point, flags). This makes it harder to maintain and also more difficult to understand the structure.

In this PR these non-relevant properties are (deterministically) generated at run time.

Then there was still the choice whether to stick with json or move the graph definition to go code. In my opinion, expressing the graph in go code can make it easier to understand and shorter because several useful constructors can be defined (for example: symmetricTestChannel).

The shorter the graph definition is, the less effort it is to bring up a dedicated graph to test a specific scenario. I can imagine that the number of these scenarios will grow significantly. Not only because the routing algorithm is not that extensively unit tested currently (unit test added in #1321 was missing for example), but also because future extensions like AMP will need additional unit tests.

With regards to a single test graph for all tests vs special graphs for special tests, I think a mixture will emerge. Some scenarios are just quite difficult to fit (together) into a single graph. There is the option to modify the graph runtime, but I think this is makes it more difficult to understand what is happening.

@joostjager

This comment has been minimized.

Collaborator

joostjager commented Jun 10, 2018

Please let me know what you think of this new style of describing the graph. If you agree with my view, I can also convert basic_graph.json to an in-code definition which I am sure will be much shorter.

@Roasbeef

This comment has been minimized.

Member

Roasbeef commented Jun 28, 2018

The set of json was added initially as when things were a bit more contained on testnet, we could use the output of describegraph directly within the set of unit tests. They're also nice form the PoV of cross-implementation compatibility tests, as they're language agnostic. I agree that both can co-exist, and also that most newer tests should likely favor building up the graph incrementally within table driven tests.

@Roasbeef

Thanks for this PR! The prior weighting algorithm was thrown in just to start to take the fees into account, but had a major flaw as you've shown here (was originally meant to be temporary, but it was never re-visited).

IMO, the new weight function drastically improves the decisions that path finding algo will make when sending routes. We may want to tweak the risk factor later in the future, but I think the current value is a good starting point (although the code could add a bit more detail in the comments w.r.t to how you arrived at the current value).

@@ -26,6 +26,12 @@ const (
// infinity is used as a starting distance in our shortest path search.
infinity = math.MaxInt64
// RiskFactorBillionths controls the influence of time lock delta

This comment has been minimized.

@Roasbeef

Roasbeef Jun 28, 2018

Member

What was the process used to arrive at this value?

This comment has been minimized.

@joostjager

joostjager Jun 28, 2018

Collaborator

Comment added

// The final component is then 1 plus the timelock delta.
timeWeight := int64(1 + e.TimeLockDelta)
// timeWeight is the penalty for the time lock delta of this channel.

This comment has been minimized.

@Roasbeef

Roasbeef Jun 28, 2018

Member

timeWeight -> timeLockPenalty

@@ -2,6 +2,7 @@ package routing
import (
"bytes"
"crypto/sha256"

This comment has been minimized.

@Roasbeef

Roasbeef Jun 28, 2018

Member

Commit message nit on this: the title itself could use a bit more detail. Simply from "unit test added" one can't gain any knowledge w.r.t exactly what's being tested in the additional set of unit tests.

This comment has been minimized.

@joostjager

joostjager Jun 28, 2018

Collaborator

Squashed unit test commit. I prefer implementation and covering test to be part of the same commit. But otherwise valid point about commit message.

Capacity btcutil.Amount
}
// parseTestGraph returns a fully populated ChannelGraph given a path to a JSON

This comment has been minimized.

@Roasbeef

Roasbeef Jun 28, 2018

Member

The godoc comment doesn't match the name of the function.

aliasMap := make(map[string]*btcec.PublicKey)
nodeIndex := byte(0)
addAlias := func(alias string) (*channeldb.LightningNode, error) {

This comment has been minimized.

@Roasbeef

Roasbeef Jun 28, 2018

Member

This function does much more than add an alias. Would recommend calling it something like: addNodeWithAlias.

}
channelID++

This comment has been minimized.

@Roasbeef

Roasbeef Jun 28, 2018

Member

Extra new line here.

func TestFindLowestFeePath(t *testing.T) {
t.Parallel()
var testChannels = []*testChannel{

This comment has been minimized.

@Roasbeef

Roasbeef Jun 28, 2018

Member

Style nit: no need to add a var here, can simply assign with :=.

This comment has been minimized.

@Roasbeef

Roasbeef Jun 28, 2018

Member

Could also use a comment here explaining the rationale behind the set up: that roasbeef should favor sending through b instead of a.

return graph, cleanUp, aliasMap, nil
}
func TestFindLowestFeePath(t *testing.T) {

This comment has been minimized.

@Roasbeef

Roasbeef Jun 28, 2018

Member

Missing godoc string on the test itself.

if err != nil {
t.Fatalf("unable to find path: %v", err)
}
route, err := newRoute(paymentAmt, sourceVertex, path, startingHeight,

This comment has been minimized.

@Roasbeef

Roasbeef Jun 28, 2018

Member

Style nit: should be folded similar to the findPath method above.

@joostjager joostjager force-pushed the joostjager:newfee branch from 4326a48 to 0880ffb Jun 28, 2018

routing: new weight function
In this commit, a new weight function is introduced. This will create a
meaningful effect of time lock on route selection. Also, removes the
squaring of the fee term. This led to suboptimal routes.

Unit test added that covers the weight function and asserts that the
lowest fee route is indeed returned.

@joostjager joostjager force-pushed the joostjager:newfee branch from 0880ffb to b527967 Jun 28, 2018

@joostjager

This comment has been minimized.

Collaborator

joostjager commented Jun 28, 2018

@Roasbeef comments addressed and rebased.

@Roasbeef

LGTM 🎉

@Roasbeef Roasbeef merged commit c045def into lightningnetwork:master Jun 29, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.04%) to 54.682%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joostjager joostjager deleted the joostjager:newfee branch Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment