Skip to content

Commit

Permalink
reintroduce clear_resharding_data (#10211)
Browse files Browse the repository at this point in the history
Tested on updated #10193

`is_next_block_epoch_start` reads `BlockInfo` of the first block of the
epoch, which is not correct in the case of garbage collection.
The previous version of the test was passing, because that `BlockInfo`
was still in the cache of the epoch manager (as we call
`is_next_block_epoch_start` on every block)

Implemented safer version of `is_next_block_epoch_start` --
`is_last_block_in_finished_epoch` (didn't think too much about the name,
open to suggestions).
`is_last_block_in_finished_epoch` works by relying on the fact that if
we processed block, and it was the last block of an epoch according to
`is_next_block_epoch_start`, then we wrote an `EpochInfo` with the hash
of that block, and `EpochInfo` is not garbage collectible.
  • Loading branch information
posvyatokum authored Nov 20, 2023
1 parent d0a771d commit 5fa5470
Show file tree
Hide file tree
Showing 6 changed files with 35 additions and 9 deletions.
11 changes: 5 additions & 6 deletions chain/chain/src/chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1007,12 +1007,11 @@ impl Chain {
*block_hash,
GCMode::Canonical(tries.clone()),
)?;
// TODO(resharding): Call clear_resharding_data once we figure out what's wrong
// chain_store_update.clear_resharding_data(
// self.runtime_adapter.as_ref(),
// self.epoch_manager.as_ref(),
// *block_hash,
// )?;
chain_store_update.clear_resharding_data(
self.runtime_adapter.as_ref(),
self.epoch_manager.as_ref(),
*block_hash,
)?;
gc_blocks_remaining -= 1;
} else {
return Err(Error::GCError(
Expand Down
2 changes: 1 addition & 1 deletion chain/chain/src/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2340,7 +2340,7 @@ impl<'a> ChainStoreUpdate<'a> {
) -> Result<(), Error> {
// Need to check if this is the last block of the epoch where resharding is completed
// which means shard layout changed in the previous epoch
if !epoch_manager.is_next_block_epoch_start(&block_hash)? {
if !epoch_manager.is_last_block_in_finished_epoch(&block_hash)? {
return Ok(());
}

Expand Down
4 changes: 4 additions & 0 deletions chain/chain/src/test_utils/kv_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,10 @@ impl EpochManagerAdapter for MockEpochManager {
!= self.get_epoch_and_valset(prev_prev_hash)?.0)
}

fn is_last_block_in_finished_epoch(&self, hash: &CryptoHash) -> Result<bool, EpochError> {
self.is_next_block_epoch_start(hash)
}

fn get_epoch_id_from_prev_block(
&self,
parent_hash: &CryptoHash,
Expand Down
11 changes: 11 additions & 0 deletions chain/epoch-manager/src/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ pub trait EpochManagerAdapter: Send + Sync {
/// Returns true, if given hash is last block in it's epoch.
fn is_next_block_epoch_start(&self, parent_hash: &CryptoHash) -> Result<bool, EpochError>;

/// Returns true, if given hash is in an epoch that already finished.
/// `is_next_block_epoch_start` works even if we didn't fully process the provided block.
/// This function works even if we garbage collected `BlockInfo` of the first block of the epoch.
/// Thus, this function is better suited for use in garbage collection.
fn is_last_block_in_finished_epoch(&self, hash: &CryptoHash) -> Result<bool, EpochError>;

/// Get epoch id given hash of previous block.
fn get_epoch_id_from_prev_block(&self, parent_hash: &CryptoHash)
-> Result<EpochId, EpochError>;
Expand Down Expand Up @@ -475,6 +481,11 @@ impl EpochManagerAdapter for EpochManagerHandle {
epoch_manager.is_next_block_epoch_start(parent_hash)
}

fn is_last_block_in_finished_epoch(&self, hash: &CryptoHash) -> Result<bool, EpochError> {
let epoch_manager = self.read();
epoch_manager.is_last_block_in_finished_epoch(hash)
}

fn get_epoch_id_from_prev_block(
&self,
parent_hash: &CryptoHash,
Expand Down
14 changes: 14 additions & 0 deletions chain/epoch-manager/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1086,6 +1086,20 @@ impl EpochManager {
self.is_next_block_in_next_epoch(&block_info)
}

/// Relies on the fact that last block hash of an epoch is an EpochId of next next epoch.
/// If this block is the last one in some epoch, and we fully processed it, there will be `EpochInfo` record with `hash` key.
fn is_last_block_in_finished_epoch(&self, hash: &CryptoHash) -> Result<bool, EpochError> {
match self.get_epoch_info(&EpochId(*hash)) {
Ok(_) => Ok(true),
Err(EpochError::IOErr(msg)) => Err(EpochError::IOErr(msg)),
Err(EpochError::MissingBlock(_)) => Ok(false),
Err(err) => {
warn!(target: "epoch_manager", ?err, "Unexpected error in is_last_block_in_finished_epoch");
Ok(false)
}
}
}

pub fn get_epoch_id_from_prev_block(
&self,
parent_hash: &CryptoHash,
Expand Down
2 changes: 0 additions & 2 deletions integration-tests/src/tests/client/resharding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1059,13 +1059,11 @@ fn test_shard_layout_upgrade_gc_impl(resharding_type: ReshardingType, rng_seed:
}

#[test]
#[ignore]
fn test_shard_layout_upgrade_gc() {
test_shard_layout_upgrade_gc_impl(ReshardingType::V1, 44);
}

#[test]
#[ignore]
fn test_shard_layout_upgrade_gc_v2() {
// TODO(resharding) remove those checks once rolled out
if checked_feature!("stable", SimpleNightshadeV2, PROTOCOL_VERSION) {
Expand Down

0 comments on commit 5fa5470

Please sign in to comment.