Report Pathfinding Outcomes to Scorer in SimNode #291
Conversation
chuksys
force-pushed
the
report-pathfinding-outcomes
branch
3 times, most recently
from
d151ed3 to
32c4089
Compare
|
Looking really good overall! Let's sort out these pesky MPP issues and make the clock generic then we're gg. |
|
Thanks for the feedback. Will get this sorted asap! |
chuksys
force-pushed
the
report-pathfinding-outcomes
branch
from
32c4089 to
c9ef909
Compare
Added Some Key Changes
|
There was a problem hiding this comment.
Some questions from a very high level pass.
Reverted SimNode Immutability: I've reverted some changes introduced in 8ded28e to allow SimNode to mutate its internal fields where necessary.
What's the motivation for reverting this change? If possible it's preferable to not require implementations of a trait to be mutable. Would rather add a mutex where we need it than re-introduce trait-required mutability.
simln-lib/src/sim_node.rs
Outdated
| @@ -1067,6 +1058,7 @@ pub async fn ln_node_from_graph( | |||
| node.1 .0.clone(), | |||
| graph.clone(), | |||
| routing_graph.clone(), | |||
| SystemClock {} | |||
There was a problem hiding this comment.
We should pass this clock in as a parameter to give callers the freedom to choose what impl they want
simln-lib/src/sim_node.rs
Outdated
| @@ -1211,6 +1216,77 @@ impl SimNetwork for SimGraph { | |||
| } | |||
| } | |||
|
|
|||
| /// This trait defines the "interface" for payment propagation logic | |||
| pub trait PaymentPropagator: Send + Sync + 'static { | |||
There was a problem hiding this comment.
This seems like a lot of boilerplate just for tests (and possibly somewhere we should use async_trait rather than have these boxes + pins?
Doesn't seem like it would be too much test code to just use real propagation and create payments that fail at the points we want?
There was a problem hiding this comment.
Yeah, you are right - I actually used async_trait in a fixup of the commit; However I think it's a good idea to use real propagation rather than have all this boilerplate. I'll work on that.
Thanks for this suggestion. Will work on this. |
sim-cli/src/parsing.rs
Outdated
| @@ -313,7 +313,7 @@ pub async fn create_simulation_with_network( | |||
| // custom actions on the simulated network. For the nodes we'll pass our simulation, cast them | |||
| // to a dyn trait and exclude any nodes that shouldn't be included in random activity | |||
| // generation. | |||
| let nodes = ln_node_from_graph(simulation_graph.clone(), routing_graph).await; | |||
| let nodes = ln_node_from_graph(simulation_graph.clone(), routing_graph, SystemClock {}).await?; | |||
There was a problem hiding this comment.
Should use the clock passed in?
simln-lib/src/clock.rs
Outdated
| @@ -12,7 +12,7 @@ pub trait Clock: Send + Sync { | |||
| } | |||
|
|
|||
| /// Provides a wall clock implementation of the Clock trait. | |||
| #[derive(Clone)] | |||
| #[derive(Clone, Copy)] | |||
There was a problem hiding this comment.
We actually don't need to - will take that out.
simln-lib/src/sim_node.rs
Outdated
| @@ -1019,11 +1061,12 @@ impl SimGraph { | |||
| } | |||
|
|
|||
| /// Produces a map of node public key to lightning node implementation to be used for simulations. | |||
| pub async fn ln_node_from_graph( | |||
| pub async fn ln_node_from_graph<C: Clock + std::marker::Copy>( | |||
There was a problem hiding this comment.
Rather than require this to be copy, pass in Arc<C>? This is what we do elsewhere
This commit ensures that the payment apis can only send to a single path. Payment is rejected if the route has multiple paths.
This commit asserts that payment failure on first hop returns Some(0). On payment failure, this ensures the correct index of hop where failure occured is reported to scorer.
chuksys
force-pushed
the
report-pathfinding-outcomes
branch
from
2eeda7a to
3dfc98a
Compare
Description
This PR reports pathfinding outcomes to scorer in SimNode. This significantly improves the realism of pathfinding simulations by allowing the scorer to update based on actual payment success or failure.
This is an attempt to complete the work started on this branch.
Changes
SimNode.in_flightusingInFlightPayment.IndexFailurePaymentOutcomevariant, which specifically captures the index of the channel in the payment path that caused the failure.SimNodeusingSimulationClockfor accurate duration reporting.SimNode.scorerusing the newly tracked path and failure index (if applicable).This PR closes #279