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

Fix exit verification at fork boundaries #4184

Closed
paulhauner opened this issue Apr 13, 2023 · 3 comments
Closed

Fix exit verification at fork boundaries #4184

paulhauner opened this issue Apr 13, 2023 · 3 comments
Labels
consensus An issue/PR that touches consensus code, such as state_processing or block verification. v4.2.0 Q2 2023

Comments

@paulhauner
Copy link
Member

Description

In #4183 we made a patch to fix high CPU usage for exit verification across skip slots.

This patch doesn't cover the case where we receive an exit for epoch E + 1 when the head state is in epoch E and those two epochs are across a fork boundary.

@paulhauner paulhauner added the v4.1.0 Post-Capella minor release label Apr 13, 2023
@michaelsproul michaelsproul added the consensus An issue/PR that touches consensus code, such as state_processing or block verification. label Apr 13, 2023
@michaelsproul
Copy link
Member

I think the signature check is OK actually, because it uses the exit's internal epoch:

let domain = spec.get_domain(
exit.epoch,
Domain::VoluntaryExit,
&state.fork(),
state.genesis_validators_root(),
);

But the other checks relative to current epoch are liable to fail, e.g.

// Exits must specify an epoch when they become valid; they are not valid before then.
verify!(
state.current_epoch() >= exit.epoch,
ExitInvalid::FutureEpoch {
state: state.current_epoch(),
exit: exit.epoch
}
);

Plumbing an optional wall-clock epoch into verify_exit could be one way to verify future exits from the current head state

@paulhauner paulhauner added v4.2.0 Q2 2023 v4.1.0 Post-Capella minor release and removed v4.1.0 Post-Capella minor release v4.2.0 Q2 2023 labels Apr 13, 2023
@paulhauner
Copy link
Member Author

I've started working on a fix in #4183, so we might not need this issue after all.

bors bot pushed a commit that referenced this issue Apr 14, 2023
## Issue Addressed

NA

## Proposed Changes

Similar to #4181 but without the version bump and a more nuanced fix.

Patches the high CPU usage seen after the Capella fork which was caused by processing exits when there are skip slots.

## Additional Info

~~This is an imperfect solution that will cause us to drop some exits at the fork boundary. This is tracked at #4184.~~
@michaelsproul
Copy link
Member

Closed by #4183

ghost pushed a commit to oone-world/lighthouse that referenced this issue Jul 13, 2023
## Issue Addressed

NA

## Proposed Changes

Similar to sigp#4181 but without the version bump and a more nuanced fix.

Patches the high CPU usage seen after the Capella fork which was caused by processing exits when there are skip slots.

## Additional Info

~~This is an imperfect solution that will cause us to drop some exits at the fork boundary. This is tracked at sigp#4184.~~
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
## Issue Addressed

NA

## Proposed Changes

Similar to sigp#4181 but without the version bump and a more nuanced fix.

Patches the high CPU usage seen after the Capella fork which was caused by processing exits when there are skip slots.

## Additional Info

~~This is an imperfect solution that will cause us to drop some exits at the fork boundary. This is tracked at sigp#4184.~~
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. v4.2.0 Q2 2023
Projects
None yet
Development

No branches or pull requests

2 participants