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

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
10b0b4a
override authorities runtime call
parity2305 Feb 22, 2021
21846f8
addressing feedback
ParthDesai Feb 22, 2021
25c20f0
addressing feedback and restoring semantics
parity2305 Feb 23, 2021
0f2db99
updated helper function
parity2305 Feb 23, 2021
db6dc2b
naming convention + comment
parity2305 Feb 23, 2021
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
21 changes: 20 additions & 1 deletion runtime/parachains/src/runtime_api_impl/v1.rs
View file
Open in desktop
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,24 @@ 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 current and historical authority ids using session_info module.
ParthDesai marked this conversation as resolved.
Show resolved Hide resolved
pub fn get_historical_authority_ids<T: initializer::Config>() -> Vec<AuthorityDiscoveryId> {
ordian marked this conversation as resolved.
Show resolved Hide resolved
ParthDesai marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

The API of this function has now changed and it can include duplicates.

This function should include the call to AuthorityDiscovery::next_authorities which can be achieved by adding an extra bound on T if necessary (although I believe it is implied by initializer::Config.

Copy link
Contributor Author

@ParthDesai ParthDesai Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@rphmeier I am slightly confused right now. Is session_info[current_session_index].discovery_keys is equivalent to AutorityDiscovery::next_authorities() in every case? (Based on your earlier comment above) In that case, I can directly do earliest_session..=current_session, instead of calling AuthorityDiscovery::next_authorities.

Copy link
Contributor

@rphmeier rphmeier Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No, that's not what I said at all. Please re-review my comment! session_info[current_session_index].discovery_keys is the same as AuthorityDiscovery::current_authorities. Why would it be otherwise? This is the only thing that makes sense.

Iterating up to current_session_index is fine but we also want to include the AuthorityDiscovery::next_authorities here.

but != because.

Copy link
Contributor Author

@ParthDesai ParthDesai Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@rphmeier Ok. Makes sense. I have updated code to reflect this. cc: @ordian

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 •
edited
Loading

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 = vec![];
Copy link
Member

Choose a reason for hiding this comment

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

yeah, we need to include authorities for the next session as well here
the rationale is outlined in paritytech/substrate#6788


for session_index in earliest_stored_session..current_session_index {
ParthDesai marked this conversation as resolved.
Show resolved Hide resolved
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
}

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

impl authority_discovery_primitives::AuthorityDiscoveryApi<Block> for Runtime {
fn authorities() -> Vec<AuthorityDiscoveryId> {
AuthorityDiscovery::authorities()
// Past session's authority ids
let mut past_authority_ids = runtime_api_impl::get_historical_authority_ids::<Runtime>();
// Current and next session's authority ids
let mut current_and_next_authority_ids = AuthorityDiscovery::authorities();
ParthDesai marked this conversation as resolved.
Show resolved Hide resolved

past_authority_ids.append(&mut current_and_next_authority_ids);
let mut authorities = past_authority_ids;

authorities.sort();
authorities.dedup();

authorities
}
}

Expand Down