Skip to content

Add TermTransition and replace EpochTransition - #1519

Closed
foriequal0 wants to merge 3 commits into
CodeChain-io:masterfrom
foriequal0:feature/term-change
Closed

foriequal0 wants to merge 3 commits into
CodeChain-io:masterfrom
foriequal0:feature/term-change

Conversation

@foriequal0

@foriequal0 foriequal0 commented May 9, 2019 •

Copy link
Copy Markdown
Contributor

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

@majecty majecty left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@sgkim126

sgkim126 commented May 13, 2019 •

Copy link
Copy Markdown
Contributor

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 force-pushed the feature/term-change branch from f758595 to 81d7d0c Compare May 13, 2019 09:09
@foriequal0 foriequal0 removed the do-not-merge Do not merge (for mergify.io) label May 13, 2019
@sgkim126

Copy link
Copy Markdown
Contributor

@foriequal0 Please resolve the conflict

@foriequal0

Copy link
Copy Markdown
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