Add pathfinder trait #273
Add pathfinder trait #273
Conversation
sangbida
force-pushed
the
sangbida/custom-pathfinding
branch
4 times, most recently
from
125e109 to
2dc39e5
Compare
simln-lib/src/sim_node.rs
Outdated
| dest: PublicKey, | ||
| amount_msat: u64, | ||
| pathfinding_graph: &NetworkGraph<&'a WrappedLog>, | ||
| scorer: &ProbabilisticScorer<Arc<NetworkGraph<&'a WrappedLog>>, &'a WrappedLog>, |
There was a problem hiding this comment.
This is a very LDK-specific item to pass to a general Pathfinder trait. Eg, if you're using this trait to pull in LND's pathfinding you really don't care about LDK's scorer.
I think we should look at moving these scorers out of SimNode and into being tracked in the DefaultPathFinder - can just track in a hashmap I think?
|
I'm away this week and intermittently through June, so this PR might take some time to complete. |
sangbida
force-pushed
the
sangbida/custom-pathfinding
branch
from
f65d20d to
1c78c95
Compare
sangbida
force-pushed
the
sangbida/custom-pathfinding
branch
from
1c78c95 to
3e72658
Compare
sangbida
force-pushed
the
sangbida/custom-pathfinding
branch
from
46606e9 to
b7416fe
Compare
| let scorer_graph = NetworkGraph::new(bitcoin::Network::Regtest, &WrappedLog {}); | ||
| let scorer = ProbabilisticScorer::new( | ||
| ProbabilisticScoringDecayParameters::default(), | ||
| Arc::new(scorer_graph), | ||
| &WrappedLog {}, | ||
| ); |
There was a problem hiding this comment.
I think that this takes us a step backwards from the goal of having one scorer per SimNode? I would have thought that we'd have a HashMap<Pubkey, Scorer> here so that we can look up the source node's scorer and use it per find_route call.
Having said that, this PR has made me realize that we're not actually reporting any of our outcomes to the scorers (which means we're basically using an empty one every time). Wrote up details in #279 - sadly I think this PR will need to wait for that, as it'll directly impact this API (we now also need updating as part of Pathfinder, though it may be as simple as just including the ScorerUpdate trait).
There was a problem hiding this comment.
Aah! That's interesting. When I started on this issue I thought the primary objective was to have generic pathfinding and completely forgot about having a single scorer for a SimNode 🤦
I did add a hashmap with the pubkey-scorer but ran into a bunch of lifetime issues so decided to simplify it with a fresh scorer each time :(
Also, this issue only pertains to scoring with the DefaultPathfinder, right? Because the onus of maintaining scorer feedback would be on the user maintaining their custom Pathfinder.
So if you add a feedback mechanism for the scorer it would make sense for it to live inside DefaultPathFinder?
There was a problem hiding this comment.
There was a problem hiding this comment.
Because the onus of maintaining scorer feedback would be on the user maintaining their custom Pathfinder.
True, but we'd need to have to provide a way for the simulator to report success/failure to the custom pathfinder - they could use track_payment but I think it makes sense for us to require that they implement report_success/failure as part of the trait so that we can handle that all automatically?
I could create a PR on top of this branch to solve for the scorer feedback mechanism in #279 or I could close this PR for now until the scorer feedback issue is resolved and refactor this code so it works with the feedback mechanism.
I think it makes sense to get feedback in and then come back to this PR? If you'd be happy to take stab at #279 that would be great! I think we should start with that, leave this PR open and then rebase this on top of it?
There was a problem hiding this comment.
So thinking that the final pathfinder api would be:
find_route
report_payment_success
report_payment_failure
``
Very similar to what LDK has right now (in `ScorerUpdate`). We then set the simulator up to report success/failure to whatever pathfinder is plugged in.
There was a problem hiding this comment.
I like your Pathfinder API approach! That makes sense to me! I've updated the description to reflect that the development is paused until we have more feedback and we have closed out #279.
I'm happy to take a stab at #279 but I will be going on holiday for around 2 weeks in a few days so it might just sit there for a while. I'm happy for someone else to pick it up in the meantime so there can be progress and I'll rebase this PR against it, but if no one has picked it up by then I'll definitely pick it up :)
There was a problem hiding this comment.
I'm happy to take a stab at #279 but I will be going on holiday for around 2 weeks in a few days so it might just sit there for a while. I'm happy for someone else to pick it up in the meantime so there can be progress and I'll rebase this PR against it, but if no one has picked it up by then I'll definitely pick it up :)
SGTM!
UPDATE: Development on this PR is currently paused until more feedback around custom pathfinding is gathered and #279 is closed.
Description
This PR introduces a new
PathFindertrait which allows for more flexible pathfinding. Instead of being locked into the LDK pathfinding algorithm, custom pathfinding algorithms can be swapped in. ADefaultPathFinderhas been provided to maintain backwards compatibility.Changes
PathFindertrait: Provides a clean interface for implementing custom pathfinding algorithmsDefaultPathFinder: Uses LDK'sfind_routefunction with probabilistic scoring as the default pathfinding strategy