Skip to content

Conversation

michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented May 31, 2023

Issue Addressed

Closes #4332

Proposed Changes

Remove the CountUnrealized type, defaulting unrealized justification to on. This fixes the #4332 issue by ensuring that importing the same block to fork choice always results in the same outcome.

Finalized sync speed may be slightly impacted by this change, but that is deemed an acceptable trade-off until the optimisation from #4118 is implemented.

TODO:

  • Also check that the block isn't a duplicate before importing

@michaelsproul michaelsproul added the work-in-progress PR is a work-in-progress label May 31, 2023
@michaelsproul michaelsproul added ready-for-review The code is ready for review consensus An issue/PR that touches consensus code, such as state_processing or block verification. v4.3.0 Estimated Q2 2023 and removed work-in-progress PR is a work-in-progress labels Jun 1, 2023
@paulhauner
Copy link
Member

@michaelsproul do you want to go ahead with a review without the duplicate check TODO?

@paulhauner paulhauner self-requested a review June 1, 2023 06:20
@paulhauner paulhauner self-assigned this Jun 1, 2023
@michaelsproul
Copy link
Member Author

I kinda added that todo as I was putting it up, maybe let's wait until I get to it before reviewing. Soz

@michaelsproul michaelsproul added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels Jun 1, 2023
@michaelsproul michaelsproul added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Jun 7, 2023
@michaelsproul
Copy link
Member Author

Ready for review now assuming CI passes

@jimmygchen
Copy link
Member

Should we also remove the deprecated count-unrealized & count-unrealized-full cli params since we're removing the code completely?

@michaelsproul
Copy link
Member Author

michaelsproul commented Jun 9, 2023

Should we also remove the deprecated count-unrealized & count-unrealized-full cli params since we're removing the code completely?

Usually we wait a few releases with the flags retained as no-ops, so people have some time to remove the flags from their setups. On that topic there are a lot of older deprecated flags lying around that we could safely delete now.

Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Looks great! and tests are passing 🚀

Co-authored-by: Jimmy Chen <jchen.tc@gmail.com>
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Nice one, LGTM!

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

bors r+

bors bot pushed a commit that referenced this pull request Jun 16, 2023
## Issue Addressed

Closes #4332

## Proposed Changes

Remove the `CountUnrealized` type, defaulting unrealized justification to _on_. This fixes the #4332 issue by ensuring that importing the same block to fork choice always results in the same outcome.

Finalized sync speed may be slightly impacted by this change, but that is deemed an acceptable trade-off until the optimisation from #4118 is implemented.

TODO:

- [x] Also check that the block isn't a duplicate before importing
@bors
Copy link

bors bot commented Jun 16, 2023

@bors bors bot changed the title Remove CountUnrealized [Merged by Bors] - Remove CountUnrealized Jun 16, 2023
@bors bors bot closed this Jun 16, 2023
ghost pushed a commit to oone-world/lighthouse that referenced this pull request Jul 13, 2023
## Issue Addressed

Closes sigp#4332

## Proposed Changes

Remove the `CountUnrealized` type, defaulting unrealized justification to _on_. This fixes the sigp#4332 issue by ensuring that importing the same block to fork choice always results in the same outcome.

Finalized sync speed may be slightly impacted by this change, but that is deemed an acceptable trade-off until the optimisation from sigp#4118 is implemented.

TODO:

- [x] Also check that the block isn't a duplicate before importing
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
Closes sigp#4332

Remove the `CountUnrealized` type, defaulting unrealized justification to _on_. This fixes the sigp#4332 issue by ensuring that importing the same block to fork choice always results in the same outcome.

Finalized sync speed may be slightly impacted by this change, but that is deemed an acceptable trade-off until the optimisation from sigp#4118 is implemented.

TODO:

- [x] Also check that the block isn't a duplicate before importing
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
Closes sigp#4332

Remove the `CountUnrealized` type, defaulting unrealized justification to _on_. This fixes the sigp#4332 issue by ensuring that importing the same block to fork choice always results in the same outcome.

Finalized sync speed may be slightly impacted by this change, but that is deemed an acceptable trade-off until the optimisation from sigp#4118 is implemented.

TODO:

- [x] Also check that the block isn't a duplicate before importing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus An issue/PR that touches consensus code, such as state_processing or block verification. ready-for-merge This PR is ready to merge. v4.3.0 Estimated Q2 2023
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants