Fix the trimming of the record of the first block height of each epoch #1898
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brentstone
force-pushed
the
brent/fix-epoch-trimming
branch
2 times, most recently
from
f7698db to
7c8dfb0
Compare
brentstone
commented
Sep 16, 2023
tzemanovic
reviewed
Sep 18, 2023
brentstone
force-pushed
the
brent/fix-epoch-trimming
branch
from
7c8dfb0 to
7113b8b
Compare
|
pls update wasm |
Closed
Fraccaman
previously approved these changes
Sep 20, 2023
tzemanovic
reviewed
Sep 21, 2023
core/src/types/storage.rs
Outdated
| /// Look-up the epoch of a given block height. | ||
| /// Look up the epoch of a given block height. | ||
| /// TODO: handle passing of a future block height? (only valid for current | ||
| /// or past) |
There was a problem hiding this comment.
We can't determine future epochs because epoch progression depends on 2 params - min. num of blocks and min duration. I think if the given block_height > current_height we can return None
There was a problem hiding this comment.
ah, nvm, we don't have enough context here to check current height. The current behavior is that we return the current epoch, which is probably fine
There was a problem hiding this comment.
Yeah, I put this here because it doesn't rly make sense to pass a future block height into this, but figured if this function were to eventually also take in the current block height, we could return None perhaps
tzemanovic
reviewed
Sep 21, 2023
tzemanovic
previously approved these changes
Sep 21, 2023
brentstone
dismissed stale reviews from tzemanovic and Fraccaman
via
4a83bbe
brentstone
added a commit
that referenced
this pull request
Sep 21, 2023
* brent/fix-epoch-trimming: fixup! remove unnecessary `Epochs` methods changelog: #1898 remove unnecessary `Epochs` methods fix tests remove trimming of `pred_epochs`
Fraccaman
added a commit
that referenced
this pull request
Sep 25, 2023
* origin/brent/fix-epoch-trimming: fixup! remove unnecessary `Epochs` methods changelog: #1898 remove unnecessary `Epochs` methods fix tests remove trimming of `pred_epochs`
Fraccaman
added a commit
that referenced
this pull request
Sep 25, 2023
* origin/brent/fix-epoch-trimming: fixup! remove unnecessary `Epochs` methods changelog: #1898 remove unnecessary `Epochs` methods fix tests remove trimming of `pred_epochs`
|
Merged into v0.23.0 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Describe your changes
Closes #1897. To resolve the issue, we no longer trim this data and now keep the first block height of every past epoch in storage.
Indicate on which release or other PRs this topic is based on
Based on v0.22.0
Checklist before merging to
draft