From 018a6667316d2622f0b6bf3bc8665e6a4822a887 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Wed, 22 Apr 2020 14:46:19 +1000 Subject: [PATCH] Address clippy arith lints (#1038) --- beacon_node/rest_api/src/helpers.rs | 15 +++++++++++---- beacon_node/rest_api/src/validator.rs | 7 ++++--- eth2/types/src/attestation.rs | 10 ++++++++-- eth2/types/src/beacon_state.rs | 5 +++-- eth2/types/src/selection_proof.rs | 5 +++-- validator_client/src/attestation_service.rs | 6 ++++-- 6 files changed, 33 insertions(+), 15 deletions(-) diff --git a/beacon_node/rest_api/src/helpers.rs b/beacon_node/rest_api/src/helpers.rs index 893d583c2..661b561c8 100644 --- a/beacon_node/rest_api/src/helpers.rs +++ b/beacon_node/rest_api/src/helpers.rs @@ -9,7 +9,7 @@ use network::NetworkMessage; use ssz::Decode; use store::{iter::AncestorIter, Store}; use types::{ - Attestation, BeaconState, CommitteeIndex, Epoch, EthSpec, Hash256, RelativeEpoch, + Attestation, BeaconState, ChainSpec, CommitteeIndex, Epoch, EthSpec, Hash256, RelativeEpoch, SignedAggregateAndProof, SignedBeaconBlock, Slot, }; @@ -251,15 +251,22 @@ pub fn publish_beacon_block_to_network( pub fn publish_raw_attestations_to_network( mut chan: NetworkChannel, attestations: Vec>, + spec: &ChainSpec, ) -> Result<(), ApiError> { let messages = attestations .into_iter() .map(|attestation| { // create the gossip message to send to the network - let subnet_id = attestation.subnet_id(); - PubsubMessage::Attestation(Box::new((subnet_id, attestation))) + let subnet_id = attestation + .subnet_id(spec) + .map_err(|e| ApiError::ServerError(format!("Unable to get subnet id: {:?}", e)))?; + + Ok(PubsubMessage::Attestation(Box::new(( + subnet_id, + attestation, + )))) }) - .collect::>(); + .collect::, ApiError>>()?; // Publish the attestations to the p2p network via gossipsub. if let Err(e) = chan.try_send(NetworkMessage::Publish { messages }) { diff --git a/beacon_node/rest_api/src/validator.rs b/beacon_node/rest_api/src/validator.rs index 55ab8ccef..609a52e64 100644 --- a/beacon_node/rest_api/src/validator.rs +++ b/beacon_node/rest_api/src/validator.rs @@ -507,10 +507,11 @@ pub fn publish_attestations( } } })?; - Ok(attestations) + + Ok((attestations, beacon_chain)) }) - .and_then(|attestations| { - publish_raw_attestations_to_network::(network_chan, attestations) + .and_then(|(attestations, beacon_chain)| { + publish_raw_attestations_to_network::(network_chan, attestations, &beacon_chain.spec) }) .and_then(|_| response_builder?.body_no_ssz(&())), ) diff --git a/eth2/types/src/attestation.rs b/eth2/types/src/attestation.rs index 8fb38c837..34c11f26f 100644 --- a/eth2/types/src/attestation.rs +++ b/eth2/types/src/attestation.rs @@ -3,6 +3,7 @@ use super::{ Signature, SignedRoot, SubnetId, }; use crate::{test_utils::TestRandom, Hash256}; +use safe_arith::{ArithError, SafeArith}; use serde_derive::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; @@ -13,6 +14,7 @@ use tree_hash_derive::TreeHash; pub enum Error { SszTypesError(ssz_types::Error), AlreadySigned(usize), + SubnetCountIsZero(ArithError), } /// Details an attestation that can be slashable. @@ -86,8 +88,12 @@ impl Attestation { /// /// Note, this will return the subnet id for an aggregated attestation. This is done /// to avoid checking aggregate bits every time we wish to get an id. - pub fn subnet_id(&self) -> SubnetId { - SubnetId::new(self.data.index % T::default_spec().attestation_subnet_count) + pub fn subnet_id(&self, spec: &ChainSpec) -> Result { + self.data + .index + .safe_rem(spec.attestation_subnet_count) + .map(SubnetId::new) + .map_err(Error::SubnetCountIsZero) } } diff --git a/eth2/types/src/beacon_state.rs b/eth2/types/src/beacon_state.rs index 02e0d31bc..4f980a019 100644 --- a/eth2/types/src/beacon_state.rs +++ b/eth2/types/src/beacon_state.rs @@ -485,7 +485,7 @@ impl BeaconState { let committee = self.get_beacon_committee(slot, index)?; let modulo = std::cmp::max( 1, - committee.committee.len() as u64 / spec.target_aggregators_per_committee, + (committee.committee.len() as u64).safe_div(spec.target_aggregators_per_committee)?, ); let signature_hash = hash(&slot_signature.as_bytes()); let signature_hash_int = u64::from_le_bytes( @@ -493,7 +493,8 @@ impl BeaconState { .try_into() .expect("first 8 bytes of signature should always convert to fixed array"), ); - Ok(signature_hash_int % modulo == 0) + + Ok(signature_hash_int.safe_rem(modulo)? == 0) } /// Returns the beacon proposer index for the `slot` in the given `relative_epoch`. diff --git a/eth2/types/src/selection_proof.rs b/eth2/types/src/selection_proof.rs index 02d24821a..18c62ba40 100644 --- a/eth2/types/src/selection_proof.rs +++ b/eth2/types/src/selection_proof.rs @@ -1,4 +1,5 @@ use crate::{ChainSpec, Domain, EthSpec, Fork, Hash256, SecretKey, Signature, SignedRoot, Slot}; +use safe_arith::{ArithError, SafeArith}; use std::convert::TryInto; use tree_hash::TreeHash; @@ -24,7 +25,7 @@ impl SelectionProof { Self(Signature::new(message.as_bytes(), secret_key)) } - pub fn is_aggregator(&self, modulo: u64) -> bool { + pub fn is_aggregator(&self, modulo: u64) -> Result { let signature_hash = self.0.tree_hash_root(); let signature_hash_int = u64::from_le_bytes( signature_hash[0..8] @@ -33,7 +34,7 @@ impl SelectionProof { .expect("first 8 bytes of signature should always convert to fixed array"), ); - signature_hash_int % modulo == 0 + signature_hash_int.safe_rem(modulo).map(|rem| rem == 0) } } diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index c8220775d..c1842519b 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -271,12 +271,14 @@ impl AttestationService { .validator_store .produce_selection_proof(duty.validator_pubkey(), slot)?; let modulo = duty.duty.aggregator_modulo?; - let subscription = ValidatorSubscription { validator_index, attestation_committee_index, slot, - is_aggregator: selection_proof.is_aggregator(modulo), + is_aggregator: selection_proof + .is_aggregator(modulo) + .map_err(|e| crit!(log_1, "Unable to determine aggregator: {:?}", e)) + .ok()?, }; Some((subscription, (duty, selection_proof)))