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

NonFinalizedState: Finish implementation and testing according to RFC0005 #1137

Merged
merged 1 commit into from
Oct 9, 2020

Conversation

yaahc
Copy link
Contributor

@yaahc yaahc commented Oct 8, 2020

Motivation

This PR is tracking the follow up changes necessary to fully implement the
non-finalized state according to the state RFC and to test the implementation.

  • Correctly handle duplicate blocks in CommitBlock
  • test the best chain is selected correctly
  • test the best chain height is correctly returned
  • check that the right queued blocks are pruned

Related Issues

@yaahc yaahc changed the base branch from main to chainset-impl October 8, 2020 01:28
@yaahc yaahc changed the title Implement and test non-finalized state according to RFC0005 Finish implementation and testing of non-finalized state according to RFC0005 Oct 8, 2020
Base automatically changed from chainset-impl to main October 8, 2020 03:07
@teor2345

This comment has been minimized.

@yaahc yaahc force-pushed the non-finalized-state-finalization branch from 2de48ab to a296c6b Compare October 8, 2020 03:44
@yaahc yaahc changed the title Finish implementation and testing of non-finalized state according to RFC0005 NonFinalizedState: Finish implementation and testing according to RFC0005 Oct 8, 2020
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This refactor is good as it is, and I would like to merge it straight away.

(If we keep building on it, then any future reviews will have to read through this refactor code - even though we know it's good.)

I think the TODO list in this PR should actually become a tracking issue.

@teor2345
Copy link
Contributor

teor2345 commented Oct 8, 2020

I think the TODO list in this PR should actually become a tracking issue.

Or maybe a section in the RFC5 tracking issue - whatever works best for the developers and reviewers.

@teor2345
Copy link
Contributor

teor2345 commented Oct 9, 2020

This is good work, and it's an improvement, so I'm going to merge it now.

I moved the remaining TODOs into the state tracking issue #1049.

@teor2345 teor2345 merged commit b3634fa into main Oct 9, 2020
@teor2345 teor2345 deleted the non-finalized-state-finalization branch October 9, 2020 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants