From a7b675460d4e82c76fb2a176c8852348b711d6bc Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 16 Oct 2021 05:07:23 +0000 Subject: [PATCH] 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 --- .github/workflows/test-suite.yml | 16 ++- Makefile | 17 ++- beacon_node/beacon_chain/src/beacon_chain.rs | 8 +- beacon_node/beacon_chain/src/test_utils.rs | 8 +- beacon_node/operation_pool/src/attestation.rs | 4 +- beacon_node/operation_pool/src/lib.rs | 121 +++++++++--------- 6 files changed, 100 insertions(+), 74 deletions(-) diff --git a/.github/workflows/test-suite.yml b/.github/workflows/test-suite.yml index 44e876258..a7423e9eb 100644 --- a/.github/workflows/test-suite.yml +++ b/.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 - run: make test-beacon-chain-base - - name: Run beacon_chain tests for Altair hard fork - run: make test-beacon-chain-altair + - name: Run beacon_chain tests for all known forks + run: make test-beacon-chain + op-pool-tests: + 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 diff --git a/Makefile b/Makefile index 6ff132bda..91be0ba03 100644 --- a/Makefile +++ b/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. -test-beacon-chain: test-beacon-chain-base test-beacon-chain-altair +# Run the tests in the `beacon_chain` crate for all known forks. +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 diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 47cabbbfe..16bbf8a74 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -2761,7 +2761,8 @@ impl BeaconChain { 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 BeaconChain { 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, BlockProductionError> { @@ -2853,7 +2853,7 @@ impl BeaconChain { attester_slashings: attester_slashings.into(), attestations, deposits, - voluntary_exits, + voluntary_exits: voluntary_exits.into(), }, }), BeaconState::Altair(_) => { @@ -2871,7 +2871,7 @@ impl BeaconChain { attester_slashings: attester_slashings.into(), attestations, deposits, - voluntary_exits, + voluntary_exits: voluntary_exits.into(), sync_aggregate, }, }) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 1be7d746c..536df8f58 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/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() -> ChainSpec { FORK_NAME_ENV_VAR, e ) }); - let fork = match fork_name.as_str() { - "base" => ForkName::Base, - "altair" => ForkName::Altair, - other => panic!("unknown FORK_NAME: {}", other), - }; + let fork = ForkName::from_str(fork_name.as_str()) + .unwrap_or_else(|()| panic!("unknown FORK_NAME: {}", fork_name)); fork.make_genesis_spec(E::default_spec()) } else { E::default_spec() diff --git a/beacon_node/operation_pool/src/attestation.rs b/beacon_node/operation_pool/src/attestation.rs index 2ed580cae..11537e6ec 100644 --- a/beacon_node/operation_pool/src/attestation.rs +++ b/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, + pub att: &'a Attestation, /// Mapping of validator indices and their rewards. - fresh_validators_rewards: HashMap, + pub fresh_validators_rewards: HashMap, } impl<'a, T: EthSpec> AttMaxCover<'a, T> { diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index dad375b32..0b979c9f4 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -354,10 +354,15 @@ impl OperationPool { /// 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, - ) -> (Vec, Vec>) { + spec: &ChainSpec, + ) -> ( + Vec, + Vec>, + Vec, + ) { let proposer_slashings = filter_limit_operations( self.proposer_slashings.read().values(), |slashing| { @@ -371,7 +376,7 @@ impl OperationPool { // 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::>(); @@ -391,10 +396,19 @@ impl OperationPool { 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 OperationPool { } /// Get a list of voluntary exits for inclusion in a block. - pub fn get_voluntary_exits( + fn get_voluntary_exits( &self, state: &BeaconState, + filter: F, spec: &ChainSpec, - ) -> Vec { + ) -> Vec + 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::{ - common::{base::get_base_reward, get_attesting_indices}, - VerifyOperation, - }; + use state_processing::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( num_committees: usize, ) -> (BeaconChainHarness>, ChainSpec) { - let spec = E::default_spec(); + let spec = test_spec::(); let num_validators = num_committees * E::slots_per_epoch() as usize * spec.target_committee_size; - let harness = get_harness::(num_validators, None); + let harness = get_harness::(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::(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::::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 = - earliest_attestation_validators(att, &state, state.as_base().unwrap()); - 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::( - 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, - ) + let mut fresh_validators_rewards = + AttMaxCover::new(att, &state, total_active_balance, spec) .unwrap() - / spec.proposer_reward_quotient - }) - .sum(); + .fresh_validators_rewards; + + // 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]); }