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

backing: move the min votes threshold to the runtime #1200

Merged
merged 15 commits into from
Aug 31, 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
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,14 @@ impl RuntimeApiSubsystemClient for BlockChainRpcClient {
.await?)
}

async fn minimum_backing_votes(
&self,
at: Hash,
session_index: polkadot_primitives::SessionIndex,
) -> Result<u32, ApiError> {
Ok(self.rpc_client.parachain_host_minimum_backing_votes(at, session_index).await?)
}

async fn staging_async_backing_params(&self, at: Hash) -> Result<AsyncBackingParams, ApiError> {
Ok(self.rpc_client.parachain_host_staging_async_backing_params(at).await?)
}
Expand Down
10 changes: 10 additions & 0 deletions cumulus/client/relay-chain-rpc-interface/src/rpc_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,16 @@ impl RelayChainRpcClient {
.await
}

/// Get the minimum number of backing votes for a candidate.
pub async fn parachain_host_minimum_backing_votes(
Copy link
Member

Choose a reason for hiding this comment

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

Is this even needed in cumulus? It should not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't compile without it, I see all runtime APIs have counterparts here.
I'm not sure why they're needed though

Copy link
Member

Choose a reason for hiding this comment

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

I had similar question some time ago, and got this suggestion from @skunert: paritytech/cumulus#2160 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't fully agree with that. For UX simplicity, I think the interface should expose the bare necessities for collators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can open a separate issue to discuss this further

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see: #1332

&self,
at: RelayHash,
_session_index: SessionIndex,
) -> Result<u32, RelayChainError> {
self.call_remote_runtime_function("ParachainHost_minimum_backing_votes", at, None::<()>)
.await
}

#[allow(missing_docs)]
pub async fn parachain_host_staging_async_backing_params(
&self,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ pub use impls::{RococoWococoMessageHandler, WococoRococoMessageHandler};
pub use parachains_common::{AccountId, Balance};
pub use paste;
use polkadot_parachain::primitives::HrmpChannelId;
use polkadot_primitives::runtime_api::runtime_decl_for_parachain_host::ParachainHostV6;
pub use polkadot_runtime_parachains::inclusion::{AggregateMessageOrigin, UmpQueueId};
pub use sp_core::{sr25519, storage::Storage, Get};
use sp_tracing;
Expand Down
1 change: 1 addition & 0 deletions polkadot/node/core/backing/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ pub enum Error {
RuntimeApiUnavailable(#[source] oneshot::Canceled),

#[error("a channel was closed before receipt in try_join!")]
#[fatal]
JoinMultiple(#[source] oneshot::Canceled),

#[error("Obtaining erasure chunks failed")]
Expand Down
55 changes: 33 additions & 22 deletions polkadot/node/core/backing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ use futures::{

use error::{Error, FatalResult};
use polkadot_node_primitives::{
minimum_votes, AvailableData, InvalidCandidate, PoV, SignedFullStatementWithPVD,
StatementWithPVD, ValidationResult,
AvailableData, InvalidCandidate, PoV, SignedFullStatementWithPVD, StatementWithPVD,
ValidationResult,
};
use polkadot_node_subsystem::{
messages::{
Expand All @@ -98,7 +98,9 @@ use polkadot_node_subsystem_util::{
backing_implicit_view::{FetchError as ImplicitViewFetchError, View as ImplicitView},
request_from_runtime, request_session_index_for_child, request_validator_groups,
request_validators,
runtime::{prospective_parachains_mode, ProspectiveParachainsMode},
runtime::{
self, prospective_parachains_mode, request_min_backing_votes, ProspectiveParachainsMode,
},
Validator,
};
use polkadot_primitives::{
Expand Down Expand Up @@ -219,6 +221,8 @@ struct PerRelayParentState {
awaiting_validation: HashSet<CandidateHash>,
/// Data needed for retrying in case of `ValidatedCandidateCommand::AttestNoPoV`.
fallbacks: HashMap<CandidateHash, AttestingData>,
/// The minimum backing votes threshold.
minimum_backing_votes: u32,
}

struct PerCandidateState {
Expand Down Expand Up @@ -400,8 +404,8 @@ impl TableContextTrait for TableContext {
self.groups.get(group).map_or(false, |g| g.iter().any(|a| a == authority))
}

fn requisite_votes(&self, group: &ParaId) -> usize {
self.groups.get(group).map_or(usize::MAX, |g| minimum_votes(g.len()))
fn get_group_size(&self, group: &ParaId) -> Option<usize> {
self.groups.get(group).map(|g| g.len())
}
}

Expand Down Expand Up @@ -965,39 +969,38 @@ async fn construct_per_relay_parent_state<Context>(
($x: expr) => {
match $x {
Ok(x) => x,
Err(e) => {
gum::warn!(
target: LOG_TARGET,
err = ?e,
"Failed to fetch runtime API data for job",
);
Err(err) => {
// Only bubble up fatal errors.
error::log_error(Err(Into::<runtime::Error>::into(err).into()))?;

// We can't do candidate validation work if we don't have the
// requisite runtime API data. But these errors should not take
// down the node.
return Ok(None);
}
return Ok(None)
},
}
}
};
}

let parent = relay_parent;

let (validators, groups, session_index, cores) = futures::try_join!(
let (session_index, validators, groups, cores) = futures::try_join!(
request_session_index_for_child(parent, ctx.sender()).await,
request_validators(parent, ctx.sender()).await,
request_validator_groups(parent, ctx.sender()).await,
request_session_index_for_child(parent, ctx.sender()).await,
request_from_runtime(parent, ctx.sender(), |tx| {
RuntimeApiRequest::AvailabilityCores(tx)
},)
.await,
)
.map_err(Error::JoinMultiple)?;

let session_index = try_runtime_api!(session_index);
let validators: Vec<_> = try_runtime_api!(validators);
let (validator_groups, group_rotation_info) = try_runtime_api!(groups);
let session_index = try_runtime_api!(session_index);
let cores = try_runtime_api!(cores);
let minimum_backing_votes =
try_runtime_api!(request_min_backing_votes(parent, session_index, ctx.sender()).await);

let signing_context = SigningContext { parent_hash: parent, session_index };
let validator =
Expand Down Expand Up @@ -1061,6 +1064,7 @@ async fn construct_per_relay_parent_state<Context>(
issued_statements: HashSet::new(),
awaiting_validation: HashSet::new(),
fallbacks: HashMap::new(),
minimum_backing_votes,
}))
}

Expand Down Expand Up @@ -1563,10 +1567,13 @@ async fn post_import_statement_actions<Context>(
rp_state: &mut PerRelayParentState,
summary: Option<&TableSummary>,
) -> Result<(), Error> {
if let Some(attested) = summary
.as_ref()
.and_then(|s| rp_state.table.attested_candidate(&s.candidate, &rp_state.table_context))
{
if let Some(attested) = summary.as_ref().and_then(|s| {
rp_state.table.attested_candidate(
&s.candidate,
&rp_state.table_context,
rp_state.minimum_backing_votes,
)
}) {
let candidate_hash = attested.candidate.hash();

// `HashSet::insert` returns true if the thing wasn't in there already.
Expand Down Expand Up @@ -2009,7 +2016,11 @@ fn handle_get_backed_candidates_message(
};
rp_state
.table
.attested_candidate(&candidate_hash, &rp_state.table_context)
.attested_candidate(
&candidate_hash,
&rp_state.table_context,
rp_state.minimum_backing_votes,
)
.and_then(|attested| table_attested_to_backed(attested, &rp_state.table_context))
})
.collect();
Expand Down
31 changes: 22 additions & 9 deletions polkadot/node/core/backing/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use polkadot_node_subsystem::{
use polkadot_node_subsystem_test_helpers as test_helpers;
use polkadot_primitives::{
CandidateDescriptor, GroupRotationInfo, HeadData, PersistedValidationData, PvfExecTimeoutKind,
ScheduledCore, SessionIndex,
ScheduledCore, SessionIndex, LEGACY_MIN_BACKING_VOTES,
};
use sp_application_crypto::AppCrypto;
use sp_keyring::Sr25519Keyring;
Expand Down Expand Up @@ -80,6 +80,7 @@ struct TestState {
head_data: HashMap<ParaId, HeadData>,
signing_context: SigningContext,
relay_parent: Hash,
minimum_backing_votes: u32,
}

impl TestState {
Expand Down Expand Up @@ -150,6 +151,7 @@ impl Default for TestState {
validation_data,
signing_context,
relay_parent,
minimum_backing_votes: LEGACY_MIN_BACKING_VOTES,
}
}
}
Expand Down Expand Up @@ -250,6 +252,16 @@ async fn test_startup(virtual_overseer: &mut VirtualOverseer, test_state: &TestS
}
);

// Check that subsystem job issues a request for the session index for child.
assert_matches!(
virtual_overseer.recv().await,
AllMessages::RuntimeApi(
RuntimeApiMessage::Request(parent, RuntimeApiRequest::SessionIndexForChild(tx))
) if parent == test_state.relay_parent => {
tx.send(Ok(test_state.signing_context.session_index)).unwrap();
}
);

// Check that subsystem job issues a request for a validator set.
assert_matches!(
virtual_overseer.recv().await,
Expand All @@ -270,23 +282,24 @@ async fn test_startup(virtual_overseer: &mut VirtualOverseer, test_state: &TestS
}
);

// Check that subsystem job issues a request for the session index for child.
// Check that subsystem job issues a request for the availability cores.
assert_matches!(
virtual_overseer.recv().await,
AllMessages::RuntimeApi(
RuntimeApiMessage::Request(parent, RuntimeApiRequest::SessionIndexForChild(tx))
RuntimeApiMessage::Request(parent, RuntimeApiRequest::AvailabilityCores(tx))
) if parent == test_state.relay_parent => {
tx.send(Ok(test_state.signing_context.session_index)).unwrap();
tx.send(Ok(test_state.availability_cores.clone())).unwrap();
}
);

// Check that subsystem job issues a request for the availability cores.
// Check if subsystem job issues a request for the minimum backing votes.
assert_matches!(
virtual_overseer.recv().await,
AllMessages::RuntimeApi(
RuntimeApiMessage::Request(parent, RuntimeApiRequest::AvailabilityCores(tx))
) if parent == test_state.relay_parent => {
tx.send(Ok(test_state.availability_cores.clone())).unwrap();
AllMessages::RuntimeApi(RuntimeApiMessage::Request(
parent,
RuntimeApiRequest::MinimumBackingVotes(session_index, tx),
)) if parent == test_state.relay_parent && session_index == test_state.signing_context.session_index => {
tx.send(Ok(test_state.minimum_backing_votes)).unwrap();
}
);
}
Expand Down
30 changes: 21 additions & 9 deletions polkadot/node/core/backing/src/tests/prospective_parachains.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,13 +138,24 @@ async fn activate_leaf(
}

for (hash, number) in ancestry_iter.take(requested_len) {
// Check that subsystem job issues a request for a validator set.
let msg = match next_overseer_message.take() {
Some(msg) => msg,
None => virtual_overseer.recv().await,
};

// Check that subsystem job issues a request for the session index for child.
assert_matches!(
msg,
AllMessages::RuntimeApi(
RuntimeApiMessage::Request(parent, RuntimeApiRequest::SessionIndexForChild(tx))
) if parent == hash => {
tx.send(Ok(test_state.signing_context.session_index)).unwrap();
}
);

// Check that subsystem job issues a request for a validator set.
assert_matches!(
virtual_overseer.recv().await,
AllMessages::RuntimeApi(
RuntimeApiMessage::Request(parent, RuntimeApiRequest::Validators(tx))
) if parent == hash => {
Expand All @@ -164,23 +175,24 @@ async fn activate_leaf(
}
);

// Check that subsystem job issues a request for the session index for child.
// Check that subsystem job issues a request for the availability cores.
assert_matches!(
virtual_overseer.recv().await,
AllMessages::RuntimeApi(
RuntimeApiMessage::Request(parent, RuntimeApiRequest::SessionIndexForChild(tx))
RuntimeApiMessage::Request(parent, RuntimeApiRequest::AvailabilityCores(tx))
) if parent == hash => {
tx.send(Ok(test_state.signing_context.session_index)).unwrap();
tx.send(Ok(test_state.availability_cores.clone())).unwrap();
}
);

// Check that subsystem job issues a request for the availability cores.
// Check if subsystem job issues a request for the minimum backing votes.
assert_matches!(
virtual_overseer.recv().await,
AllMessages::RuntimeApi(
RuntimeApiMessage::Request(parent, RuntimeApiRequest::AvailabilityCores(tx))
) if parent == hash => {
tx.send(Ok(test_state.availability_cores.clone())).unwrap();
AllMessages::RuntimeApi(RuntimeApiMessage::Request(
parent,
RuntimeApiRequest::MinimumBackingVotes(session_index, tx),
)) if parent == hash && session_index == test_state.signing_context.session_index => {
tx.send(Ok(test_state.minimum_backing_votes)).unwrap();
}
);
}
Expand Down
15 changes: 15 additions & 0 deletions polkadot/node/core/runtime-api/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ pub(crate) struct RequestResultCache {
LruMap<Hash, Vec<(SessionIndex, CandidateHash, vstaging::slashing::PendingSlashes)>>,
key_ownership_proof:
LruMap<(Hash, ValidatorId), Option<vstaging::slashing::OpaqueKeyOwnershipProof>>,
minimum_backing_votes: LruMap<SessionIndex, u32>,

staging_para_backing_state: LruMap<(Hash, ParaId), Option<vstaging::BackingState>>,
staging_async_backing_params: LruMap<Hash, vstaging::AsyncBackingParams>,
Expand Down Expand Up @@ -97,6 +98,7 @@ impl Default for RequestResultCache {
disputes: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
unapplied_slashes: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
key_ownership_proof: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
minimum_backing_votes: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),

staging_para_backing_state: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
staging_async_backing_params: LruMap::new(ByLength::new(DEFAULT_CACHE_CAP)),
Expand Down Expand Up @@ -434,6 +436,18 @@ impl RequestResultCache {
None
}

pub(crate) fn minimum_backing_votes(&mut self, session_index: SessionIndex) -> Option<u32> {
self.minimum_backing_votes.get(&session_index).copied()
}

pub(crate) fn cache_minimum_backing_votes(
&mut self,
session_index: SessionIndex,
minimum_backing_votes: u32,
) {
self.minimum_backing_votes.insert(session_index, minimum_backing_votes);
}

pub(crate) fn staging_para_backing_state(
&mut self,
key: (Hash, ParaId),
Expand Down Expand Up @@ -469,6 +483,7 @@ pub(crate) enum RequestResult {
// The structure of each variant is (relay_parent, [params,]*, result)
Authorities(Hash, Vec<AuthorityDiscoveryId>),
Validators(Hash, Vec<ValidatorId>),
MinimumBackingVotes(Hash, SessionIndex, u32),
ValidatorGroups(Hash, (Vec<Vec<ValidatorIndex>>, GroupRotationInfo)),
AvailabilityCores(Hash, Vec<CoreState>),
PersistedValidationData(Hash, ParaId, OccupiedCoreAssumption, Option<PersistedValidationData>),
Expand Down
18 changes: 18 additions & 0 deletions polkadot/node/core/runtime-api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ where
self.requests_cache.cache_authorities(relay_parent, authorities),
Validators(relay_parent, validators) =>
self.requests_cache.cache_validators(relay_parent, validators),
MinimumBackingVotes(_, session_index, minimum_backing_votes) => self
.requests_cache
.cache_minimum_backing_votes(session_index, minimum_backing_votes),
ValidatorGroups(relay_parent, groups) =>
self.requests_cache.cache_validator_groups(relay_parent, groups),
AvailabilityCores(relay_parent, cores) =>
Expand Down Expand Up @@ -301,6 +304,15 @@ where
Request::StagingAsyncBackingParams(sender) =>
query!(staging_async_backing_params(), sender)
.map(|sender| Request::StagingAsyncBackingParams(sender)),
Request::MinimumBackingVotes(index, sender) => {
if let Some(value) = self.requests_cache.minimum_backing_votes(index) {
self.metrics.on_cached_request();
let _ = sender.send(Ok(value));
None
} else {
Some(Request::MinimumBackingVotes(index, sender))
}
},
}
}

Expand Down Expand Up @@ -551,6 +563,12 @@ where
ver = Request::SUBMIT_REPORT_DISPUTE_LOST_RUNTIME_REQUIREMENT,
sender
),
Request::MinimumBackingVotes(index, sender) => query!(
MinimumBackingVotes,
minimum_backing_votes(index),
ver = Request::MINIMUM_BACKING_VOTES_RUNTIME_REQUIREMENT,
sender
),

Request::StagingParaBackingState(para, sender) => {
query!(
Expand Down
Loading