Skip to content

Conversation

@thomash-acinq
Copy link
Member

As suggested by @renepickhardt in #2785

Copy link

@renepickhardt renepickhardt left a comment

Choose a reason for hiding this comment

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

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!

@t-bast
Copy link
Member

t-bast commented Nov 20, 2024

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.

@thomash-acinq thomash-acinq marked this pull request as draft September 30, 2025 13:32
@thomash-acinq thomash-acinq force-pushed the split branch 4 times, most recently from c059503 to d2d43a3 Compare October 7, 2025 12:46
@thomash-acinq thomash-acinq marked this pull request as ready for review October 7, 2025 13:05
@thomash-acinq thomash-acinq merged commit 51a144c into master Oct 14, 2025
1 of 2 checks passed
@t-bast t-bast deleted the split branch October 14, 2025 11:30
@t-bast
Copy link
Member

t-bast commented Oct 14, 2025

@pm47 this will require a deployment of the front machines, and a change in the path-finding experiments to test this new splitting strategy.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants