-
Notifications
You must be signed in to change notification settings - Fork 618
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
feat(sandbox): fast forward timestamp and epoch height #6211
feat(sandbox): fast forward timestamp and epoch height #6211
Conversation
…/sandbox-fast-forwarding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! I didn't expect timestamp could also be update in a straightforward way. Historically we had a PR to "time travel": #3661 which was complicated. I think your did both fast forward block height and time travel!
…-fast-forward-timestamp-and-epoch-height
…/sandbox-fast-forward-timestamp-and-epoch-height
…/sandbox-fast-forward-timestamp-and-epoch-height
@bowenwang1996 @mm-near updated this PR with the new epoch boundary logic, where we jump to each epoch boundary, produce some blocks, then update the epoch height, so no custom sandbox related epoch_manager logic is needed. Let me know if I'm missing anything |
…/sandbox-fast-forward-timestamp-and-epoch-height
…/sandbox-fast-forward-timestamp-and-epoch-height
…/sandbox-fast-forward-timestamp-and-epoch-height
… of https://github.com/near/nearcore into phuong/sandbox-fast-forward-timestamp-and-epoch-height
} | ||
|
||
#[cfg(feature = "sandbox")] | ||
#[derive(Eq, PartialEq, Debug)] | ||
pub enum SandboxResponse { | ||
SandboxPatchStateFinished(bool), | ||
SandboxFastForwardFinished(bool), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not important, but a better name here would be FastForwardInProgress
, to not think aboUt "fast forward not even started" case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with what was already there for patching state. It's only being used internally right now, so not too important to change IMO
chain/client/src/client.rs
Outdated
height: next_height, | ||
seen: to_timestamp(Clock::utc()), | ||
})?; | ||
self.chain.mut_store().save_latest_known(LatestKnown { height: next_height, seen: at })?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I don't like that at
here at LatestKnown
and the timestamp
of the block are different, but this is pre-existing behavior. So, no need to change anything in this PR, but I woneder if the following change makes sense as a follow-up:
diff --git a/chain/client/src/client.rs b/chain/client/src/client.rs
index c6091c459..f5982fa6b 100644
--- a/chain/client/src/client.rs
+++ b/chain/client/src/client.rs
@@ -451,21 +451,10 @@ impl Client {
prev_next_bp_hash
};
- #[cfg(feature = "sandbox")]
- let delta_time = self.sandbox_delta_time();
-
- let at = to_timestamp(Clock::utc());
+ #[cfg(not(feature = "sandbox"))]
let timestamp_override = Option::<u64>::None;
-
- // Sandbox can override the timestamp normally produced from a Block. This new timestamp
- // is from when a fast_forward request that has been processed.
#[cfg(feature = "sandbox")]
- let (at, timestamp_override) = if delta_time > 0 {
- let at = at + delta_time;
- (at, Some(at))
- } else {
- (at, timestamp_override)
- };
+ let timestamp_override = to_timetamp(Clock::now()) + self.sandbox_delta_time();
// Get block extra from previous block.
let mut block_merkle_tree =
@@ -538,9 +527,11 @@ impl Client {
block_merkle_root,
timestamp_override,
);
+ let latest_known =
+ LatestKnown { height: block.header().height(), seen: block.header().timestamp() };
// Update latest known even before returning block out, to prevent race conditions.
- self.chain.mut_store().save_latest_known(LatestKnown { height: next_height, seen: at })?;
+ self.chain.mut_store().save_latest_known(latest_known)?;
metrics::BLOCK_PRODUCED_TOTAL.inc();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up changing it to what you suggested, since I had to change it already for swapping the return type of sandbox_delta_time
to Duration
. And pretty much same logic at the end of the day too, so it should work the same
@@ -67,6 +67,10 @@ pub struct Client { | |||
#[cfg(feature = "test_features")] | |||
pub adv_produce_blocks_only_valid: bool, | |||
|
|||
/// Fast Forward accrued delta height used to calculate fast forwarded timestamps for each block. | |||
#[cfg(feature = "sandbox")] | |||
pub(crate) accrued_fastforward_delta: near_primitives::types::BlockHeightDelta, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, if only we did the work for virtualizing time, than we'd be able to express this in a much cleaner way, by just making the time go forward faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this was definitely a hack to circumvent that. Maybe we can revisit this in the future, but for now, this was pretty simple to do just for contract testing side of things
…/sandbox-fast-forward-timestamp-and-epoch-height
Realized was missing some extra stuff like updating timestamps/epoch-height after fast forwarding special thanks to @TrevorJTClarke. This is a corrollary PR to #6158 to address and add timestamps and epoch-height updates.
How it works:
The timestamps are updated via the delta block height converted to a fast-forward timestamp delta; which then gets added to a
Clock::utc()
to get the fast-forward timestamp. Then the next block with use the previous timestamp to go from there. Epoch height update uses current block height / epoch_length to get the height instead of the usual increment by one. This is handled this way since not much is passed into it besides theBlockInfo
andEpochManager
is separate from everything else such as chain/client/client-actor.Also, unfortunately, there’s no easy way that I can see of for testing the epoch height update since that is apart of
near-epoch-manager
crate and is created separately from everything else; so can't really test it with block production. Let me know if there is something I'm missing out on