Skip to content

Conversation

thomash-acinq
Copy link
Member

@thomash-acinq thomash-acinq commented Sep 8, 2021

Add a new way of scoring paths that estimates the success probabilities of paths and put a cost on failed payment attempts.

For now the success probabilities are estimated in a very simple way based on capacity: I assume that the balance is a random variable uniformly distributed between 0 and capacity and multiply probabilities for all the channels on the path.
The formula for estimating the probability could be changed easily, that's not the focus of this PR. The goal of this PR is to introduce a new scoring of paths that uses this probability and can be evaluated using the new AB testing framework from #1930.

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2021

Codecov Report

Merging #1942 (7e0b4e2) into master (8b29edb) will decrease coverage by 0.03%.
The diff coverage is 95.74%.

@@            Coverage Diff             @@
##           master    #1942      +/-   ##
==========================================
- Coverage   87.68%   87.65%   -0.04%     
==========================================
  Files         158      158              
  Lines       12389    12406      +17     
  Branches      533      515      -18     
==========================================
+ Hits        10863    10874      +11     
- Misses       1526     1532       +6     
Impacted Files Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 50.95% <0.00%> (ø)
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 93.92% <100.00%> (+0.14%) ⬆️
...cinq/eclair/remote/EclairInternalsSerializer.scala 97.87% <100.00%> (+0.04%) ⬆️
.../src/main/scala/fr/acinq/eclair/router/Graph.scala 98.29% <100.00%> (+0.10%) ⬆️
...cala/fr/acinq/eclair/router/RouteCalculation.scala 99.27% <100.00%> (ø)
...src/main/scala/fr/acinq/eclair/router/Router.scala 93.87% <100.00%> (ø)
...clair/blockchain/bitcoind/rpc/BatchingClient.scala 86.36% <0.00%> (-13.64%) ⬇️
...main/scala/fr/acinq/eclair/router/Validation.scala 90.76% <0.00%> (-1.54%) ⬇️
...ala/fr/acinq/eclair/balance/ChannelsListener.scala 96.55% <0.00%> (+3.44%) ⬆️

@t-bast
Copy link
Member

t-bast commented Sep 8, 2021

When you're ready for feedback, can you update the description of this PR with details about how you plan on estimating success probabilities? I think it's worth reviewing the concept before reviewing actual code, to ensure you don't implement something that will be a concept NACK.

@thomash-acinq
Copy link
Member Author

This PR is not ready yet because it depends on #1930 which is still being reviewed.

I estimate probabilities in a very simple way based on capacity: I assume that the balance is a random variable uniformly distributed between 0 and capacity and multiply probabilities for all the channels on the path.
But how I compute the probability doesn't really matter, we can change it easily. The goal of this PR is to introduce a new scoring of paths that uses this probability.

@t-bast
Copy link
Member

t-bast commented Sep 8, 2021

Ok thanks, please update the description with that info, it's important to know!

People will be looking at our PRs but they will mostly read the description, not the code, and we don't want them to get the wrong idea on what eclair actually does inside its path-finding.

Because based on the title of the PR alone, you could have started to store historic information about failed payments, and that would have been a concept NACK for me.

@thomash-acinq thomash-acinq force-pushed the abcd branch 2 times, most recently from f65c332 to 3d62da5 Compare September 9, 2021 12:02
@pm47 pm47 mentioned this pull request Sep 9, 2021
@thomash-acinq thomash-acinq marked this pull request as ready for review September 9, 2021 12:20
@thomash-acinq thomash-acinq requested a review from t-bast September 9, 2021 12:20
@thomash-acinq thomash-acinq force-pushed the abcd branch 4 times, most recently from 4397ae5 to ebb0427 Compare September 9, 2021 15:39
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

First pass on the general design / future extensibility.
Let me know if it's too vague, and we can discuss it to refine the proposal.

Copy link
Member Author

@thomash-acinq thomash-acinq left a comment

Choose a reason for hiding this comment

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

I've also fixed to the hopCost computation. I was only counting the last hop.

@thomash-acinq thomash-acinq force-pushed the abcd branch 2 times, most recently from ae55672 to bf6993f Compare September 20, 2021 12:19
@thomash-acinq thomash-acinq merged commit 273fae9 into master Sep 20, 2021
@thomash-acinq thomash-acinq deleted the abcd branch September 20, 2021 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants