Skip to content

Commit

Permalink
refactor(state-sync): don't make the caller of StateSync::run() expli… (
Browse files Browse the repository at this point in the history 
#9655)

…citly save state

The caller of StateSync::run() is expected to save the HashMap passed to
this function if StateSync::run() returns StateSyncResult::Changed. This
means that StateSync::run() needs to keep track of whether it changed
anything in this hashmap throughout all the functions it calls, and then
have the caller replace its stored hash map with the cloned one passed
to this function. This is like implementing a function fn f(x: &mut u32)
{} with fn(x: &mut u32) -> bool {} where the caller is expected to clone
the argument and save it if the function returns true, which kind of
complicates the functions involved and makes refactoring other things a
little messier. It is also kind of error prone:
#6939
  • Loading branch information
@marcelo-gonzalez
marcelo-gonzalez authored Oct 19, 2023
1 parent 4e947b1 commit 49c11a5
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 98 deletions.
10 changes: 6 additions & 4 deletions chain/client/src/client.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2408,10 +2408,12 @@ impl Client {
use_colour,
self.runtime_adapter.clone(),
)? {
StateSyncResult::Unchanged => {}
StateSyncResult::Changed(fetch_block) => {
debug!(target: "catchup", "state sync finished but waiting to fetch block");
assert!(!fetch_block);
StateSyncResult::InProgress => {}
StateSyncResult::RequestBlock => {
// here RequestBlock should not be returned, because the StateSyncInfos in
// self.chain.store().iterate_state_sync_infos() should have been stored by
// Chain::postprocess_block() on the block with hash sync_hash.
panic!("catchup state sync indicates sync block isn't on our chain")
}
StateSyncResult::Completed => {
debug!(target: "catchup", "state sync completed now catch up blocks");
Expand Down
82 changes: 43 additions & 39 deletions chain/client/src/client_actor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1622,25 +1622,39 @@ impl ClientActor {
_ => false,
};
if sync_state {
let (sync_hash, mut new_shard_sync, just_enter_state_sync) =
match &self.client.sync_status {
SyncStatus::StateSync(StateSyncStatus {
sync_hash,
sync_status: shard_sync,
}) => (*sync_hash, shard_sync.clone(), false),
_ => {
let sync_hash = unwrap_and_report!(self.find_sync_hash());
(sync_hash, HashMap::default(), true)
match self.client.sync_status {
SyncStatus::StateSync(_) => (),
_ => {
let sync_hash = unwrap_and_report!(self.find_sync_hash());
if !self.client.config.archive {
unwrap_and_report!(self
.client
.chain
.reset_data_pre_state_sync(sync_hash));
}
};
let s = StateSyncStatus { sync_hash, sync_status: HashMap::default() };
self.client.sync_status = SyncStatus::StateSync(s);
}
};
let state_sync_status = match &mut self.client.sync_status {
SyncStatus::StateSync(s) => s,
_ => unreachable!(),
};

let me =
self.client.validator_signer.as_ref().map(|x| x.validator_id().clone());
let block_header =
unwrap_and_report!(self.client.chain.get_block_header(&sync_hash));
let block_header = unwrap_and_report!(self
.client
.chain
.get_block_header(&state_sync_status.sync_hash));
let prev_hash = *block_header.prev_hash();
let epoch_id =
self.client.chain.get_block_header(&sync_hash).unwrap().epoch_id().clone();
let epoch_id = self
.client
.chain
.get_block_header(&state_sync_status.sync_hash)
.unwrap()
.epoch_id()
.clone();
let shards_to_sync =
(0..self.client.epoch_manager.num_shards(&epoch_id).unwrap())
.filter(|x| {
Expand All @@ -1654,16 +1668,12 @@ impl ClientActor {
})
.collect();

if !self.client.config.archive && just_enter_state_sync {
unwrap_and_report!(self.client.chain.reset_data_pre_state_sync(sync_hash));
}

let use_colour =
matches!(self.client.config.log_summary_style, LogSummaryStyle::Colored);
match unwrap_and_report!(self.client.state_sync.run(
&me,
sync_hash,
&mut new_shard_sync,
state_sync_status.sync_hash,
&mut state_sync_status.sync_status,
&mut self.client.chain,
self.client.epoch_manager.as_ref(),
&self.network_info.highest_height_peers,
Expand All @@ -1674,26 +1684,20 @@ impl ClientActor {
use_colour,
self.client.runtime_adapter.clone(),
)) {
StateSyncResult::Unchanged => (),
StateSyncResult::Changed(fetch_block) => {
self.client.sync_status = SyncStatus::StateSync(StateSyncStatus {
sync_hash,
sync_status: new_shard_sync,
});
if fetch_block {
if let Some(peer_info) =
self.network_info.highest_height_peers.choose(&mut thread_rng())
StateSyncResult::InProgress => (),
StateSyncResult::RequestBlock => {
if let Some(peer_info) =
self.network_info.highest_height_peers.choose(&mut thread_rng())
{
let id = peer_info.peer_info.id.clone();

if let Ok(header) =
self.client.chain.get_block_header(&state_sync_status.sync_hash)
{
let id = peer_info.peer_info.id.clone();

if let Ok(header) =
self.client.chain.get_block_header(&sync_hash)
for hash in
vec![*header.prev_hash(), *header.hash()].into_iter()
{
for hash in
vec![*header.prev_hash(), *header.hash()].into_iter()
{
self.client.request_block(hash, id.clone());
}
self.client.request_block(hash, id.clone());
}
}
}
Expand All @@ -1705,7 +1709,7 @@ impl ClientActor {

unwrap_and_report!(self.client.chain.reset_heads_post_state_sync(
&me,
sync_hash,
state_sync_status.sync_hash,
&mut block_processing_artifacts,
self.get_apply_chunks_done_callback(),
));
Expand Down
Loading

0 comments on commit 49c11a5

Please sign in to comment.