sim-lib: only use start delay if explicitly specified #190
Conversation
carlaKC
force-pushed
the
189-paymentdelay
branch
from
5dd13cd
to
44bb6b6
Compare
|
cc @bjohnson5 WDYT? |
sim-lib/src/lib.rs
Outdated
| ); | ||
| } | ||
| // On our first run, add a one-time start delay if set for our payment. | ||
| let opt_start = node_generator.payment_start(); |
There was a problem hiding this comment.
nit: This line doesn't really need to happen every loop.
Calling payment_start() and assigning opt_start is only necessary on the first iteration. It might be worth moving that into the second condition so that if current_count == 0 fails it will short circuit and not execute the payment_start() call.
let wait = if current_count == 0 && node_generator.payment_start().is_some() {
let start = node_generator.payment_start().unwrap(); // Safe due to is_some check above.
log::debug!(
"Using a start delay. The first payment for {source} will be at {:?}.",
start
);
start
} else {
...Still not ideal to call payment_start() twice but I could not think of a way to get the value into the if block otherwise.
There was a problem hiding this comment.
Still not ideal to call payment_start() twice but I could not think of a way to get the value into the if block otherwise.
Yeah, this was my original reasoning for creating op_start (+ learned that if let has more limited syntax than I thought). Agree that this is probably the best we can do - will add a note on the trait that it's expected to return the same value on every call.
There was a problem hiding this comment.
Actually just went for a more verbose set of branches, I think it's nicer than adding an external expectation on payment_start's behavior (even if in practice it always is the same value) + added a comment explaining.
There was a problem hiding this comment.
Totally agree, that looks really good now. It is also easier to read and understand what is happening.
|
@carlaKC Untested ACK. I can test this change next week sometime, but don't let that be a blocker for merging. |
carlaKC
force-pushed
the
189-paymentdelay
branch
2 times, most recently
from
3e63dd3
to
1231da8
Compare
As is, we'll run with a 0 second start delay for: - Random activities - Defined activities with no `start_secs` specified This very likely isn't the intent for defined activities (people should set `start_secs=0` if they want their payments to fire immediately), and doesn't work for random activity because we just fire all payments on start. Instead, we switch over to using our payment wait as the start delay in the absence of this field being set for defined activities and always for random activities.
|
Merging so that I can tag a patch with the fix (need the docker image elsewhere). Would be great to get post-merge review on this one anyway 🙏 |
As is, we'll run with a 0 second start delay for:
start_secsspecifiedThis very likely isn't the intent for defined activities (people should set
start_secs=0if they want their payments to fire immediately), and doesn't work for random activity because we just fire all payments on start.Instead, we switch over to using our payment wait as the start delay in the absence of this field being set for defined activities and always for random activities.
Fixes #189