feat: run simulator with fixed seed #178

enigbe · 2024-04-05T19:00:36Z

What this PR does

Adds functionality to run the simulator with an optional fixed seed. If present, the seed allows the random activity generator to run deterministically.

Note the following:

The RNG used, i.e. ChaCha8Rng, expects a seed type of u64

Related Issue

Addresses the issue outlined in Feature: Run random activity generation with fixed seed #171

carlaKC · 2024-05-06T19:01:52Z

Needs rebase!

carlaKC

Just reacquainting myself with this PR!

sim-lib/src/random_activity.rs

carlaKC · 2024-05-15T19:24:53Z

sim-lib/src/random_activity.rs

+                2 * expected_payment,
+                expected_payment,
+                1.0,
+                Some([42; 32])


Any specific reasoning for using None above and Some here for seeds? non-blocking

The seed parameter is optional so I thought one test could run with it provided, and another without it. If consistency matters to you I can write a second string of tests for both cases.

What are you referring to with "non-blocking"?

The seed parameter is optional so I thought one test could run with it provided, and another without it. If consistency matters to you I can write a second string of tests for both cases.

Was just wondering if there was any reason for the choice for those specific tests using the specific params, but makes sense!

What are you referring to with "non-blocking"?

"non-blocking" for merge - so I have a question here but it's not something that must be addressed/answered for the PR to move forward, just asking for my own understanding 👍

sim-lib/src/random_activity.rs

sim-cli/src/main.rs

sim-lib/src/random_activity.rs

sim-lib/Cargo.toml

carlaKC · 2024-06-24T12:18:30Z

@enigbe is there any chance you'll be able to get to this in the next few days? I'd really like to get it merged because I've got some projects depending on it (and would like to tag a patch so that I can use our built in docker workflow).

If not, I'd be interested in taking over the PR to move it along. Please let me know!

enigbe · 2024-06-24T20:05:32Z

@carlaKC I am sorry for the delay but I can finish this on Wednesday if it's not too late.

carlaKC

Thanks for the update, heading in the right direction!

Should have thought of this on the last review, but I think that we actually want a single fixed RNG, otherwise we'll have several rngs all producing the same set of values which leads to weird and not-desirable behavior (eg, all payment amounts being the same).

sim-lib/src/defined_activity.rs

sim-lib/src/random_activity.rs

sim-lib/src/lib.rs

carlaKC · 2024-06-27T13:02:48Z

sim-lib/src/random_activity.rs

 pub struct RandomPaymentActivity {
    multiplier: f64,
    expected_payment_amt: u64,
    source_capacity: u64,
    event_dist: Exp<f64>,
+    seeded_rng: SeededRng,


Not ideal in terms of abstraction, but I think we're going to need to create a single RNG from the fixed seed, and provide it to every instance of RandomPaymentActivity that we have. Because as is, we create multiple rngs with the same seed, so each node will execute payments with the same series of rng values (if you have nodes with the same capacity, they all end up sending the same amount for their payments).

Tried this out here.

I have made some changes so that all instances of RandomPaymentActivity share a single RNG but the deterministic outcome we want is not consistent. This could be down to how I have set my nodes but there is some undesired variance in the payment amount generated after multiple simulator runs.

Here is a small table showing the simulation run count and the payment amount generated at each run. The table is being updated every time I run the simulator to identify if the variance is as a result of my node setup or the code.

Please try this on your PC to see if you get similar outcomes.

I think this is to be expected - tasks will complete in slightly different orders on each run (and payments will complete in different amounts of time), so they'll grab the common RNG is different orders. We can't make promises for totally deterministic runs, but I think that this is a good shot at it because it'll aggregate out over several runs.

But well observed, let's make a note of it in the readme that we're not promising fully deterministic simulations.

Can also confirm this by just writing a small unit test for MutRng::new and asserting that we get the same values for the same seeds.

carlaKC

Just some nitpicks left then this is ready to go! Thanks for picking this back up 🫶

Go ahead and squash fixes on next push. Would also be great to add a note to the readme that this option is now available, but with limitations.

sim-lib/src/lib.rs

sim-lib/src/random_activity.rs

carlaKC · 2024-06-28T14:13:25Z

sim-lib/src/random_activity.rs

 pub struct RandomPaymentActivity {
    multiplier: f64,
    expected_payment_amt: u64,
    source_capacity: u64,
    event_dist: Exp<f64>,
+    seeded_rng: SeededRng,


Can also confirm this by just writing a small unit test for MutRng::new and asserting that we get the same values for the same seeds.

sim-lib/src/random_activity.rs

carlaKC · 2024-06-28T14:20:58Z

sim-lib/src/lib.rs

+struct MutRng(pub MutRngType);
+impl MutRng {


Don't personally feel like adding a wrapper struct on MutRngType adds much value (assuming it's there so we can have an impl block + constructor) here over just passing around params of the type, but not blocking merge.

It makes it easier to create new MutRng types within code and in tests. I think it is much cleaner to have MutRng::new(T) that Arc::new(Mutex::new(Box::new(T)))

sim-lib/src/lib.rs

bjohnson5

LGTM

fix: run simulator with optional fixed seed **Initial commit** What this commit does: - Makes it possible to run the simulator with an optional u64 fixed-seed for deterministic outcomes for randomly generated payment activities Notes: - This commit defines SeededRng: a thread-safe, mutually exclusive option of any type that implements RngCore and Send. - SeededRng is a field in both NetworkGraphView and RandomPaymentActivity. Both the DestinationGenerator and PaymentGenerator hold references to SeededRng in their trait implementations. If SeededRng is defined as an Option<Box<dyn RngCore + Send>>, it will be impossible to gain exclusive access (&mut) to self.seeded_rng, which is shared access (&). Mutable reference to the SeededRng is required by the distribution samplers. - Thus, SeededRng as previously defined (Option<Box<dyn RngCore + Send>>) is wrapped in Arc<Mutex<>> for exclusive access. - additionally, removes unnecessary async from defined & random activity tests **Commit update** feat: use single shared RNG across all executor kits - Creates a new type MutRng that wraps any type that implements RngCore and provides shared and exclusive access to the type. - Creates a RNG on the 'Simulation' that's shared across and accessible to all 'ExecutorKit's - add MutRng test & fix documentation issues

random activity generation

carlaKC

Last few comments (can be done in a followup) - thanks for all the work on this one!

carlaKC · 2024-07-08T12:35:04Z

sim-lib/src/lib.rs

+/// Newtype for `MutRngType` to encapsulate and hide implementation details for
+/// creating new `MutRngType` types. Provides convenient API for the same purpose.
+#[derive(Clone)]
+struct MutRng(pub MutRngType);


Does not need pub?

carlaKC · 2024-07-08T12:42:20Z

sim-lib/src/lib.rs

+///
+/// **Note**: `StdMutex`, i.e. (`std::sync::Mutex`), is used here to avoid making the traits
+/// `DestinationGenerator` and `PaymentGenerator` async.
+type MutRngType = Arc<StdMutex<Box<dyn RngCore + Send>>>;


Box is unnecessary here?

carlaKC · 2024-07-08T12:44:23Z

README.md

@@ -132,6 +132,7 @@ of the random activity that is generated:
     capacity in a month.
  * `capacity-multiplier=0.5` means that each node sends half their 
    capacity in a month.
+* `--fix-seed`: a `u64` value that allows you to generate random activities deterministically from the provided seed, albeit with some limitations. The  simulations are not guaranteed to be perfectly deterministic because tasks complete in slightly different orders on each run of the simulator. With a fixed seed, we can guarantee that the order in which activities are dispatched will be deterministic.


Nice docs!

nit: double space in The simulations

enigbe requested review from okjodom and carlaKC April 5, 2024 19:00

enigbe self-assigned this Apr 5, 2024

enigbe force-pushed the feat-run-simulator-with-fixed-seed branch from 88d9c94 to 953e40a Compare April 5, 2024 19:32

carlaKC reviewed May 15, 2024

View reviewed changes

enigbe force-pushed the feat-run-simulator-with-fixed-seed branch 3 times, most recently from 58c2fa2 to b1f5833 Compare May 20, 2024 10:00

enigbe requested review from carlaKC and bjohnson5 May 20, 2024 10:05

bjohnson5 approved these changes May 20, 2024

View reviewed changes

carlaKC reviewed May 20, 2024

View reviewed changes

sim-lib/src/random_activity.rs Outdated Show resolved Hide resolved

sim-lib/Cargo.toml Outdated Show resolved Hide resolved

enigbe force-pushed the feat-run-simulator-with-fixed-seed branch 2 times, most recently from e6f10a6 to 18f9308 Compare June 27, 2024 08:42

enigbe requested review from carlaKC and bjohnson5 June 27, 2024 08:51

carlaKC reviewed Jun 27, 2024

View reviewed changes

enigbe requested a review from carlaKC June 28, 2024 12:03

carlaKC reviewed Jun 28, 2024

View reviewed changes

bjohnson5 reviewed Jun 28, 2024

View reviewed changes

enigbe added 2 commits June 29, 2024 09:28

docs: update docs to reflect introduction of feature for deterministic

ce67b98

random activity generation

enigbe force-pushed the feat-run-simulator-with-fixed-seed branch from 77cea78 to ce67b98 Compare June 29, 2024 08:31

enigbe requested a review from carlaKC June 29, 2024 08:36

carlaKC approved these changes Jul 8, 2024

View reviewed changes

carlaKC merged commit f5c8430 into bitcoin-dev-project:main Jul 8, 2024
2 checks passed

carlaKC mentioned this pull request Jul 8, 2024

Feature: Run random activity generation with fixed seed #171

Closed

enigbe mentioned this pull request Jul 8, 2024

chore(rng): clean up documentation and RNG type #191

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: run simulator with fixed seed #178

feat: run simulator with fixed seed #178

enigbe commented Apr 5, 2024 •

edited

Loading

carlaKC commented May 6, 2024

carlaKC left a comment

carlaKC May 15, 2024

enigbe May 17, 2024 •

edited

Loading

carlaKC May 17, 2024

carlaKC commented Jun 24, 2024

enigbe commented Jun 24, 2024

carlaKC left a comment

carlaKC Jun 27, 2024

enigbe Jun 28, 2024 •

edited

Loading

carlaKC Jun 28, 2024

carlaKC Jun 28, 2024

carlaKC left a comment

carlaKC Jun 28, 2024

carlaKC Jun 28, 2024

enigbe Jun 28, 2024

bjohnson5 left a comment

carlaKC left a comment

carlaKC Jul 8, 2024

carlaKC Jul 8, 2024

carlaKC Jul 8, 2024

feat: run simulator with fixed seed #178

feat: run simulator with fixed seed #178

Conversation

enigbe commented Apr 5, 2024 • edited Loading

What this PR does

Related Issue

carlaKC commented May 6, 2024

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enigbe May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Jun 24, 2024

enigbe commented Jun 24, 2024

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enigbe Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjohnson5 left a comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enigbe commented Apr 5, 2024 •

edited

Loading

enigbe May 17, 2024 •

edited

Loading

enigbe Jun 28, 2024 •

edited

Loading