Split MPP by maximizing expected delivered amount#2792
Conversation
renepickhardt
left a comment
There was a problem hiding this comment.
Concept ACK (can't give a proper ACK in github on this repo)
As noted in my comment on the code I checked that finding the max should be equivalent to 1/x - 1/(c_1 - x) - ... - 1/(c_n - x). With my poor scala skills I belive the code computes that formular. (Actually https://www.codeconvert.ai/scala-to-python-converter suggested to me that my understanding of the scala code for private def optimizeExpectedValue seemed correctly in which case I think your code does what it is supposed to do.
That being said: I find it extremely elegant to do the binary search in this way to find the maximum of the expected value curve. Kudos to this! I made this less elegant in my eval framework as my code (with evaluating on a grid of 100 points) works with any probabilistic prior and I wanted to check also non uniform priors.
If I understand correctly you also didn't include your local knowledge / belief about remote channel's liquidity into the max expected value computation but only used the already allocated inflight htlcs. (I think substracting the used capacity is correctly)
ps: I am not sure if you get notifications on closed PR's but I commented with two diagrams that splitting of the liquidity which one has certainty about still makes sense.
All that being said: As mentioned on my original PR I would be very delighted and happy if you can share results from the mainnet A/B test of your node and give me permission to utilize them in the publication / paper that I expect to finalize soon. In particular I am curious if the method works as intendet.
Thank you for this patch!
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Outdated
Show resolved
Hide resolved
|
That PR was unfortunately forgotten as we had a lot on our plate around that time, do you think it makes sense to work on it now and integrate it before the next release @thomash-acinq? I'll spend time reviewing it if you think it's useful to have. |
As suggested by @renepickhardt in #2785
c059503 to
d2d43a3
Compare
eclair-core/src/main/scala/fr/acinq/eclair/remote/EclairInternalsSerializer.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Outdated
Show resolved
Hide resolved
it's still allowed for local channels (in several HTLCs)
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Show resolved
Hide resolved
|
@pm47 this will require a deployment of the front machines, and a change in the path-finding experiments to test this new splitting strategy. |
As suggested by @renepickhardt in #2785