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

Implement slash protection #254

Closed
paulhauner opened this issue Feb 23, 2019 · 12 comments · Fixed by #1116
Closed

Implement slash protection #254

paulhauner opened this issue Feb 23, 2019 · 12 comments · Fixed by #1116
Assignees
Labels
val-client Relates to the validator client binary

Comments

@paulhauner
Copy link
Member

Description

The validator client does not store the messages it signs and is therefore vulnerable to generating slashable messages

Steps to resolve

  • Determine a suitable database format.
  • Ensure all signed messages are stored in the database.
  • Ensure that newly signed messages are not in conflict with any previously signed messages.

Additional Info

Database formats

We want this database to be validator specific and portable. For example, I want to be able to move my validator from one machine to another, whilst maintaining that validators message history. As such, I suggest each validator has their own database, stored alongside their keys and config info (see #253 for suggestions on directory structure).

We should choose some single-process database that has high resistance to corruption.

@paulhauner paulhauner added the val-client Relates to the validator client binary label Feb 23, 2019
@paulhauner paulhauner changed the title VC: Implement slash protection Implement slash protection Feb 23, 2019
@g-r-a-n-t
Copy link
Contributor

Hey @paulhauner, this something I could pick up?

@paulhauner
Copy link
Member Author

Hey, thanks for reaching out.

@AgeManning is about to start a refactor of the validator client (primarily making it more async) so it's perhaps not the best time to start coding on this.

However, there's a bit of research that needs to happen first. Primarily about designing the schema of the database so it's space and time efficient. I'd be happy to work with you if you want to start thinking about data structures?

@g-r-a-n-t
Copy link
Contributor

@paulhauner Yeah, that sounds good. Maybe after I've done a bit of digging on my own we could hop on a call and discuss what makes sense.

@paulhauner
Copy link
Member Author

Sounds good!

@paulhauner
Copy link
Member Author

I had a think about this. I think we can make a maximally safe (but not profit maximizing) system given the following rules:

  • Unless it is the validators first attestation, every attestation must have an attestation.data.target.epoch that is greater than it's previous attestation.

  • Unless it is the validators first block proposal, every block.slot must be in a greater epoch than it's previous block proposal.

I'm in favor of using this solution for now, unless we can easily think of a better solution.

To suit this, we only need to store:

struct ValidatorHistory {
  /// The `attestation.data.target.epoch` of the validators most recent attestation.
  previous_attestation_target_epoch: Option<Epoch>,
  /// The `epoch(block.slot)` of the validators most recent block proposal.
  previous_block_proposal_epoch: Option<Epoch>
}

I think we should store these parameters in a very simple file, perhaps just the SSZ representation of the above struct.

The validator client current stores it's keys in ~/.lighthouse-validator, one directory per validator. I think we should put that ValidatorHistory file in there, along with a lockfile. The lockfile should effectively restrict read/write of a validators directory to only one running lighthouse process.

@paulhauner
Copy link
Member Author

@michaelsproul you might find this interesting :)

@g-r-a-n-t
Copy link
Contributor

Would it be an improvement to only read the file upon initialization of the client and then use a threadsafe ValidatorHistory object stored in memory from then on, only writing to the file when it's updated or upon termination. I would imagine blocking messages on reading and writing to this file could be slow. Looking at the client code tho, I'm not sure where this would be stored.

@paulhauner
Copy link
Member Author

This sounds like a sensible optimization!

@g-r-a-n-t
Copy link
Contributor

Alright, well, sounds like that's pretty much set :) I'll get to coding this whenever the time is right.

@michaelsproul
Copy link
Member

michaelsproul commented Oct 18, 2019

Re: @paulhauner

I had a think about this. I think we can make a maximally safe (but not profit maximizing) system given the following rules:

  • Unless it is the validators first attestation, every attestation must have an attestation.data.target.epoch that is greater than it's previous attestation.

  • Unless it is the validators first block proposal, every block.slot must be in a greater epoch than it's previous block proposal.

I agree with these two maxims, but we need an extra one to guard against surround votes:

  • Every attestation must have an attestation.data.source.epoch that is greater than or equal to its previous attestation.

Otherwise you might sign an attestation which surrounds a previous one. The existing rules only guard against signing a new attestation surrounded by a previous one.

@paulhauner
Copy link
Member Author

@michaelsproul, agreed. I was wrong.

I think it's safe to invalidate anything I've said here and work off the docs created by @pscott (I linked you to them in chat) :)

@michaelsproul
Copy link
Member

No worries, I just necrobumped this issue while trying to remember what the minimal safe algorithm was when reviewing Scott's algo.

@paulhauner paulhauner added this to To Do in Security Review Mar 2, 2020
@paulhauner paulhauner moved this from To Do to In Progress in Security Review Mar 2, 2020
@michaelsproul michaelsproul self-assigned this May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
val-client Relates to the validator client binary
Projects
No open projects
Security Review
Work in Progress
Development

Successfully merging a pull request may close this issue.

3 participants