Skip to content

Do not prevote to the block with irrelevant generation time #1512

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

Merged

Conversation

HoOngEe
Copy link
Contributor

@HoOngEe HoOngEe commented May 3, 2019

If the proposal generation time is 30 seconds or more before from now or
if 5 seconds or more later from now, the block generation time is now considered irrelevant.

It closes #1505

@HoOngEe HoOngEe force-pushed the feature/do-not-precommit-past-future-block branch 3 times, most recently from 899444d to 210842b Compare May 7, 2019 10:26
@HoOngEe HoOngEe changed the title [WIP] Do not precommit to the block with irrelevant generation time [WIP] Do not prevote to the block with irrelevant generation time May 8, 2019
} else {
None
}
TwoThirdsMajority::Lock(locked_view, block_hash) if locked_view == &self.view => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that it's the same with the previous code. Is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, It's the pre-commit step. Do I need to separate it to another commit or just revert it ?

Copy link
Contributor

@sgkim126 sgkim126 May 8, 2019

Choose a reason for hiding this comment

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

Why do you change it if it doesn't change the behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I was changing the pre-commit step and it helps to introduce checking block generation time and rejecting if irrelevant. After I had reverted it, I thought it still helps because it simplifies the routine. But it's meaningless in the sense that it doesn't change the behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

And it makes code fragile when changing because you used the underscore arm.
When someone adds a new enumerator, the compiler knows where to fix it in the original code, but it doesn't know in this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I understand. I couldn't think about that.

@HoOngEe HoOngEe force-pushed the feature/do-not-precommit-past-future-block branch 2 times, most recently from 9205a94 to a72d782 Compare May 8, 2019 08:53
@HoOngEe HoOngEe changed the title [WIP] Do not prevote to the block with irrelevant generation time Do not prevote to the block with irrelevant generation time May 8, 2019
@HoOngEe
Copy link
Contributor Author

HoOngEe commented May 8, 2019

I made a English review request to @ScarletBlue on the codechain.yml cli-option help message.

@HoOngEe HoOngEe force-pushed the feature/do-not-precommit-past-future-block branch from a72d782 to 2ffd6da Compare May 9, 2019 02:25
@HoOngEe HoOngEe force-pushed the feature/do-not-precommit-past-future-block branch from 2ffd6da to 0ab67a6 Compare May 9, 2019 09:04
ScarletBlue
ScarletBlue previously approved these changes May 14, 2019
@sgkim126
Copy link
Contributor

@HoOngEe please resolve the conflicts

If the proposal generation time is 30 seconds or more before from now or
if 5 seconds or more later from now, the block generation time is now considered irrelevant.
@HoOngEe HoOngEe force-pushed the feature/do-not-precommit-past-future-block branch from 3c769e0 to 559fe91 Compare May 16, 2019 07:21
@HoOngEe HoOngEe force-pushed the feature/do-not-precommit-past-future-block branch from 559fe91 to ae6659a Compare May 16, 2019 07:30
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

@mergify mergify bot merged commit 1ccf0f2 into CodeChain-io:master May 16, 2019
@HoOngEe HoOngEe deleted the feature/do-not-precommit-past-future-block branch May 16, 2019 12:32
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.

Do not pre-vote for blocks generated in the past or future
4 participants