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 nothing scheduled on session boundary #1403

Merged
merged 7 commits into from
Sep 6, 2023
Merged
Show file tree
Hide file tree
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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions polkadot/primitives/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ pub use v5::{
BackedCandidate, Balance, BlakeTwo256, Block, BlockId, BlockNumber, CandidateCommitments,
CandidateDescriptor, CandidateEvent, CandidateHash, CandidateIndex, CandidateReceipt,
CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, CollatorId, CollatorSignature,
CommittedCandidateReceipt, CompactStatement, ConsensusLog, CoreIndex, CoreOccupied, CoreState,
DisputeState, DisputeStatement, DisputeStatementSet, DownwardMessage, EncodeAs, ExecutorParam,
CommittedCandidateReceipt, CompactStatement, ConsensusLog, CoreIndex, CoreState, DisputeState,
DisputeStatement, DisputeStatementSet, DownwardMessage, EncodeAs, ExecutorParam,
ExecutorParams, ExecutorParamsHash, ExplicitDisputeStatement, GroupIndex, GroupRotationInfo,
Hash, HashT, HeadData, Header, HrmpChannelId, Id, InboundDownwardMessage, InboundHrmpMessage,
IndexedVec, InherentData, InvalidDisputeStatementKind, Moment, MultiDisputeStatementSet, Nonce,
Expand Down
54 changes: 0 additions & 54 deletions polkadot/primitives/src/v5/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -830,60 +830,6 @@ pub struct ParathreadEntry {
pub retries: u32,
}

/// An assignment for a parachain scheduled to be backed and included in a relay chain block.
#[derive(Clone, Encode, Decode, PartialEq, TypeInfo, RuntimeDebug)]
pub struct Assignment {
/// Assignment's ParaId
pub para_id: Id,
}

impl Assignment {
/// Create a new `Assignment`.
pub fn new(para_id: Id) -> Self {
Self { para_id }
}
}

/// An entry tracking a paras
#[derive(Clone, Encode, Decode, TypeInfo, PartialEq, RuntimeDebug)]
Copy link
Member Author

Choose a reason for hiding this comment

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

These types are only used internally, no need to expose them via primitives.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wonderful. Thanks for refactoring this and reducing the API surface.

pub struct ParasEntry<N = BlockNumber> {
/// The `Assignment`
pub assignment: Assignment,
/// The number of times the entry has timed out in availability.
pub availability_timeouts: u32,
/// The block height where this entry becomes invalid.
pub ttl: N,
}

impl<N> ParasEntry<N> {
/// Return `Id` from the underlying `Assignment`.
pub fn para_id(&self) -> Id {
self.assignment.para_id
}

/// Create a new `ParasEntry`.
pub fn new(assignment: Assignment, now: N) -> Self {
ParasEntry { assignment, availability_timeouts: 0, ttl: now }
}
}

/// What is occupying a specific availability core.
#[derive(Clone, Encode, Decode, TypeInfo, RuntimeDebug)]
#[cfg_attr(feature = "std", derive(PartialEq))]
pub enum CoreOccupied<N> {
/// The core is not occupied.
Free,
/// A paras.
Paras(ParasEntry<N>),
}

impl<N> CoreOccupied<N> {
/// Is core free?
pub fn is_free(&self) -> bool {
matches!(self, Self::Free)
}
}

/// A helper data-type for tracking validator-group rotations.
#[derive(Clone, Encode, Decode, TypeInfo, RuntimeDebug)]
#[cfg_attr(feature = "std", derive(PartialEq))]
Expand Down
1 change: 1 addition & 0 deletions polkadot/runtime/parachains/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ rand_chacha = { version = "0.3.1", default-features = false }
static_assertions = { version = "1.1.0", optional = true }
polkadot-parachain-primitives = { path = "../../parachain", default-features = false }
polkadot-runtime-metrics = { path = "../metrics", default-features = false}
polkadot-core-primitives = { path = "../../core-primitives", default-features = false }

[dev-dependencies]
futures = "0.3.21"
Expand Down
4 changes: 2 additions & 2 deletions polkadot/runtime/parachains/src/assigner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@
//! The Polkadot multiplexing assignment provider.
//! Provides blockspace assignments for both bulk and on demand parachains.
use frame_system::pallet_prelude::BlockNumberFor;
use primitives::{v5::Assignment, CoreIndex, Id as ParaId};
use primitives::{CoreIndex, Id as ParaId};

use crate::{
configuration, paras,
scheduler::common::{AssignmentProvider, AssignmentProviderConfig},
scheduler::common::{Assignment, AssignmentProvider, AssignmentProviderConfig},
};

pub use pallet::*;
Expand Down
5 changes: 2 additions & 3 deletions polkadot/runtime/parachains/src/assigner_on_demand/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ mod tests;

use crate::{
configuration, paras,
scheduler::common::{AssignmentProvider, AssignmentProviderConfig},
scheduler::common::{Assignment, AssignmentProvider, AssignmentProviderConfig},
};

use frame_support::{
Expand All @@ -46,7 +46,7 @@ use frame_support::{
},
};
use frame_system::pallet_prelude::*;
use primitives::{v5::Assignment, CoreIndex, Id as ParaId};
use primitives::{CoreIndex, Id as ParaId};
use sp_runtime::{
traits::{One, SaturatedConversion},
FixedPointNumber, FixedPointOperand, FixedU128, Perbill, Saturating,
Expand Down Expand Up @@ -606,7 +606,6 @@ impl<T: Config> AssignmentProvider<BlockNumberFor<T>> for Pallet<T> {
fn get_provider_config(_core_idx: CoreIndex) -> AssignmentProviderConfig<BlockNumberFor<T>> {
let config = <configuration::Pallet<T>>::config();
AssignmentProviderConfig {
availability_period: config.paras_availability_period,
Copy link
Member Author

Choose a reason for hiding this comment

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

No point in having this per assignment provider as the values have been unified.

max_availability_timeouts: config.on_demand_retries,
ttl: config.on_demand_ttl,
}
Expand Down
6 changes: 2 additions & 4 deletions polkadot/runtime/parachains/src/assigner_on_demand/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,11 @@ use crate::{
System, Test,
},
paras::{ParaGenesisArgs, ParaKind},
scheduler::common::Assignment,
};
use frame_support::{assert_noop, assert_ok, error::BadOrigin};
use pallet_balances::Error as BalancesError;
use primitives::{
v5::{Assignment, ValidationCode},
BlockNumber, SessionIndex,
};
use primitives::{v5::ValidationCode, BlockNumber, SessionIndex};
use sp_std::collections::btree_map::BTreeMap;

fn schedule_blank_para(id: ParaId, parakind: ParaKind) {
Expand Down
6 changes: 2 additions & 4 deletions polkadot/runtime/parachains/src/assigner_parachains.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@

use crate::{
configuration, paras,
scheduler::common::{AssignmentProvider, AssignmentProviderConfig},
scheduler::common::{Assignment, AssignmentProvider, AssignmentProviderConfig},
};
use frame_system::pallet_prelude::BlockNumberFor;
pub use pallet::*;
use primitives::{v5::Assignment, CoreIndex, Id as ParaId};
use primitives::{CoreIndex, Id as ParaId};

#[frame_support::pallet]
pub mod pallet {
Expand Down Expand Up @@ -57,9 +57,7 @@ impl<T: Config> AssignmentProvider<BlockNumberFor<T>> for Pallet<T> {
fn push_assignment_for_core(_: CoreIndex, _: Assignment) {}

fn get_provider_config(_core_idx: CoreIndex) -> AssignmentProviderConfig<BlockNumberFor<T>> {
let config = <configuration::Pallet<T>>::config();
AssignmentProviderConfig {
availability_period: config.paras_availability_period,
// The next assignment already goes to the same [`ParaId`], no timeout tracking needed.
max_availability_timeouts: 0,
// The next assignment already goes to the same [`ParaId`], this can be any number
Expand Down
14 changes: 8 additions & 6 deletions polkadot/runtime/parachains/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,20 @@ use crate::{
configuration, inclusion, initializer, paras,
paras::ParaKind,
paras_inherent,
scheduler::{self, common::AssignmentProviderConfig},
scheduler::{
self,
common::{Assignment, AssignmentProviderConfig},
CoreOccupied, ParasEntry,
},
session_info, shared,
};
use bitvec::{order::Lsb0 as BitOrderLsb0, vec::BitVec};
use frame_support::pallet_prelude::*;
use frame_system::pallet_prelude::*;
use primitives::{
collator_signature_payload,
v5::{Assignment, ParasEntry},
AvailabilityBitfield, BackedCandidate, CandidateCommitments, CandidateDescriptor,
CandidateHash, CollatorId, CollatorSignature, CommittedCandidateReceipt, CompactStatement,
CoreIndex, CoreOccupied, DisputeStatement, DisputeStatementSet, GroupIndex, HeadData,
collator_signature_payload, AvailabilityBitfield, BackedCandidate, CandidateCommitments,
CandidateDescriptor, CandidateHash, CollatorId, CollatorSignature, CommittedCandidateReceipt,
CompactStatement, CoreIndex, DisputeStatement, DisputeStatementSet, GroupIndex, HeadData,
Id as ParaId, IndexedVec, InherentData as ParachainsInherentData, InvalidDisputeStatementKind,
PersistedValidationData, SessionIndex, SigningContext, UncheckedSigned,
ValidDisputeStatementKind, ValidationCode, ValidatorId, ValidatorIndex, ValidityAttestation,
Expand Down
17 changes: 13 additions & 4 deletions polkadot/runtime/parachains/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,20 @@ pub struct HostConfiguration<BlockNumber> {
///
/// Must be non-zero.
pub group_rotation_frequency: BlockNumber,
/// The availability period, in blocks. This is the amount of blocks
/// after inclusion that validators have to make the block available and signal its
/// availability to the chain.
/// The minimum availability period, in blocks.
///
/// Must be at least 1.
/// This is the minimum amount of blocks after a core became occupied that validators have time
/// to make the block available.
///
/// This value only has effect on group rotations. If backers backed something at the end of
/// their rotation, the occupied core affects the backing group that comes afterwards. We limit
/// the effect one backing group can have on the next to `paras_availability_period` blocks.
///
/// Within a group rotation there is no timeout as backers are only affecting themselves.
///
/// Must be at least 1. With a value of 1, the previous group will not be able to negatively
/// affect the following group at the expense of a tight availability timeline at group
/// rotation boundaries.
pub paras_availability_period: BlockNumber,
/// The amount of blocks ahead to schedule paras.
pub scheduling_lookahead: u32,
Expand Down
Loading
Loading