Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sector expiration #1721

Merged
merged 3 commits into from
Jul 31, 2023
Merged

Fix sector expiration #1721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9862c3e
Fix sector expiration
nazar-pc Jul 31, 2023
0a01c90
Store archived segment header before sending notification
nazar-pc Jul 31, 2023
edb8b4d
Better logging around sector expiration
nazar-pc Jul 31, 2023
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
19 changes: 11 additions & 8 deletions crates/sc-consensus-subspace/src/archiver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use sp_objects::ObjectsApi;
use sp_runtime::generic::SignedBlock;
use sp_runtime::traits::{Block as BlockT, CheckedSub, Header, NumberFor, One, Zero};
use std::future::Future;
use std::slice;
use std::sync::atomic::{AtomicU16, Ordering};
use std::sync::Arc;
use subspace_archiving::archiver::{Archiver, NewArchivedSegment};
Expand Down Expand Up @@ -687,6 +688,16 @@ where
for archived_segment in archiver.add_block(encoded_block, block_object_mappings) {
let segment_header = archived_segment.segment_header;

if let Err(error) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this change(github doesn't allow comments on unchanged lines) ..

L612 above (block_number.checked_sub(&confirmation_depth_k.into() ...). This looks like an edge case where we won't archive blocks when the u32 wraps around. When this happens, there could be a slow path that just walks back the parent hashes to get to the block k_conf_depth old).

Or we could opportunistically cache the block hashes as import notifications come in (this won't work on restart or reorg), then fall back to the slow path

Copy link
Member Author

Choose a reason for hiding this comment

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

u32 will never wrap around, our block number type is u32

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean when it reaches U32::MAX and goes back ..

Copy link
Member Author

Choose a reason for hiding this comment

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

Block number will never reach u32::MAX

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it because mainnet needs to be running for a very loong time (at 1 block per 6 sec) to get there? nice problem to have though :-

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. Even if blocks are produced every second running out of u32 will not be an issue to worry for quite a few decades. I hope Subspace lives long enough for this to be a real issue.

segment_headers_store.add_segment_headers(slice::from_ref(&segment_header))
{
error!(
target: "subspace",
"Failed to store segment headers: {error}"
);
return;
}

send_archived_segment_notification(
&archived_segment_notification_sender,
archived_segment,
Expand All @@ -697,14 +708,6 @@ where
}

if !new_segment_headers.is_empty() {
if let Err(error) = segment_headers_store.add_segment_headers(&new_segment_headers)
rahulksnv marked this conversation as resolved.
Show resolved Hide resolved
{
error!(
target: "subspace",
"Failed to store segment headers: {error}"
);
return;
}
let maybe_block_number_to_finalize = {
let mut segment_headers = segment_headers.lock();
segment_headers.put(block_number + One::one(), new_segment_headers);
Expand Down
10 changes: 9 additions & 1 deletion crates/sc-consensus-subspace/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ pub enum Error<Header: HeaderT> {
/// Stored segment header extrinsic was not found
#[error("Stored segment header extrinsic was not found: {0:?}")]
SegmentHeadersExtrinsicNotFound(Vec<SegmentHeader>),
/// Duplicated segment commitment
/// Different segment commitment found
#[error(
"Different segment commitment for segment index {0} was found in storage, likely fork \
below archiving point"
Expand Down Expand Up @@ -1161,6 +1161,14 @@ where
.segment_commitment();

if &found_segment_commitment != segment_commitment {
warn!(
target: "subspace",
"Different segment commitment for segment index {} was found in storage, \
likely fork below archiving point. expected {:?}, found {:?}",
segment_index,
segment_commitment,
found_segment_commitment
);
return Err(ConsensusError::ClientImport(
Error::<Block::Header>::DifferentSegmentCommitment(segment_index).to_string(),
));
Expand Down
50 changes: 48 additions & 2 deletions crates/subspace-farmer/src/single_disk_plot/plotting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,10 @@ where

while let Some(()) = archived_segments_receiver.next().await {
let archived_segment_header = last_archived_segment.load(Ordering::SeqCst);
trace!(
segment_index = %archived_segment_header.segment_index(),
"New archived segment received",
);

// It is fine to take a synchronous read lock here because the only time
// write lock is taken is during plotting, which we know doesn't happen
Expand All @@ -460,7 +464,23 @@ where
.collect_into(&mut sectors_to_check);
for (sector_index, history_size) in sectors_to_check.drain(..) {
if let Some(sector_expire_at) = sectors_expire_at.get(&sector_index) {
if *sector_expire_at >= archived_segment_header.segment_index() {
trace!(
%sector_index,
%history_size,
%sector_expire_at,
"Checking sector for expiration"
);
// +1 means we will start replotting a bit before it actually expires to avoid
// storing expired sectors
if *sector_expire_at
<= (archived_segment_header.segment_index() + SegmentIndex::ONE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this + 1 be made a method like inc()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly, do you think it would be more readable? Rust doesn't have increments and I got used to it eventually.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just preference, this is fine too

{
debug!(
%sector_index,
%history_size,
%sector_expire_at,
"Sector expires soon #1, scheduling replotting"
);
// Time to replot
sector_indices_to_replot.push(sector_index);
}
Expand All @@ -471,6 +491,12 @@ where
.sector_expiration_check(min_sector_lifetime)
.map(|expiration_check_history_size| expiration_check_history_size.segment_index())
{
trace!(
%sector_index,
%history_size,
%expiration_check_segment_index,
"Determined sector expiration check segment index"
);
let maybe_sector_expiration_check_segment_commitment =
if let Some(segment_commitment) =
archived_segment_commitments_cache.get(&expiration_check_segment_index)
Expand Down Expand Up @@ -508,12 +534,32 @@ where
metadata; qed",
);

trace!(
%sector_index,
%history_size,
sector_expire_at = %expiration_history_size.segment_index(),
"Determined sector expiration segment index"
);
// +1 means we will start replotting a bit before it actually expires to avoid
// storing expired sectors
if expiration_history_size.segment_index()
>= archived_segment_header.segment_index()
<= (archived_segment_header.segment_index() + SegmentIndex::ONE)
{
debug!(
%sector_index,
%history_size,
sector_expire_at = %expiration_history_size.segment_index(),
"Sector expires soon #2, scheduling replotting"
);
// Time to replot
sector_indices_to_replot.push(sector_index);
} else {
trace!(
%sector_index,
%history_size,
sector_expire_at = %expiration_history_size.segment_index(),
"Sector expires later, remembering sector expiration"
);
// Store expiration so we don't have to recalculate it later
sectors_expire_at
.insert(sector_index, expiration_history_size.segment_index());
Expand Down
2 changes: 1 addition & 1 deletion crates/subspace-verification/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ where
}
};

if expiration_history_size >= *current_history_size {
if expiration_history_size <= *current_history_size {
return Err(Error::SectorExpired {
expiration_history_size,
current_history_size: *current_history_size,
Expand Down