Consider opportunity cost of local channel fees in pathfinding #7361
Conversation
dannydeezy
force-pushed
the
dannydeezy/opportunity-cost-of-local-channels-pathfinding
branch
from
a690a02 to
6795bd4
Compare
|
dannydeezy
force-pushed
the
dannydeezy/opportunity-cost-of-local-channels-pathfinding
branch
from
6795bd4 to
5cd8016
Compare
dannydeezy
changed the title
|
There was a problem hiding this comment.
cACK
I'll leave the path finding details up to the path finding wizards 🧙♂️
I think it's not that much of a big change, plus it's configurable thus the ACK
P.S: another nit: make sure your commits follow the guidelines, i.e. <package>: <message describing change>
| // LocalOpportunityCost defines whether to consider the local fee rate | ||
| // of the first hop channel when evaluating routes. While you do not | ||
| // pay this fee since it is your channel, you might want to consider | ||
| // it in order to preserve valuale liquidity. |
There was a problem hiding this comment.
nit: typo "valuale" -> "valuable"
| AprioriWeight: cfg.AprioriWeight, | ||
| MinRouteProbability: cfg.MinRouteProbability, | ||
| LocalOpportunityCost: cfg.LocalOpportunityCost, | ||
| AttemptCost: cfg.AttemptCost, |
There was a problem hiding this comment.
why did formatting break here?
There was a problem hiding this comment.
i'm not sure, the formatting doesn't seem to work on my machine currently
| @@ -660,7 +669,7 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, | |||
| // | |||
| // Source node has no predecessor to pay a fee. Therefore set | |||
| // fee to zero, because it should not be included in the fee | |||
| // limit check and edge weight. | |||
| // limit check and edge weight. | |||
| // LocalOpportunityCost is true, then we account for the | ||
| // fee on this channel. | ||
| var opportunityCostFee lnwire.MilliSatoshi | ||
| if cfg.LocalOpportunityCost && fromVertex == source { |
There was a problem hiding this comment.
instead of adding a new block of code you could merge this logic with line 679 like this
if fromVertex != source && cfg.LocalOpportunityCost {
fee = edge.policy.ComputeFee(amountToSend)
timeLockDelta = edge.policy.TimeLockDelta
}not mandatory but imo it's cleaner and the fee is set only once there
There was a problem hiding this comment.
i was looking at doing something like that, but couple issues:
- i dont think we want to edit
feebecause that is used to calculatetotalFeeon line 700 which might affect the actual amount sent, which is not what we want (because we don't actually pay this newopportunityCostFee) - we don't want to add the timeLockDelta of this edge to our total timelock calculation for a similar reason
| @@ -970,6 +973,94 @@ func runFindLowestFeePath(t *testing.T, useCache bool) { | |||
| } | |||
| } | |||
|
|
|||
| func runFindPathWithOpportunityCost(t *testing.T, useCache bool) { | |||
There was a problem hiding this comment.
nit: add a short godoc describing what your test checks
|
@dannydeezy, remember to re-request review from reviewers when ready |
|
Closing due to inactivity |
2 similar comments
|
Closing due to inactivity |
|
Closing due to inactivity |
See: #7360