-
Notifications
You must be signed in to change notification settings - Fork 959
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
Remove equivocating validators from fork choice consideration #2845
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice! a couple of minor suggestions
attestation_1 = attester_slashing.attestation_1 | ||
attestation_2 = attester_slashing.attestation_2 | ||
assert is_slashable_attestation_data(attestation_1.data, attestation_2.data) | ||
state = store.block_states[store.justified_checkpoint.root] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use the state of the justified block here? It seems strange, as the slashing will have been verified previously against the head state and may fail verification here, or may verify with a different set of slashable indices. In Lighthouse I'm using the head state for this reason, but can change to justified if necessary
Lighthouse comment about this: https://github.com/sigp/lighthouse/blob/94bc85e1f731217c113ff79b427cdeb73b3309cd/consensus/fork_choice/src/fork_choice.rs#L1108-L1116
Lighthouse PR: sigp/lighthouse#3371
cc @adiasg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use the state of the justified block here?
To maintain uniformity with the fork choice - the validator set at the justified state is used for computing the fork choice.
It seems strange, as the slashing will have been verified previously against the head state and may fail verification here, or may verify with a different set of slashable indices.
The intention of on_attester_slashing
is to censor equivocating validators as soon as an implicating AttesterSlashing
is received by the node, irrespective of whether those validators are slashable. We are not checking is_slashable_validator
here (which is quite dependent on the state!), rather only is_slashable_attestation_data
& is_valid_indexed_attestation
which do not have much state dependence.
The Lighthouse PR is adding to store.equivocating_indices
only those validator indices from attester_slashing
that are slashable in the head state. This should be changed to remove the slashability check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Sorry I don't know what got into me, I got so caught up in re-using get_slashable_indices
that I didn't realise I shouldn't be filtering already slashed validators at all. Thanks again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries at all! 😄
## Issue Addressed Closes #3241 Closes #3242 ## Proposed Changes * [x] Implement logic to remove equivocating validators from fork choice per ethereum/consensus-specs#2845 * [x] Update tests to v1.2.0-rc.1. The new test which exercises `equivocating_indices` is passing. * [x] Pull in some SSZ abstractions from the `tree-states` branch that make implementing Vec-compatible encoding for types like `BTreeSet` and `BTreeMap`. * [x] Implement schema upgrades and downgrades for the database (new schema version is V11). * [x] Apply attester slashings from blocks to fork choice ## Additional Info * This PR doesn't need the `BTreeMap` impl, but `tree-states` does, and I don't think there's any harm in keeping it. But I could also be convinced to drop it. Blocked on #3322.
## Issue Addressed Closes #3241 Closes #3242 ## Proposed Changes * [x] Implement logic to remove equivocating validators from fork choice per ethereum/consensus-specs#2845 * [x] Update tests to v1.2.0-rc.1. The new test which exercises `equivocating_indices` is passing. * [x] Pull in some SSZ abstractions from the `tree-states` branch that make implementing Vec-compatible encoding for types like `BTreeSet` and `BTreeMap`. * [x] Implement schema upgrades and downgrades for the database (new schema version is V11). * [x] Apply attester slashings from blocks to fork choice ## Additional Info * This PR doesn't need the `BTreeMap` impl, but `tree-states` does, and I don't think there's any harm in keeping it. But I could also be convinced to drop it. Blocked on #3322.
## Issue Addressed Closes #3241 Closes #3242 ## Proposed Changes * [x] Implement logic to remove equivocating validators from fork choice per ethereum/consensus-specs#2845 * [x] Update tests to v1.2.0-rc.1. The new test which exercises `equivocating_indices` is passing. * [x] Pull in some SSZ abstractions from the `tree-states` branch that make implementing Vec-compatible encoding for types like `BTreeSet` and `BTreeMap`. * [x] Implement schema upgrades and downgrades for the database (new schema version is V11). * [x] Apply attester slashings from blocks to fork choice ## Additional Info * This PR doesn't need the `BTreeMap` impl, but `tree-states` does, and I don't think there's any harm in keeping it. But I could also be convinced to drop it. Blocked on #3322.
## Issue Addressed Closes #3241 Closes #3242 ## Proposed Changes * [x] Implement logic to remove equivocating validators from fork choice per ethereum/consensus-specs#2845 * [x] Update tests to v1.2.0-rc.1. The new test which exercises `equivocating_indices` is passing. * [x] Pull in some SSZ abstractions from the `tree-states` branch that make implementing Vec-compatible encoding for types like `BTreeSet` and `BTreeMap`. * [x] Implement schema upgrades and downgrades for the database (new schema version is V11). * [x] Apply attester slashings from blocks to fork choice ## Additional Info * This PR doesn't need the `BTreeMap` impl, but `tree-states` does, and I don't think there's any harm in keeping it. But I could also be convinced to drop it. Blocked on #3322.
## Issue Addressed Closes #3241 Closes #3242 ## Proposed Changes * [x] Implement logic to remove equivocating validators from fork choice per ethereum/consensus-specs#2845 * [x] Update tests to v1.2.0-rc.1. The new test which exercises `equivocating_indices` is passing. * [x] Pull in some SSZ abstractions from the `tree-states` branch that make implementing Vec-compatible encoding for types like `BTreeSet` and `BTreeMap`. * [x] Implement schema upgrades and downgrades for the database (new schema version is V11). * [x] Apply attester slashings from blocks to fork choice ## Additional Info * This PR doesn't need the `BTreeMap` impl, but `tree-states` does, and I don't think there's any harm in keeping it. But I could also be convinced to drop it. Blocked on #3322.
## Issue Addressed Closes #3241 Closes #3242 ## Proposed Changes * [x] Implement logic to remove equivocating validators from fork choice per ethereum/consensus-specs#2845 * [x] Update tests to v1.2.0-rc.1. The new test which exercises `equivocating_indices` is passing. * [x] Pull in some SSZ abstractions from the `tree-states` branch that make implementing Vec-compatible encoding for types like `BTreeSet` and `BTreeMap`. * [x] Implement schema upgrades and downgrades for the database (new schema version is V11). * [x] Apply attester slashings from blocks to fork choice ## Additional Info * This PR doesn't need the `BTreeMap` impl, but `tree-states` does, and I don't think there's any harm in keeping it. But I could also be convinced to drop it. Blocked on #3322.
Discard equivocating attestations from the fork choice
Store
, and discount all future votes equivocating validators for fork choice purposes. Specifically:on_attester_slashing
handler in the fork choice that should be called whenever anAttesterSlashing
is receivedStore.equivocating_indices
get_latest_attesting_balance
, filter the validator set usingStore.equivocating_indices
to drop those validatorsNote: This patches the equivocation balancing attack noted in Section 4 here.
Note 2: The protocol is not vulnerable to the avalanche attack (section 3) because equivocations will be discarded.