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.

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
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -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};
@@ -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,
@@ -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);
10 changes: 9 additions & 1 deletion crates/sc-consensus-subspace/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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"
@@ -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(),
));
50 changes: 48 additions & 2 deletions crates/subspace-farmer/src/single_disk_plot/plotting.rs
Original file line number Diff line number Diff line change
@@ -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
@@ -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);
}
@@ -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)
@@ -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());
2 changes: 1 addition & 1 deletion crates/subspace-verification/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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,