path finding: Disable bit decoupling#2115
Conversation
cfromknecht
left a comment
cfromknecht
left a comment
There was a problem hiding this comment.
LGTM! you know it's good when the only comments are regarding commit msgs :P
| // channel bandwidth than what is found in the graph. | ||
| bandwidthHints map[uint64]lnwire.MilliSatoshi | ||
| } | ||
|
|
There was a problem hiding this comment.
This structs is quite a mix of parameters. How about creating two separate structs?
-
graphData (
tx,graph,additionalEdges,bandwidthHints) -
restrictions (
ignoredNodes,ignoredEdges,feeLimit) (Absolute Time Lock Limit #1941 is adding a time lock limit to this)
And keep passing in sourceNode, targetNode and amt as direct function arguments. So 5 arguments in total.
There was a problem hiding this comment.
That sounds like a reasonable way of splitting this up :)
| // channel bandwidth than what is found in the graph. | ||
| // channel bandwidth than what is found in the graph. If set, it will | ||
| // override the capacities and disabled flags found in the graph when | ||
| // doing path finding. In particular, it should be set to the current |
There was a problem hiding this comment.
If set, it will override the disabled flags found in the graph.
It isn't bandwidthHints that overrides the disabled flag, is it? Only the fact that a channel is local, overrides the disabled flag?
There was a problem hiding this comment.
Yes, you're right. I forgot to update the comment with the latest revision. I was contemplating whether it would also make sense to let the bandwidth hint override disabled flags for non-local channels, but figured for now that is not needed (as we don't use it).
halseth
force-pushed
the
disable-bit-decoupling
branch
from
1e5366f to
40651f6
Compare
halseth
added
P2
|
I think as is it looks good, but I have two concerns about bandwidth hints:
(but these are preexisting issues) |
To avoid the findPath parameter list getting out of hand, we define new structs that wraps the mandatory and optional parameters to findPath.
To decouple our own path finding from the graph state, we don't consider the disable bit when attempting to use local channels. Instead the bandwidth hints will be zero for local inactive channels. We alos modify the unit test to check that the disable flag is ignored for local edges.
This commit adds a new test that checks that the bandwidth hints are considered correclty for local channels, and that disable flags are ignored in this case.
halseth
force-pushed
the
disable-bit-decoupling
branch
from
b82989f to
5f8425c
Compare
Currently
Good observation! We should definitely make them bidirectional, as future more advanced routing might make use of that. Sounds like these two improvements can go together in a follow-up PR or two, as it shouldn't be changing the behaviour achieved here, and as you mentioned are preexisting issues. |
|
Yes, agree that this can be follow up PRs. For those PRs: We can choose to use Alternatively, we could supply a bandwidth hint only for the direction smaller to greater pubkey and assume the opposite direction bandwidth as the capacity minus that bandwidth. In that case, it isn't possible anymore to disable both directions using the bandwidth hint. |
|
I think explicitly having one hint for each direction is cleaner. That let us treat the directions induvidually. Also seems like the implicit method could lead to problems with the channel reserve (since bandwidth then not necessarily will be capacity-opposite bandwidht). |
4 participants
This PR consist of the first part of #2091 (Strict channel enabling):
This essentially decouples the graph's disabled status from the local path finding, as the local channel status will be more up-to-date than what is found in the graph.