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

Run fork choice spec tests #2545

Closed
michaelsproul opened this issue Aug 27, 2021 · 1 comment
Closed

Run fork choice spec tests #2545

michaelsproul opened this issue Aug 27, 2021 · 1 comment
Labels
A1 major-task A significant amount of work or conceptual task. test improvement Improve tests

Comments

@michaelsproul
Copy link
Member

Description

We should add a test runner for the upstream fork choice tests: https://github.com/ethereum/consensus-specs/tree/dev/tests/formats/fork_choice

@michaelsproul michaelsproul added test improvement Improve tests major-task A significant amount of work or conceptual task. A1 labels Aug 27, 2021
bors bot pushed a commit that referenced this issue Nov 8, 2021
## Issue Addressed

Resolves #2545

## Proposed Changes

Adds the long-overdue EF tests for fork choice. Although we had pretty good coverage via other implementations that closely followed our approach, it is nonetheless important for us to implement these tests too.

During testing I found that we were using a hard-coded `SAFE_SLOTS_TO_UPDATE_JUSTIFIED` value rather than one from the `ChainSpec`. This caused a failure during a minimal preset test. This doesn't represent a risk to mainnet or testnets, since the hard-coded value matched the mainnet preset.

## Failing Cases

There is one failing case which is presently marked as `SkippedKnownFailure`:

```
case 4 ("new_finalized_slot_is_justified_checkpoint_ancestor") from /home/paul/development/lighthouse/testing/ef_tests/consensus-spec-tests/tests/minimal/phase0/fork_choice/on_block/pyspec_tests/new_finalized_slot_is_justified_checkpoint_ancestor failed with NotEqual:
head check failed: Got Head { slot: Slot(40), root: 0x9183dbaed4191a862bd307d476e687277fc08469fc38618699863333487703e7 } | Expected Head { slot: Slot(24), root: 0x105b49b51bf7103c182aa58860b039550a89c05a4675992e2af703bd02c84570 }
```

This failure is due to #2741. It's not a particularly high priority issue at the moment, so we fix it after merging this PR.
@paulhauner
Copy link
Member

Resolved in #2737

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A1 major-task A significant amount of work or conceptual task. test improvement Improve tests
Projects
None yet
Development

No branches or pull requests

2 participants