Fix duplicate proposer slashing bug (#1086)

Remove parallelism from proposer slashing verification.

Closes #1065

eth2/operation_pool/src/lib.rs

@@ -485,7 +485,7 @@ mod release_tests {
         state_builder.teleport_to_slot(slot);
         state_builder.build_caches(&spec).unwrap();
         let (state, keypairs) = state_builder.build();
-        (state, keypairs, MainnetEthSpec::default_spec())
+        (state, keypairs, spec)
     }
 
     #[test]
@@ -890,4 +890,44 @@ mod release_tests {
             seen_indices.extend(fresh_indices);
         }
     }
+
+    /// Insert two slashings for the same proposer and ensure only one is returned.
+    #[test]
+    fn duplicate_proposer_slashing() {
+        let spec = MainnetEthSpec::default_spec();
+        let num_validators = 32;
+        let mut state_builder =
+            TestingBeaconStateBuilder::<MainnetEthSpec>::from_default_keypairs_file_if_exists(
+                num_validators,
+                &spec,
+            );
+        state_builder.build_caches(&spec).unwrap();
+        let (state, keypairs) = state_builder.build();
+        let op_pool = OperationPool::new();
+
+        let proposer_index = 0;
+        let slashing1 = TestingProposerSlashingBuilder::double_vote::<MainnetEthSpec>(
+            ProposerSlashingTestTask::Valid,
+            proposer_index,
+            &keypairs[proposer_index as usize].sk,
+            &state.fork,
+            state.genesis_validators_root,
+            &spec,
+        );
+        let slashing2 = ProposerSlashing {
+            signed_header_1: slashing1.signed_header_2.clone(),
+            signed_header_2: slashing1.signed_header_1.clone(),
+        };
+
+        // Both slashings should be accepted by the pool.
+        op_pool
+            .insert_proposer_slashing(slashing1.clone(), &state, &spec)
+            .unwrap();
+        op_pool
+            .insert_proposer_slashing(slashing2.clone(), &state, &spec)
+            .unwrap();
+
+        // Should only get the second slashing back.
+        assert_eq!(op_pool.get_slashings(&state, &spec).0, vec![slashing2]);
+    }
 }

eth2/state_processing/src/per_block_processing.rs

@@ -283,26 +283,25 @@ pub fn process_proposer_slashings<T: EthSpec>(
     verify_signatures: VerifySignatures,
     spec: &ChainSpec,
 ) -> Result<(), BlockProcessingError> {
-    // Verify proposer slashings in parallel.
+    // Verify and apply proposer slashings in series.
+    // We have to verify in series because an invalid block may contain multiple slashings
+    // for the same validator, and we need to correctly detect and reject that.
     proposer_slashings
-        .par_iter()
+        .into_iter()
         .enumerate()
         .try_for_each(|(i, proposer_slashing)| {
             verify_proposer_slashing(proposer_slashing, &state, verify_signatures, spec)
-                .map_err(|e| e.into_with_index(i))
-        })?;
+                .map_err(|e| e.into_with_index(i))?;
 
-    // Update the state.
-    for proposer_slashing in proposer_slashings {
-        slash_validator(
-            state,
-            proposer_slashing.signed_header_1.message.proposer_index as usize,
-            None,
-            spec,
-        )?;
-    }
+            slash_validator(
+                state,
+                proposer_slashing.signed_header_1.message.proposer_index as usize,
+                None,
+                spec,
+            )?;
 
-    Ok(())
+            Ok(())
+        })
 }
 
 /// Validates each `AttesterSlashing` and updates the state, short-circuiting on an invalid object.

eth2/state_processing/src/per_block_processing/tests.rs

@@ -1029,6 +1029,37 @@ fn invalid_proposer_slashing_not_slashable() {
     );
 }
 
+#[test]
+fn invalid_proposer_slashing_duplicate_slashing() {
+    let spec = MainnetEthSpec::default_spec();
+    let builder = get_builder(&spec, SLOT_OFFSET, VALIDATOR_COUNT);
+    let test_task = ProposerSlashingTestTask::Valid;
+    let (mut block, mut state) =
+        builder.build_with_proposer_slashing(test_task, 1, None, None, &spec);
+
+    let slashing = block.message.body.proposer_slashings[0].clone();
+    let slashed_proposer = slashing.signed_header_1.message.proposer_index;
+    block.message.body.proposer_slashings.push(slashing);
+
+    let result = per_block_processing(
+        &mut state,
+        &block,
+        None,
+        BlockSignatureStrategy::NoVerification,
+        &spec,
+    );
+
+    // Expecting ProposerNotSlashable for the 2nd slashing because the validator has been
+    // slashed by the 1st slashing.
+    assert_eq!(
+        result,
+        Err(BlockProcessingError::ProposerSlashingInvalid {
+            index: 1,
+            reason: ProposerSlashingInvalid::ProposerNotSlashable(slashed_proposer)
+        })
+    );
+}
+
 #[test]
 fn invalid_bad_proposal_1_signature() {
     let spec = MainnetEthSpec::default_spec();