Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 22 commits
f9aa23f
232e901
c9f9492
85eb896
b70475a
9b0d8ed
f5b1eb1
691a2dc
f35d1a5
31a9397
0a4631b
d7370d0
671350d
a001fd2
6cb23f0
f6b18e6
df6fa4e
1eec9d2
f8cc241
cc3872c
61e168a
15836ad
864a20d
f9a507c
17a4518
b7d6fb4
5d2de98
b66850d
8adb77f
0158ad1
3e4f833
79e1a25
427652f
eab87db
07775d7
6a1903d
39f429b
7bc2983
bf98f33
832a604
7c53fdc
d6f3261
e3634c6
625e036
d9e674c
3e6aa18
7e0761d
98d1d50
2a1ca7f
8170d09
66adcad
83203a0
0d90208
c1a31d6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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.
Could you describe why this change is necessary? In general the relationship described here does not always hold.
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.
this updates the epoch height to after fast-forwarding the block height. This was the only way I could feasibly update it since
epoch_manager
is separate from everything else. Also, what cases does this not hold for? This is just for sandbox so it might not be necessary to account for themThere 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.
While it is guaranteed that the block height of the end of an epoch is at least
epoch_length
+ the height of the block in the beginning of the epoch, there is no guarantee on the upper bound. In fact, it is possible for a block height in one epoch to be arbitrarily large. Could you let me know the reason for modifying epoch height? What guarantees do you need to have here?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.
This should be fine since when we receive a fast-forward request, it happens in the middle of an epoch and the newly forward block height would trigger updating the epoch-height at an arbitrary block height.
The guarantees needed for epoch height updates would be what you mentioned at the end of an epoch:
epoch_length + height of block in beginning of the epoch
This is needed because what I mentioned earlier in the other post about producing blocks and simulating those. So after producing X amount of blocks (fast-forwarding), the user of a contract would intuitively assume that the epoch height also got updated to reflect X blocks being produced.
Also, correct me on any of these things, but that's my thinking at least
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.
Actually this should be done automatically as long as block height is more than
epoch_length
+ height of block in the beginning of the epoch. I don't think we need custom logic here.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.
hmm interesting. But wouldn't that mean the epoch-height is only incremented by 1? There was only logic I saw that incremented the epoch-height by 1 when the next epoch was in view:
nearcore/chain/epoch_manager/src/validator_selection.rs
Line 194 in 3149332
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.
Yes. Do you require it to be incremented by more than 1? If so, what is the use 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.
not so much that it needs to be incremented more than 1 right there, but the custom logic is what I'm saying allows for us to skip to the designated epoch height after fast forwarding.
But now I'm not sure what you're referring to then with the above statement. Is there some other logic that would allow us to jump to the new epoch height after fast forwarding? I didn't see anything and epoch height remained the same as if fast-forwarding the block height never happened
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 think you just need to produce three more blocks to end the epoch, which could be implemented as part of the sandbox
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.
It shouldn't need to produce more blocks to end the epoch though, but regardless I tested it without the custom logic and it's not producing what's expected after fast forwarding. I'm getting epoch height of 3, when it should be 503, after waiting for roughly 50-60 blocks to be produced. You can see for yourself with the following diff and running
python3 pytests/tests/sandbox/fast_forward.py
:And after including the custom logic again, the python tests show the correct epoch height of 503, so it's definitely needed here
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.
This logic looks quite dubious to me. Why do you need to change epoch info retroactively? Also we should have unit test for this.
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.
So, there's a clone that happens when we grab
next_epoch_info
. This is why we need to update it again. I'll try to add a unit test for this. Also, need to mention there's one caveat to this epoch update: since we fast-forward the block height in the middle of an epoch, that epoch height needs to be retroactively adjusted to reflect the fast-forwarded block height as well. Added comments mentioning it too