Trampoline: Improve channel selection logic #586
Conversation
Signed-off-by: Peter Neuroth <[email protected]>
It makes more sense to filter directly from the get go instead of skipping channels that have an unsufficient spendable_msat amount and can cause zero value htlcs. Signed-off-by: Peter Neuroth <[email protected]>
The way we allocated channels for a trampoline payment could lead to a case where we could get stuck selecting channels for a payment when the rest amount was lower than the lower bound of the channel. This commit introduces a new selection logic that tries to be greedy first but will split payments more carefully if it is actually needded. Signed-off-by: Peter Neuroth <[email protected]>
| } | ||
|
|
||
| // Option 1: Skip the current channel. | ||
| rec(channels, idx + 1, target_msat, current, best); |
There was a problem hiding this comment.
This starts by skipping the largest channels. I think it would be best to start with the ideal case, and the most probable case: the htlc fits in a single channel (the first channel). Basically the base cases that would work most of the time is 'take the maximum from each channel until the amount fits.
I think the algorithm here should return immediately once 'a' solution has been found. It does not have to be the best solution.
So skipping the current channel should happen last. (unless channels are ordered from smallest to largest).
There was a problem hiding this comment.
I was assuming that it might be helpful to fully drain channels to allow for re-allocation or consolidation of those channels, and since we sort the channels: largest spendable msat first, What should happen here is that we prefer to drain smaller channels fist. Choosing the best case instead of just choosing a case that works was part of the same assumtion I made. I can add a test to verify this behavior or we can change to any other policy you prefer.
There was a problem hiding this comment.
That does sound good actually, to drain the smaller channels first.
|
|
||
| // Try allocations from the upper bound down to the lower bound to | ||
| // maximize channel usage. | ||
| for alloc in (lower..=upper).rev() { |
There was a problem hiding this comment.
This will iterate a lot? min_msat being 1 and amount being 1000 sat (1M msat) will iterate 1M times per channel? So 1M * 1M for 2 channels?
What if (thinking out loud here) you would start with the maximum per channel. Then if that doesn't work, try to leave the minimum minimum (so the lowest min_htlc_out_msat of the remaining channels), if that doesn't work leave the second lowest minimum, etc. So the allocation of the current channel would become min(spendable_msat, target_msat - min_htlc_out_msat of the next channel).
There was a problem hiding this comment.
Yes that will iterate a lot and I like your improvement! What if we would, instead of choosing the lowest min_htlc_out_msat, choose the min_htlc_out_msat of the second smallest channel (the next bigger channel) to try to drain as many channels as possible to make them available for re-allocation?
There was a problem hiding this comment.
That sounds ok. Do add a fallback to the third smallest channel, then the fourth smallest, etc, if it doesn't work.
The way we allocated channels for a trampoline payment could lead to a case where we could get stuck selecting channels for a payment when the rest amount was lower than the lower bound of the channel.
This PR introduces a new selection logic that tries to be greedy first but will split payments more carefully if it is actually needded.
Reviewers: Please check carefully and let me know if you need more tests.