From c761fbc3181fcf94ef9201a8174806bacd859caa Mon Sep 17 00:00:00 2001 From: Justin Date: Thu, 2 May 2019 09:24:24 +0100 Subject: [PATCH 1/4] Clean up verify_indexed_attestation Cosmetic changes: * Add 4 lines of comments (now every statement has a comment) * Avoid unnecessary `assert` (the end goal for me is for `assert`s to be exclusive to the operation processing helpers). * Merge `return`s into one (increase readability, reduce verbosity) * Use shorter-named `bit_0_indices` and `bit_1_indices` helper variables Substantive change: * Remove the condition that `len(0_indices) + len(1_indices) > 0`. This condition is redundant in the context of `process_attester_slashing` because of `slashed_any`. It is largely artificial in `process_attestation` where validators are incentivised to maximise new attestations. --- specs/core/0_beacon-chain.md | 54 +++++++++++++++++------------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/specs/core/0_beacon-chain.md b/specs/core/0_beacon-chain.md index 2a0b0c11d..963c2ac44 100644 --- a/specs/core/0_beacon-chain.md +++ b/specs/core/0_beacon-chain.md @@ -1018,37 +1018,33 @@ def convert_to_indexed(state: BeaconState, attestation: Attestation) -> IndexedA ```python def verify_indexed_attestation(state: BeaconState, indexed_attestation: IndexedAttestation) -> bool: """ - Verify validity of ``indexed_attestation`` fields. + Verify validity of ``indexed_attestation``. """ - custody_bit_0_indices = indexed_attestation.custody_bit_0_indices - custody_bit_1_indices = indexed_attestation.custody_bit_1_indices + bit_0_indices = indexed_attestation.custody_bit_0_indices + bit_1_indices = indexed_attestation.custody_bit_1_indices - # Ensure no duplicate indices across custody bits - assert len(set(custody_bit_0_indices).intersection(set(custody_bit_1_indices))) == 0 - - if len(custody_bit_1_indices) > 0: # [TO BE REMOVED IN PHASE 1] - return False - - if not (1 <= len(custody_bit_0_indices) + len(custody_bit_1_indices) <= MAX_INDICES_PER_ATTESTATION): - return False - - if custody_bit_0_indices != sorted(custody_bit_0_indices): - return False - - if custody_bit_1_indices != sorted(custody_bit_1_indices): - return False - - return bls_verify_multiple( - pubkeys=[ - bls_aggregate_pubkeys([state.validator_registry[i].pubkey for i in custody_bit_0_indices]), - bls_aggregate_pubkeys([state.validator_registry[i].pubkey for i in custody_bit_1_indices]), - ], - message_hashes=[ - hash_tree_root(AttestationDataAndCustodyBit(data=indexed_attestation.data, custody_bit=0b0)), - hash_tree_root(AttestationDataAndCustodyBit(data=indexed_attestation.data, custody_bit=0b1)), - ], - signature=indexed_attestation.signature, - domain=get_domain(state, DOMAIN_ATTESTATION, indexed_attestation.data.target_epoch), + return ( + # Verify no index has custody bit equal to 1 [to be removed in phase 1] + len(bit_1_indices) == 0 and + # Verify max number of indices + len(bit_0_indices) + len(bit_1_indices) <= MAX_INDICES_PER_ATTESTATION and + # Verify index sets are disjoint + len(set(bit_0_indices).intersection(bit_1_indices)) == 0 and + # Verify indices are sorted + bit_0_indices == sorted(bit_0_indices) and bit_1_indices == sorted(bit_1_indices) and + # Verify aggregate signature + bls_verify_multiple( + pubkeys=[ + bls_aggregate_pubkeys([state.validator_registry[i].pubkey for i in bit_0_indices]), + bls_aggregate_pubkeys([state.validator_registry[i].pubkey for i in bit_1_indices]), + ], + message_hashes=[ + hash_tree_root(AttestationDataAndCustodyBit(data=indexed_attestation.data, custody_bit=0b0)), + hash_tree_root(AttestationDataAndCustodyBit(data=indexed_attestation.data, custody_bit=0b1)), + ], + signature=indexed_attestation.signature, + domain=get_domain(state, DOMAIN_ATTESTATION, indexed_attestation.data.target_epoch), + ) ) ``` From 973f07223537e33dd05587b895143eb45ee6e3d0 Mon Sep 17 00:00:00 2001 From: Justin Drake Date: Thu, 2 May 2019 09:25:29 +0100 Subject: [PATCH 2/4] Remove unnecessary test --- .../block_processing/test_process_attestation.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/test_libs/pyspec/tests/block_processing/test_process_attestation.py b/test_libs/pyspec/tests/block_processing/test_process_attestation.py index bcf71376c..165f0c84a 100644 --- a/test_libs/pyspec/tests/block_processing/test_process_attestation.py +++ b/test_libs/pyspec/tests/block_processing/test_process_attestation.py @@ -142,14 +142,3 @@ def test_non_empty_custody_bitfield(state): pre_state, post_state = run_attestation_processing(state, attestation, False) return pre_state, attestation, post_state - - -def test_empty_aggregation_bitfield(state): - attestation = get_valid_attestation(state) - state.slot += spec.MIN_ATTESTATION_INCLUSION_DELAY - - attestation.aggregation_bitfield = b'\x00' * len(attestation.aggregation_bitfield) - - pre_state, post_state = run_attestation_processing(state, attestation, False) - - return pre_state, attestation, post_state From 62c44ffce3068b0e9d9146c3e4ca30deb68583eb Mon Sep 17 00:00:00 2001 From: Justin Drake Date: Tue, 7 May 2019 17:34:19 +0100 Subject: [PATCH 3/4] Refactor to validate_indexed_attestation --- specs/core/0_beacon-chain.md | 54 +++++++++++++++++------------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/specs/core/0_beacon-chain.md b/specs/core/0_beacon-chain.md index 963c2ac44..27c7fd461 100644 --- a/specs/core/0_beacon-chain.md +++ b/specs/core/0_beacon-chain.md @@ -82,7 +82,7 @@ - [`get_bitfield_bit`](#get_bitfield_bit) - [`verify_bitfield`](#verify_bitfield) - [`convert_to_indexed`](#convert_to_indexed) - - [`verify_indexed_attestation`](#verify_indexed_attestation) + - [`validate_indexed_attestation`](#validate_indexed_attestation) - [`is_slashable_attestation_data`](#is_slashable_attestation_data) - [`integer_squareroot`](#integer_squareroot) - [`get_delayed_activation_exit_epoch`](#get_delayed_activation_exit_epoch) @@ -1013,38 +1013,36 @@ def convert_to_indexed(state: BeaconState, attestation: Attestation) -> IndexedA ) ``` -### `verify_indexed_attestation` +### `validate_indexed_attestation` ```python -def verify_indexed_attestation(state: BeaconState, indexed_attestation: IndexedAttestation) -> bool: +def validate_indexed_attestation(state: BeaconState, indexed_attestation: IndexedAttestation) -> None: """ Verify validity of ``indexed_attestation``. """ bit_0_indices = indexed_attestation.custody_bit_0_indices bit_1_indices = indexed_attestation.custody_bit_1_indices - return ( - # Verify no index has custody bit equal to 1 [to be removed in phase 1] - len(bit_1_indices) == 0 and - # Verify max number of indices - len(bit_0_indices) + len(bit_1_indices) <= MAX_INDICES_PER_ATTESTATION and - # Verify index sets are disjoint - len(set(bit_0_indices).intersection(bit_1_indices)) == 0 and - # Verify indices are sorted - bit_0_indices == sorted(bit_0_indices) and bit_1_indices == sorted(bit_1_indices) and - # Verify aggregate signature - bls_verify_multiple( - pubkeys=[ - bls_aggregate_pubkeys([state.validator_registry[i].pubkey for i in bit_0_indices]), - bls_aggregate_pubkeys([state.validator_registry[i].pubkey for i in bit_1_indices]), - ], - message_hashes=[ - hash_tree_root(AttestationDataAndCustodyBit(data=indexed_attestation.data, custody_bit=0b0)), - hash_tree_root(AttestationDataAndCustodyBit(data=indexed_attestation.data, custody_bit=0b1)), - ], - signature=indexed_attestation.signature, - domain=get_domain(state, DOMAIN_ATTESTATION, indexed_attestation.data.target_epoch), - ) + # Verify no index has custody bit equal to 1 [to be removed in phase 1] + assert len(bit_1_indices) == 0 + # Verify max number of indices + assert len(bit_0_indices) + len(bit_1_indices) <= MAX_INDICES_PER_ATTESTATION + # Verify index sets are disjoint + assert len(set(bit_0_indices).intersection(bit_1_indices)) == 0 + # Verify indices are sorted + assert bit_0_indices == sorted(bit_0_indices) and bit_1_indices == sorted(bit_1_indices) + # Verify aggregate signature + assert bls_verify_multiple( + pubkeys=[ + bls_aggregate_pubkeys([state.validator_registry[i].pubkey for i in bit_0_indices]), + bls_aggregate_pubkeys([state.validator_registry[i].pubkey for i in bit_1_indices]), + ], + message_hashes=[ + hash_tree_root(AttestationDataAndCustodyBit(data=indexed_attestation.data, custody_bit=0b0)), + hash_tree_root(AttestationDataAndCustodyBit(data=indexed_attestation.data, custody_bit=0b1)), + ], + signature=indexed_attestation.signature, + domain=get_domain(state, DOMAIN_ATTESTATION, indexed_attestation.data.target_epoch), ) ``` @@ -1669,8 +1667,8 @@ def process_attester_slashing(state: BeaconState, attestation_1 = attester_slashing.attestation_1 attestation_2 = attester_slashing.attestation_2 assert is_slashable_attestation_data(attestation_1.data, attestation_2.data) - assert verify_indexed_attestation(state, attestation_1) - assert verify_indexed_attestation(state, attestation_2) + validate_indexed_attestation(state, attestation_1) + validate_indexed_attestation(state, attestation_2) slashed_any = False attesting_indices_1 = attestation_1.custody_bit_0_indices + attestation_1.custody_bit_1_indices @@ -1707,7 +1705,7 @@ def process_attestation(state: BeaconState, attestation: Attestation) -> None: assert data.crosslink_data_root == ZERO_HASH # [to be removed in phase 1] # Check signature and bitfields - assert verify_indexed_attestation(state, convert_to_indexed(state, attestation)) + validate_indexed_attestation(state, convert_to_indexed(state, attestation)) # Cache pending attestation pending_attestation = PendingAttestation( From 513c44bd3d19c70da61359376dc440fc86bddc23 Mon Sep 17 00:00:00 2001 From: Danny Ryan Date: Wed, 8 May 2019 08:38:14 -0600 Subject: [PATCH 4/4] add back in empty attestation test --- .../block_processing/test_process_attestation.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test_libs/pyspec/tests/block_processing/test_process_attestation.py b/test_libs/pyspec/tests/block_processing/test_process_attestation.py index 165f0c84a..c986cc4c8 100644 --- a/test_libs/pyspec/tests/block_processing/test_process_attestation.py +++ b/test_libs/pyspec/tests/block_processing/test_process_attestation.py @@ -142,3 +142,14 @@ def test_non_empty_custody_bitfield(state): pre_state, post_state = run_attestation_processing(state, attestation, False) return pre_state, attestation, post_state + + +def test_empty_aggregation_bitfield(state): + attestation = get_valid_attestation(state) + state.slot += spec.MIN_ATTESTATION_INCLUSION_DELAY + + attestation.aggregation_bitfield = b'\x00' * len(attestation.aggregation_bitfield) + + pre_state, post_state = run_attestation_processing(state, attestation) + + return pre_state, attestation, post_state