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.

Sign up for GitHub

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

Jump to bottom

Reduce chance of Commits in different views if a node restarts. #643

Closed
wants to merge 49 commits into from
Closed

Reduce chance of Commits in different views if a node restarts. #643

wants to merge 49 commits into from

Conversation

jsolman
Copy link
Contributor

@jsolman jsolman commented Mar 19, 2019

If a node restarts and comes back and there have been commits in different views. This will cause the node to restore into the view it was on before it was restarted, thus minimizing the chance of a stall due to split Commit in different views.

@jsolman jsolman requested a review from erikzhang March 19, 2019 03:34
@jsolman jsolman changed the title Reduce chance Commits in different views on node restart. Reduce chance Commits in different views if a node restarts. Mar 19, 2019
@jsolman jsolman changed the title Reduce chance Commits in different views if a node restarts. Reduce chance of Commits in different views if a node restarts. Mar 19, 2019
shargon
shargon previously approved these changes Mar 20, 2019
View reviewed changes
erikzhang and others added 8 commits March 21, 2019 12:39
@jsolman
Copy link
Contributor Author

jsolman commented Mar 22, 2019

I've tested these changes and they are working as expected.

@jsolman jsolman mentioned this pull request Mar 23, 2019
Merged
@vncoelho vncoelho mentioned this pull request Mar 25, 2019
Closed
vncoelho
vncoelho reviewed Mar 25, 2019
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@erikzhang @jsolman @shargon, is this PR safe? I did not test it but it looks clear.
I think it is useful, thus, let's merge this inside 642.

@erikzhang erikzhang changed the base branch from consensus/preventStall to master March 25, 2019 15:52
shargon
shargon previously approved these changes Mar 25, 2019
View reviewed changes
@erikzhang
Copy link
Member

Don't merge before NGD test.

@vncoelho
Copy link
Member

vncoelho commented Mar 25, 2019 •
edited
Loading

I built and tested here and, then, saw the approved.
I almost clicked, @erikzhang, 5s more...aehauehauea

@vncoelho
Copy link
Member

How about this PR, @jsolman and @erikzhang?

@erikzhang
Copy link
Member

Is this PR still necessary now?

@vncoelho
Copy link
Member

for this current moment no...
but it looks like to be a good extra protection

@vncoelho
Copy link
Member

In the literature, hardware faults are well considered. I think we should think ahead.

@jsolman
Copy link
Contributor Author

jsolman commented Apr 19, 2019

Not really needed. Closing.

@jsolman jsolman closed this Apr 19, 2019
@jsolman
Copy link
Contributor Author

jsolman commented Apr 19, 2019 •
edited
Loading

Feel free to re-open if you want it as a precaution, but since the commits get locked, it isn’t really needed. If the commits get split node operators will be able to tell from their logs.

@shargon shargon deleted the consensus/preventStall2 branch July 25, 2019 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vncoelho vncoelho vncoelho left review comments

@shargon shargon shargon left review comments

@erikzhang erikzhang Awaiting requested review from erikzhang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jsolman @erikzhang @vncoelho @shargon