From ddd9654f7001adb1ac254a9cefa76c297af91a45 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 1 Apr 2019 13:34:43 +1100 Subject: [PATCH] op-pool: fix bug in attestation_score The attestation scoring function was looking only at the previous epoch, but should really look at whichever epoch is appropriate for a given attestation. We also avoid including attestations that don't pay us any reward, as they simply bloat the chain. --- eth2/operation_pool/src/lib.rs | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/eth2/operation_pool/src/lib.rs b/eth2/operation_pool/src/lib.rs index c42527b60..0c5d78fe4 100644 --- a/eth2/operation_pool/src/lib.rs +++ b/eth2/operation_pool/src/lib.rs @@ -74,15 +74,26 @@ impl AttestationId { /// the aggregate attestation introduces, and is proportional to the size of the reward we will /// receive for including it in a block. // TODO: this could be optimised with a map from validator index to whether that validator has -// attested in the *current* epoch. Alternatively, we could cache an index that allows us to -// quickly look up the attestations in the current epoch for a given shard. -fn attestation_score(attestation: &Attestation, state: &BeaconState) -> usize { +// attested in each of the current and previous epochs. Currently quadractic in number of validators. +fn attestation_score(attestation: &Attestation, state: &BeaconState, spec: &ChainSpec) -> usize { // Bitfield of validators whose attestations are new/fresh. let mut new_validators = attestation.aggregation_bitfield.clone(); - state - .current_epoch_attestations + let attestation_epoch = attestation.data.slot.epoch(spec.slots_per_epoch); + + let state_attestations = if attestation_epoch == state.current_epoch(spec) { + &state.current_epoch_attestations + } else if attestation_epoch == state.previous_epoch(spec) { + &state.previous_epoch_attestations + } else { + return 0; + }; + + state_attestations .iter() + // In a single epoch, an attester should only be attesting for one shard. + // TODO: we avoid including slashable attestations in the state here, + // but maybe we should do something else with them (like construct slashings). .filter(|current_attestation| current_attestation.data.shard == attestation.data.shard) .for_each(|current_attestation| { // Remove the validators who have signed the existing attestation (they are not new) @@ -176,7 +187,9 @@ impl OperationPool { .filter(|attestation| validate_attestation(state, attestation, spec).is_ok()) // Scored by the number of new attestations they introduce (descending) // TODO: need to consider attestations introduced in THIS block - .map(|att| (att, attestation_score(att, state))) + .map(|att| (att, attestation_score(att, state, spec))) + // Don't include any useless attestations (score 0) + .filter(|&(_, score)| score != 0) .sorted_by_key(|&(_, score)| std::cmp::Reverse(score)) // Limited to the maximum number of attestations per block .take(spec.max_attestations as usize) @@ -695,7 +708,7 @@ mod tests { num_committees * (spec.slots_per_epoch * spec.target_committee_size) as usize; let mut state_builder = TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(num_validators, spec); - let slot_offset = 100000; + let slot_offset = 1000 * spec.slots_per_epoch + spec.slots_per_epoch / 2; let slot = spec.genesis_slot + slot_offset; state_builder.teleport_to_slot(slot, spec); state_builder.build_caches(spec).unwrap(); @@ -726,7 +739,7 @@ mod tests { assert_eq!( att1.aggregation_bitfield.num_set_bits(), - attestation_score(&att1, state) + attestation_score(&att1, state, spec) ); state @@ -735,7 +748,7 @@ mod tests { assert_eq!( committee.committee.len() - 2, - attestation_score(&att2, &state) + attestation_score(&att2, state, spec) ); } }