bitcoin-dev-project / sim-ln

Payment activity generator for the lightning network
MIT License
63 stars 28 forks source link

Use one RNG per Event Producer #192

Closed carlaKC closed 3 weeks ago

carlaKC commented 4 months ago

This PR updates #171 to create one RNG per event producer to decrease the variance across runs of the simulator when running with a fixed seed. This approach was originally taken in #171, but each RNG was seeded with the same value which led to nodes with the same capacity having the exact same payment flows (not what we want). I suggested that we share a RNG to get around that, but it introduces other problems - specifically that all tasks in the simulator are vying for mutex access to the same RNG, so the order of operations highly effects the determinism of the simulator.

This PR takes a new approach, which "salts" the seed with material from each node's public key to make the RNGs per-task deterministic but different.

Two design decisions that I wasn't totally happy with:

  1. We use a single NetworkGenerator because it holds the whole graph, so we can't pass a unique RNG in for each event producer. The solution here is to include the RNG in the API of the network generator in each call:

    • It seemed inconsistent to do this for network generator and not PaymentGenerator (though we could, because this one is generated per event producer).
    • We end up passing in unnecessary RNG to the DefinedPaymentGenerator implementation of PaymentGenerator where we don't actually need it.
  2. We seed the RNG with Option<u64> and then salt it with u64, this should probably by an Option<u64, u64> because we don't need the salt if we don't have a seed, but it seems unnecessary to unwrap and re-wrap (got lazy, might fix).

To get an idea of the improvement, I ran the simulator 50x with the two approaches and measured the coefficient of variance (mean / std dev) for the revenue that one node has in the simulation.

A larger number indicates that our variance is lower, so this is quite a dramatic improvement which IMO makes it worthwhile to make the API changes.

carlaKC commented 4 months ago

@enigbe while using the simulator with fixed seed I noticed that the variance issues that you pointed out get much worse with larger networks (makes sense, there are more events taking place so we have more variance).

I should have thought about this option during review when you had the original version with one rng per producer (as I've done here), sorry! Took me running this to get more of an intuition into where we need to fix things up.

enigbe commented 4 months ago

@enigbe while using the simulator with fixed seed I noticed that the variance issues that you pointed out get much worse with larger networks (makes sense, there are more events taking place so we have more variance).

I should have thought about this option during review when you had the original version with one rng per producer (as I've done here), sorry! Took me running this to get more of an intuition into where we need to fix things up.

Please don't apologize. It was fine implementing it in the way you suggested (helps me learn more about the project). I will review this today/tomorrow.

carlaKC commented 3 weeks ago

Closing due to (my own) inactivity, I'll open it back up when I've got some capacity for this.

Would still like to find an approach that doesn't require carting a rng around our traits, but couldn't figure that out when I looked at it last.