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

Finality testing #989

Merged
merged 9 commits into from
Apr 24, 2019
Merged

Finality testing #989

merged 9 commits into from
Apr 24, 2019

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Apr 23, 2019

Add tests to explicitly test each of the 4 finality rules.
These are pretty heavy wrt timing but very important tests.

Note, built off of #988 so merge in #988 before review

@djrtwo djrtwo requested a review from hwwhww April 24, 2019 03:04
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

The logic looks good to me!

Two possible refactoring approaches:

  1. Moving the assertions of changed finalized_epoch to check_finality. For example:

    def check_finality(state,
                    prev_state,
                    current_justified_changed,
                    previous_justified_changed,
                    finalized_is_current_justified,
                    finalized_is_previous_justified):
    
        ...
    
        if finalized_is_current_justified:
            assert state.finalized_epoch == prev_state.current_justified_epoch
            assert state.finalized_root == prev_state.current_justified_root
        elif finalized_is_previous_justified:
            assert state.finalized_epoch == prev_state.previous_justified_epoch
            assert state.finalized_root == prev_state.previous_justified_root
        else:
            assert state.finalized_epoch == prev_state.finalized_epoch
            assert state.finalized_root == prev_state.finalized_root
  2. As Decouple justification and finalization processing #925 decouples some justification and finalization, we can use old_previous_justified_epoch and old_current_justified_epoch in check_finality for improving readability when comparing with the spec.

test_libs/pyspec/tests/test_finality.py Outdated Show resolved Hide resolved
test_libs/pyspec/tests/test_finality.py Outdated Show resolved Hide resolved
test_libs/pyspec/tests/test_finality.py Outdated Show resolved Hide resolved
test_libs/pyspec/tests/test_finality.py Outdated Show resolved Hide resolved
test_libs/pyspec/tests/test_finality.py Outdated Show resolved Hide resolved
test_libs/pyspec/tests/test_finality.py Outdated Show resolved Hide resolved
test_libs/pyspec/tests/test_finality.py Outdated Show resolved Hide resolved
@djrtwo djrtwo merged commit 129e468 into dev Apr 24, 2019
@djrtwo djrtwo deleted the finality-testing branch April 24, 2019 17:56
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.

2 participants