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

[Merged by Bors] - Refine and test slashing protection semantics #1885

Closed
wants to merge 7 commits into from

Conversation

michaelsproul
Copy link
Member

Issue Addressed

Closes #1873

Proposed Changes

Fixes the bug in slashing protection import (#1873) by pruning the database upon import.

Also expands the test generator to cover this case and a few others which are under discussion here:

https://ethereum-magicians.org/t/eip-3076-validator-client-interchange-format-slashing-protection/4883

Additional Info

Depending on the outcome of the discussion on Eth Magicians, we can either wait for consensus before merging, or merge our preferred solution and patch things later.

@michaelsproul michaelsproul added work-in-progress PR is a work-in-progress database A0 labels Nov 11, 2020
@michaelsproul michaelsproul added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Nov 23, 2020
@michaelsproul
Copy link
Member Author

michaelsproul commented Nov 23, 2020

This is now ready for review.

Guide for reviewers: the most important parts of this PR are the changes to import_interchange_info. The main changes are:

  • Prune the signed_blocks and signed_attestations to the low watermark from the interchange file in order to solve the bug raised in Fix bug in slashing protection import #1873.
  • Allow partial import of an interchange file: ignore records that appear slashable wrt to the existing database, or the interchange file itself. This still leaves some sharp corners on the UX for re-imports, but they will have to be addressed in subsequent work. I've taken the approach of making the code as safe as possible while changing as little as possible, i.e. not introducing drastic changes just before mainnet. Some other interchange implementers are pursuing an import strategy that allows seemingly slashable data to be imported (with better UX + less manual intervention), but I'm hesitant to put this into Lighthouse without first thoroughly testing all of the corner cases.
  • Update the CLI command to output some summary information, so the user has some idea of the state of their database after the import, and direct them to a page in the Lighthouse book (and Discord) if they hit one of the sharp corners on import.

You can likely skim over several hundred lines of changes to the test generator and test runner.

@paulhauner paulhauner changed the base branch from master to unstable November 23, 2020 23:19
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.

Looks great, I read thought it twice and didn't find anything. I did skip the tests though.

Merge at will :)

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

bors r+

bors bot pushed a commit that referenced this pull request Nov 24, 2020
## Issue Addressed

Closes #1873

## Proposed Changes

Fixes the bug in slashing protection import (#1873) by pruning the database upon import.

Also expands the test generator to cover this case and a few others which are under discussion here:

https://ethereum-magicians.org/t/eip-3076-validator-client-interchange-format-slashing-protection/4883

## Additional Info

Depending on the outcome of the discussion on Eth Magicians, we can either wait for consensus before merging, or merge our preferred solution and patch things later.
@bors bors bot changed the title Refine and test slashing protection semantics [Merged by Bors] - Refine and test slashing protection semantics Nov 24, 2020
@bors bors bot closed this Nov 24, 2020
@michaelsproul michaelsproul deleted the refine-eip-3076 branch November 24, 2020 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0 database ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix bug in slashing protection import
2 participants