From 85e23a8ba92c64aaa555372f3d4deea7ec709bb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 13 Feb 2024 14:24:14 +0100 Subject: [PATCH 1/4] Parachains-Aura: Only produce once per slot Given how the block production is driven for Parachains right now, with the enabling of async backing we would produce two blocks per slot. Until we have a proper collator implementation, the "hack" is to prevent the production of multiple blocks per slot. --- cumulus/client/consensus/aura/src/lib.rs | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/cumulus/client/consensus/aura/src/lib.rs b/cumulus/client/consensus/aura/src/lib.rs index 6ededa7a92c1..8e4bc658e44b 100644 --- a/cumulus/client/consensus/aura/src/lib.rs +++ b/cumulus/client/consensus/aura/src/lib.rs @@ -42,7 +42,14 @@ use sp_core::crypto::Pair; use sp_inherents::CreateInherentDataProviders; use sp_keystore::KeystorePtr; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, Member, NumberFor}; -use std::{convert::TryFrom, marker::PhantomData, sync::Arc}; +use std::{ + convert::TryFrom, + marker::PhantomData, + sync::{ + atomic::{AtomicU64, Ordering}, + Arc, + }, +}; mod import_queue; @@ -61,6 +68,7 @@ pub struct AuraConsensus { create_inherent_data_providers: Arc, aura_worker: Arc>, slot_duration: SlotDuration, + last_slot_processed: Arc, _phantom: PhantomData, } @@ -70,6 +78,7 @@ impl Clone for AuraConsensus { create_inherent_data_providers: self.create_inherent_data_providers.clone(), aura_worker: self.aura_worker.clone(), slot_duration: self.slot_duration, + last_slot_processed: self.last_slot_processed.clone(), _phantom: PhantomData, } } @@ -156,6 +165,7 @@ where Box::new(AuraConsensus { create_inherent_data_providers: Arc::new(create_inherent_data_providers), aura_worker: Arc::new(Mutex::new(worker)), + last_slot_processed: Default::default(), slot_duration, _phantom: PhantomData, }) @@ -221,6 +231,18 @@ where Some((validation_data.max_pov_size / 2) as usize), ); + // With async backing this function will be called every relay chain block. + // + // Most parachains currently run with 12 seconds slots and thus, they would try to produce + // multiple blocks per slot which very likely would fail on chain. Thus, we have this "hack" + // to only produce on block per slot. + // + // With https://github.com/paritytech/polkadot-sdk/issues/3168 this implementation will be + // obsolete and also the underlying issue will be fixed. + if self.last_slot_processed.fetch_max(*info.slot, Ordering::Relaxed) >= *info.slot { + return None + } + let res = self.aura_worker.lock().await.on_slot(info).await?; Some(ParachainCandidate { block: res.block, proof: res.storage_proof }) From ad21ab8c21c700714a63af9f78aaebcb3fca8f8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 13 Feb 2024 16:42:25 +0100 Subject: [PATCH 2/4] Also fix the basic collator... --- cumulus/client/consensus/aura/src/collator.rs | 8 +++++++- .../client/consensus/aura/src/collators/basic.rs | 16 ++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/cumulus/client/consensus/aura/src/collator.rs b/cumulus/client/consensus/aura/src/collator.rs index db0799235bca..5b7669c88f47 100644 --- a/cumulus/client/consensus/aura/src/collator.rs +++ b/cumulus/client/consensus/aura/src/collator.rs @@ -258,6 +258,7 @@ where pub struct SlotClaim { author_pub: Pub, pre_digest: DigestItem, + slot: Slot, timestamp: Timestamp, } @@ -272,7 +273,7 @@ impl SlotClaim { P::Public: Codec, P::Signature: Codec, { - SlotClaim { author_pub, timestamp, pre_digest: aura_internal::pre_digest::

(slot) } + SlotClaim { author_pub, timestamp, pre_digest: aura_internal::pre_digest::

(slot), slot } } /// Get the author's public key. @@ -285,6 +286,11 @@ impl SlotClaim { &self.pre_digest } + /// Get the slot assigned to this claim. + pub fn slot(&self) -> Slot { + self.slot + } + /// Get the timestamp corresponding to the relay-chain slot this claim was /// generated against. pub fn timestamp(&self) -> Timestamp { diff --git a/cumulus/client/consensus/aura/src/collators/basic.rs b/cumulus/client/consensus/aura/src/collators/basic.rs index 78f6b726aff0..d2d81e718a6f 100644 --- a/cumulus/client/consensus/aura/src/collators/basic.rs +++ b/cumulus/client/consensus/aura/src/collators/basic.rs @@ -141,6 +141,8 @@ where collator_util::Collator::::new(params) }; + let mut last_processed_slot = 0; + while let Some(request) = collation_requests.next().await { macro_rules! reject_with_error { ($err:expr) => {{ @@ -192,6 +194,20 @@ where Err(e) => reject_with_error!(e), }; + // With async backing this function will be called every relay chain block. + // + // Most parachains currently run with 12 seconds slots and thus, they would try to + // produce multiple blocks per slot which very likely would fail on chain. Thus, we have + // this "hack" to only produce on block per slot. + // + // With https://github.com/paritytech/polkadot-sdk/issues/3168 this implementation will be + // obsolete and also the underlying issue will be fixed. + if last_processed_slot >= *claim.slot() { + continue + } else { + last_processed_slot = *claim.slot(); + } + let (parachain_inherent_data, other_inherent_data) = try_request!( collator .create_inherent_data( From 520b0659a5cbfe325d2a7e3c0d65f730c1374732 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 13 Feb 2024 20:30:44 +0100 Subject: [PATCH 3/4] Review comment --- cumulus/client/consensus/aura/src/collators/basic.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cumulus/client/consensus/aura/src/collators/basic.rs b/cumulus/client/consensus/aura/src/collators/basic.rs index d2d81e718a6f..8740b06005d6 100644 --- a/cumulus/client/consensus/aura/src/collators/basic.rs +++ b/cumulus/client/consensus/aura/src/collators/basic.rs @@ -204,8 +204,6 @@ where // obsolete and also the underlying issue will be fixed. if last_processed_slot >= *claim.slot() { continue - } else { - last_processed_slot = *claim.slot(); } let (parachain_inherent_data, other_inherent_data) = try_request!( @@ -244,6 +242,8 @@ where request.complete(None); tracing::debug!(target: crate::LOG_TARGET, "No block proposal"); } + + last_processed_slot = *claim.slot(); } } } From a17b49a44fd6471a3498df7e58be32768925015d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 13 Feb 2024 20:43:51 +0100 Subject: [PATCH 4/4] Prdoc --- prdoc/pr_3308.prdoc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 prdoc/pr_3308.prdoc diff --git a/prdoc/pr_3308.prdoc b/prdoc/pr_3308.prdoc new file mode 100644 index 000000000000..611461e25fff --- /dev/null +++ b/prdoc/pr_3308.prdoc @@ -0,0 +1,14 @@ +title: Parachains-Aura: Only produce once per slot + +doc: + - audience: Node Dev + description: | + With the introduction of asynchronous backing the relay chain allows parachain to include blocks every 6 seconds. + The Cumulus Aura implementations, besides the lookahead collator, are building blocks when there is a free slot for + the parachain in the relay chain. Most parachains are still running with a 12s slot duration and not allowing + to build multiple blocks per slot. But, the block production logic will be triggered every 6s, resulting in error + logs like: "no space left for the block in the unincluded segment". This is solved by ensuring that we don't build + multiple blocks per slot. + +crates: + - name: "cumulus-client-consensus-aura"