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 inherents race condition #1974

Merged
merged 8 commits into from
Oct 26, 2023
Merged

Fix inherents race condition #1974

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7b09d16
Remove unnecessary field from `SubspaceBlockImport` because it is als…
nazar-pc Sep 18, 2023
af9f66e
Fix race conditions related to inherents and remove now unnecessary w…
nazar-pc Sep 18, 2023
5fc1c5c
Tiny cleanup
nazar-pc Sep 18, 2023
3cff66d
Apply the same tweak to test service
nazar-pc Sep 18, 2023
9b06cef
Merge remote-tracking branch 'origin/main' into fix-inherents-race-co…
nazar-pc Oct 13, 2023
cd295a9
Merge remote-tracking branch 'origin/main' into fix-inherents-race-co…
nazar-pc Oct 25, 2023
56c07b0
Ensure consensus block is fully processed by operator before next slo…
NingLin-P Oct 25, 2023
6ccb95e
Apply review suggestions
NingLin-P Oct 25, 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
36 changes: 15 additions & 21 deletions crates/sc-consensus-subspace/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,11 @@ impl<Block: BlockT> SubspaceLink<Block> {
self.archived_segment_notification_stream.clone()
}

/// Get stream with notifications about each imported block.
/// Get stream with notifications about each imported block right BEFORE import actually
/// happens.
///
/// NOTE: all Subspace checks have already happened for this block, but block can still
/// potentially fail to import in Substrate's internals.
pub fn block_importing_notification_stream(
&self,
) -> SubspaceNotificationStream<BlockImportingNotification<Block>> {
Expand Down Expand Up @@ -599,8 +603,6 @@ where
{
inner: I,
client: Arc<Client>,
block_importing_notification_sender:
SubspaceNotificationSender<BlockImportingNotification<Block>>,
subspace_link: SubspaceLink<Block>,
create_inherent_data_providers: CIDP,
chain_constants: ChainConstants,
Expand All @@ -620,7 +622,6 @@ where
SubspaceBlockImport {
inner: self.inner.clone(),
client: self.client.clone(),
block_importing_notification_sender: self.block_importing_notification_sender.clone(),
subspace_link: self.subspace_link.clone(),
create_inherent_data_providers: self.create_inherent_data_providers.clone(),
chain_constants: self.chain_constants,
Expand All @@ -641,14 +642,9 @@ where
AS: AuxStore + Send + Sync + 'static,
BlockNumber: From<<<Block as BlockT>::Header as HeaderT>::Number>,
{
// TODO: Create a struct for these parameters
#[allow(clippy::too_many_arguments)]
fn new(
client: Arc<Client>,
block_import: I,
block_importing_notification_sender: SubspaceNotificationSender<
BlockImportingNotification<Block>,
>,
subspace_link: SubspaceLink<Block>,
create_inherent_data_providers: CIDP,
chain_constants: ChainConstants,
Expand All @@ -658,7 +654,6 @@ where
Self {
client,
inner: block_import,
block_importing_notification_sender,
subspace_link,
create_inherent_data_providers,
chain_constants,
Expand Down Expand Up @@ -1000,8 +995,6 @@ where
let added_weight = calculate_block_weight(subspace_digest_items.solution_range);
let total_weight = parent_weight + added_weight;

let info = self.client.info();

aux_schema::write_block_weight(block_hash, total_weight, |values| {
block
.auxiliary
Expand Down Expand Up @@ -1034,12 +1027,14 @@ where
}
}

// The fork choice rule is that we pick the heaviest chain (i.e. smallest solution
// range), if there's a tie we go with the longest chain.
// The fork choice rule is that we pick the heaviest chain (i.e. smallest solution range),
// if there's a tie we go with the longest chain
let fork_choice = {
let info = self.client.info();

let last_best_weight = if &info.best_hash == block.header.parent_hash() {
// the parent=genesis case is already covered for loading parent weight,
// so we don't need to cover again here.
// the parent=genesis case is already covered for loading parent weight, so we don't
// need to cover again here
parent_weight
} else {
aux_schema::load_block_weight(&*self.client, info.best_hash)
Expand All @@ -1055,10 +1050,10 @@ where
};
block.fork_choice = Some(fork_choice);

let import_result = self.inner.import_block(block).await?;
let (acknowledgement_sender, mut acknowledgement_receiver) = mpsc::channel(0);

self.block_importing_notification_sender
self.subspace_link
.block_importing_notification_sender
.notify(move || BlockImportingNotification {
block_number,
acknowledgement_sender,
Expand All @@ -1068,7 +1063,7 @@ where
// Wait for all the acknowledgements to finish.
}

Ok(import_result)
self.inner.import_block(block).await
rahulksnv marked this conversation as resolved.
Show resolved Hide resolved
}

async fn check_block(
Expand Down Expand Up @@ -1129,7 +1124,7 @@ where
reward_signing_notification_stream,
archived_segment_notification_sender,
archived_segment_notification_stream,
block_importing_notification_sender: block_importing_notification_sender.clone(),
block_importing_notification_sender,
block_importing_notification_stream,
// TODO: Consider making `confirmation_depth_k` non-zero
segment_headers: Arc::new(Mutex::new(LruCache::new(
Expand All @@ -1145,7 +1140,6 @@ where
let import = SubspaceBlockImport::new(
client,
wrapped_block_import,
block_importing_notification_sender,
link.clone(),
create_inherent_data_providers,
chain_constants,
Expand Down
24 changes: 1 addition & 23 deletions crates/sc-consensus-subspace/src/slot_worker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use crate::archiver::SegmentHeadersStore;
use crate::{
BlockImportingNotification, NewSlotInfo, NewSlotNotification, RewardSigningNotification,
SubspaceLink,
};
use crate::{NewSlotInfo, NewSlotNotification, RewardSigningNotification, SubspaceLink};
use futures::channel::mpsc;
use futures::{StreamExt, TryFutureExt};
use log::{debug, error, info, warn};
Expand Down Expand Up @@ -539,25 +536,6 @@ where
}
}

// TODO: This is a workaround for potential root cause of
// https://github.com/subspace/subspace/issues/871, also being discussed in
// https://substrate.stackexchange.com/questions/7886/is-block-creation-guaranteed-to-be-running-after-parent-block-is-fully-imported
if maybe_pre_digest.is_some() {
let block_number = *parent_header.number() + One::one();
let (acknowledgement_sender, mut acknowledgement_receiver) = mpsc::channel(0);

self.subspace_link
.block_importing_notification_sender
.notify(move || BlockImportingNotification {
block_number,
acknowledgement_sender,
});

while (acknowledgement_receiver.next().await).is_some() {
// Wait for all the acknowledgements to finish.
}
}

maybe_pre_digest.map(|pre_digest| (pre_digest, pot_justification))
}

Expand Down
3 changes: 1 addition & 2 deletions test/subspace-test-service/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,7 +895,6 @@ where
let block_number = *block.header.number();
block.fork_choice = Some(ForkChoiceStrategy::LongestChain);

let import_result = self.inner.import_block(block).await?;
let (acknowledgement_sender, mut acknowledgement_receiver) = mpsc::channel(0);

// Must drop `block_import_acknowledgement_sender` after the notification otherwise the receiver
Expand Down Expand Up @@ -932,7 +931,7 @@ where
}
}

Ok(import_result)
self.inner.import_block(block).await
}

async fn check_block(
Expand Down