make block time configurable to sim contract tests that depend on env::block_time

[ailisp]

This is the case in one of my contract, that I need to check if env::block_time have a day, a month or a year after previously recorded timestamp. Simulate actual block produce time need to many blocks

[matklad]

Unrelated to the PR, but it this intended to be a public API that the user calls manually? From the looks of it, feels like it is not. Then, this shouldn't be a pub fn.

If it is the public API, we probably should avoid breaking it.

[ailisp]

Yes this is not a public api

[austinabell]

How flexible can changes be made? It's unclear to me why the block is forced to be cloned, when the only use just drops the values.

It's definitely a low priority and probably won't slow anything down until this mocked block size increases and might be optimized away anyway, but either moving the block into an Arc/produce or to something like austinabell@923ddcc (made on top of @matklad's changes in other PR) might be worthwhile if it is needed for extensive simulations?

[matklad]

Yes this is not a public api

We should mark this as pub(crate) in a separate PR then, and, more generally, make sure that non-public API isn't actually public. At the moment, this function is exposed to consumers of sdk-sim.

@austinabell jfyi, a slightly more rustic way to do what you want would be something like this:

diff --git a/near-sdk-sim/src/runtime.rs b/near-sdk-sim/src/runtime.rs
index 3abd685..66ae827 100644
--- a/near-sdk-sim/src/runtime.rs
+++ b/near-sdk-sim/src/runtime.rs
@@ -111,12 +111,12 @@ impl Block {
         }
     }
 
-    pub fn produce(&self, new_state_root: CryptoHash, epoch_length: u64) -> Block {
+    pub fn produce(self, new_state_root: CryptoHash, epoch_length: u64) -> Block {
         Self {
             gas_price: self.gas_price,
             gas_limit: self.gas_limit,
             block_timestamp: self.block_timestamp + 1_000_000_000,
-            prev_block: Some(Box::new(self.clone())),
+            prev_block: Some(Box::new(self)),
             state_root: new_state_root,
             block_height: self.block_height + 1,
             epoch_height: (self.block_height + 1) / epoch_length,
@@ -275,7 +275,8 @@ impl RuntimeStandalone {
         let (update, _) =
             self.tries.apply_all(&apply_result.trie_changes, 0).expect("Unexpected Storage error");
         update.commit().expect("Unexpected io error");
-        self.cur_block = self.cur_block.produce(apply_result.state_root, self.genesis.epoch_length);
+        let cur_block = std::mem::take(&mut self.cur_block);
+        self.cur_block = cur_block.produce(apply_result.state_root, self.genesis.epoch_length);
 
         Ok(())
     }

I considered doing that for #385 , but decided that making the Clone itself cheap (and non stack overflowing) is a better fix, as it makes the Block type into a more well-behaved thing.

[austinabell]

@austinabell jfyi, a slightly more rustic way to do what you want would be something like this:
...
I considered doing that for #385 , but decided that making the Clone itself cheap (and non stack overflowing) is a better fix, as it makes the Block type into a more well-behaved thing.

Ah, yeah that's what I was referring to in my former suggestion, I just linked the latter because I already played around with it to see what it would look like because it was less clear.

I only suggested because, although these simulated blocks may never get large, avoiding moving around the block on the stack to instead just allocate the block once and increment the arc when produce is called could be preferable in very large sims.

I don't mean to bikeshed, as I was just throwing out an idea, but figured I'd give some context :)

[matklad]

Oh dear, so we actually have near_sdk::Duration which is defined as type Duration = u64 instead of using core::time::Duration 😨 ? Seems like we should fix that separatelly, added to #387

[austinabell]

I guess that depends on if it's important to have seconds and nanoseconds separated, what would you consider the fix, migrating to core::time::Duration or creating a new type? My assumption is this is used to have a more compact default serialization without having to convert from nanos to secs/nanos.

Also to note that the default serde impl for Duration is a map type (https://docs.serde.rs/src/serde/ser/impls.rs.html#595-606) and borsh doesn't have an impl yet

[matklad]

Good observations! I think another complication is that, ultimately, the source of time is the block_timestamp ABI call, and we can't plug that into std::time::SystemTime::now or . So, we do seem to have to define our own time at least. I'd suggest the following API:

pub mod time {
    pub struct ChainTime { ns_since_epoch: u64 }
    impl ChainTime {
        pub const UNIX_EPOCH: ChainTime;
        pub fn current_block() -> ChainTime;
        ...
    }
    pub use core::time::Duration;
}

I am torn on whether we should re-use std Duration or roll our own. I'd say "of course, we should use std", but for the two issues:

• In our ABI, we restrict time to be u64 (500 years or so), std's duration can span longer than that. So, we might have some impedance mismatch there.
• for JSON, duration serializes seconds as u64, and we try to avoid u64 in json

I don't see borsh as a problem -- it's easy enough to add impl Borsh for core::time::Duration.

I guess, if we already have contracs serializing duration in json as string of nanos, it makes sense to roll our own maybe.

[austinabell]

• for JSON, duration serializes seconds as u64, and we try to avoid u64 in json

Why avoid u64 in json? Isn't that what it is now for the sdk::Duration? serde_json supports u64, am I not aware of some cases where the 64-bit precision is lost?

I don't see borsh as a problem -- it's easy enough to add impl Borsh for core::time::Duration.

Agreed, just noting that you need to convert from seconds/nanos to just nanos during serialization if you want it as a single value

I guess, if we already have contracs serializing duration in json as string of nanos, it makes sense to roll our own maybe.

I think I might be missing how u64 is serialized as a string. serde_json by default serializes as an integer afaik, and I don't see where this is overridden in the sdk.

[matklad]

@austinabell see https://github.com/matklad/near-sdk-rs/blob/97fc632fcc58eb7ff7faad0be54ea8ec91dbf694/near-sdk/src/json_types/mod.rs#L15-L17

serde does serialize u64 as a string of digits just fine, but, when you try to read that back from JavaScript, you silently loose precision, as JavaScript doesn't have u64, it only has f64

[austinabell]

ohhh my mistake, I didn't notice the capital U, thanks for the clarification

[ailisp]

JavaScript doesn't have u64, it only has f64

This is correct, it does silently lost precision.
json standard (rfc-7159) only guarantee int up to 2**53-1:

Note that when such software is used, numbers that are integers and
are in the range [-(2**53)+1, (2**53)-1] are interoperable in the
sense that implementations will agree exactly on their numeric values.

In nearcore we have nano second in u64 serialized to genesis.json, to ensure u64 safety, it must be marked with u64_dec_format to force it's serialized to string instead of int:

#[serde(with = "u64_dec_format")] pub timestamp_nanosec: u64,

And, in this case, read genesis.json from any JSON parser in any language will not lose precision, because it's a string