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

Override RuntimeApi::authorities() to include past sessions #2494

Merged
merged 5 commits into from
Feb 23, 2021
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
24 changes: 23 additions & 1 deletion runtime/parachains/src/runtime_api_impl/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@ use primitives::v1::{
Id as ParaId, OccupiedCoreAssumption, SessionIndex, ValidationCode,
CommittedCandidateReceipt, ScheduledCore, OccupiedCore, CoreOccupied, CoreIndex,
GroupIndex, CandidateEvent, PersistedValidationData, SessionInfo,
InboundDownwardMessage, InboundHrmpMessage, Hash,
InboundDownwardMessage, InboundHrmpMessage, Hash, AuthorityDiscoveryId
};
use frame_support::debug;
use crate::{initializer, inclusion, scheduler, configuration, paras, session_info, dmp, hrmp, shared};


/// Implementation for the `validators` function of the runtime API.
pub fn validators<T: initializer::Config>() -> Vec<ValidatorId> {
<inclusion::Module<T>>::validators()
Expand Down Expand Up @@ -231,6 +232,27 @@ pub fn session_index_for_child<T: initializer::Config>() -> SessionIndex {
<shared::Module<T>>::session_index()
}

/// Implementation for the `AuthorityDiscoveryApi::authorities()` function of the runtime API.
/// It is a heavy call, but currently only used for authority discovery, so it is fine.
/// Gets next, current and some historical authority ids using session_info module.
pub fn relevant_authority_ids<T: initializer::Config + pallet_authority_discovery::Config>() -> Vec<AuthorityDiscoveryId> {
let current_session_index = session_index_for_child::<T>();
let earliest_stored_session = <session_info::Module<T>>::earliest_stored_session();
Comment on lines +239 to +240
Copy link
Member

Choose a reason for hiding this comment

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

I imagine this might be racy and error-prone at session boundaries, we can maybe iterate directly over the Sessions storage, but that's probably racy as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ordian At session boundary, If there is a race condition between getting session indexes above and loop execution, we might miss latest session's authority ids, which can break the semantics. So, to cover widest possible range here we can query session info from earliest..=current+1.

Copy link
Contributor

@rphmeier rphmeier Feb 22, 2021

Choose a reason for hiding this comment

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

This should be fine at session boundaries as session changes are applied at the end of the parent block. The code comment in fn session_index_for_child is outdated and wrong and that threw me for a loop while investigating this comment.

Iterating up to current_session_index is fine but we also want to include the AuthorityDiscovery::next_authorities here. Although AuthorityDiscovery updates its current and next authorities at the beginning of the parent block, we are past the end of the parent block here and current should have made their way into SessionInfo.

let mut authority_ids = <pallet_authority_discovery::Module<T>>::next_authorities();

for session_index in earliest_stored_session..=current_session_index {
let info = <session_info::Module<T>>::session_info(session_index);
if let Some(mut info) = info {
authority_ids.append(&mut info.discovery_keys);
}
}

authority_ids.sort();
authority_ids.dedup();

authority_ids
}

/// Implementation for the `validation_code` function of the runtime API.
pub fn validation_code<T: initializer::Config>(
para_id: ParaId,
Expand Down
2 changes: 1 addition & 1 deletion runtime/rococo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -849,7 +849,7 @@ sp_api::impl_runtime_apis! {

impl authority_discovery_primitives::AuthorityDiscoveryApi<Block> for Runtime {
fn authorities() -> Vec<AuthorityDiscoveryId> {
AuthorityDiscovery::authorities()
runtime_api_impl::relevant_authority_ids::<Runtime>()
}
}

Expand Down