cerc-io/lighthouse
16
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

Update attestation gossip verification for v0.12 (#1236)

Browse Source
This commit is contained in:
Michael Sproul 2020-06-05 11:32:46 +10:00 committed by GitHub
parent fe03ff0f21
commit 52d60cce1d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 58 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
beacon_node/beacon_chain/src/attestation_verification.rs
View File

@ -235,12 +235,7 @@ impl<T: BeaconChainTypes> VerifiedAggregatedAttestation<T> {
// We do not queue future attestations for later processing. // We do not queue future attestations for later processing.
verify_propagation_slot_range(chain, attestation)?; verify_propagation_slot_range(chain, attestation)?;
// Ensure the aggregated attestation has not already been seen locally. // Ensure the valid aggregated attestation has not already been seen locally.
//
// TODO: this part of the code is not technically to spec, however I have raised a PR to
// change it:
//
// https://github.com/ethereum/eth2.0-specs/pull/1749
let attestation_root = attestation.tree_hash_root(); let attestation_root = attestation.tree_hash_root();
if chain if chain
.observed_attestations .observed_attestations
@ -278,6 +273,11 @@ impl<T: BeaconChainTypes> VerifiedAggregatedAttestation<T> {
// attestation and do not delay consideration for later. // attestation and do not delay consideration for later.
verify_head_block_is_known(chain, &attestation)?; verify_head_block_is_known(chain, &attestation)?;
// Ensure that the attestation has participants.
if attestation.aggregation_bits.is_zero() {
return Err(Error::EmptyAggregationBitfield);
}
let indexed_attestation = map_attestation_committee(chain, attestation, |committee| { let indexed_attestation = map_attestation_committee(chain, attestation, |committee| {
// Note: this clones the signature which is known to be a relatively slow operation. // Note: this clones the signature which is known to be a relatively slow operation.
// //
@ -292,42 +292,16 @@ impl<T: BeaconChainTypes> VerifiedAggregatedAttestation<T> {
return Err(Error::InvalidSelectionProof { aggregator_index }); return Err(Error::InvalidSelectionProof { aggregator_index });
} }
/* // Ensure the aggregator is a member of the committee for which it is aggregating.
* I have raised a PR that will likely get merged in v0.12.0: if !committee.committee.contains(&(aggregator_index as usize)) {
*
* https://github.com/ethereum/eth2.0-specs/pull/1732
*
* If this PR gets merged, uncomment this code and remove the code below.
*
if !committee
.committee
.iter()
.any(|validator_index| *validator_index as u64 == aggregator_index)
{
return Err(Error::AggregatorNotInCommittee { aggregator_index }); return Err(Error::AggregatorNotInCommittee { aggregator_index });
} }
*/
get_indexed_attestation(committee.committee, &attestation) get_indexed_attestation(committee.committee, &attestation)
.map_err(|e| BeaconChainError::from(e).into()) .map_err(|e| BeaconChainError::from(e).into())
})?; })?;
// Ensure the aggregator is in the attestation. // Ensure that all signatures are valid.
//
// I've raised an issue with this here:
//
// https://github.com/ethereum/eth2.0-specs/pull/1732
//
// I suspect PR my will get merged in v0.12 and we'll need to delete this code and
// uncomment the code above.
if !indexed_attestation
.attesting_indices
.iter()
.any(|validator_index| *validator_index as u64 == aggregator_index)
{
return Err(Error::AggregatorNotInCommittee { aggregator_index });
}
if !verify_signed_aggregate_signatures(chain, &signed_aggregate, &indexed_attestation)? { if !verify_signed_aggregate_signatures(chain, &signed_aggregate, &indexed_attestation)? {
return Err(Error::InvalidSignature); return Err(Error::InvalidSignature);
} }
@ -691,8 +665,8 @@ pub fn verify_attestation_signature<T: BeaconChainTypes>(
/// includes three signatures: /// includes three signatures:
/// ///
/// - `signed_aggregate.signature` /// - `signed_aggregate.signature`
/// - `signed_aggregate.signature.message.selection proof` /// - `signed_aggregate.message.selection_proof`
/// - `signed_aggregate.signature.message.aggregate.signature` /// - `signed_aggregate.message.aggregate.signature`
/// ///
/// # Returns /// # Returns
/// ///

87
beacon_node/beacon_chain/tests/attestation_verification.rs
View File

@ -87,7 +87,7 @@ fn get_valid_unaggregated_attestation<T: BeaconChainTypes>(
fn get_valid_aggregated_attestation<T: BeaconChainTypes>( fn get_valid_aggregated_attestation<T: BeaconChainTypes>(
chain: &BeaconChain<T>, chain: &BeaconChain<T>,
mut aggregate: Attestation<T::EthSpec>, aggregate: Attestation<T::EthSpec>,
) -> (SignedAggregateAndProof<T::EthSpec>, usize, SecretKey) { ) -> (SignedAggregateAndProof<T::EthSpec>, usize, SecretKey) {
let state = &chain.head().expect("should get head").beacon_state; let state = &chain.head().expect("should get head").beacon_state;
let current_slot = chain.slot().expect("should get slot"); let current_slot = chain.slot().expect("should get slot");
@ -97,11 +97,10 @@ fn get_valid_aggregated_attestation<T: BeaconChainTypes>(
.expect("should get committees"); .expect("should get committees");
let committee_len = committee.committee.len(); let committee_len = committee.committee.len();
let (aggregator_committee_pos, aggregator_index, aggregator_sk) = committee let (aggregator_index, aggregator_sk) = committee
.committee .committee
.iter() .iter()
.enumerate() .find_map(|&val_index| {
.find_map(|(committee_pos, &val_index)| {
let aggregator_sk = generate_deterministic_keypair(val_index).sk; let aggregator_sk = generate_deterministic_keypair(val_index).sk;
let proof = SelectionProof::new::<T::EthSpec>( let proof = SelectionProof::new::<T::EthSpec>(
@ -113,26 +112,13 @@ fn get_valid_aggregated_attestation<T: BeaconChainTypes>(
); );
if proof.is_aggregator(committee_len, &chain.spec).unwrap() { if proof.is_aggregator(committee_len, &chain.spec).unwrap() {
Some((committee_pos, val_index, aggregator_sk)) Some((val_index, aggregator_sk))
} else { } else {
None None
} }
}) })
.expect("should find aggregator for committee"); .expect("should find aggregator for committee");
// FIXME(v0.12): this can be removed once the verification rules are updated for v0.12
// I needed to add it because the test only *happened* to work because aggregator and attester
// indices were the same before!
aggregate
.sign(
&aggregator_sk,
aggregator_committee_pos,
&state.fork,
chain.genesis_validators_root,
&chain.spec,
)
.expect("should sign attestation");
let signed_aggregate = SignedAggregateAndProof::from_aggregate( let signed_aggregate = SignedAggregateAndProof::from_aggregate(
aggregator_index as u64, aggregator_index as u64,
aggregate, aggregate,
@ -235,7 +221,7 @@ fn aggregated_gossip_verification() {
/* /*
* The following two tests ensure: * The following two tests ensure:
* *
* Spec v0.11.2 * Spec v0.12.0
* *
* aggregate.data.slot is within the last ATTESTATION_PROPAGATION_SLOT_RANGE slots (with a * aggregate.data.slot is within the last ATTESTATION_PROPAGATION_SLOT_RANGE slots (with a
* MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance) -- i.e. aggregate.data.slot + * MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance) -- i.e. aggregate.data.slot +
@ -273,13 +259,14 @@ fn aggregated_gossip_verification() {
// slot and the propagation tolerance will allow an extra slot. // slot and the propagation tolerance will allow an extra slot.
earliest_permissible_slot earliest_permissible_slot
} }
if attestation_slot == early_slot && earliest_permissible_slot == current_slot - E::slots_per_epoch() - 1 if attestation_slot == early_slot
&& earliest_permissible_slot == current_slot - E::slots_per_epoch() - 1
); );
/* /*
* The following test ensures: * The following test ensures:
* *
* Spec v0.11.2 * Spec v0.12.0
* *
* The block being voted for (aggregate.data.beacon_block_root) passes validation. * The block being voted for (aggregate.data.beacon_block_root) passes validation.
*/ */
@ -298,10 +285,31 @@ fn aggregated_gossip_verification() {
if beacon_block_root == unknown_root if beacon_block_root == unknown_root
); );
/*
* The following test ensures:
*
* Spec v0.12.0
*
* The attestation has participants.
*/
assert_invalid!(
"aggregate with no participants",
{
let mut a = valid_aggregate.clone();
let aggregation_bits = &mut a.message.aggregate.aggregation_bits;
aggregation_bits.difference_inplace(&aggregation_bits.clone());
assert!(aggregation_bits.is_zero());
a.message.aggregate.signature = AggregateSignature::new();
a
},
AttnError::EmptyAggregationBitfield
);
/* /*
* This test ensures: * This test ensures:
* *
* Spec v0.11.2 * Spec v0.12.0
* *
* The aggregator signature, signed_aggregate_and_proof.signature, is valid. * The aggregator signature, signed_aggregate_and_proof.signature, is valid.
*/ */
@ -321,7 +329,7 @@ fn aggregated_gossip_verification() {
/* /*
* The following test ensures: * The following test ensures:
* *
* Spec v0.11.2 * Spec v0.12.0
* *
* The aggregate_and_proof.selection_proof is a valid signature of the aggregate.data.slot by * The aggregate_and_proof.selection_proof is a valid signature of the aggregate.data.slot by
* the validator with index aggregate_and_proof.aggregator_index. * the validator with index aggregate_and_proof.aggregator_index.
@ -368,7 +376,7 @@ fn aggregated_gossip_verification() {
/* /*
* The following test ensures: * The following test ensures:
* *
* Spec v0.11.2 * Spec v0.12.0
* *
* The signature of aggregate is valid. * The signature of aggregate is valid.
*/ */
@ -402,11 +410,11 @@ fn aggregated_gossip_verification() {
/* /*
* The following test ensures: * The following test ensures:
* *
* Spec v0.11.2 * Spec v0.12.0
* *
* The aggregator's validator index is within the aggregate's committee -- i.e. * The aggregator's validator index is within the committee -- i.e.
* aggregate_and_proof.aggregator_index in get_attesting_indices(state, aggregate.data, * aggregate_and_proof.aggregator_index in get_beacon_committee(state, aggregate.data.slot,
* aggregate.aggregation_bits). * aggregate.data.index).
*/ */
let unknown_validator = VALIDATOR_COUNT as u64; let unknown_validator = VALIDATOR_COUNT as u64;
@ -431,7 +439,7 @@ fn aggregated_gossip_verification() {
/* /*
* The following test ensures: * The following test ensures:
* *
* Spec v0.11.2 * Spec v0.12.0
* *
* aggregate_and_proof.selection_proof selects the validator as an aggregator for the slot -- * aggregate_and_proof.selection_proof selects the validator as an aggregator for the slot --
* i.e. is_aggregator(state, aggregate.data.slot, aggregate.data.index, * i.e. is_aggregator(state, aggregate.data.slot, aggregate.data.index,
@ -441,7 +449,7 @@ fn aggregated_gossip_verification() {
let (non_aggregator_index, non_aggregator_sk) = let (non_aggregator_index, non_aggregator_sk) =
get_non_aggregator(&harness.chain, &valid_aggregate.message.aggregate); get_non_aggregator(&harness.chain, &valid_aggregate.message.aggregate);
assert_invalid!( assert_invalid!(
"aggregate with from non-aggregator", "aggregate from non-aggregator",
{ {
SignedAggregateAndProof::from_aggregate( SignedAggregateAndProof::from_aggregate(
non_aggregator_index as u64, non_aggregator_index as u64,
@ -459,6 +467,8 @@ fn aggregated_gossip_verification() {
if index == non_aggregator_index as u64 if index == non_aggregator_index as u64
); );
// NOTE: from here on, the tests are stateful, and rely on the valid attestation having been
// seen. A refactor to give each test case its own state might be nice at some point
assert!( assert!(
harness harness
.chain .chain
@ -468,20 +478,17 @@ fn aggregated_gossip_verification() {
); );
/* /*
* The following tests ensures: * The following test ensures:
* *
* NOTE: this is a slight deviation from the spec, see: * Spec v0.12.0
* https://github.com/ethereum/eth2.0-specs/pull/1749
* *
* Spec v0.11.2 * The valid aggregate attestation defined by hash_tree_root(aggregate) has not already been
* * seen (via aggregate gossip, within a block, or through the creation of an equivalent
* The aggregate attestation defined by hash_tree_root(aggregate) has not already been seen * aggregate locally).
* (via aggregate gossip, within a block, or through the creation of an equivalent aggregate
* locally).
*/ */
assert_invalid!( assert_invalid!(
"aggregate with that has already been seen", "aggregate that has already been seen",
valid_aggregate.clone(), valid_aggregate.clone(),
AttnError::AttestationAlreadyKnown(hash) AttnError::AttestationAlreadyKnown(hash)
if hash == valid_aggregate.message.aggregate.tree_hash_root() if hash == valid_aggregate.message.aggregate.tree_hash_root()
@ -490,7 +497,7 @@ fn aggregated_gossip_verification() {
/* /*
* The following test ensures: * The following test ensures:
* *
* Spec v0.11.2 * Spec v0.12.0
* *
* The aggregate is the first valid aggregate received for the aggregator with index * The aggregate is the first valid aggregate received for the aggregator with index
* aggregate_and_proof.aggregator_index for the epoch aggregate.data.target.epoch. * aggregate_and_proof.aggregator_index for the epoch aggregate.data.target.epoch.