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

Fix and improve path finding #719

Merged
merged 6 commits into from Oct 18, 2018

Conversation

Projects
None yet
2 participants
@bartekn
Member

bartekn commented Oct 17, 2018

This PR fixes several bugs and adds improvements making path finding results better, returning them in a shorter time.

The most severe bug was found in pathNode.Cost method. pathNode is a linked list node with Tail field pointing to the next asset on the path. So the front of the list is the currently extended source asset and the tail is always the destination asset. The previous code was wrongly consuming offers in the wrong order. Fixing this bug required to walk through the current path from the tail (the destination asset) to front (the current source).

I added a new test explaining why the old code was wrong. Consider the path: AAA -> BBB -> CCC. The core of the problem is that the previous code was using DestinationAmount as a starting value for AAA -> BBB trade, in other words: "give me amount of AAA needed to buy DestinationAmount of BBB". It doesn't make any sense. What you should do first is: "give me amount of BBB needed to buy DestinationAmount of CCC". So for the destination amount equal 10 and the following offers (scc notation):

offer :trader, {for:["AAA", :gateway], sell:["BBB", :gateway]}, 1, 11
offer :trader, {for:["BBB", :gateway], sell:["CCC", :gateway]}, 10, 0.1

path finding trade order 1

The old algorithm would not find a path as it is not possible to buy 1 BBB with 10 AAA and 1 BBB is needed to buy 10 CCC.

Fixing this introduced a huge optimization opportunity. Basically the cost of trades between current source asset and the destination asset does not change after prepending the path with a new asset. So we can cache the results instead of checking every order book on the path every time we add a new asset in the front of the path.

Another improvement was to remove A -> B -> A subpaths from the paths as they will always be a bad deal. This also removed all destination asset -> A -> destination asset paths.

Finally, added MaxPathLength that is configurable via CLI switch or environment variable and defines the maximum length of the path that /paths will return, with a default value of 4 (source asset -> A -> B -> destination asset). The default value of 4 comes as a result of observation of paths found between many different assets: most of them have the maximum length of 4 and if the path length was higher it was a bad deal. The MaxPathLength allowed another optimization in search.extendSearch method. Basically if p.Depth == s.MaxLength-1 and connected asset is not a target we don't extend such path as it will not be possible to extend the path any longer.

After implementing all the optimizations average search time for my queries dropped from 10 seconds to 2 seconds giving a better results.

Finally, a single search is run in REPEATABLE READ, READ ONLY DB transaction to have a stable view of the offers table.

@bartekn bartekn added the horizon label Oct 17, 2018

@bartekn bartekn referenced this pull request Oct 17, 2018

Open

Pathfinding bug #324

@bartekn bartekn requested a review from nikhilsaraf Oct 17, 2018

@bartekn bartekn added this to the Horizon v0.15.0 milestone Oct 17, 2018

@bartekn bartekn changed the base branch from master to horizon-0.15.0 Oct 17, 2018

@@ -237,7 +237,7 @@ func (base *Base) GetAccountID(name string) (result xdr.AccountId) {
// conventions
func (base *Base) GetAmount(name string) (result xdr.Int64) {
var err error
result, err = amount.Parse(base.GetString("destination_amount"))

This comment has been minimized.

@nikhilsaraf

nikhilsaraf Oct 18, 2018

Member

why did this change -- was the name param never used correctly earlier?

This comment has been minimized.

@bartekn

bartekn Oct 18, 2018

Member

Yes, this was working correctly only when name="destination_amount".

@nikhilsaraf

LGTM! thanks for running through this on a call. nice catch!

bartekn added some commits Oct 17, 2018

@bartekn bartekn force-pushed the fix-path-finding branch from cb317fb to f20bbfc Oct 18, 2018

@bartekn

This comment has been minimized.

Member

bartekn commented Oct 18, 2018

Thanks @nikhilsaraf! I will merge it now and it will be autodeployed to https://horizon-dev-pubnet.stellar.org/ so more people can easily test it. In the end we can revert this PR if there's a need but I believe it should work fine and give better results.

@bartekn bartekn merged commit f165c3e into horizon-0.15.0 Oct 18, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@bartekn bartekn deleted the fix-path-finding branch Oct 18, 2018

bartekn added a commit that referenced this pull request Oct 19, 2018

bartekn added a commit that referenced this pull request Oct 25, 2018

Fix and improve path finding (#719)
This commit fixes several bugs and adds improvements making path finding
results better, returning them in a shorter time.

The most severe bug was found in `pathNode.Cost` method. `pathNode` is a
linked list node with `Tail` field pointing to the next asset on the
path. So the front of the list is the currently extended source asset
and the tail is always the destination asset. The previous code was
wrongly consuming offers in the wrong order. Fixing this bug required to
walk through the current path from the tail (the destination asset) to
front (the current source).

I added a new test explaining why the old code was wrong. Consider the
path: `AAA -> BBB -> CCC`. The core of the problem is that the previous
code was using `DestinationAmount` as a starting value for `AAA -> BBB`
trade, in other words: "give me amount of `AAA` needed to buy
`DestinationAmount` of `BBB`". It doesn't make any sense. What you
should do first is: "give me amount of `BBB` needed to buy
`DestinationAmount` of `CCC`". So for the destination amount equal `10`
and the following offers (`scc` notation):
```
offer :trader, {for:["AAA", :gateway], sell:["BBB", :gateway]}, 1, 11
offer :trader, {for:["BBB", :gateway], sell:["CCC", :gateway]}, 10, 0.1
```
The old algorithm would not find a path as it is not possible to buy
`1 BBB` with `10 AAA` and `1 BBB` is needed to buy `10 CCC`.

Fixing this introduced a huge optimization opportunity. Basically the
cost of trades between current source asset and the destination asset
does not change after prepending the path with a new asset. So we can
cache the results instead of checking every order book on the path every
time we add a new asset in the front of the path.

Another improvement was to remove `A -> B -> A` subpaths from the paths
as they will always be a bad deal. This also removed all `destination
asset -> A -> destination asset` paths.

Finally, added `MaxPathLength` that is configurable via CLI switch or
environment variable and defines the maximum length of the path that
`/paths` will return, with a default value of `4` (`source asset -> A ->
B -> destination asset`). The default value of 4 comes as a result of
observation of paths found between many different assets: most of them
have the maximum length of 4 and if the path length was higher it was a
bad deal. The `MaxPathLength` allowed another optimization in
`search.extendSearch` method. Basically if `p.Depth == s.MaxLength-1`
and connected asset is not a target we don't extend such path as it will
not be possible to extend the path any longer.

After implementing all the optimizations average search time for my
queries dropped from 10 seconds to 2 seconds giving a better results.

Finally, a single search is run in `REPEATABLE READ, READ ONLY` DB
transaction to have a stable view of the offers table.

bartekn added a commit that referenced this pull request Oct 25, 2018

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