Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Undo v4 changes, move them to vstaging #7103

Open
wants to merge 26 commits into
base: alex/parathreads_review
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
b6b9f8e
Undo v4 changes, move them to vstaging
Apr 19, 2023
c76ce40
Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging
Apr 19, 2023
e2d5869
minor
Apr 19, 2023
fc276d6
Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging
Apr 25, 2023
7baa04f
Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging
May 19, 2023
94d24d5
Fix imports
May 19, 2023
f943c1a
Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging
May 19, 2023
59f6bc2
Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging
May 29, 2023
d65d96e
include version dispatch on availablity_cores() call
May 30, 2023
cbfa7a1
ifmt
May 30, 2023
2471334
Fix test-runtime
May 30, 2023
678b1d8
fix node runtime api test
May 30, 2023
13fa655
Undo RuntimeDebug -> Debug changes in v4
May 30, 2023
2cf57a8
runtime-api tests fix
May 30, 2023
4d01e65
Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging
May 31, 2023
af57dd0
Fix removing api version by accident
May 31, 2023
b413cc7
Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging
Jun 1, 2023
6fae5a3
Remove naked unwrap()s
Jun 1, 2023
a665df0
Add availability_cores_on_demand to mock and use that in tests
Jun 1, 2023
42bc527
Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging
Jun 2, 2023
ea8a0c6
Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging
Jun 5, 2023
a198b95
Merge branch 'alex/parathreads_review' into alex/parathreads_vstaging
Jun 6, 2023
918243e
Address PR comments
Jun 8, 2023
49ebc7d
Rename availability_cores_on_demand to ..._vstaging
Jun 8, 2023
c49582c
pebkac
Jun 8, 2023
6273880
minor refactor
Jun 8, 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
24 changes: 15 additions & 9 deletions node/core/runtime-api/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ use polkadot_node_primitives::{BabeAllowedSlots, BabeEpoch, BabeEpochConfigurati
use polkadot_node_subsystem::SpawnGlue;
use polkadot_node_subsystem_test_helpers::make_subsystem_context;
use polkadot_primitives::{
runtime_api::ParachainHost, AuthorityDiscoveryId, Block, CandidateEvent,
CommittedCandidateReceipt, CoreState, GroupRotationInfo, Id as ParaId, InboundDownwardMessage,
InboundHrmpMessage, OccupiedCoreAssumption, PersistedValidationData, PvfCheckStatement,
ScrapedOnChainVotes, SessionIndex, SessionInfo, ValidationCode, ValidationCodeHash,
ValidatorId, ValidatorIndex, ValidatorSignature,
runtime_api::ParachainHost, v4::CoreState, vstaging, AuthorityDiscoveryId, Block,
CandidateEvent, CommittedCandidateReceipt, GroupRotationInfo, Id as ParaId,
InboundDownwardMessage, InboundHrmpMessage, OccupiedCoreAssumption, PersistedValidationData,
PvfCheckStatement, ScrapedOnChainVotes, SessionIndex, SessionInfo, ValidationCode,
ValidationCodeHash, ValidatorId, ValidatorIndex, ValidatorSignature,
};
use sp_api::ProvideRuntimeApi;
use sp_authority_discovery::AuthorityDiscoveryApi;
Expand All @@ -42,6 +42,7 @@ struct MockRuntimeApi {
validators: Vec<ValidatorId>,
validator_groups: Vec<Vec<ValidatorIndex>>,
availability_cores: Vec<CoreState>,
availability_cores_on_demand: Vec<vstaging::CoreState>,
availability_cores_wait: Arc<Mutex<()>>,
validation_data: HashMap<ParaId, PersistedValidationData>,
session_index_for_child: SessionIndex,
Expand Down Expand Up @@ -90,6 +91,11 @@ sp_api::mock_impl_runtime_apis! {
self.availability_cores.clone()
}

fn availability_cores_on_demand(&self) -> Vec<vstaging::CoreState> {
let _lock = self.availability_cores_wait.lock().unwrap();
self.availability_cores_on_demand.clone()
}

fn persisted_validation_data(
&self,
para: ParaId,
Expand Down Expand Up @@ -334,7 +340,7 @@ fn requests_availability_cores() {
})
.await;

assert_eq!(rx.await.unwrap().unwrap(), runtime_api.availability_cores);
assert_eq!(rx.await.unwrap().unwrap(), runtime_api.availability_cores_on_demand);

ctx_handle.send(FromOrchestra::Signal(OverseerSignal::Conclude)).await;
};
Expand Down Expand Up @@ -889,9 +895,9 @@ fn multiple_requests_in_parallel_are_working() {

let join = future::join_all(receivers);

join.await
.into_iter()
.for_each(|r| assert_eq!(r.unwrap().unwrap(), runtime_api.availability_cores));
join.await.into_iter().for_each(|r| {
assert_eq!(r.unwrap().unwrap(), runtime_api.availability_cores_on_demand)
});

ctx_handle.send(FromOrchestra::Signal(OverseerSignal::Conclude)).await;
};
Expand Down
4 changes: 2 additions & 2 deletions node/network/collator-protocol/src/validator_side/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ use polkadot_node_subsystem::messages::{AllMessages, RuntimeApiMessage, RuntimeA
use polkadot_node_subsystem_test_helpers as test_helpers;
use polkadot_node_subsystem_util::TimeoutExt;
use polkadot_primitives::{
v4::CollatorRestrictionKind, vstaging::CollatorRestrictions, CollatorPair, CoreState,
GroupIndex, GroupRotationInfo, OccupiedCore, ScheduledCore, ValidatorId, ValidatorIndex,
CollatorPair, CollatorRestrictionKind, CollatorRestrictions, CoreState, GroupIndex,
GroupRotationInfo, OccupiedCore, ScheduledCore, ValidatorId, ValidatorIndex,
};
use polkadot_primitives_test_helpers::{
dummy_candidate_descriptor, dummy_candidate_receipt_bad_sig, dummy_hash,
Expand Down
13 changes: 12 additions & 1 deletion node/subsystem-types/src/runtime_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,18 @@ where
&self,
at: Hash,
) -> Result<Vec<CoreState<Hash, BlockNumber>>, ApiError> {
self.runtime_api().availability_cores(at)
let version = self
Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed somewhere that such transformations in the client are not encouraged and we prefer doing this in the subsystem. Unfortunately I don't remember the exact reasons.

For your particular case I think it's okay though. Depending on the runtime api you call either the new or the old version of the function. And you do this workaround to avoid the changed_in issues with the runtime api.

Copy link
Author

Choose a reason for hiding this comment

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

Which file would this ideally go to then? What exactly is the subsystem again? I'm only aware of the node/client and runtime distinction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Subsystem is a module (lets say) from the node - e.g. approval-voting, dispute-coordinator etc.
You can put a function in any helpers module (e.g. https://github.com/paritytech/polkadot/blob/dbae30efe080a1d41fe54ef4da8af47614c9ca93/node/subsystem-util/src/runtime/mod.rs) and call it each time you need the availability_cores.

But I'd suggest not to touch it for now.

.api_version_parachain_host(at)
.await
.unwrap_or_default()
.unwrap_or_default();
if version >= 5 {
self.runtime_api().availability_cores_on_demand(at)
} else {
self.runtime_api()
.availability_cores(at)
.map(|css| css.into_iter().map(vstaging::corestate_from_v4).collect())
}
}

async fn persisted_validation_data(
Expand Down
20 changes: 12 additions & 8 deletions primitives/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,23 +41,27 @@ pub use v4::{
CandidateCommitments, CandidateDescriptor, CandidateEvent, CandidateHash, CandidateIndex,
CandidateReceipt, CheckedDisputeStatementSet, CheckedMultiDisputeStatementSet, CollatorId,
CollatorSignature, CommittedCandidateReceipt, CompactStatement, ConsensusLog, CoreIndex,
CoreOccupied, 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, OccupiedCore,
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,
OccupiedCoreAssumption, OutboundHrmpMessage, ParathreadClaim, ParathreadEntry,
PersistedValidationData, PvfCheckStatement, PvfExecTimeoutKind, PvfPrepTimeoutKind,
RuntimeMetricLabel, RuntimeMetricLabelValue, RuntimeMetricLabelValues, RuntimeMetricLabels,
RuntimeMetricOp, RuntimeMetricUpdate, ScheduledCore, ScrapedOnChainVotes, SessionIndex,
SessionInfo, Signature, Signed, SignedAvailabilityBitfield, SignedAvailabilityBitfields,
SignedStatement, SigningContext, Slot, UncheckedSigned, UncheckedSignedAvailabilityBitfield,
RuntimeMetricOp, RuntimeMetricUpdate, ScrapedOnChainVotes, SessionIndex, SessionInfo,
Signature, Signed, SignedAvailabilityBitfield, SignedAvailabilityBitfields, SignedStatement,
SigningContext, Slot, UncheckedSigned, UncheckedSignedAvailabilityBitfield,
UncheckedSignedAvailabilityBitfields, UncheckedSignedStatement, UpgradeGoAhead,
UpgradeRestriction, UpwardMessage, ValidDisputeStatementKind, ValidationCode,
ValidationCodeHash, ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation,
ValidityError, ASSIGNMENT_KEY_TYPE_ID, LOWEST_PUBLIC_ID, MAX_CODE_SIZE, MAX_HEAD_DATA_SIZE,
MAX_POV_SIZE, PARACHAINS_INHERENT_IDENTIFIER, PARACHAIN_KEY_TYPE_ID,
};

pub use vstaging::{
CollatorRestrictionKind, CollatorRestrictions, CoreOccupied, CoreState, OccupiedCore,
ScheduledCore,
};

#[cfg(feature = "std")]
pub use v4::{AssignmentPair, CollatorPair, ValidatorPair};
9 changes: 7 additions & 2 deletions primitives/src/runtime_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@

use crate::{
vstaging, BlockNumber, CandidateCommitments, CandidateEvent, CandidateHash,
CommittedCandidateReceipt, CoreState, DisputeState, ExecutorParams, GroupRotationInfo,
CommittedCandidateReceipt, DisputeState, ExecutorParams, GroupRotationInfo,
OccupiedCoreAssumption, PersistedValidationData, PvfCheckStatement, ScrapedOnChainVotes,
SessionIndex, SessionInfo, ValidatorId, ValidatorIndex, ValidatorSignature,
};
Expand All @@ -135,7 +135,12 @@ sp_api::decl_runtime_apis! {

/// Yields information on all availability cores as relevant to the child block.
/// Cores are either free or occupied. Free cores can have paras assigned to them.
fn availability_cores() -> Vec<CoreState<H, N>>;
fn availability_cores() -> Vec<crate::v4::CoreState<H, N>>;

/// Yields information on all availability cores as relevant to the child block.
/// Cores are either free or occupied. Free cores can have paras assigned to them.
#[api_version(5)]
fn availability_cores_on_demand() -> Vec<vstaging::CoreState<H, N>>;
Copy link
Member

Choose a reason for hiding this comment

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

So this only gets us on-demand availability cores? I guess not, from the docs and from my understanding. So where does the name come from?

Copy link
Author

Choose a reason for hiding this comment

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

It's supposed to be the v5 availability_cores call. I thought just calling it availability_cores_v5 would be a bit meaningless but I see how the current name is misleading as well.


/// Yields the persisted validation data for the given `ParaId` along with an assumption that
/// should be used if the para currently occupies a core.
Expand Down
126 changes: 7 additions & 119 deletions primitives/src/v4/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ use bitvec::vec::BitVec;
use parity_scale_codec::{Decode, Encode};
use scale_info::TypeInfo;
use sp_std::{
collections::btree_set::BTreeSet,
marker::PhantomData,
prelude::*,
slice::{Iter, IterMut},
Expand All @@ -29,7 +28,7 @@ use sp_std::{

use application_crypto::KeyTypeId;
use inherents::InherentIdentifier;
use primitives::{OpaquePeerId, RuntimeDebug};
use primitives::RuntimeDebug;
use runtime_primitives::traits::{AppVerify, Header as HeaderT};
use sp_arithmetic::traits::{BaseArithmetic, Saturating};

Expand All @@ -50,9 +49,6 @@ pub use polkadot_parachain::primitives::{

use serde::{Deserialize, Serialize};

#[cfg(feature = "std")]
use sc_network::PeerId;

pub use sp_authority_discovery::AuthorityId as AuthorityDiscoveryId;
pub use sp_consensus_slots::Slot;
pub use sp_staking::SessionIndex;
Expand Down Expand Up @@ -818,121 +814,14 @@ pub struct ParathreadEntry {
pub retries: u32,
}

/// An Assignemnt for a paras going to produce a paras block.
#[derive(Clone, Encode, Decode, PartialEq, TypeInfo, RuntimeDebug)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember these are actually not part of v4 at the moment and you are undoing previous work done in alex/parathreads_review, right?

Copy link
Author

Choose a reason for hiding this comment

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

Exactly! At the end of the day, we'll need to make sure v4 is just as it is on master. Which I think it is here but will confirm later down the road when we have the final feature branch with a diff to master.

pub struct Assignment {
/// Assignment's ParaId
pub para_id: Id,
/// Assignment's CollatorRestrictions
pub collator_restrictions: CollatorRestrictions,
}

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

/// Restrictions on collators for a specific paras block.
#[derive(Clone, Encode, Decode, PartialEq, TypeInfo, RuntimeDebug)]
pub struct CollatorRestrictions {
/// Collators to prefer/allow.
/// Empty set means no restrictions.
collator_peer_ids: BTreeSet<OpaquePeerId>,
restriction_kind: CollatorRestrictionKind,
}

impl CollatorRestrictions {
/// Specialised new function for parachains.
pub fn none() -> Self {
CollatorRestrictions {
collator_peer_ids: BTreeSet::new(),
restriction_kind: CollatorRestrictionKind::Preferred,
}
}

/// Create a new `CollatorRestrictions`.
pub fn new(
collator_peer_ids: BTreeSet<OpaquePeerId>,
restriction_kind: CollatorRestrictionKind,
) -> Self {
CollatorRestrictions { collator_peer_ids, restriction_kind }
}

/// Is peer_id allowed to collate?
#[cfg(feature = "std")]
pub fn can_collate(&self, peer_id: &PeerId) -> bool {
self.collator_peer_ids.is_empty() ||
match self.restriction_kind {
CollatorRestrictionKind::Preferred => true,
CollatorRestrictionKind::Required => {
let peer_id = OpaquePeerId(peer_id.to_bytes());
self.collator_peer_ids.contains(&peer_id)
},
}
}
}

/// How to apply the collator restrictions.
#[derive(Clone, Encode, Decode, PartialEq, TypeInfo, RuntimeDebug)]
pub enum CollatorRestrictionKind {
/// peer ids mentioned will be preferred in connections, but others are still allowed.
Preferred,
/// Any collator with a `PeerId` not in the set of `CollatorRestrictions` will be rejected.
Required,
}

/// An entry tracking a paras
#[derive(Clone, Encode, Decode, TypeInfo, PartialEq, RuntimeDebug)]
pub struct ParasEntry {
/// The `Assignment`
pub assignment: Assignment,
/// Number of times this has been retried.
pub retries: u32,
}

impl From<Assignment> for ParasEntry {
fn from(assignment: Assignment) -> Self {
ParasEntry { assignment, retries: 0 }
}
}

impl ParasEntry {
/// Create a new `ParasEntry`.
pub fn new(assignment: Assignment) -> Self {
ParasEntry { assignment, retries: 0 }
}

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

/// Return `CollatorRestrictions` from the underlying `Assignment`.
pub fn collator_restrictions(&self) -> &CollatorRestrictions {
&self.assignment.collator_restrictions
}
}

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

impl CoreOccupied {
/// Is core free?
pub fn is_free(&self) -> bool {
match self {
Self::Free => true,
Self::Paras(_) => false,
}
}
/// A parathread.
Parathread(ParathreadEntry),
/// A parachain.
Parachain,
}

/// A helper data-type for tracking validator-group rotations.
Expand Down Expand Up @@ -1062,11 +951,10 @@ impl<H, N> OccupiedCore<H, N> {
#[derive(Clone, Encode, Decode, TypeInfo, RuntimeDebug)]
#[cfg_attr(feature = "std", derive(PartialEq))]
pub struct ScheduledCore {
// TODO: Is the same as Assignment
/// The ID of a para scheduled.
pub para_id: Id,
/// The collator restrictions.
pub collator_restrictions: CollatorRestrictions,
/// The collator required to author the block, if any.
pub collator: Option<CollatorId>,
}

/// The state of a particular availability core.
Expand Down
Loading