This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
backing: move the min votes threshold to the runtime #7577
Draft
alindima
wants to merge
6
commits into
master
Choose a base branch
from
alindima-add-min-backing-votes-to-runtime
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
5feadf4
move min backing votes const to runtime
alindima eb864e0
add runtime migration
alindima 8c9a8d8
Merge remote-tracking branch 'origin/master' into alindima-add-min-ba…
alindima 09c9f4e
introduce api versioning for min_backing votes
alindima fe51ef8
also add min_backing_votes runtime calls to statement-distribution
alindima c846d6d
remove explicit version runtime API call
alindima File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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::{ | ||
|
@@ -96,8 +96,7 @@ use polkadot_node_subsystem::{ | |
use polkadot_node_subsystem_util::{ | ||
self as util, | ||
backing_implicit_view::{FetchError as ImplicitViewFetchError, View as ImplicitView}, | ||
request_from_runtime, request_session_index_for_child, request_validator_groups, | ||
request_validators, | ||
request_from_runtime, request_validator_groups, request_validators, | ||
runtime::{prospective_parachains_mode, ProspectiveParachainsMode}, | ||
Validator, | ||
}; | ||
|
@@ -116,6 +115,7 @@ use statement_table::{ | |
}, | ||
Config as TableConfig, Context as TableContextTrait, Table, | ||
}; | ||
use util::runtime::{request_min_backing_votes, RuntimeInfo}; | ||
|
||
mod error; | ||
|
||
|
@@ -219,6 +219,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 { | ||
|
@@ -275,6 +277,8 @@ struct State { | |
background_validation_tx: mpsc::Sender<(Hash, ValidatedCandidateCommand)>, | ||
/// The handle to the keystore used for signing. | ||
keystore: KeystorePtr, | ||
/// Runtime info cached per-session. | ||
runtime_info: RuntimeInfo, | ||
} | ||
|
||
impl State { | ||
|
@@ -289,6 +293,7 @@ impl State { | |
per_candidate: HashMap::new(), | ||
background_validation_tx, | ||
keystore, | ||
runtime_info: RuntimeInfo::new(None), | ||
} | ||
} | ||
} | ||
|
@@ -400,8 +405,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()) | ||
} | ||
} | ||
|
||
|
@@ -943,7 +948,14 @@ async fn handle_active_leaves_update<Context>( | |
|
||
// construct a `PerRelayParent` from the runtime API | ||
// and insert it. | ||
let per = construct_per_relay_parent_state(ctx, maybe_new, &state.keystore, mode).await?; | ||
let per = construct_per_relay_parent_state( | ||
ctx, | ||
maybe_new, | ||
&state.keystore, | ||
&mut state.runtime_info, | ||
mode, | ||
) | ||
.await?; | ||
|
||
if let Some(per) = per { | ||
state.per_relay_parent.insert(maybe_new, per); | ||
|
@@ -959,6 +971,7 @@ async fn construct_per_relay_parent_state<Context>( | |
ctx: &mut Context, | ||
relay_parent: Hash, | ||
keystore: &KeystorePtr, | ||
runtime_info: &mut RuntimeInfo, | ||
mode: ProspectiveParachainsMode, | ||
) -> Result<Option<PerRelayParentState>, Error> { | ||
macro_rules! try_runtime_api { | ||
|
@@ -983,10 +996,18 @@ async fn construct_per_relay_parent_state<Context>( | |
|
||
let parent = relay_parent; | ||
|
||
let (validators, groups, session_index, cores) = futures::try_join!( | ||
let session_index = | ||
try_runtime_api!(runtime_info.get_session_index_for_child(ctx.sender(), parent).await); | ||
|
||
let minimum_backing_votes = | ||
request_min_backing_votes(parent, ctx.sender(), |parent, sender| { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this wrapper? Can't |
||
runtime_info.get_min_backing_votes(sender, session_index, parent) | ||
}) | ||
.await?; | ||
|
||
let (validators, groups, cores) = futures::try_join!( | ||
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) | ||
},) | ||
|
@@ -996,7 +1017,6 @@ async fn construct_per_relay_parent_state<Context>( | |
|
||
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 signing_context = SigningContext { parent_hash: parent, session_index }; | ||
|
@@ -1061,6 +1081,7 @@ async fn construct_per_relay_parent_state<Context>( | |
issued_statements: HashSet::new(), | ||
awaiting_validation: HashSet::new(), | ||
fallbacks: HashMap::new(), | ||
minimum_backing_votes, | ||
})) | ||
} | ||
|
||
|
@@ -1563,10 +1584,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. | ||
|
@@ -2009,7 +2033,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(); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,7 @@ const DEFAULT_CACHE_CAP: NonZeroUsize = match NonZeroUsize::new(128) { | |
pub(crate) struct RequestResultCache { | ||
authorities: LruCache<Hash, Vec<AuthorityDiscoveryId>>, | ||
validators: LruCache<Hash, Vec<ValidatorId>>, | ||
minimum_backing_votes: LruCache<Hash, u32>, | ||
validator_groups: LruCache<Hash, (Vec<Vec<ValidatorIndex>>, GroupRotationInfo)>, | ||
availability_cores: LruCache<Hash, Vec<CoreState>>, | ||
persisted_validation_data: | ||
|
@@ -78,6 +79,7 @@ impl Default for RequestResultCache { | |
Self { | ||
authorities: LruCache::new(DEFAULT_CACHE_CAP), | ||
validators: LruCache::new(DEFAULT_CACHE_CAP), | ||
minimum_backing_votes: LruCache::new(DEFAULT_CACHE_CAP), | ||
validator_groups: LruCache::new(DEFAULT_CACHE_CAP), | ||
availability_cores: LruCache::new(DEFAULT_CACHE_CAP), | ||
persisted_validation_data: LruCache::new(DEFAULT_CACHE_CAP), | ||
|
@@ -131,6 +133,18 @@ impl RequestResultCache { | |
self.validators.put(relay_parent, validators); | ||
} | ||
|
||
pub(crate) fn minimum_backing_votes(&mut self, relay_parent: &Hash) -> Option<u32> { | ||
self.minimum_backing_votes.get(relay_parent).copied() | ||
} | ||
|
||
pub(crate) fn cache_minimum_backing_votes( | ||
&mut self, | ||
relay_parent: Hash, | ||
minimum_backing_votes: u32, | ||
) { | ||
self.minimum_backing_votes.put(relay_parent, minimum_backing_votes); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. This should be cached per |
||
} | ||
|
||
pub(crate) fn validator_groups( | ||
&mut self, | ||
relay_parent: &Hash, | ||
|
@@ -472,6 +486,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, u32), | ||
ValidatorGroups(Hash, (Vec<Vec<ValidatorIndex>>, GroupRotationInfo)), | ||
AvailabilityCores(Hash, Vec<CoreState>), | ||
PersistedValidationData(Hash, ParaId, OccupiedCoreAssumption, Option<PersistedValidationData>), | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this wasn't introduced by you, but this macro completely defeats the point of our error handling. If we don't want to bring down the node, the correct way of handling this would be to make all runtime errors non fatal, instead of swallowing them in a macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only fatal error we have here is
canceled
which never happens, except the node is shutting down, so this is fine.