Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix light client informant while syncing #9932

Merged
merged 6 commits into from
Nov 26, 2018
Merged

Fix light client informant while syncing #9932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a049111
Add `is_idle` to LightSync to check importing status
ngotchac Nov 15, 2018
054b73a
Use SyncStateWrapper to make sure is_idle gets updates
ngotchac Nov 15, 2018
d53da11
Update is_major_import to use verified queue size as well
ngotchac Nov 16, 2018
f5821ec
Add comment for `is_idle`
ngotchac Nov 19, 2018
a149a03
Add Debug to `SyncStateWrapper`
ngotchac Nov 19, 2018
2025161
`fn get` -> `fn into_inner`
ngotchac Nov 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 79 additions & 47 deletions ethcore/sync/src/light_sync/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

use std::collections::{HashMap, HashSet};
use std::mem;
use std::ops::Deref;
use std::sync::Arc;
use std::time::{Instant, Duration};

Expand Down Expand Up @@ -213,6 +214,44 @@ enum SyncState {
Rounds(SyncRound),
}

/// A wrapper around the SyncState that makes sure to
/// update the giving reference to `is_idle`
#[derive(Debug)]
struct SyncStateWrapper {
ngotchac marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not really sure about the usefulness of this struct, it seems that set function is only called once in set_state of LightClient, since fields are private.
Ok, I got it, it avoid bad use in future client inner implementation, maybe then we could move is_idle to it:

struct SyncStateWrapper {
  state: Mutex::new(SyncState::Idle),
  is_idle: Mutex::new(true),
}

and keep deref to state. Then we would have an easier set function (without handle ref).
This could be a bad idea if we need to keep is_idle mutex open (does not seems so).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that you want to keep an open SyncState Mutex while is_idle still being accessible. With the suggested changes, you cannot have a lock on SyncStateWrapper (otherwise is_idle wouldn't be accessible), so you would have to expose the inner SyncState lock, which could be used to directly change the state, without updating is_idle. Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, my suggestion was your second hypothesis (no mutex around SyncStateWrapper) , it does need to exposes the inner lock, on the other hand it keeps both locks together.
There is maybe less chance to mistake with current version, a mistake would be to pass any &mut bool to set_state which would be clearly intentional.

state: SyncState,
}

impl SyncStateWrapper {
/// Create a new wrapper for SyncState::Idle
pub fn idle() -> Self {
SyncStateWrapper {
state: SyncState::Idle,
}
}

/// Set the new state's value, making sure `is_idle` gets updated
pub fn set(&mut self, state: SyncState, is_idle_handle: &mut bool) {
*is_idle_handle = match state {
SyncState::Idle => true,
_ => false,
};
self.state = state;
}

/// Returns the internal state's value
pub fn into_inner(self) -> SyncState {
self.state
}
}

impl Deref for SyncStateWrapper {
type Target = SyncState;

fn deref(&self) -> &SyncState {
&self.state
}
}

struct ResponseCtx<'a> {
peer: PeerId,
req_id: ReqId,
Expand All @@ -235,7 +274,9 @@ pub struct LightSync<L: AsLightClient> {
pending_reqs: Mutex<HashMap<ReqId, PendingReq>>, // requests from this handler
client: Arc<L>,
rng: Mutex<OsRng>,
state: Mutex<SyncState>,
state: Mutex<SyncStateWrapper>,
// We duplicate this state tracking to avoid deadlocks in `is_major_importing`.
is_idle: Mutex<bool>,
ngotchac marked this conversation as resolved.
Show resolved Hide resolved
}

#[derive(Debug, Clone)]
Expand Down Expand Up @@ -309,16 +350,17 @@ impl<L: AsLightClient + Send + Sync> Handler for LightSync<L> {

if new_best.is_none() {
debug!(target: "sync", "No peers remain. Reverting to idle");
*self.state.lock() = SyncState::Idle;
self.set_state(&mut self.state.lock(), SyncState::Idle);
} else {
let mut state = self.state.lock();

*state = match mem::replace(&mut *state, SyncState::Idle) {
let next_state = match mem::replace(&mut *state, SyncStateWrapper::idle()).into_inner() {
SyncState::Idle => SyncState::Idle,
SyncState::AncestorSearch(search) =>
SyncState::AncestorSearch(search.requests_abandoned(unfulfilled)),
SyncState::Rounds(round) => SyncState::Rounds(round.requests_abandoned(unfulfilled)),
};
self.set_state(&mut state, next_state);
}

self.maintain_sync(ctx.as_basic());
Expand Down Expand Up @@ -390,12 +432,13 @@ impl<L: AsLightClient + Send + Sync> Handler for LightSync<L> {
data: headers,
};

*state = match mem::replace(&mut *state, SyncState::Idle) {
let next_state = match mem::replace(&mut *state, SyncStateWrapper::idle()).into_inner() {
SyncState::Idle => SyncState::Idle,
SyncState::AncestorSearch(search) =>
SyncState::AncestorSearch(search.process_response(&ctx, &*self.client)),
SyncState::Rounds(round) => SyncState::Rounds(round.process_response(&ctx)),
};
self.set_state(&mut state, next_state);
}

self.maintain_sync(ctx.as_basic());
Expand All @@ -408,12 +451,18 @@ impl<L: AsLightClient + Send + Sync> Handler for LightSync<L> {

// private helpers
impl<L: AsLightClient> LightSync<L> {
/// Sets the LightSync's state, and update
/// `is_idle`
fn set_state(&self, state: &mut SyncStateWrapper, next_state: SyncState) {
state.set(next_state, &mut self.is_idle.lock());
}

// Begins a search for the common ancestor and our best block.
// does not lock state, instead has a mutable reference to it passed.
fn begin_search(&self, state: &mut SyncState) {
fn begin_search(&self, state: &mut SyncStateWrapper) {
if let None = *self.best_seen.lock() {
// no peers.
*state = SyncState::Idle;
self.set_state(state, SyncState::Idle);
return;
}

Expand All @@ -422,7 +471,8 @@ impl<L: AsLightClient> LightSync<L> {

trace!(target: "sync", "Beginning search for common ancestor from {:?}",
(chain_info.best_block_number, chain_info.best_block_hash));
*state = SyncState::AncestorSearch(AncestorSearch::begin(chain_info.best_block_number));
let next_state = SyncState::AncestorSearch(AncestorSearch::begin(chain_info.best_block_number));
self.set_state(state, next_state);
}

// handles request dispatch, block import, state machine transitions, and timeouts.
Expand All @@ -435,7 +485,7 @@ impl<L: AsLightClient> LightSync<L> {
let chain_info = client.chain_info();

let mut state = self.state.lock();
debug!(target: "sync", "Maintaining sync ({:?})", &*state);
debug!(target: "sync", "Maintaining sync ({:?})", **state);

// drain any pending blocks into the queue.
{
Expand All @@ -445,11 +495,12 @@ impl<L: AsLightClient> LightSync<L> {
loop {
if client.queue_info().is_full() { break }

*state = match mem::replace(&mut *state, SyncState::Idle) {
let next_state = match mem::replace(&mut *state, SyncStateWrapper::idle()).into_inner() {
SyncState::Rounds(round)
=> SyncState::Rounds(round.drain(&mut sink, Some(DRAIN_AMOUNT))),
other => other,
};
self.set_state(&mut state, next_state);

if sink.is_empty() { break }
trace!(target: "sync", "Drained {} headers to import", sink.len());
Expand Down Expand Up @@ -483,15 +534,15 @@ impl<L: AsLightClient> LightSync<L> {
let network_score = other.as_ref().map(|target| target.head_td);
trace!(target: "sync", "No target to sync to. Network score: {:?}, Local score: {:?}",
network_score, best_td);
*state = SyncState::Idle;
self.set_state(&mut state, SyncState::Idle);
return;
}
};

match mem::replace(&mut *state, SyncState::Idle) {
match mem::replace(&mut *state, SyncStateWrapper::idle()).into_inner() {
SyncState::Rounds(SyncRound::Abort(reason, remaining)) => {
if remaining.len() > 0 {
*state = SyncState::Rounds(SyncRound::Abort(reason, remaining));
self.set_state(&mut state, SyncState::Rounds(SyncRound::Abort(reason, remaining)));
return;
}

Expand All @@ -505,7 +556,7 @@ impl<L: AsLightClient> LightSync<L> {
AbortReason::NoResponses => {}
AbortReason::TargetReached => {
debug!(target: "sync", "Sync target reached. Going idle");
*state = SyncState::Idle;
self.set_state(&mut state, SyncState::Idle);
return;
}
}
Expand All @@ -514,15 +565,15 @@ impl<L: AsLightClient> LightSync<L> {
self.begin_search(&mut state);
}
SyncState::AncestorSearch(AncestorSearch::FoundCommon(num, hash)) => {
*state = SyncState::Rounds(SyncRound::begin((num, hash), sync_target));
self.set_state(&mut state, SyncState::Rounds(SyncRound::begin((num, hash), sync_target)));
}
SyncState::AncestorSearch(AncestorSearch::Genesis) => {
// Same here.
let g_hash = chain_info.genesis_hash;
*state = SyncState::Rounds(SyncRound::begin((0, g_hash), sync_target));
self.set_state(&mut state, SyncState::Rounds(SyncRound::begin((0, g_hash), sync_target)));
}
SyncState::Idle => self.begin_search(&mut state),
other => *state = other, // restore displaced state.
other => self.set_state(&mut state, other), // restore displaced state.
}
}

Expand All @@ -543,12 +594,13 @@ impl<L: AsLightClient> LightSync<L> {
}
drop(pending_reqs);

*state = match mem::replace(&mut *state, SyncState::Idle) {
let next_state = match mem::replace(&mut *state, SyncStateWrapper::idle()).into_inner() {
SyncState::Idle => SyncState::Idle,
SyncState::AncestorSearch(search) =>
SyncState::AncestorSearch(search.requests_abandoned(&unfulfilled)),
SyncState::Rounds(round) => SyncState::Rounds(round.requests_abandoned(&unfulfilled)),
};
self.set_state(&mut state, next_state);
}
}

Expand Down Expand Up @@ -605,34 +657,14 @@ impl<L: AsLightClient> LightSync<L> {
None
};

*state = match mem::replace(&mut *state, SyncState::Idle) {
let next_state = match mem::replace(&mut *state, SyncStateWrapper::idle()).into_inner() {
SyncState::Rounds(round) =>
SyncState::Rounds(round.dispatch_requests(dispatcher)),
SyncState::AncestorSearch(search) =>
SyncState::AncestorSearch(search.dispatch_request(dispatcher)),
other => other,
};
}
}

fn is_major_importing_do_wait(&self, wait: bool) -> bool {
const EMPTY_QUEUE: usize = 3;

if self.client.as_light_client().queue_info().unverified_queue_size > EMPTY_QUEUE {
return true;
}
let mg_state = if wait {
self.state.lock()
} else {
if let Some(mg_state) = self.state.try_lock() {
mg_state
} else {
return false;
}
};
match *mg_state {
SyncState::Idle => false,
_ => true,
self.set_state(&mut state, next_state);
}
}
}
Expand All @@ -651,7 +683,8 @@ impl<L: AsLightClient> LightSync<L> {
pending_reqs: Mutex::new(HashMap::new()),
client: client,
rng: Mutex::new(OsRng::new()?),
state: Mutex::new(SyncState::Idle),
state: Mutex::new(SyncStateWrapper::idle()),
is_idle: Mutex::new(true),
})
}
}
Expand All @@ -666,9 +699,6 @@ pub trait SyncInfo {

/// Whether major sync is underway.
fn is_major_importing(&self) -> bool;

/// Whether major sync is underway, skipping some synchronization.
fn is_major_importing_no_sync(&self) -> bool;
}

impl<L: AsLightClient> SyncInfo for LightSync<L> {
Expand All @@ -681,11 +711,13 @@ impl<L: AsLightClient> SyncInfo for LightSync<L> {
}

fn is_major_importing(&self) -> bool {
self.is_major_importing_do_wait(true)
}
const EMPTY_QUEUE: usize = 3;

let queue_info = self.client.as_light_client().queue_info();
let is_verifying = queue_info.unverified_queue_size + queue_info.verified_queue_size > EMPTY_QUEUE;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether there was a reason in #5002 not to take into account verified_queue_size
https://github.com/paritytech/parity-ethereum/pull/5002/files#diff-e8c26f6188b2b873a7467abd3041b843R584
@rphmeier? Although that was a long time ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about that. But from my local testing, it lead to wrong information being displayed (Import logs too soon)

let is_syncing = !*self.is_idle.lock();

fn is_major_importing_no_sync(&self) -> bool {
self.is_major_importing_do_wait(false)
is_verifying || is_syncing
}

}
2 changes: 1 addition & 1 deletion parity/informant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ impl InformantData for LightNodeInformantData {
fn executes_transactions(&self) -> bool { false }

fn is_major_importing(&self) -> bool {
self.sync.is_major_importing_no_sync()
self.sync.is_major_importing()
}

fn report(&self) -> Report {
Expand Down