Estimate balances of remote edges#2263
Conversation
|
A general observation, I think we should better separate two things:
Those are already two separate objects ( |
thomash-acinq
force-pushed
the
balance-estimate-only
branch
from
95d1aa4 to
283c64d
Compare
thomash-acinq
force-pushed
the
balance-estimate-only
branch
from
283c64d to
d107c5b
Compare
Now there is a third one: @t-bast This only records some metrics but the estimates are not use anywhere. |
- Add a bit more documentation - Small code cleanup / refactor
t-bast
left a comment
t-bast
left a comment
There was a problem hiding this comment.
This isn't an easy PR to review! I've spent some time playing around with the code, and did a few refactoring and clean-up in #2266 without changing any business logic.
I have one main fundamental comment about how we handle multiple channels, and a few questions on some of the formulas, but otherwise it looks mostly good to me, this is good work!
| sender.send(paymentFSM, addCompleted(HtlcResult.RemoteFail(UpdateFailHtlc(ByteVector32.Zeroes, 0, Sphinx.FailurePacket.create(sharedSecrets1.head._1, failure))))) | ||
|
|
||
| // payment lifecycle will ask the router to temporarily exclude this channel from its route calculations | ||
| routerForwarder.expectMsgType[ChannelCouldNotRelay] |
There was a problem hiding this comment.
We need more tests to verify that the payment lifecycle correctly sends all balance-related events to the router.
| } | ||
| } | ||
|
|
||
| def addChannel(capacity: Satoshi): BalanceEstimate = copy(high = high + toMilliSatoshi(capacity), totalCapacity = totalCapacity + capacity) |
There was a problem hiding this comment.
I'm still making the same comment I initially made on the first iteration of this feature: I think it really doesn't make sense to add channel capacities, I believe we should take the max of the channel capacities instead (which means we need to keep a list of channel capacities to correctly handle removing channels).
If nodes have more than a few channels between them, the high bound will be completely unreasonable and the decay will tend to restore it way too quickly to an impossible amount (since the balance estimates will be used in the context of a single HTLC, not for the whole flow of a payment).
Also, you're not even using it consistently: BalanceEstimates.addChannel doesn't add the capacities whereas other functions do...which shows we probably need more unit tests 😉
What about having the balance estimate contain a capacities: Seq[MilliSatoshi]? This also lets us know how many channels exist between these peers, which may help us choose to use this pair of nodes for more than one HTLC when using MPP.
thomash-acinq
force-pushed
the
balance-estimate-only
branch
from
1396f0a to
efad592
Compare
|
replaced by #2272 |
3 participants
Same as #2071 but do not use the balance estimates for anything.