Add Altair tests to op pool (#2723)

## Issue Addressed

NA

## Proposed Changes

Adds some more testing for Altair to the op pool. Credits to @michaelsproul for some appropriated efforts here.

## Additional Info

NA


Co-authored-by: Michael Sproul <michael@sigmaprime.io>

.github/workflows/test-suite.yml

@@ -64,10 +64,18 @@ jobs:
     - uses: actions/checkout@v1
     - name: Get latest version of stable Rust
       run: rustup update stable
-    - name: Run beacon_chain tests for base hard fork
+    - name: Run beacon_chain tests for all known forks
-      run: make test-beacon-chain-base
+      run: make test-beacon-chain
-    - name: Run beacon_chain tests for Altair hard fork
+  op-pool-tests:
-      run: make test-beacon-chain-altair
+    name: op-pool-tests
+    runs-on: ubuntu-latest
+    needs: cargo-fmt
+    steps:
+    - uses: actions/checkout@v1
+    - name: Get latest version of stable Rust
+      run: rustup update stable
+    - name: Run operation_pool tests for all known forks
+      run: make test-op-pool
   debug-tests-ubuntu:
     name: debug-tests-ubuntu
     runs-on: ubuntu-latest

Makefile

@@ -2,6 +2,7 @@
 
 EF_TESTS = "testing/ef_tests"
 BEACON_CHAIN_CRATE = "beacon_node/beacon_chain"
+OP_POOL_CRATE = "beacon_node/operation_pool"
 STATE_TRANSITION_VECTORS = "testing/state_transition_vectors"
 GIT_TAG := $(shell git describe --tags --candidates 1)
 BIN_DIR = "bin"
@@ -13,6 +14,10 @@ BUILD_PATH_AARCH64 = "target/$(AARCH64_TAG)/release"
 
 PINNED_NIGHTLY ?= nightly
 
+# List of all hard forks. This list is used to set env variables for several tests so that
+# they run for different forks.
+FORKS=phase0 altair
+
 # Builds the Lighthouse binary in release (optimized).
 #
 # Binaries will most likely be found in `./target/release`
@@ -107,12 +112,20 @@ run-ef-tests:
 	cargo test --release --manifest-path=$(EF_TESTS)/Cargo.toml --features "ef_tests,milagro"
 	./$(EF_TESTS)/check_all_files_accessed.py $(EF_TESTS)/.accessed_file_log.txt $(EF_TESTS)/consensus-spec-tests
 
-# Run the tests in the `beacon_chain` crate.
+# Run the tests in the `beacon_chain` crate for all known forks.
-test-beacon-chain: test-beacon-chain-base test-beacon-chain-altair
+test-beacon-chain: $(patsubst %,test-beacon-chain-%,$(FORKS))
 
 test-beacon-chain-%:
 	env FORK_NAME=$* cargo test --release --features fork_from_env --manifest-path=$(BEACON_CHAIN_CRATE)/Cargo.toml
 
+# Run the tests in the `operation_pool` crate for all known forks.
+test-op-pool: $(patsubst %,test-op-pool-%,$(FORKS))
+
+test-op-pool-%:
+	env FORK_NAME=$* cargo test --release \
+		--features 'beacon_chain/fork_from_env'\
+		--manifest-path=$(OP_POOL_CRATE)/Cargo.toml
+
 # Runs only the tests/state_transition_vectors tests.
 run-state-transition-tests:
 	make -C $(STATE_TRANSITION_VECTORS) test

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -2761,7 +2761,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, voluntary_exits) =
+            self.op_pool.get_slashings_and_exits(&state, &self.spec);
 
         let eth1_data = eth1_chain.eth1_data_for_block_production(&state, &self.spec)?;
         let deposits = eth1_chain
@@ -2821,7 +2822,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
         let slot = state.slot();
         let proposer_index = state.get_beacon_proposer_index(state.slot(), &self.spec)? as u64;
-        let voluntary_exits = self.op_pool.get_voluntary_exits(&state, &self.spec).into();
 
         // Closure to fetch a sync aggregate in cases where it is required.
         let get_sync_aggregate = || -> Result<SyncAggregate<_>, BlockProductionError> {
@@ -2853,7 +2853,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                     attester_slashings: attester_slashings.into(),
                     attestations,
                     deposits,
-                    voluntary_exits,
+                    voluntary_exits: voluntary_exits.into(),
                 },
             }),
             BeaconState::Altair(_) => {
@@ -2871,7 +2871,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                         attester_slashings: attester_slashings.into(),
                         attestations,
                         deposits,
-                        voluntary_exits,
+                        voluntary_exits: voluntary_exits.into(),
                         sync_aggregate,
                     },
                 })

beacon_node/beacon_chain/src/test_utils.rs

@@ -26,6 +26,7 @@ use slot_clock::TestingSlotClock;
 use state_processing::state_advance::complete_state_advance;
 use std::borrow::Cow;
 use std::collections::{HashMap, HashSet};
+use std::str::FromStr;
 use std::sync::Arc;
 use std::time::Duration;
 use store::{config::StoreConfig, BlockReplay, HotColdDB, ItemStore, LevelDB, MemoryStore};
@@ -126,11 +127,8 @@ pub fn test_spec<E: EthSpec>() -> ChainSpec {
                 FORK_NAME_ENV_VAR, e
             )
         });
-        let fork = match fork_name.as_str() {
+        let fork = ForkName::from_str(fork_name.as_str())
-            "base" => ForkName::Base,
+            .unwrap_or_else(|()| panic!("unknown FORK_NAME: {}", fork_name));
-            "altair" => ForkName::Altair,
-            other => panic!("unknown FORK_NAME: {}", other),
-        };
         fork.make_genesis_spec(E::default_spec())
     } else {
         E::default_spec()

beacon_node/operation_pool/src/attestation.rs

@@ -12,9 +12,9 @@ use types::{
 #[derive(Debug, Clone)]
 pub struct AttMaxCover<'a, T: EthSpec> {
     /// Underlying attestation.
-    att: &'a Attestation<T>,
+    pub att: &'a Attestation<T>,
     /// Mapping of validator indices and their rewards.
-    fresh_validators_rewards: HashMap<u64, u64>,
+    pub fresh_validators_rewards: HashMap<u64, u64>,
 }
 
 impl<'a, T: EthSpec> AttMaxCover<'a, T> {

beacon_node/operation_pool/src/lib.rs

@@ -354,10 +354,15 @@ impl<T: EthSpec> OperationPool<T> {
     /// This function computes both types of slashings together, because
     /// attester slashings may be invalidated by proposer slashings included
     /// earlier in the block.
-    pub fn get_slashings(
+    pub fn get_slashings_and_exits(
         &self,
         state: &BeaconState<T>,
-    ) -> (Vec<ProposerSlashing>, Vec<AttesterSlashing<T>>) {
+        spec: &ChainSpec,
+    ) -> (
+        Vec<ProposerSlashing>,
+        Vec<AttesterSlashing<T>>,
+        Vec<SignedVoluntaryExit>,
+    ) {
         let proposer_slashings = filter_limit_operations(
             self.proposer_slashings.read().values(),
             |slashing| {
@@ -371,7 +376,7 @@ impl<T: EthSpec> OperationPool<T> {
 
         // Set of validators to be slashed, so we don't attempt to construct invalid attester
         // slashings.
-        let to_be_slashed = proposer_slashings
+        let mut to_be_slashed = proposer_slashings
             .iter()
             .map(|s| s.signed_header_1.message.proposer_index)
             .collect::<HashSet<_>>();
@@ -391,10 +396,19 @@ impl<T: EthSpec> OperationPool<T> {
             T::MaxAttesterSlashings::to_usize(),
         )
         .into_iter()
-        .map(|cover| cover.object().clone())
+        .map(|cover| {
+            to_be_slashed.extend(cover.covering_set().keys());
+            cover.object().clone()
+        })
         .collect();
 
-        (proposer_slashings, attester_slashings)
+        let voluntary_exits = self.get_voluntary_exits(
+            state,
+            |exit| !to_be_slashed.contains(&exit.message.validator_index),
+            spec,
+        );
+
+        (proposer_slashings, attester_slashings, voluntary_exits)
     }
 
     /// Prune proposer slashings for validators which are exited in the finalized epoch.
@@ -454,14 +468,18 @@ impl<T: EthSpec> OperationPool<T> {
     }
 
     /// Get a list of voluntary exits for inclusion in a block.
-    pub fn get_voluntary_exits(
+    fn get_voluntary_exits<F>(
         &self,
         state: &BeaconState<T>,
+        filter: F,
         spec: &ChainSpec,
-    ) -> Vec<SignedVoluntaryExit> {
+    ) -> Vec<SignedVoluntaryExit>
+    where
+        F: Fn(&SignedVoluntaryExit) -> bool,
+    {
         filter_limit_operations(
             self.voluntary_exits.read().values(),
-            |exit| verify_exit(state, exit, VerifySignatures::False, spec).is_ok(),
+            |exit| filter(exit) && verify_exit(state, exit, VerifySignatures::False, spec).is_ok(),
             T::MaxVoluntaryExits::to_usize(),
         )
     }
@@ -609,15 +627,11 @@ mod release_tests {
     use super::attestation::earliest_attestation_validators;
     use super::*;
     use beacon_chain::test_utils::{
-        BeaconChainHarness, EphemeralHarnessType, RelativeSyncCommittee,
+        test_spec, BeaconChainHarness, EphemeralHarnessType, RelativeSyncCommittee,
     };
     use lazy_static::lazy_static;
-    use state_processing::{
+    use state_processing::VerifyOperation;
-        common::{base::get_base_reward, get_attesting_indices},
-        VerifyOperation,
-    };
     use std::collections::BTreeSet;
-    use std::iter::FromIterator;
     use types::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT;
     use types::*;
 
@@ -647,11 +661,11 @@ mod release_tests {
     fn attestation_test_state<E: EthSpec>(
         num_committees: usize,
     ) -> (BeaconChainHarness<EphemeralHarnessType<E>>, ChainSpec) {
-        let spec = E::default_spec();
+        let spec = test_spec::<E>();
 
         let num_validators =
             num_committees * E::slots_per_epoch() as usize * spec.target_committee_size;
-        let harness = get_harness::<E>(num_validators, None);
+        let harness = get_harness::<E>(num_validators, Some(spec.clone()));
 
         (harness, spec)
     }
@@ -682,6 +696,12 @@ mod release_tests {
     #[test]
     fn test_earliest_attestation() {
         let (harness, ref spec) = attestation_test_state::<MainnetEthSpec>(1);
+
+        // Only run this test on the phase0 hard-fork.
+        if spec.altair_fork_epoch != None {
+            return;
+        }
+
         let mut state = harness.get_current_state();
         let slot = state.slot();
         let committees = state
@@ -725,7 +745,6 @@ mod release_tests {
                     .num_set_bits()
             );
 
-            // FIXME(altair): handle altair in these tests
             state
                 .as_base_mut()
                 .unwrap()
@@ -1146,46 +1165,25 @@ mod release_tests {
         let total_active_balance = state.get_total_active_balance().unwrap();
 
         // Set of indices covered by previous attestations in `best_attestations`.
-        let mut seen_indices = BTreeSet::new();
+        let mut seen_indices = BTreeSet::<u64>::new();
         // Used for asserting that rewards are in decreasing order.
         let mut prev_reward = u64::max_value();
 
         for att in &best_attestations {
-            let fresh_validators_bitlist =
+            let mut fresh_validators_rewards =
-                earliest_attestation_validators(att, &state, state.as_base().unwrap());
+                AttMaxCover::new(att, &state, total_active_balance, spec)
-            let committee = state
-                .get_beacon_committee(att.data.slot, att.data.index)
-                .expect("should get beacon committee");
-
-            let att_indices = BTreeSet::from_iter(
-                get_attesting_indices::<MainnetEthSpec>(
-                    committee.committee,
-                    &fresh_validators_bitlist,
-                )
-                .unwrap(),
-            );
-
-            let fresh_indices = &att_indices - &seen_indices;
-
-            let rewards = fresh_indices
-                .iter()
-                .map(|validator_index| {
-                    get_base_reward(
-                        &state,
-                        *validator_index as usize,
-                        total_active_balance,
-                        spec,
-                    )
                     .unwrap()
-                        / spec.proposer_reward_quotient
+                    .fresh_validators_rewards;
-                })
+
-                .sum();
+            // Remove validators covered by previous attestations.
+            fresh_validators_rewards
+                .retain(|validator_index, _| !seen_indices.contains(validator_index));
 
             // Check that rewards are in decreasing order
+            let rewards = fresh_validators_rewards.values().sum();
             assert!(prev_reward >= rewards);
-
             prev_reward = rewards;
-            seen_indices.extend(fresh_indices);
+            seen_indices.extend(fresh_validators_rewards.keys());
         }
     }
 
@@ -1211,7 +1209,10 @@ mod release_tests {
             .insert_proposer_slashing(slashing2.clone().validate(&state, &harness.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_and_exits(&state, &harness.spec).0,
+            vec![slashing2]
+        );
     }
 
     // Sanity check on the pruning of proposer slashings
@@ -1224,7 +1225,10 @@ mod release_tests {
         let slashing = harness.make_proposer_slashing(0);
         op_pool.insert_proposer_slashing(slashing.clone().validate(&state, &harness.spec).unwrap());
         op_pool.prune_proposer_slashings(&state);
-        assert_eq!(op_pool.get_slashings(&state).0, vec![slashing]);
+        assert_eq!(
+            op_pool.get_slashings_and_exits(&state, &harness.spec).0,
+            vec![slashing]
+        );
     }
 
     // Sanity check on the pruning of attester slashings
@@ -1241,7 +1245,10 @@ mod release_tests {
             state.fork(),
         );
         op_pool.prune_attester_slashings(&state);
-        assert_eq!(op_pool.get_slashings(&state).1, vec![slashing]);
+        assert_eq!(
+            op_pool.get_slashings_and_exits(&state, &harness.spec).1,
+            vec![slashing]
+        );
     }
 
     // Check that we get maximum coverage for attester slashings (highest qty of validators slashed)
@@ -1274,7 +1281,7 @@ mod release_tests {
             state.fork(),
         );
 
-        let best_slashings = op_pool.get_slashings(&state);
+        let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec);
         assert_eq!(best_slashings.1, vec![slashing_4, slashing_3]);
     }
 
@@ -1308,7 +1315,7 @@ mod release_tests {
             state.fork(),
         );
 
-        let best_slashings = op_pool.get_slashings(&state);
+        let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec);
         assert_eq!(best_slashings.1, vec![slashing_1, slashing_3]);
     }
 
@@ -1339,7 +1346,7 @@ mod release_tests {
             state.fork(),
         );
 
-        let best_slashings = op_pool.get_slashings(&state);
+        let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec);
         assert_eq!(best_slashings.1, vec![a_slashing_1, a_slashing_3]);
     }
 
@@ -1371,11 +1378,11 @@ mod release_tests {
             state.fork(),
         );
 
-        let best_slashings = op_pool.get_slashings(&state);
+        let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec);
         assert_eq!(best_slashings.1, vec![slashing_1, slashing_3]);
     }
 
-    //Max coverage should be affected by the overall effective balances
+    // Max coverage should be affected by the overall effective balances
     #[test]
     fn max_coverage_effective_balances() {
         let harness = get_harness(32, None);
@@ -1403,7 +1410,7 @@ mod release_tests {
             state.fork(),
         );
 
-        let best_slashings = op_pool.get_slashings(&state);
+        let best_slashings = op_pool.get_slashings_and_exits(&state, &harness.spec);
         assert_eq!(best_slashings.1, vec![slashing_2, slashing_3]);
     }