[Merged by Bors] - Remove equivocating validators from fork choice #3371

michaelsproul · 2022-07-25T08:28:36Z

Issue Addressed

Closes #3241
Closes #3242

Proposed Changes

Implement logic to remove equivocating validators from fork choice per Remove equivocating validators from fork choice consideration ethereum/consensus-specs#2845
Update tests to v1.2.0-rc.1. The new test which exercises equivocating_indices is passing.
Pull in some SSZ abstractions from the tree-states branch that make implementing Vec-compatible encoding for types like BTreeSet and BTreeMap.
Implement schema upgrades and downgrades for the database (new schema version is V11).
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.

michaelsproul · 2022-07-26T02:39:44Z

This PR is ready for review!

A few notes for reviewers:

There is currently no pruning of the equivocating indices. I think this would require some careful consideration to get right, so I've opened an issue here: Prune fork choice equivocating indices #3373
We could consider using a HashMap for the equivocating indices, but I'm hesitant to implement SSZ encode/decode for hashmaps given that it would be non-deterministic. Given that we don't expect this map to become too large (at most ~100K entries), I think the BTree map should be sufficient. Likewise memory usage should be modest: 8 bytes * O(n log n) ~= 4MB.

realbigsean

Nice! Looks great, couldn't find any issues. Left one comment that I'm not sure even makes a functional difference so feel free to ignore it.

realbigsean · 2022-07-26T20:56:56Z

consensus/fork_choice/src/fork_choice.rs

+        // anyway. If a validator is slashable at the head but not at justification then it means
+        // their activation has occurred since justification, and we arguably shouldn't be counting
+        // their attestations.
+        let slashed_indices = get_slashable_indices(head_state, slashing)?;


Would it be better to use get_slashable_indices modular here and have the is_slashable method do nothing? Seems like it's more in line with what's specified.

I think if we wanted to match the spec as closely as possible without actually loading the justified state then we could use get_slashable_indices_modular and check validator.is_slashable_at(head_state.current_justified_checkpoint().epoch). But I suspect that the spec is only written like that because the justified state is easy to access without plumbing the head state through. I've asked about it on the spec PR here: ethereum/consensus-specs#2845 (comment). We can hold off on merging for a bit until we hear back

I was wrong 😅

Fixed in this commit, which also enabled a few other simplifications ad0621e.

realbigsean

Nice the updates look good to me, I'll leave it open in case @paulhauner wants to have a look

paulhauner

Nice stuff, looks good!

Only one comment about SSZ decode in growing Vecs.

paulhauner · 2022-07-28T04:27:42Z

consensus/ssz/src/decode/impls.rs

-    //
-    // We assume that if a max length is provided then the application is able to handle an
-    // allocation of this size.
-    let mut values = if max_len.is_some() {


It'll be interesting to see the outcome of losing max-length pre-allocation for VariableLists which contain variable-length items. As an example we'll no longer allocate a list of length 128 for attestations in a block, instead we'll grow it by 1 each attestation. It seems VariableList is used a bit by the networking stack, too.

I'm not certain if this will be a regression or not. I'll keep an eye on the memory stats on our nodes and maybe even do a heaptrack.

Ooh good point. I kind of hoped the magic of collect and size hints would let us keep the pre-allocation, but it seems it only works if the iterator implements TrustedLen: https://github.com/rust-lang/rust/blob/48316dfea1914d25189fa441e7310449ed76a446/library/alloc/src/vec/spec_extend.rs#L36. And the iterator from process_results doesn't (because it can fail).

It would be quite straight-forward to add a new_with_capacity function to TryFromIter if it turns out we need it

Or we could just change the TryFromIter implementation for Vec so that it always uses the size_hint

paulhauner · 2022-07-28T05:12:03Z

bors r+

## 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.

bors · 2022-07-28T05:39:42Z

Build failed (retrying...):

execution-engine-integration-ubuntu

## 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.

michaelsproul · 2022-07-28T05:46:11Z

bors r-

bors · 2022-07-28T05:46:12Z

Canceled.

michaelsproul · 2022-07-28T05:46:29Z

bors r+

## 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.

bors · 2022-07-28T06:47:18Z

Build failed (retrying...):

beacon-chain-tests

paulhauner · 2022-07-28T07:00:39Z

bors r-

bors · 2022-07-28T07:00:41Z

Canceled.

paulhauner · 2022-07-28T07:00:45Z

bors r+

## 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.

bors · 2022-07-28T07:20:14Z

Build failed (retrying...):

release-tests-ubuntu

## 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.

bors · 2022-07-28T07:39:59Z

Build failed:

execution-engine-integration-ubuntu

paulhauner · 2022-07-28T09:02:44Z

I'll queue this so it runs after the current batch.

bors r+

## 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.

bors · 2022-07-28T12:24:56Z

Pull request successfully merged into unstable.

Build succeeded:

@paulhauner

## Issue Addressed Fixes a potential regression in memory fragmentation identified by @paulhauner here: #3371 (comment). ## Proposed Changes Immediately allocate a vector with sufficient size to hold all decoded elements in SSZ decoding. The `size_hint` is derived from the range iterator here: https://github.com/sigp/lighthouse/blob/2983235650811437b44199f9c94e517e948a1e9b/consensus/ssz/src/decode/impls.rs#L489 ## Additional Info I'd like to test this out on some infra for a substantial duration to see if it affects total fragmentation.

@paulhauner

## Issue Addressed Fixes a potential regression in memory fragmentation identified by @paulhauner here: #3371 (comment). ## Proposed Changes Immediately allocate a vector with sufficient size to hold all decoded elements in SSZ decoding. The `size_hint` is derived from the range iterator here: https://github.com/sigp/lighthouse/blob/2983235650811437b44199f9c94e517e948a1e9b/consensus/ssz/src/decode/impls.rs#L489 ## Additional Info I'd like to test this out on some infra for a substantial duration to see if it affects total fragmentation.

@paulhauner

## Issue Addressed Fixes a potential regression in memory fragmentation identified by @paulhauner here: #3371 (comment). ## Proposed Changes Immediately allocate a vector with sufficient size to hold all decoded elements in SSZ decoding. The `size_hint` is derived from the range iterator here: https://github.com/sigp/lighthouse/blob/2983235650811437b44199f9c94e517e948a1e9b/consensus/ssz/src/decode/impls.rs#L489 ## Additional Info I'd like to test this out on some infra for a substantial duration to see if it affects total fragmentation.

@paulhauner

## Issue Addressed Fixes a potential regression in memory fragmentation identified by @paulhauner here: #3371 (comment). ## Proposed Changes Immediately allocate a vector with sufficient size to hold all decoded elements in SSZ decoding. The `size_hint` is derived from the range iterator here: https://github.com/sigp/lighthouse/blob/2983235650811437b44199f9c94e517e948a1e9b/consensus/ssz/src/decode/impls.rs#L489 ## Additional Info I'd like to test this out on some infra for a substantial duration to see if it affects total fragmentation.

@paulhauner

## Issue Addressed Fixes a potential regression in memory fragmentation identified by @paulhauner here: #3371 (comment). ## Proposed Changes Immediately allocate a vector with sufficient size to hold all decoded elements in SSZ decoding. The `size_hint` is derived from the range iterator here: https://github.com/sigp/lighthouse/blob/2983235650811437b44199f9c94e517e948a1e9b/consensus/ssz/src/decode/impls.rs#L489 ## Additional Info I'd like to test this out on some infra for a substantial duration to see if it affects total fragmentation.

@paulhauner

## Issue Addressed Fixes a potential regression in memory fragmentation identified by @paulhauner here: sigp#3371 (comment). ## Proposed Changes Immediately allocate a vector with sufficient size to hold all decoded elements in SSZ decoding. The `size_hint` is derived from the range iterator here: https://github.com/sigp/lighthouse/blob/2983235650811437b44199f9c94e517e948a1e9b/consensus/ssz/src/decode/impls.rs#L489 ## Additional Info I'd like to test this out on some infra for a substantial duration to see if it affects total fragmentation.

michaelsproul added work-in-progress PR is a work-in-progress backwards-incompat Backwards-incompatible API change labels Jul 25, 2022

michaelsproul added 5 commits July 26, 2022 10:41

Update EF tests to v1.2.0-rc.1

52b348f

Implement SSZ for BTreeSet/Map

22cb172

Remove equivocating validators from fork choice

ae9371e

Resolve FIXMEs

d054b44

Post rebase fixups

960bfbd

michaelsproul force-pushed the rm-equivocating-fork-choice branch from 181fd0f to 960bfbd Compare July 26, 2022 00:57

michaelsproul added blocked and removed work-in-progress PR is a work-in-progress labels Jul 26, 2022

Ignore EF tests for Capella

94bc85e

michaelsproul changed the base branch from unstable to stable July 26, 2022 02:27

michaelsproul changed the base branch from stable to unstable July 26, 2022 02:28

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. and removed blocked labels Jul 26, 2022

michaelsproul mentioned this pull request Jul 26, 2022

Prune fork choice equivocating indices #3373

Open

michaelsproul added the v2.5.0 Required for Goerli merge release label Jul 26, 2022

michaelsproul requested a review from realbigsean July 26, 2022 05:56

realbigsean approved these changes Jul 26, 2022

View reviewed changes

michaelsproul mentioned this pull request Jul 26, 2022

Remove equivocating validators from fork choice consideration ethereum/consensus-specs#2845

Merged

michaelsproul added 2 commits July 27, 2022 11:57

Fix calculation of slashable indices, simplify

ad0621e

Simplify testing

bf1b8ac

realbigsean approved these changes Jul 27, 2022

View reviewed changes

paulhauner approved these changes Jul 28, 2022

View reviewed changes

paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jul 28, 2022

bors bot changed the title ~~Remove equivocating validators from fork choice~~ [Merged by Bors] - Remove equivocating validators from fork choice Jul 28, 2022

bors bot closed this Jul 28, 2022

This was referenced Jul 28, 2022

Remove equivocating validators from fork choice #3241

Closed

Update EF consensus tests to v1.2.0 #3242

Closed

michaelsproul mentioned this pull request Aug 3, 2022

[Merged by Bors] - Pre-allocate vectors in SSZ decoding #3417

Closed

michaelsproul deleted the rm-equivocating-fork-choice branch August 3, 2022 03:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Remove equivocating validators from fork choice #3371

[Merged by Bors] - Remove equivocating validators from fork choice #3371

michaelsproul commented Jul 25, 2022 •

edited

Loading

michaelsproul commented Jul 26, 2022

realbigsean left a comment

realbigsean Jul 26, 2022

michaelsproul Jul 26, 2022

michaelsproul Jul 27, 2022

realbigsean left a comment

paulhauner left a comment

paulhauner Jul 28, 2022

michaelsproul Jul 28, 2022 •

edited

Loading

michaelsproul Jul 28, 2022 •

edited

Loading

paulhauner commented Jul 28, 2022

bors bot commented Jul 28, 2022

michaelsproul commented Jul 28, 2022

bors bot commented Jul 28, 2022

michaelsproul commented Jul 28, 2022

bors bot commented Jul 28, 2022

paulhauner commented Jul 28, 2022

bors bot commented Jul 28, 2022

paulhauner commented Jul 28, 2022

bors bot commented Jul 28, 2022

bors bot commented Jul 28, 2022

paulhauner commented Jul 28, 2022

bors bot commented Jul 28, 2022

[Merged by Bors] - Remove equivocating validators from fork choice #3371

[Merged by Bors] - Remove equivocating validators from fork choice #3371

Conversation

michaelsproul commented Jul 25, 2022 • edited Loading

Issue Addressed

Proposed Changes

Additional Info

michaelsproul commented Jul 26, 2022

realbigsean left a comment

Choose a reason for hiding this comment

realbigsean Jul 26, 2022

Choose a reason for hiding this comment

michaelsproul Jul 26, 2022

Choose a reason for hiding this comment

michaelsproul Jul 27, 2022

Choose a reason for hiding this comment

realbigsean left a comment

Choose a reason for hiding this comment

paulhauner left a comment

Choose a reason for hiding this comment

paulhauner Jul 28, 2022

Choose a reason for hiding this comment

michaelsproul Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

michaelsproul Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

paulhauner commented Jul 28, 2022

bors bot commented Jul 28, 2022

michaelsproul commented Jul 28, 2022

bors bot commented Jul 28, 2022

michaelsproul commented Jul 28, 2022

bors bot commented Jul 28, 2022

paulhauner commented Jul 28, 2022

bors bot commented Jul 28, 2022

paulhauner commented Jul 28, 2022

bors bot commented Jul 28, 2022

bors bot commented Jul 28, 2022

paulhauner commented Jul 28, 2022

bors bot commented Jul 28, 2022

michaelsproul commented Jul 25, 2022 •

edited

Loading

michaelsproul Jul 28, 2022 •

edited

Loading

michaelsproul Jul 28, 2022 •

edited

Loading