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

Update to consensus spec v1.4.0-beta.6 #5094

Merged
merged 7 commits into from
Feb 8, 2024
Merged

Update to consensus spec v1.4.0-beta.6 #5094

merged 7 commits into from
Feb 8, 2024

Conversation

realbigsean
Copy link
Member

Issue Addressed

Resolves #5083

Proposed Changes

Additional Info

@realbigsean realbigsean added ready-for-review The code is ready for review low-hanging-fruit Easy to resolve, get it before someone else does! deneb labels Jan 19, 2024
@realbigsean
Copy link
Member Author

Looks like a couple unit tests are broken

@realbigsean realbigsean added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels Jan 19, 2024
@realbigsean realbigsean added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Jan 20, 2024
@realbigsean
Copy link
Member Author

realbigsean commented Jan 20, 2024 •
edited
Loading

the new fork choice change introduces current_epoch which doesn't really make sense in the tests that were failing. I sort of hacked around it to make sure the tests always use justified epoch + 3 as the current epoch in order to make the latest fc filter change irrelevant to the tests. I'm wondering if these tests should just be deprecated though because we now have fork choice tests in the consensus spec.

@michaelsproul michaelsproul changed the title Update to conensus spec v1.4.0-beta.6 Update to consensus spec v1.4.0-beta.6 Jan 21, 2024
@realbigsean realbigsean added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jan 23, 2024
@realbigsean realbigsean added the v5.0.0 Q1 2024 label Jan 31, 2024
@paulhauner
Copy link
Member

I just pushed 1a06c72 which fixes the recover_from_invalid_head... tests.

@paulhauner
Copy link
Member

I reverted 0c875b0 and added 1beddec. I believe the latter is the appropriate fix since it precisely modifies the existing tests to be compatible with the new FC changes.

The fork choice definition tests that I've changed are very basic tests. I highly doubt they're of any value to consensus-specs, however I'm not certain enough that they're fully overlapping to delete these tests from our codebase.

@realbigsean realbigsean added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Feb 7, 2024
realbigsean
realbigsean commented Feb 7, 2024
View reviewed changes
Copy link
Member Author

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

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

Thanks for the help @paulhauner, love the new comments on the tests.

beacon_node/beacon_chain/tests/payload_invalidation.rs Outdated Show resolved Hide resolved
@realbigsean
Copy link
Member Author

@dapplion are you able to check this one out again?

dapplion
dapplion approved these changes Feb 8, 2024
View reviewed changes
Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Looks good! Appreciate the thorough comments

@realbigsean
Copy link
Member Author

@Mergifyio queue

@realbigsean realbigsean removed the ready-for-review The code is ready for review label Feb 8, 2024
@realbigsean realbigsean added the ready-for-merge This PR is ready to merge. label Feb 8, 2024
@mergify Mergify
Copy link

mergify bot commented Feb 8, 2024

queue

🛑 The pull request has been removed from the queue default

The pull request #5094 has been manually updated.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@realbigsean
Copy link
Member Author

@Mergifyio requeue

@mergify Mergify
Copy link

mergify bot commented Feb 8, 2024

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@realbigsean
Copy link
Member Author

@Mergifyio queue

@mergify Mergify
Copy link

mergify bot commented Feb 8, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 4172d9f

mergify bot added a commit that referenced this pull request Feb 8, 2024
@mergify
Merge of #5094
d613d78
@mergify mergify bot mentioned this pull request Feb 8, 2024
Closed
5 tasks
@mergify mergify bot merged commit 4172d9f into sigp:unstable Feb 8, 2024
29 checks passed
danielrachi1 pushed a commit to danielrachi1/lighthouse that referenced this pull request Feb 14, 2024
@realbigsean @danielrachi1
Update to consensus spec v1.4.0-beta.6 (sigp#5094)
eba34f0 
* get latest ef tests passing

* fix tests

* Fix invalid payload recovery tests

* Merge branch 'unstable' into update-to-spec-v1.4.0-beta.6

* Revert "fix tests"

This reverts commit 0c875b0.

* Fix fork choice def. tests

* Update beacon_node/beacon_chain/tests/payload_invalidation.rs
@michaelsproul michaelsproul mentioned this pull request Feb 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulhauner paulhauner paulhauner left review comments

@dapplion dapplion dapplion approved these changes

Assignees
No one assigned
Labels
deneb low-hanging-fruit Easy to resolve, get it before someone else does! ready-for-merge This PR is ready to merge. v5.0.0 Q1 2024
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@realbigsean @paulhauner @dapplion