Implement time travel for NEAR testing infrastructure #3661

pczarn · 2020-11-28T12:27:06Z

Implements near/bounties#31

Document all occurences of Utc::now

Inspect whether they need to be replaced with a time proxy.

confirm with us the correctness of each workitem before moving on to the next one

Please review and confirm the correctness of point one:

Inspect all the occurrences of Utc::now and documenting which ones need to be replaced with a proxy.
Design a time-proxy that will return Utc::now without the adversarial feature flag, but otherwise enables manipulating time via an RPC end point
Implement the proxy, add the meta-test described above.

pczarn · 2020-11-28T13:10:58Z

Upon basic inspection, the occurences of Instant::now are all related to timing or testing.

bowenwang1996

where is the time_proxy module? Also I don't think we need to have a Utc submodule there

bowenwang1996 · 2020-11-30T23:44:34Z

chain/chunks/src/lib.rs

@@ -260,6 +260,7 @@ impl SealsManager {
                };

                let chosen = Self::get_random_part_ords(candidates);
+                // `sent` is not accessed, so we don't use the time proxy.


It is not used right now, but will be enabled after #3657 is merged

will fix this catch

pczarn · 2020-12-03T17:29:35Z

The time_proxy module is to-be-done as the second step. I believe I will invoke this stuff by doing use utils::time_proxy::UtcProxy, then calling UtcProxy::now, what do you think?

Alas, I caught a cold (this is a cold season after all) so this task will need to wait a few days.

pczarn · 2021-01-12T17:31:53Z

I'm back to coding. I'll pick up this task shortly. Please excuse the delay.

pczarn · 2021-01-24T11:07:11Z

I have the second sub-task compiling, and the last sub-task as a draft.

Could you think of more tests utilizing time travel? If we spend effort implementing the feature that enables testing, we might as well add more testing.

pczarn · 2021-03-23T10:12:40Z

Without time travel, the test completes in 22 seconds. With time travel enabled, the test completes in 50 seconds half of the time, and gets stuck the other half of the time. The core simply does not work well with time changes.

edit: it's possible that my proxy replacements are wrong or insufficient or incongruent in the core.

edit: one proxy replacement was incorrect.

pczarn · 2021-03-31T14:37:53Z

@bowenwang1996 @SkidanovAlex @chefsale @mfornet @pmnoxx please review, it's now passing the tests and checks.

* Tests for another use case as well: not stopping time, but instead slowing it down by 70%, and making sure that on the same (relatively long) period of time 70% fewer blocks are produced.

SkidanovAlex · 2021-04-14T20:42:25Z

Hi, @pczarn, the changes look good to me.
Let's remove the proxify map, I think it overly complicates the machinery (and make the time machine always affect all the locations when enabled).

Removes code that is not necessary for typical operation. Makes time travel affect all proxied locations when enabled. You may want to revert this commit in case you'd wish to hunt for incorrect proxifications using `scripts/find_time_travel_setting.py`.

pczarn · 2021-04-15T07:46:35Z

done in the latest 2 commits @SkidanovAlex

pczarn · 2021-04-28T09:58:51Z

@SkidanovAlex please allocate some time for a review

bowenwang1996 · 2021-05-13T19:16:22Z

pytest/tests/adversarial/time_travel.py

+            print(f'block {h}  {fr}->{to}')
+        return True
+
+# consensus_config = {"consensus": {"min_num_peers": 0}}


should this be removed?

SkidanovAlex · 2021-05-14T20:22:03Z

The code looks good!
Thanks @pczarn!

@bowenwang1996 let's not merge until I figure out the issue with cluster.py.

olonho · 2021-06-22T09:30:35Z

runtime/runtime-params-estimator/src/testbed_runners.rs

@@ -154,7 +154,7 @@ fn end_count_instructions() -> u64 {
 }

 fn start_count_time() -> Consumed {
-    Consumed::Instant(Instant::now())
+    Consumed::Instant(Instant::system_time())


What's the point of this change? We do not need system time here, we need a way to count monotonic ticks between two moments.
Instant::now does exactly what we need, i.e.

A measurement of a monotonically nondecreasing clock. Opaque and useful only with Duration.

While description of SystemTime

A measurement of the system clock, useful for talking to external entities like the file system or other processes.

is not behavior we're looking for here.

Perhaps you are misled by the fact that the change has no impact. The call still goes through to Instant::now as before. Still, I wanted to introduce this name to bring attention to the fact that we decided not to go through a proxy.

A more descriptive name such as system_now or unproxied_now could be used here.

@olonho does my comment resolve your doubts?

olonho

We need to use monotonic clock when measuring system elapsed time in estimator.

stale · 2021-07-06T09:36:05Z

This PR has been automatically marked as stale because it has not had recent activity in the 2 weeks.
It will be closed in 3 days if no further activity occurs.
Thank you for your contributions.

stale · 2021-11-12T18:04:09Z

This PR has been automatically marked as stale because it has not had recent activity in the 2 weeks.
It will be closed in 3 days if no further activity occurs.
Thank you for your contributions.

pczarn · 2021-11-13T18:12:30Z

@SkidanovAlex what's the status of this PR?

bowenwang1996 · 2021-11-15T17:16:05Z

Hey @pczarn, due to the fact that this PR is pretty outdated, we decided to implement it from scratch instead of rebasing this PR on master. See (#5123) for details. Did you get the payment?

pczarn · 2021-11-17T10:22:35Z

@bowenwang1996 yes I did receive the payment for this PR. This PR is a good chance for me to learn since I can look into somebody's reimplementation. I can already see some things that could have been better in this PR.

pmnoxx · 2021-11-17T20:30:56Z

@pczarn I think that overall this is a good PR.However, in order to ship it safely a few steps would have been required.

In the ideal scenario we could have done it like this:

(introduce one source of time): We have multiple ways to get time. SystemTime, chrono:Utc, Instant.
The instant has Instant::elapsed() method, which can get current time by itself, so replacing Instant with a wrapper is a must.
However, having multiple types of wrappers in the code base for multiple types introduces unnecessary complexity.
The best approach would be to start with introducing new wrapper for that, that can be used everywhere, take a look at feat: Create near-clock crate for mocking time - pt 1 #5177.
Implement a prototype for mocking - the structs are in (Mocking the system clock #5123). However, the mocking is not complete, it doesn't take care of Instant::elapsed and System time.
Then do the replacement step by step, ideally separately for each crate or group of crates. Like (chain/network ; chain/json* / chain/client; core/) - Ideally up to 10 files a a time, to make it easy to review
Add mocking, just like you did, and replace now(), elapsed() methods.
Also rust runs tests in parallel, so we have to handle it somehow. We decided on propagating TimeFactory throughout the code. And that will be a big change as well.

bowenwang1996 · 2021-11-17T21:39:42Z

@bowenwang1996 yes I did receive the payment for this PR. This PR is a good chance for me to learn since I can look into somebody's reimplementation. I can already see some things that could have been better in this PR.

Thanks for your contribution! If you see places for improvements, feel free to submit new PRs.

pczarn requested review from ailisp, bowenwang1996, evgenykuzyakov, frol, MaksymZavershynskyi and SkidanovAlex as code owners November 28, 2020 12:27

bowenwang1996 reviewed Nov 30, 2020

View reviewed changes

pczarn mentioned this pull request Feb 4, 2021

feat: Syncing chunks from archival nodes #3302

Merged

pczarn force-pushed the adversarial-time-travel branch from 4aec605 to 2f84d67 Compare March 27, 2021 13:25

pczarn requested review from chefsale, khorolets, mfornet, mhalambek, olonho, pmnoxx and willemneal as code owners March 27, 2021 13:25

pczarn force-pushed the adversarial-time-travel branch 3 times, most recently from d0e2886 to aeb233c Compare March 31, 2021 13:59

pczarn changed the title ~~[WIP] Implement time travel for NEAR testing infrastructure~~ Implement time travel for NEAR testing infrastructure Mar 31, 2021

pczarn force-pushed the adversarial-time-travel branch 2 times, most recently from 317b39a to 9b19c1b Compare April 6, 2021 15:49

pczarn force-pushed the adversarial-time-travel branch 2 times, most recently from c6560db to 6803ace Compare April 8, 2021 09:22

pczarn added 3 commits April 14, 2021 20:03

Refactor time primitives

123ed45

Add adversarial time slowdown test

d1d8acd

* Tests for another use case as well: not stopping time, but instead slowing it down by 70%, and making sure that on the same (relatively long) period of time 70% fewer blocks are produced.

Fix for adversarial time travel

208b1a9

pczarn force-pushed the adversarial-time-travel branch from 4e2e671 to 208b1a9 Compare April 14, 2021 18:03

Remove proxify machinery

de48d9d

Removes code that is not necessary for typical operation. Makes time travel affect all proxied locations when enabled. You may want to revert this commit in case you'd wish to hunt for incorrect proxifications using `scripts/find_time_travel_setting.py`.

Remove proxy and system_time arguments that aren't needed

3cc40a6

pczarn force-pushed the adversarial-time-travel branch from bed4a63 to 3cc40a6 Compare April 15, 2021 08:01

ailisp approved these changes May 13, 2021

View reviewed changes

bowenwang1996 approved these changes May 13, 2021

View reviewed changes

bowenwang1996 added the S-donotmerge Status: do not merge this PR label May 17, 2021

olonho reviewed Jun 22, 2021

View reviewed changes

olonho suggested changes Jun 22, 2021

View reviewed changes

stale bot added the S-stale label Jul 6, 2021

ailisp mentioned this pull request Aug 9, 2021

Support time-travel / fast-forwarding near/near-workspaces-js#1

Open

stale bot removed the S-stale label Oct 29, 2021

stale bot added S-stale and removed S-stale labels Nov 12, 2021

bowenwang1996 closed this Nov 17, 2021

pmnoxx mentioned this pull request Nov 20, 2021

please ignore #5397

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement time travel for NEAR testing infrastructure #3661

Implement time travel for NEAR testing infrastructure #3661

pczarn commented Nov 28, 2020 •

edited by EgorKulikov

Loading

pczarn commented Nov 28, 2020

bowenwang1996 left a comment

bowenwang1996 Nov 30, 2020

pczarn Dec 3, 2020

pczarn commented Dec 3, 2020 •

edited

Loading

pczarn commented Jan 12, 2021

pczarn commented Jan 24, 2021

pczarn commented Mar 23, 2021 •

edited

Loading

pczarn commented Mar 31, 2021 •

edited

Loading

SkidanovAlex commented Apr 14, 2021

pczarn commented Apr 15, 2021

pczarn commented Apr 28, 2021 •

edited

Loading

bowenwang1996 May 13, 2021

SkidanovAlex commented May 14, 2021

olonho Jun 22, 2021

pczarn Oct 29, 2021 •

edited

Loading

pczarn Nov 13, 2021

olonho left a comment

stale bot commented Jul 6, 2021

stale bot commented Nov 12, 2021

pczarn commented Nov 13, 2021

bowenwang1996 commented Nov 15, 2021 •

edited

Loading

pczarn commented Nov 17, 2021 •

edited

Loading

pmnoxx commented Nov 17, 2021 •

edited

Loading

bowenwang1996 commented Nov 17, 2021

Implement time travel for NEAR testing infrastructure #3661

Implement time travel for NEAR testing infrastructure #3661

Conversation

pczarn commented Nov 28, 2020 • edited by EgorKulikov Loading

pczarn commented Nov 28, 2020

bowenwang1996 left a comment

Choose a reason for hiding this comment

bowenwang1996 Nov 30, 2020

Choose a reason for hiding this comment

pczarn Dec 3, 2020

Choose a reason for hiding this comment

pczarn commented Dec 3, 2020 • edited Loading

pczarn commented Jan 12, 2021

pczarn commented Jan 24, 2021

pczarn commented Mar 23, 2021 • edited Loading

pczarn commented Mar 31, 2021 • edited Loading

SkidanovAlex commented Apr 14, 2021

pczarn commented Apr 15, 2021

pczarn commented Apr 28, 2021 • edited Loading

bowenwang1996 May 13, 2021

Choose a reason for hiding this comment

SkidanovAlex commented May 14, 2021

olonho Jun 22, 2021

Choose a reason for hiding this comment

pczarn Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

pczarn Nov 13, 2021

Choose a reason for hiding this comment

olonho left a comment

Choose a reason for hiding this comment

stale bot commented Jul 6, 2021

stale bot commented Nov 12, 2021

pczarn commented Nov 13, 2021

bowenwang1996 commented Nov 15, 2021 • edited Loading

pczarn commented Nov 17, 2021 • edited Loading

pmnoxx commented Nov 17, 2021 • edited Loading

bowenwang1996 commented Nov 17, 2021

pczarn commented Nov 28, 2020 •

edited by EgorKulikov

Loading

pczarn commented Dec 3, 2020 •

edited

Loading

pczarn commented Mar 23, 2021 •

edited

Loading

pczarn commented Mar 31, 2021 •

edited

Loading

pczarn commented Apr 28, 2021 •

edited

Loading

pczarn Oct 29, 2021 •

edited

Loading

bowenwang1996 commented Nov 15, 2021 •

edited

Loading

pczarn commented Nov 17, 2021 •

edited

Loading

pmnoxx commented Nov 17, 2021 •

edited

Loading