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

re-enable forkchoice/on_block and disabling only non working tests #4447

Merged

Conversation

tbenr
Copy link
Contributor

@tbenr tbenr commented Oct 5, 2021

PR Description

re-enable fork_choice/on_block reference test types.
Introducing per test exclusion.

ForkChoiceTestExecutor instance for fork_choice/on_block has been configured to skip:

  • new_finalized_slot_is_justified_checkpoint_ancestor
  • new_justified_is_later_than_store_justified

Documentation

  • I thought about documentation and added the documentation label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@tbenr tbenr changed the title re-enable forkchoice/onblock and disabling only new_finalized_slot_is_justified_checkpoint_ancestor re-enable forkchoice/onblock and disabling only non working tests Oct 5, 2021
@tbenr tbenr changed the title re-enable forkchoice/onblock and disabling only non working tests re-enable forkchoice/on_block and disabling only non working tests Oct 5, 2021
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM.

added "new_justified_is_later_than_store_justified" in the "fork_choice/on_block" exclusion list
@tbenr tbenr force-pushed the skip_only_specific_fork_choiche_tests branch from ecaffdf to 9123470 Compare October 6, 2021 08:05
@ajsutton ajsutton enabled auto-merge (squash) October 8, 2021 04:35
@ajsutton ajsutton merged commit 78f1e1f into Consensys:master Oct 8, 2021
"fork_choice/on_block",
new ForkChoiceTestExecutor(
"new_finalized_slot_is_justified_checkpoint_ancestor",
"new_justified_is_later_than_store_justified"))
Copy link

Choose a reason for hiding this comment

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

Hey @ajsutton @tbenr
I think new_justified_is_later_than_store_justified was fixed in ethereum/consensus-specs#2577
Do you mind checking again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hwwhww still not working. I run against freshly downloaded 1.1.3, is it correct?

Copy link

Choose a reason for hiding this comment

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

The fix was included since v1.1.0-beta.4. I think @ajsutton once verified Teku works with my tarball files locally in September?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hwwhww the weird thing is that when I run that locally it works. Not yet figured out whats going on in CI...

Copy link
Contributor

Choose a reason for hiding this comment

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

So locally I see new_finalized_slot_is_justified_checkpoint_ancestor failing but my understanding is it winds up triggering the same condition as new_justified_is_later_than_store_justified where the newly imported block is not considered the head even though it just caused the update in justification/finalization.

So I think the tests are right and Teku is incorrect in both cases. Though I think Teku's result actually makes far more sense than the spec result.

@tbenr tbenr deleted the skip_only_specific_fork_choiche_tests branch October 21, 2021 11:09
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.

None yet

3 participants