Optimize attester slashing (#1745)

## Issue Addressed

Closes #1548 

## Proposed Changes

Optimizes attester slashing choice by choosing the ones that cover the most amount of validators slashed, with the highest effective balances 

## Additional Info

Initial pass, need to write a test for it

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -1725,7 +1725,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             state.latest_block_header.canonical_root()
         };
 
-        let (proposer_slashings, attester_slashings) = self.op_pool.get_slashings(&state);
+        let (proposer_slashings, attester_slashings) =
+            self.op_pool.get_slashings(&state, &self.spec);
 
         let eth1_data = eth1_chain.eth1_data_for_block_production(&state, &self.spec)?;
         let deposits = eth1_chain

beacon_node/operation_pool/src/attester_slashing.rs

@@ -0,0 +1,71 @@
+use crate::max_cover::MaxCover;
+use state_processing::per_block_processing::get_slashable_indices_modular;
+use std::collections::{HashMap, HashSet};
+use types::{AttesterSlashing, BeaconState, ChainSpec, EthSpec};
+
+pub struct AttesterSlashingMaxCover<'a, T: EthSpec> {
+    slashing: &'a AttesterSlashing<T>,
+    effective_balances: HashMap<u64, u64>,
+}
+
+impl<'a, T: EthSpec> AttesterSlashingMaxCover<'a, T> {
+    pub fn new(
+        slashing: &'a AttesterSlashing<T>,
+        proposer_slashing_indices: &HashSet<u64>,
+        state: &BeaconState<T>,
+        spec: &ChainSpec,
+    ) -> Option<Self> {
+        let mut effective_balances: HashMap<u64, u64> = HashMap::new();
+        let epoch = state.current_epoch();
+
+        let slashable_validators =
+            get_slashable_indices_modular(state, slashing, |index, validator| {
+                validator.is_slashable_at(epoch) && !proposer_slashing_indices.contains(&index)
+            });
+
+        if let Ok(validators) = slashable_validators {
+            for vd in &validators {
+                let eff_balance = state.get_effective_balance(*vd as usize, spec).ok()?;
+                effective_balances.insert(*vd, eff_balance);
+            }
+
+            Some(Self {
+                slashing,
+                effective_balances,
+            })
+        } else {
+            None
+        }
+    }
+}
+
+impl<'a, T: EthSpec> MaxCover for AttesterSlashingMaxCover<'a, T> {
+    /// The result type, of which we would eventually like a collection of maximal quality.
+    type Object = AttesterSlashing<T>;
+    /// The type used to represent sets.
+    type Set = HashMap<u64, u64>;
+
+    /// Extract an object for inclusion in a solution.
+    fn object(&self) -> AttesterSlashing<T> {
+        self.slashing.clone()
+    }
+
+    /// Get the set of elements covered.
+    fn covering_set(&self) -> &HashMap<u64, u64> {
+        &self.effective_balances
+    }
+    /// Update the set of items covered, for the inclusion of some object in the solution.
+    fn update_covering_set(
+        &mut self,
+        _best_slashing: &AttesterSlashing<T>,
+        covered_validator_indices: &HashMap<u64, u64>,
+    ) {
+        self.effective_balances
+            .retain(|k, _| !covered_validator_indices.contains_key(k));
+    }
+
+    /// The quality of this item's covering set, usually its cardinality.
+    fn score(&self) -> usize {
+        self.effective_balances.values().sum::<u64>() as usize
+    }
+}

beacon_node/operation_pool/src/lib.rs

@@ -1,5 +1,6 @@
 mod attestation;
 mod attestation_id;
+mod attester_slashing;
 mod max_cover;
 mod persistence;
 
@@ -7,12 +8,12 @@ pub use persistence::PersistedOperationPool;
 
 use attestation::AttMaxCover;
 use attestation_id::AttestationId;
+use attester_slashing::AttesterSlashingMaxCover;
 use max_cover::maximum_cover;
 use parking_lot::RwLock;
 use state_processing::per_block_processing::errors::AttestationValidationError;
 use state_processing::per_block_processing::{
-    get_slashable_indices, get_slashable_indices_modular, verify_attestation_for_block_inclusion,
+    get_slashable_indices, verify_attestation_for_block_inclusion, verify_exit, VerifySignatures,
-    verify_exit, VerifySignatures,
 };
 use state_processing::SigVerifiedOp;
 use std::collections::{hash_map, HashMap, HashSet};
@@ -23,7 +24,6 @@ use types::{
     EthSpec, Fork, ForkVersion, Hash256, ProposerSlashing, RelativeEpoch, SignedVoluntaryExit,
     Validator,
 };
-
 #[derive(Default, Debug)]
 pub struct OperationPool<T: EthSpec + Default> {
     /// Map from attestation ID (see below) to vectors of attestations.
@@ -200,6 +200,7 @@ impl<T: EthSpec> OperationPool<T> {
     pub fn get_slashings(
         &self,
         state: &BeaconState<T>,
+        spec: &ChainSpec,
     ) -> (Vec<ProposerSlashing>, Vec<AttesterSlashing<T>>) {
         let proposer_slashings = filter_limit_operations(
             self.proposer_slashings.read().values(),
@@ -214,39 +215,25 @@ impl<T: EthSpec> OperationPool<T> {
 
         // Set of validators to be slashed, so we don't attempt to construct invalid attester
         // slashings.
-        let mut to_be_slashed = proposer_slashings
+        let to_be_slashed = proposer_slashings
             .iter()
             .map(|s| s.signed_header_1.message.proposer_index)
             .collect::<HashSet<_>>();
 
-        let epoch = state.current_epoch();
+        let reader = self.attester_slashings.read();
-        let attester_slashings = self
-            .attester_slashings
-            .read()
-            .iter()
-            .filter(|(slashing, fork)| {
-                if *fork != state.fork.previous_version && *fork != state.fork.current_version {
-                    return false;
-                }
 
-                // Take all slashings that will slash 1 or more validators.
+        let relevant_attester_slashings = reader.iter().flat_map(|(slashing, fork)| {
-                let slashed_validators =
+            if *fork == state.fork.previous_version || *fork == state.fork.current_version {
-                    get_slashable_indices_modular(state, slashing, |index, validator| {
+                AttesterSlashingMaxCover::new(&slashing, &to_be_slashed, state, spec)
-                        validator.is_slashable_at(epoch) && !to_be_slashed.contains(&index)
+            } else {
-                    });
+                None
+            }
+        });
 
-                // Extend the `to_be_slashed` set so subsequent iterations don't try to include
+        let attester_slashings = maximum_cover(
-                // useless slashings.
+            relevant_attester_slashings,
-                if let Ok(validators) = slashed_validators {
+            T::MaxAttesterSlashings::to_usize(),
-                    to_be_slashed.extend(validators);
+        );
-                    true
-                } else {
-                    false
-                }
-            })
-            .take(T::MaxAttesterSlashings::to_usize())
-            .map(|(slashing, _)| slashing.clone())
-            .collect();
 
         (proposer_slashings, attester_slashings)
     }
@@ -960,6 +947,27 @@ mod release_tests {
                 &self.spec,
             )
         }
+
+        fn attester_slashing_two_indices(
+            &self,
+            slashed_indices_1: &[u64],
+            slashed_indices_2: &[u64],
+        ) -> AttesterSlashing<MainnetEthSpec> {
+            let signer = |idx: u64, message: &[u8]| {
+                self.keypairs[idx as usize]
+                    .sk
+                    .sign(Hash256::from_slice(&message))
+            };
+            TestingAttesterSlashingBuilder::double_vote_with_additional_indices(
+                AttesterSlashingTestTask::Valid,
+                slashed_indices_1,
+                Some(slashed_indices_2),
+                signer,
+                &self.state.fork,
+                self.state.genesis_validators_root,
+                &self.spec,
+            )
+        }
     }
 
     /// Insert two slashings for the same proposer and ensure only one is returned.
@@ -979,7 +987,7 @@ mod release_tests {
         op_pool.insert_proposer_slashing(slashing2.clone().validate(state, spec).unwrap());
 
         // Should only get the second slashing back.
-        assert_eq!(op_pool.get_slashings(state).0, vec![slashing2]);
+        assert_eq!(op_pool.get_slashings(state, spec).0, vec![slashing2]);
     }
 
     // Sanity check on the pruning of proposer slashings
@@ -990,7 +998,7 @@ mod release_tests {
         let slashing = ctxt.proposer_slashing(0);
         op_pool.insert_proposer_slashing(slashing.clone().validate(state, spec).unwrap());
         op_pool.prune_proposer_slashings(state);
-        assert_eq!(op_pool.get_slashings(state).0, vec![slashing]);
+        assert_eq!(op_pool.get_slashings(state, spec).0, vec![slashing]);
     }
 
     // Sanity check on the pruning of attester slashings
@@ -1002,6 +1010,158 @@ mod release_tests {
         op_pool
             .insert_attester_slashing(slashing.clone().validate(state, spec).unwrap(), state.fork);
         op_pool.prune_attester_slashings(state, state.fork);
-        assert_eq!(op_pool.get_slashings(state).1, vec![slashing]);
+        assert_eq!(op_pool.get_slashings(state, spec).1, vec![slashing]);
+    }
+
+    // Check that we get maximum coverage for attester slashings (highest qty of validators slashed)
+    #[test]
+    fn simple_max_cover_attester_slashing() {
+        let ctxt = TestContext::new();
+        let (op_pool, state, spec) = (&ctxt.op_pool, &ctxt.state, &ctxt.spec);
+
+        let slashing_1 = ctxt.attester_slashing(&[1]);
+        let slashing_2 = ctxt.attester_slashing(&[2, 3]);
+        let slashing_3 = ctxt.attester_slashing(&[4, 5, 6]);
+        let slashing_4 = ctxt.attester_slashing(&[7, 8, 9, 10]);
+
+        op_pool.insert_attester_slashing(
+            slashing_1.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+        op_pool.insert_attester_slashing(
+            slashing_2.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+        op_pool.insert_attester_slashing(
+            slashing_3.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+        op_pool.insert_attester_slashing(
+            slashing_4.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+
+        let best_slashings = op_pool.get_slashings(state, spec);
+        assert_eq!(best_slashings.1, vec![slashing_4, slashing_3]);
+    }
+
+    // Check that we get maximum coverage for attester slashings with overlapping indices
+    #[test]
+    fn overlapping_max_cover_attester_slashing() {
+        let ctxt = TestContext::new();
+        let (op_pool, state, spec) = (&ctxt.op_pool, &ctxt.state, &ctxt.spec);
+
+        let slashing_1 = ctxt.attester_slashing(&[1, 2, 3, 4]);
+        let slashing_2 = ctxt.attester_slashing(&[1, 2, 5]);
+        let slashing_3 = ctxt.attester_slashing(&[5, 6]);
+        let slashing_4 = ctxt.attester_slashing(&[6]);
+
+        op_pool.insert_attester_slashing(
+            slashing_1.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+        op_pool.insert_attester_slashing(
+            slashing_2.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+        op_pool.insert_attester_slashing(
+            slashing_3.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+        op_pool.insert_attester_slashing(
+            slashing_4.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+
+        let best_slashings = op_pool.get_slashings(state, spec);
+        assert_eq!(best_slashings.1, vec![slashing_1, slashing_3]);
+    }
+
+    // Max coverage of attester slashings taking into account proposer slashings
+    #[test]
+    fn max_coverage_attester_proposer_slashings() {
+        let ctxt = TestContext::new();
+        let (op_pool, state, spec) = (&ctxt.op_pool, &ctxt.state, &ctxt.spec);
+
+        let p_slashing = ctxt.proposer_slashing(1);
+        let a_slashing_1 = ctxt.attester_slashing(&[1, 2, 3, 4]);
+        let a_slashing_2 = ctxt.attester_slashing(&[1, 3, 4]);
+        let a_slashing_3 = ctxt.attester_slashing(&[5, 6]);
+
+        op_pool.insert_proposer_slashing(p_slashing.clone().validate(state, spec).unwrap());
+        op_pool.insert_attester_slashing(
+            a_slashing_1.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+        op_pool.insert_attester_slashing(
+            a_slashing_2.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+        op_pool.insert_attester_slashing(
+            a_slashing_3.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+
+        let best_slashings = op_pool.get_slashings(state, spec);
+        assert_eq!(best_slashings.1, vec![a_slashing_1, a_slashing_3]);
+    }
+
+    //Max coverage checking that non overlapping indices are still recognized for their value
+    #[test]
+    fn max_coverage_different_indices_set() {
+        let ctxt = TestContext::new();
+        let (op_pool, state, spec) = (&ctxt.op_pool, &ctxt.state, &ctxt.spec);
+
+        let slashing_1 =
+            ctxt.attester_slashing_two_indices(&[1, 2, 3, 4, 5, 6], &[3, 4, 5, 6, 7, 8]);
+        let slashing_2 = ctxt.attester_slashing(&[5, 6]);
+        let slashing_3 = ctxt.attester_slashing(&[1, 2, 3]);
+
+        op_pool.insert_attester_slashing(
+            slashing_1.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+        op_pool.insert_attester_slashing(
+            slashing_2.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+        op_pool.insert_attester_slashing(
+            slashing_3.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+
+        let best_slashings = op_pool.get_slashings(state, spec);
+        assert_eq!(best_slashings.1, vec![slashing_1, slashing_3]);
+    }
+
+    //Max coverage should be affected by the overall effective balances
+    #[test]
+    fn max_coverage_effective_balances() {
+        let mut ctxt = TestContext::new();
+        ctxt.state.validators[1].effective_balance = 17_000_000_000;
+        ctxt.state.validators[2].effective_balance = 17_000_000_000;
+        ctxt.state.validators[3].effective_balance = 17_000_000_000;
+
+        let (op_pool, state, spec) = (&ctxt.op_pool, &ctxt.state, &ctxt.spec);
+
+        let slashing_1 = ctxt.attester_slashing(&[1, 2, 3]);
+        let slashing_2 = ctxt.attester_slashing(&[4, 5, 6]);
+        let slashing_3 = ctxt.attester_slashing(&[7, 8]);
+
+        op_pool.insert_attester_slashing(
+            slashing_1.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+        op_pool.insert_attester_slashing(
+            slashing_2.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+        op_pool.insert_attester_slashing(
+            slashing_3.clone().validate(state, spec).unwrap(),
+            state.fork,
+        );
+
+        let best_slashings = op_pool.get_slashings(state, spec);
+        assert_eq!(best_slashings.1, vec![slashing_2, slashing_3]);
     }
 }

consensus/types/src/test_utils/builders/testing_attester_slashing_builder.rs

@@ -21,6 +21,29 @@ impl TestingAttesterSlashingBuilder {
         genesis_validators_root: Hash256,
         spec: &ChainSpec,
     ) -> AttesterSlashing<T>
+    where
+        F: Fn(u64, &[u8]) -> Signature,
+    {
+        TestingAttesterSlashingBuilder::double_vote_with_additional_indices(
+            test_task,
+            validator_indices,
+            None,
+            signer,
+            fork,
+            genesis_validators_root,
+            spec,
+        )
+    }
+
+    pub fn double_vote_with_additional_indices<F, T: EthSpec>(
+        test_task: AttesterSlashingTestTask,
+        validator_indices: &[u64],
+        additional_validator_indices: Option<&[u64]>,
+        signer: F,
+        fork: &Fork,
+        genesis_validators_root: Hash256,
+        spec: &ChainSpec,
+    ) -> AttesterSlashing<T>
     where
         F: Fn(u64, &[u8]) -> Signature,
     {
@@ -73,13 +96,16 @@ impl TestingAttesterSlashingBuilder {
                 // Trigger bad validator indices ordering error.
                 vec![1, 0].into()
             } else {
-                validator_indices.to_vec().into()
+                match additional_validator_indices {
+                    Some(x) => x.to_vec().into(),
+                    None => validator_indices.to_vec().into(),
+                }
             },
             data: data_2,
             signature: AggregateSignature::empty(),
         };
 
-        let add_signatures = |attestation: &mut IndexedAttestation<T>| {
+        let add_signatures = |attestation: &mut IndexedAttestation<T>, indices_to_sign: &[u64]| {
             let domain = spec.get_domain(
                 attestation.data.target.epoch,
                 Domain::BeaconAttester,
@@ -88,14 +114,17 @@ impl TestingAttesterSlashingBuilder {
             );
             let message = attestation.data.signing_root(domain);
 
-            for validator_index in validator_indices {
+            for validator_index in indices_to_sign {
                 let signature = signer(*validator_index, message.as_bytes());
                 attestation.signature.add_assign(&signature);
             }
         };
 
-        add_signatures(&mut attestation_1);
+        add_signatures(&mut attestation_1, validator_indices);
-        add_signatures(&mut attestation_2);
+        add_signatures(
+            &mut attestation_2,
+            additional_validator_indices.unwrap_or(validator_indices),
+        );
 
         AttesterSlashing {
             attestation_1,