Fix bug with deposit inclusion

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -1361,6 +1361,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         let (proposer_slashings, attester_slashings) =
             self.op_pool.get_slashings(&state, &self.spec);
 
+        let eth1_data = eth1_chain.eth1_data_for_block_production(&state, &self.spec)?;
+        let deposits = eth1_chain
+            .deposits_for_block_inclusion(&state, &eth1_data, &self.spec)?
+            .into();
+
         let mut block = BeaconBlock {
             slot: state.slot,
             parent_root,
@@ -1369,14 +1374,12 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             signature: Signature::empty_signature(),
             body: BeaconBlockBody {
                 randao_reveal,
-                eth1_data: eth1_chain.eth1_data_for_block_production(&state, &self.spec)?,
+                eth1_data,
                 graffiti,
                 proposer_slashings: proposer_slashings.into(),
                 attester_slashings: attester_slashings.into(),
                 attestations: self.op_pool.get_attestations(&state, &self.spec).into(),
-                deposits: eth1_chain
-                    .deposits_for_block_inclusion(&state, &self.spec)?
-                    .into(),
+                deposits,
                 voluntary_exits: self.op_pool.get_voluntary_exits(&state, &self.spec).into(),
             },
         };

beacon_node/beacon_chain/src/eth1_chain.rs

@@ -5,6 +5,7 @@ use futures::Future;
 use integer_sqrt::IntegerSquareRoot;
 use rand::prelude::*;
 use slog::{crit, debug, error, trace, Logger};
+use state_processing::per_block_processing::get_new_eth1_data;
 use std::collections::HashMap;
 use std::iter::DoubleEndedIterator;
 use std::iter::FromIterator;
@@ -89,16 +90,21 @@ where
     /// Returns a list of `Deposits` that may be included in a block.
     ///
     /// Including all of the returned `Deposits` in a block should _not_ cause it to become
-    /// invalid.
+    /// invalid (i.e., this function should respect the maximum).
+    ///
+    /// The `eth1_data_vote` is the `Eth1Data` that the block producer would include in their
+    /// block. This vote may change the `state.eth1_data` value, which would change the deposit
+    /// count and therefore change the output of this function.
     pub fn deposits_for_block_inclusion(
         &self,
         state: &BeaconState<E>,
+        eth1_data_vote: &Eth1Data,
         spec: &ChainSpec,
     ) -> Result<Vec<Deposit>, Error> {
         if self.use_dummy_backend {
-            DummyEth1ChainBackend::default().queued_deposits(state, spec)
+            DummyEth1ChainBackend::default().queued_deposits(state, eth1_data_vote, spec)
         } else {
-            self.backend.queued_deposits(state, spec)
+            self.backend.queued_deposits(state, eth1_data_vote, spec)
         }
     }
 }
@@ -119,6 +125,7 @@ pub trait Eth1ChainBackend<T: EthSpec>: Sized + Send + Sync {
     fn queued_deposits(
         &self,
         beacon_state: &BeaconState<T>,
+        eth1_data_vote: &Eth1Data,
         spec: &ChainSpec,
     ) -> Result<Vec<Deposit>, Error>;
 }
@@ -146,7 +153,12 @@ impl<T: EthSpec> Eth1ChainBackend<T> for DummyEth1ChainBackend<T> {
         })
     }
 
-    fn queued_deposits(&self, _: &BeaconState<T>, _: &ChainSpec) -> Result<Vec<Deposit>, Error> {
+    fn queued_deposits(
+        &self,
+        _: &BeaconState<T>,
+        _: &Eth1Data,
+        _: &ChainSpec,
+    ) -> Result<Vec<Deposit>, Error> {
         Ok(vec![])
     }
 }
@@ -291,10 +303,15 @@ impl<T: EthSpec, S: Store> Eth1ChainBackend<T> for CachingEth1Backend<T, S> {
     fn queued_deposits(
         &self,
         state: &BeaconState<T>,
+        eth1_data_vote: &Eth1Data,
         _spec: &ChainSpec,
     ) -> Result<Vec<Deposit>, Error> {
-        let deposit_count = state.eth1_data.deposit_count;
         let deposit_index = state.eth1_deposit_index;
+        let deposit_count = if let Some(new_eth1_data) = get_new_eth1_data(state, eth1_data_vote) {
+            new_eth1_data.deposit_count
+        } else {
+            state.eth1_data.deposit_count
+        };
 
         if deposit_index > deposit_count {
             Err(Error::DepositIndexTooHigh)
@@ -346,7 +363,7 @@ fn eth1_block_hash_at_start_of_voting_period<T: EthSpec, S: Store>(
 ) -> Result<Hash256, Error> {
     let period = T::SlotsPerEth1VotingPeriod::to_u64();
 
-    if (state.eth1_data_votes.len() as u64) < period / 2 {
+    if !eth1_data_change_is_possible(state) {
         // If there are less than 50% of the votes in the current state, it's impossible that the
         // `eth1_data.block_hash` has changed from the value at `state.eth1_data.block_hash`.
         Ok(state.eth1_data.block_hash)
@@ -367,6 +384,12 @@ fn eth1_block_hash_at_start_of_voting_period<T: EthSpec, S: Store>(
     }
 }
 
+/// Returns true if there are enough eth1 votes in the given `state` to have updated
+/// `state.eth1_data`.
+fn eth1_data_change_is_possible<E: EthSpec>(state: &BeaconState<E>) -> bool {
+    state.eth1_data_votes.len() as u64 >= E::SlotsPerEth1VotingPeriod::to_u64() / 2
+}
+
 /// Calculates and returns `(new_eth1_data, all_eth1_data)` for the given `state`, based upon the
 /// blocks in the `block` iterator.
 ///
@@ -584,7 +607,7 @@ mod test {
 
             assert!(
                 eth1_chain
-                    .deposits_for_block_inclusion(&state, spec)
+                    .deposits_for_block_inclusion(&state, &random_eth1_data(), spec)
                     .is_ok(),
                 "should succeed if cache is empty but no deposits are required"
             );
@@ -593,7 +616,7 @@ mod test {
 
             assert!(
                 eth1_chain
-                    .deposits_for_block_inclusion(&state, spec)
+                    .deposits_for_block_inclusion(&state, &random_eth1_data(), spec)
                     .is_err(),
                 "should fail to get deposits if required, but cache is empty"
             );
@@ -637,7 +660,7 @@ mod test {
 
             assert!(
                 eth1_chain
-                    .deposits_for_block_inclusion(&state, spec)
+                    .deposits_for_block_inclusion(&state, &random_eth1_data(), spec)
                     .is_ok(),
                 "should succeed if no deposits are required"
             );
@@ -649,7 +672,7 @@ mod test {
                     state.eth1_data.deposit_count = i as u64;
 
                     let deposits_for_inclusion = eth1_chain
-                        .deposits_for_block_inclusion(&state, spec)
+                        .deposits_for_block_inclusion(&state, &random_eth1_data(), spec)
                         .expect(&format!("should find deposit for {}", i));
 
                     let expected_len =

eth2/state_processing/src/per_block_processing.rs

@@ -213,19 +213,35 @@ pub fn process_eth1_data<T: EthSpec>(
     state: &mut BeaconState<T>,
     eth1_data: &Eth1Data,
 ) -> Result<(), Error> {
+    if let Some(new_eth1_data) = get_new_eth1_data(state, eth1_data) {
+        state.eth1_data = new_eth1_data;
+    }
+
     state.eth1_data_votes.push(eth1_data.clone())?;
 
+    Ok(())
+}
+
+/// Returns `Some(eth1_data)` if adding the given `eth1_data` to `state.eth1_data_votes` would
+/// result in a change to `state.eth1_data`.
+///
+/// Spec v0.9.1
+pub fn get_new_eth1_data<T: EthSpec>(
+    state: &BeaconState<T>,
+    eth1_data: &Eth1Data,
+) -> Option<Eth1Data> {
     let num_votes = state
         .eth1_data_votes
         .iter()
         .filter(|vote| *vote == eth1_data)
         .count();
 
-    if num_votes * 2 > T::SlotsPerEth1VotingPeriod::to_usize() {
-        state.eth1_data = eth1_data.clone();
+    // The +1 is to account for the `eth1_data` supplied to the function.
+    if num_votes * 2 + 1 > T::SlotsPerEth1VotingPeriod::to_usize() {
+        Some(eth1_data.clone())
+    } else {
+        None
     }
-
-    Ok(())
 }
 
 /// Validates each `ProposerSlashing` and updates the state, short-circuiting on an invalid object.