From 99f7a7db58be9393a6592161f01a1eba62549044 Mon Sep 17 00:00:00 2001 From: divma Date: Tue, 19 Oct 2021 22:32:25 +0000 Subject: [PATCH] remove double backfill sync state (#2733) ## Issue Addressed In the backfill sync the state was maintained twice, once locally and also in the globals. This makes it so that it's maintained only once. The only behavioral change is that when backfill sync in paused, the global backfill state is updated. I asked @AgeManning about this and he deemed it a bug, so this solves it. --- .../network/src/sync/backfill_sync/mod.rs | 57 ++++++++----------- 1 file changed, 23 insertions(+), 34 deletions(-) diff --git a/beacon_node/network/src/sync/backfill_sync/mod.rs b/beacon_node/network/src/sync/backfill_sync/mod.rs index a47faa735..b9016b9fd 100644 --- a/beacon_node/network/src/sync/backfill_sync/mod.rs +++ b/beacon_node/network/src/sync/backfill_sync/mod.rs @@ -93,9 +93,6 @@ pub enum BackFillError { } pub struct BackFillSync { - /// The current state of the backfill sync. - state: BackFillState, - /// Keeps track of the current progress of the backfill. /// This only gets refreshed from the beacon chain if we enter a failed state. current_start: BatchId, @@ -176,7 +173,6 @@ impl BackFillSync { }; let bfs = BackFillSync { - state, batches: BTreeMap::new(), active_requests: HashMap::new(), processing_target: current_start, @@ -194,15 +190,15 @@ impl BackFillSync { }; // Update the global network state with the current backfill state. - bfs.update_global_state(); + bfs.set_state(state); bfs } /// Pauses the backfill sync if it's currently syncing. pub fn pause(&mut self) { - if let BackFillState::Syncing = self.state { + if let BackFillState::Syncing = self.state() { debug!(self.log, "Backfill sync paused"; "processed_epochs" => self.validated_batches, "to_be_processed" => self.current_start); - self.state = BackFillState::Paused; + self.set_state(BackFillState::Paused); } } @@ -214,7 +210,7 @@ impl BackFillSync { &mut self, network: &mut SyncNetworkContext, ) -> Result { - match self.state { + match self.state() { BackFillState::Syncing => {} // already syncing ignore. BackFillState::Paused => { if self @@ -227,7 +223,7 @@ impl BackFillSync { { // If there are peers to resume with, begin the resume. debug!(self.log, "Resuming backfill sync"; "start_epoch" => self.current_start, "awaiting_batches" => self.batches.len(), "processing_target" => self.processing_target); - self.state = BackFillState::Syncing; + self.set_state(BackFillState::Syncing); // Resume any previously failed batches. self.resume_batches(network)?; // begin requesting blocks from the peer pool, until all peers are exhausted. @@ -248,14 +244,13 @@ impl BackFillSync { return Ok(SyncStart::NotSyncing); } - self.state = BackFillState::Syncing; + self.set_state(BackFillState::Syncing); // Obtain a new start slot, from the beacon chain and handle possible errors. match self.reset_start_epoch() { Err(ResetEpochError::SyncCompleted) => { error!(self.log, "Backfill sync completed whilst in failed status"); - self.state = BackFillState::Completed; - self.update_global_state(); + self.set_state(BackFillState::Completed); return Err(BackFillError::InvalidSyncState(String::from( "chain completed", ))); @@ -265,8 +260,7 @@ impl BackFillSync { self.log, "Backfill sync not required whilst in failed status" ); - self.state = BackFillState::NotRequired; - self.update_global_state(); + self.set_state(BackFillState::NotRequired); return Err(BackFillError::InvalidSyncState(String::from( "backfill not required", ))); @@ -284,8 +278,6 @@ impl BackFillSync { } } - self.update_global_state(); - Ok(SyncStart::Syncing { completed: (self.validated_batches * BACKFILL_EPOCHS_PER_BATCH @@ -301,7 +293,7 @@ impl BackFillSync { /// If we are in a failed state, update a local variable to indicate we are able to restart /// the failed sync on the next attempt. pub fn fully_synced_peer_joined(&mut self) { - if matches!(self.state, BackFillState::Failed) { + if matches!(self.state(), BackFillState::Failed) { self.restart_failed_sync = true; } } @@ -315,7 +307,7 @@ impl BackFillSync { network: &mut SyncNetworkContext, ) -> Result<(), BackFillError> { if matches!( - self.state, + self.state(), BackFillState::Failed | BackFillState::NotRequired ) { return Ok(()); @@ -399,7 +391,7 @@ impl BackFillSync { // check if we have this batch let batch = match self.batches.get_mut(&batch_id) { None => { - if !matches!(self.state, BackFillState::Failed) { + if !matches!(self.state(), BackFillState::Failed) { // A batch might get removed when the chain advances, so this is non fatal. debug!(self.log, "Received a block for unknown batch"; "epoch" => batch_id); } @@ -476,7 +468,7 @@ impl BackFillSync { } // Set the state - self.state = BackFillState::Failed; + self.set_state(BackFillState::Failed); // Remove all batches and active requests and participating peers. self.batches.clear(); self.active_requests.clear(); @@ -489,9 +481,6 @@ impl BackFillSync { self.last_batch_downloaded = false; self.current_processing_batch = None; - // Keep the global network state up to date. - self.update_global_state(); - // NOTE: Lets keep validated_batches for posterity // Emit the log here @@ -510,7 +499,7 @@ impl BackFillSync { batch_id: BatchId, ) -> Result { // Only process batches if this chain is Syncing, and only one at a time - if self.state != BackFillState::Syncing || self.current_processing_batch.is_some() { + if self.state() != BackFillState::Syncing || self.current_processing_batch.is_some() { return Ok(ProcessResult::Successful); } @@ -568,9 +557,6 @@ impl BackFillSync { batch_id: BatchId, result: &BatchProcessResult, ) -> Result { - // On each batch process, we update the global state. - self.update_global_state(); - // The first two cases are possible in regular sync, should not occur in backfill, but we // keep this logic for handling potential processing race conditions. // result @@ -625,8 +611,7 @@ impl BackFillSync { if self.check_completed() { // chain is completed info!(self.log, "Backfill sync completed"; "blocks_processed" => self.validated_batches * T::EthSpec::slots_per_epoch()); - self.state = BackFillState::Completed; - self.update_global_state(); + self.set_state(BackFillState::Completed); Ok(ProcessResult::SyncCompleted) } else { // chain is not completed @@ -708,7 +693,7 @@ impl BackFillSync { network: &mut SyncNetworkContext, ) -> Result { // Only process batches if backfill is syncing and only process one batch at a time - if self.state != BackFillState::Syncing || self.current_processing_batch.is_some() { + if self.state() != BackFillState::Syncing || self.current_processing_batch.is_some() { return Ok(ProcessResult::Successful); } @@ -943,7 +928,7 @@ impl BackFillSync { } else { // If we are here the chain has no more synced peers info!(self.log, "Backfill sync paused"; "reason" => "insufficient_synced_peers"); - self.state = BackFillState::Paused; + self.set_state(BackFillState::Paused); Err(BackFillError::Paused) } } @@ -1031,7 +1016,7 @@ impl BackFillSync { &mut self, network: &mut SyncNetworkContext, ) -> Result<(), BackFillError> { - if !matches!(self.state, BackFillState::Syncing) { + if !matches!(self.state(), BackFillState::Syncing) { return Ok(()); } @@ -1158,8 +1143,12 @@ impl BackFillSync { } /// Updates the global network state indicating the current state of a backfill sync. - fn update_global_state(&self) { - *self.network_globals.backfill_state.write() = self.state.clone(); + fn set_state(&self, state: BackFillState) { + *self.network_globals.backfill_state.write() = state; + } + + fn state(&self) -> BackFillState { + self.network_globals.backfill_state.read().clone() } }