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

[Merged by Bors] - Fork choice modifications and cleanup #3962

Closed
wants to merge 63 commits into from
Closed

[Merged by Bors] - Fork choice modifications and cleanup #3962

wants to merge 63 commits into from

Conversation

paulhauner
Copy link
Member

@paulhauner paulhauner commented Feb 9, 2023 •
edited
Loading

Issue Addressed

NA

Proposed Changes

The CountRealizedFull concept has been removed and the --count-unrealized-full and --count-unrealized BN flags now do nothing but log a WARN when used.

Database Migration Debt

This PR removes the best_justified_checkpoint from fork choice. This field is persisted on-disk and the correct way to go about this would be to make a DB migration to remove the field. However, in this PR I've simply stubbed out the value with a junk value. I've taken this approach because if we're going to do a DB migration I'd love to remove the Options around the justified and finalized checkpoints on ProtoNode whilst we're at it. Those options were added in #2822 which was included in Lighthouse v2.1.0. The options were only put there to handle the migration and they've been set to Some ever since v2.1.0. There's no reason to keep them as options anymore.

I started adding the DB migration to this branch but I started to feel like I was bloating this rather critical PR with nice-to-haves. I've kept the partially-complete migration over in my repo so we can pick it up after this PR is merged.

@paulhauner
Add new finalized checkpoint function
7d067a7
@paulhauner
Add test
c82ef87
@paulhauner
Tidy
791cf47
@paulhauner
Use new method
bff9028
@paulhauner
Add descriptive comment
a119edc
@paulhauner
Fix clippy lints
6590da5
@paulhauner
Rename function
c977fab
@paulhauner
Rename function, add test condition
b306f67
@paulhauner
Add shortcut method
9572135
@paulhauner
Improve loop-shortcutting method
e2312d2
@paulhauner
Update comments
89d7fd3
@paulhauner
Update fork_choice function name
dfc223c
@paulhauner
Fix todo
788ecd6
@paulhauner
Fix comment
0261cda
@paulhauner
Add some custom fc tests
f35d44b
@paulhauner
Add re-org test
db112c5
@paulhauner
Remove should_update_justified_checkpoint
656f958
@paulhauner
Sketch in new filter logic
0652e9b
@paulhauner
Fix clippy lints from unused safe-slots functions
011d6a8
@paulhauner
Add more filter logic
b02753f
@paulhauner
Update tests commit
0ee0328
@paulhauner
Add withholding tests
9554274
@paulhauner
Enable CountUnrealized
6af13e0
@paulhauner
Track slashed indices
8ad941d
@paulhauner
Tidy
7065c49
@paulhauner
Remove best justified checkpoint
e6d95da
@paulhauner
Tidy
fc1e17f
@paulhauner
Fix test compile errors
e7b26f1
@paulhauner
Fix after rebase
13fbfed
@paulhauner
Remove .DSStore from git ignore
529085c
@paulhauner
Copy link
Member Author

I had to do a little refactoring of JustifiedBalances to resolve a TODO(paul) that past Paul had rudely left for me.

All the changes are contained in 42a8f4f. It wasn't anything serious and I think it's an improvement.

@paulhauner paulhauner added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Mar 21, 2023
@michaelsproul michaelsproul mentioned this pull request Mar 21, 2023
Closed
@paulhauner paulhauner added the backwards-incompat Backwards-incompatible API change label Mar 21, 2023
paulhauner added a commit that referenced this pull request Mar 21, 2023
@paulhauner
Fork choice modifications and cleanup (#3962)
c7120b0 
Squashed commit of the following:

commit d2fefc4
Author: Paul Hauner <[email protected]>
Date:   Tue Mar 21 13:53:38 2023 +1100

    Tidy diff

commit 7cc1ab1
Author: Paul Hauner <[email protected]>
Date:   Tue Mar 21 12:55:44 2023 +1100

    Appease clippy

commit 9216bda
Author: Paul Hauner <[email protected]>
Date:   Tue Mar 21 12:22:28 2023 +1100

    Fix failing CLI test

commit 77a4970
Author: Paul Hauner <[email protected]>
Date:   Tue Mar 21 10:24:33 2023 +1100

    Add migration to drop balances cache

commit d47a3e4
Author: Paul Hauner <[email protected]>
Date:   Mon Mar 20 20:29:13 2023 +1100

    Fix test compilation issue

commit 5377ac6
Author: Paul Hauner <[email protected]>
Date:   Mon Mar 20 20:07:34 2023 +1100

    Censor slashed validators from JustifiedBalances

commit dd6109c
Author: Paul Hauner <[email protected]>
Date:   Mon Mar 20 19:00:28 2023 +1100

    Update equivocating indices in `process_state`

commit 43a8405
Author: Paul Hauner <[email protected]>
Date:   Mon Mar 20 18:19:31 2023 +1100

    Deprecate `count-unrealized` flag

commit 3f60f70
Author: Paul Hauner <[email protected]>
Date:   Mon Mar 20 17:40:33 2023 +1100

    Un-ignore skipped test files

commit 2dd0f6c
Author: Paul Hauner <[email protected]>
Date:   Mon Mar 20 17:40:11 2023 +1100

    Fix bug with `JustifiedBalances` slashed indices

commit 42a8f4f
Author: Paul Hauner <[email protected]>
Date:   Mon Mar 20 17:25:58 2023 +1100

    Remove `slashed_indices` field from `JustifiedBalances`

commit 913abc0
Author: Paul Hauner <[email protected]>
Date:   Mon Mar 20 15:23:36 2023 +1100

    Skip all FC tests with base/phase0

commit 023524b
Author: Paul Hauner <[email protected]>
Date:   Mon Mar 20 15:22:47 2023 +1100

    Skip specific phase0 FC tests

commit 558fe79
Author: Paul Hauner <[email protected]>
Date:   Mon Mar 20 15:14:09 2023 +1100

    Set tests to v1.3.0-rc.4

commit 7bbe895
Merge: d4a2b02 36e163c
Author: Paul Hauner <[email protected]>
Date:   Mon Mar 20 15:09:05 2023 +1100

    Merge branch 'unstable' into fc-pr-18

commit d4a2b02
Author: Paul Hauner <[email protected]>
Date:   Thu Mar 2 11:30:05 2023 +1100

    Remove some non-supplied tests

commit ff76ffa
Author: Paul Hauner <[email protected]>
Date:   Wed Mar 1 15:27:02 2023 +1100

    Switch to unified test format

commit d664cd8
Merge: 162e97c 17d9a62
Author: Paul Hauner <[email protected]>
Date:   Wed Mar 1 14:40:47 2023 +1100

    Merge branch 'unstable' into fc-pr-18

commit 162e97c
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 16 10:15:47 2023 +1100

    Bump test version:

commit 2796606
Author: Paul Hauner <[email protected]>
Date:   Mon Feb 13 17:10:00 2023 +1100

    Add comment about running tests

commit dfe73a2
Author: Paul Hauner <[email protected]>
Date:   Mon Feb 13 17:09:01 2023 +1100

    Remove commented out cfg flag

commit dcb8427
Author: Paul Hauner <[email protected]>
Date:   Mon Feb 13 16:54:14 2023 +1100

    Fix compile error

commit e9835aa
Author: Paul Hauner <[email protected]>
Date:   Mon Feb 13 16:52:52 2023 +1100

    Skip fork choice tests

commit 6972850
Author: Paul Hauner <[email protected]>
Date:   Mon Feb 13 16:50:02 2023 +1100

    Use junk justified checkpoint

commit a3e0f3e
Author: Paul Hauner <[email protected]>
Date:   Mon Feb 13 14:12:02 2023 +1100

    Fix test compile error

commit 1b761d6
Author: Paul Hauner <[email protected]>
Date:   Fri Feb 10 14:58:28 2023 +1100

    Remove count-unrealized-full

commit 0183f6e
Merge: 6fbf9e4 5276dd0
Author: Paul Hauner <[email protected]>
Date:   Fri Feb 10 14:42:44 2023 +1100

    Merge branch 'unstable' into fc-pr-18

commit 6fbf9e4
Author: Paul Hauner <[email protected]>
Date:   Fri Feb 10 14:40:58 2023 +1100

    Remove best just checkpoint from EF tests

commit 2349e80
Author: Paul Hauner <[email protected]>
Date:   Fri Feb 10 14:37:02 2023 +1100

    Remove references to best justified checkpoint in FC tests

commit 0bd58ed
Author: Paul Hauner <[email protected]>
Date:   Fri Feb 10 14:33:46 2023 +1100

    Fix failing test

commit b175574
Author: Paul Hauner <[email protected]>
Date:   Fri Feb 10 10:28:26 2023 +1100

    Hide custom FC tests behind flag, disable standard tests

commit 0b56a73
Author: Paul Hauner <[email protected]>
Date:   Fri Feb 10 09:31:04 2023 +1100

    Tidy

commit 1b4cde8
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 9 18:17:33 2023 +1100

    Refactor filter logic for tidiness

commit 529085c
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 9 15:37:00 2023 +1100

    Remove .DSStore from git ignore

commit 13fbfed
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 9 15:36:02 2023 +1100

    Fix after rebase

commit e7b26f1
Author: Paul Hauner <[email protected]>
Date:   Wed Feb 8 17:49:00 2023 +1100

    Fix test compile errors

commit fc1e17f
Author: Paul Hauner <[email protected]>
Date:   Wed Feb 8 15:27:49 2023 +1100

    Tidy

commit e6d95da
Author: Paul Hauner <[email protected]>
Date:   Wed Feb 8 11:38:27 2023 +1100

    Remove best justified checkpoint

commit 7065c49
Author: Paul Hauner <[email protected]>
Date:   Wed Feb 8 11:21:58 2023 +1100

    Tidy

commit 8ad941d
Author: Paul Hauner <[email protected]>
Date:   Tue Feb 7 18:37:12 2023 +1100

    Track slashed indices

commit 6af13e0
Author: Paul Hauner <[email protected]>
Date:   Tue Feb 7 18:04:48 2023 +1100

    Enable CountUnrealized

commit 9554274
Author: Paul Hauner <[email protected]>
Date:   Tue Feb 7 17:25:12 2023 +1100

    Add withholding tests

commit 0ee0328
Author: Paul Hauner <[email protected]>
Date:   Tue Feb 7 17:25:05 2023 +1100

    Update tests commit

commit b02753f
Author: Paul Hauner <[email protected]>
Date:   Tue Feb 7 17:15:33 2023 +1100

    Add more filter logic

commit 011d6a8
Author: Paul Hauner <[email protected]>
Date:   Tue Feb 7 16:52:33 2023 +1100

    Fix clippy lints from unused safe-slots functions

commit 0652e9b
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 2 15:08:17 2023 +1100

    Sketch in new filter logic

commit 656f958
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 2 14:31:32 2023 +1100

    Remove should_update_justified_checkpoint

commit db112c5
Author: Paul Hauner <[email protected]>
Date:   Wed Feb 1 15:48:19 2023 +1100

    Add re-org test

commit f35d44b
Author: Paul Hauner <[email protected]>
Date:   Wed Feb 1 15:45:00 2023 +1100

    Add some custom fc tests

commit 0261cda
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 9 15:28:02 2023 +1100

    Fix comment

commit 788ecd6
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 9 12:36:50 2023 +1100

    Fix todo

commit dfc223c
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 9 12:30:17 2023 +1100

    Update fork_choice function name

commit 89d7fd3
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 9 12:28:21 2023 +1100

    Update comments

commit e2312d2
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 9 12:23:26 2023 +1100

    Improve loop-shortcutting method

commit 9572135
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 9 12:07:07 2023 +1100

    Add shortcut method

commit b306f67
Author: Paul Hauner <[email protected]>
Date:   Thu Feb 9 12:06:58 2023 +1100

    Rename function, add test condition

commit c977fab
Author: Paul Hauner <[email protected]>
Date:   Fri Jan 27 17:57:36 2023 +0100

    Rename function

commit 6590da5
Author: Paul Hauner <[email protected]>
Date:   Fri Jan 27 17:54:25 2023 +0100

    Fix clippy lints

commit a119edc
Author: Paul Hauner <[email protected]>
Date:   Fri Jan 27 17:35:30 2023 +0100

    Add descriptive comment

commit bff9028
Author: Paul Hauner <[email protected]>
Date:   Fri Jan 27 16:20:23 2023 +0100

    Use new method

commit 791cf47
Author: Paul Hauner <[email protected]>
Date:   Fri Jan 27 16:07:37 2023 +0100

    Tidy

commit c82ef87
Author: Paul Hauner <[email protected]>
Date:   Fri Jan 27 16:05:11 2023 +0100

    Add test

commit 7d067a7
Author: Paul Hauner <[email protected]>
Date:   Fri Jan 27 14:54:08 2023 +0100

    Add new finalized checkpoint function
michaelsproul
michaelsproul approved these changes Mar 21, 2023
View reviewed changes
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

I've checked this against the spec and am satisfied that it's correct!

Let's ship!

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Mar 21, 2023
@paulhauner
Copy link
Member Author

Let's ship!

Woo, thanks for the review! Also thanks for the feedback @realbigsean 🙏

bors r+

bors bot pushed a commit that referenced this pull request Mar 21, 2023
@paulhauner
Fork choice modifications and cleanup (#3962)
1f8c17b 
## Issue Addressed

NA

## Proposed Changes

- Implements ethereum/consensus-specs#3290
- Bumps `ef-tests` to [v1.3.0-rc.4](https://github.com/ethereum/consensus-spec-tests/releases/tag/v1.3.0-rc.4).

The `CountRealizedFull` concept has been removed and the `--count-unrealized-full` and `--count-unrealized` BN flags now do nothing but log a `WARN` when used.

## Database Migration Debt

This PR removes the `best_justified_checkpoint` from fork choice. This field is persisted on-disk and the correct way to go about this would be to make a DB migration to remove the field. However, in this PR I've simply stubbed out the value with a junk value. I've taken this approach because if we're going to do a DB migration I'd love to remove the `Option`s around the justified and finalized checkpoints on `ProtoNode` whilst we're at it. Those options were added in #2822 which was included in Lighthouse v2.1.0. The options were only put there to handle the migration and they've been set to `Some` ever since v2.1.0. There's no reason to keep them as options anymore.

I started adding the DB migration to this branch but I started to feel like I was bloating this rather critical PR with nice-to-haves. I've kept the partially-complete migration [over in my repo](https://github.com/paulhauner/lighthouse/tree/fc-pr-18-migration) so we can pick it up after this PR is merged.
@bors
Copy link

bors bot commented Mar 21, 2023

Pull request successfully merged into unstable.

Build succeeded:

@bors bors bot changed the title Fork choice modifications and cleanup [Merged by Bors] - Fork choice modifications and cleanup Mar 21, 2023
@bors bors bot closed this Mar 21, 2023
@michaelsproul michaelsproul deleted the fc-pr-18 branch March 21, 2023 11:40
@paulhauner paulhauner mentioned this pull request Apr 26, 2023
Closed
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
@paulhauner @Woodpile37
Fork choice modifications and cleanup (sigp#3962)
53c0151 
NA

- Implements ethereum/consensus-specs#3290
- Bumps `ef-tests` to [v1.3.0-rc.4](https://github.com/ethereum/consensus-spec-tests/releases/tag/v1.3.0-rc.4).

The `CountRealizedFull` concept has been removed and the `--count-unrealized-full` and `--count-unrealized` BN flags now do nothing but log a `WARN` when used.

This PR removes the `best_justified_checkpoint` from fork choice. This field is persisted on-disk and the correct way to go about this would be to make a DB migration to remove the field. However, in this PR I've simply stubbed out the value with a junk value. I've taken this approach because if we're going to do a DB migration I'd love to remove the `Option`s around the justified and finalized checkpoints on `ProtoNode` whilst we're at it. Those options were added in sigp#2822 which was included in Lighthouse v2.1.0. The options were only put there to handle the migration and they've been set to `Some` ever since v2.1.0. There's no reason to keep them as options anymore.

I started adding the DB migration to this branch but I started to feel like I was bloating this rather critical PR with nice-to-haves. I've kept the partially-complete migration [over in my repo](https://github.com/paulhauner/lighthouse/tree/fc-pr-18-migration) so we can pick it up after this PR is merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twoeths twoeths twoeths left review comments

@michaelsproul michaelsproul michaelsproul approved these changes

Assignees
No one assigned
Labels
backwards-incompat Backwards-incompatible API change ready-for-merge This PR is ready to merge. v4.0.0 Mainnet Capella release expected late March 2023
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paulhauner @michaelsproul @twoeths