Skip to content
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

Add TermTransition and replace EpochTransition #1519

Closed

Conversation

foriequal0
Copy link
Contributor

@foriequal0 foriequal0 commented May 9, 2019

This change should compatible with the older version of node, or data.
Removing EpochTransition leave garbage on DB for existing DB.
TermTransition does nothing for now.

Merge after: #1508

@foriequal0 foriequal0 requested a review from majecty May 9, 2019 01:30
@majecty majecty added the do-not-merge Do not merge (for mergify.io) label May 9, 2019
majecty
majecty previously approved these changes May 9, 2019
Copy link
Contributor

@majecty majecty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sgkim126
Copy link
Contributor

sgkim126 commented May 13, 2019

Any reason for keeping the do-not-merge tag?

`is_epoch_begin` flag which is determined by the result of
`epoch_tranistion` call doesn't make any differences whether it is true
or false since `on_new_block`, `on_epoch_begin` does nothing on it. So
it is safe to remove and to replace with `is_term_begin` which also does
nothing for now for the further implementation.
Inserted `EpochTransition` is not read anymore and inserting it does not
change state root. So removing it doesn't introduce breaking change unless
reusing the key index.
`EpochTransition` is not used anymore after being replaced by
`TermTransition`
@foriequal0 foriequal0 removed the do-not-merge Do not merge (for mergify.io) label May 13, 2019
@sgkim126
Copy link
Contributor

@foriequal0 Please resolve the conflict

@foriequal0
Copy link
Contributor Author

'TermId' should be changed to a sequential number instead of block number. I'll fix it and update this PR.

@foriequal0 foriequal0 changed the title Add TermTransition and replace EpochTransition [WIP] Add TermTransition and replace EpochTransition May 15, 2019
@foriequal0 foriequal0 mentioned this pull request May 16, 2019
2 tasks
@foriequal0 foriequal0 closed this May 16, 2019
@foriequal0 foriequal0 changed the title [WIP] Add TermTransition and replace EpochTransition Add TermTransition and replace EpochTransition May 16, 2019
@foriequal0 foriequal0 deleted the feature/term-change branch May 17, 2019 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants