Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sim-lib: add simulated clock to speed up simulations #242

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

carlaKC
Copy link
Contributor

@carlaKC carlaKC commented Mar 28, 2025

This PR adds a Clock trait and makes Simulation generic over this trait so that we have the ability to mock time, as described in #81. The default implementation of this trait just thinly wraps a system clock.

For the version where we want to speed things up, we track the instant that we started our clock and apply a multiplier to speed up time passed. For example, if we're running with a multiplier of 10:

  • Track our start instant as zero
  • One second of "real" time passes, which is equivalent to 10 seconds of simulated time

When dealing with durations, we can simply multiply/divide to move between simulation and real time periods.
When dealing with timestamps, we use the elapsed duration since start to figure out a sped up clock time.

This PR doesn't surface the implementation anywhere, because it doesn't really make sense to run this speed up on real nodes - things get messy when you're using different clocks; if you want things faster on your real node network, just set the activity to fire more often or the random activity to send more payments.

It is, however, very useful once we've implemented #215, because we can speed up our sim-node network.

@carlaKC
Copy link
Contributor Author

carlaKC commented Mar 28, 2025

Opening this up for conceptual review, cc @sangbida @f3r10 @elnosh. I'll do a cleanup pass on it before it requires thorough review.

It's the simplest thing that I could come up with (and has worked pretty well when I run simulations using it on a branch), but I'm very open to other opinions!

time

@carlaKC
Copy link
Contributor Author

carlaKC commented Mar 31, 2025

Note to self - will update in the next push:

Right now, SimLN is calling now for every channel policy timestamp that we add to our simulated network. If the clock is sped up fast enough, the timestamp for these channel updates will keep increasing as we add them. LDK only allows updates to be 24H in the future, so it's possible that we can hit an error here if we have a large enough graph and an fast enough simulation clock - I've hit this a few times in jamming simulations.

@sangbida
Copy link
Contributor

sangbida commented Apr 1, 2025

This looks good to me:)

I had a question about how this would be used in the wild. If the purpose is to only speed up transactions, I think this works great! I was wondering if you may run into issues with clock skew because of how tokio:sleep works? This maybe an issue if you wanted to track precise timestamps.

@carlaKC
Copy link
Contributor Author

carlaKC commented Apr 2, 2025

I had a question about how this would be used in the wild. If the purpose is to only speed up transactions, I think this works great!

The plan would be to use this alongside #248 to run sped-up transactions on a simulated network. When it comes time to surface this option, we'll only make it available if running with a fake network - trying to speed up transactions on real nodes that are running with regular clock time would indeed be a mess (if that's what you want, just setup your simulation to have more transactions).

I was wondering if you may run into issues with clock skew because of how tokio:sleep works?

I think that provided we always rely on the simulation clock we should be ok? Eg with a 10x speed up, if we have a sleep for 10 minutes we should just tell tokio::sleep to sleep for 1 minute. Maybe I'm missing your point here?

EDIT: is this the sort of thing you're concerned about? Where the clock "runs away from us" and calling libraries need to be aware of the underlying impl. Sadly I don't have any good ideas here :(

@f3r10
Copy link
Collaborator

f3r10 commented Apr 2, 2025

LGTM!!
Looking forward to seeing how it works together with #248

@carlaKC carlaKC force-pushed the 81-simulatedtime branch from 9b2da47 to e468a4d Compare April 2, 2025 19:47
@@ -511,12 +514,13 @@ struct ExecutorKit {
payment_generator: Box<dyn PaymentGenerator>,
}

impl Simulation {
impl <C: Clock + 'static> Simulation <C>{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO(carla): can this be replaced with a lifetime?

@carlaKC carlaKC force-pushed the 81-simulatedtime branch 2 times, most recently from 2fe7cd4 to 8468d51 Compare April 2, 2025 19:56
@carlaKC
Copy link
Contributor Author

carlaKC commented Apr 2, 2025

Force pushed to:

Will start with fixups once this is in real review (needs to be rebased on #248 first, waiting till that approach has stabilized).

@elnosh
Copy link
Contributor

elnosh commented Apr 2, 2025

approach looks good to me (: ran some simulations using SimulationClock and seems to be working fine.

@sangbida
Copy link
Contributor

sangbida commented Apr 2, 2025

I think I misunderstood how SimulationClock is working here. So the case I was worried about was, let's say you have a 100x multiplier, if tokio::sleep wakes up 5ms late then you have a 500ms discrepancy in the simulation time and this could potentially get compouned over time. But on rereading, the simulation clock isn't actually keeping track of the time it's just using the system clock and scaling it for the simulation, so we won't run into the issue with compounding but we may run into issues with the scheduler delay, which I think is okay since the intended use is within a mocked ln network.

carlaKC added 5 commits April 3, 2025 15:50
Add a trait to mock time so that when we get to using
simulation time, we don't need to refactor. At present,
this is just a wrapper over system time.
Allows us to keep the API for our clock a little simpler.
As is, channels were all imported with different timestamps. There's
no use for this, and it runs the risk of exceeding LDK's 24h limit
if we're using a very sped up clock. Using a single value fixes this.
@carlaKC carlaKC force-pushed the 81-simulatedtime branch from 8468d51 to 8bb2b6e Compare April 3, 2025 20:18
@carlaKC carlaKC requested a review from f3r10 April 3, 2025 20:19
@carlaKC
Copy link
Contributor Author

carlaKC commented Apr 3, 2025

Given that this is being used quite extensively in simulations and working well, I think that we can go ahead with review even though there isn't a way to test this and then base #248 on top of it (instead of the other way around)? Adding it just requires a cli flag and some validation so it shouldn't be much of a lift.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants