Override RuntimeApi::authorities() to include past sessions #2494
Conversation
| let current_session_index = session_index_for_child::<T>(); | ||
| let earliest_stored_session = <session_info::Module<T>>::earliest_stored_session(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
ordian
added
A0-please_review
| pub fn get_current_and_historical_authority_ids<T: initializer::Config>() -> Vec<AuthorityDiscoveryId> { | ||
| let current_session_index = session_index_for_child::<T>(); | ||
| let earliest_stored_session = <session_info::Module<T>>::earliest_stored_session(); | ||
| let mut authority_ids = vec![]; |
There was a problem hiding this comment.
yeah, we need to include authorities for the next session as well here
the rationale is outlined in paritytech/substrate#6788
|
@ordian Ok. I modified the code based on comments above. Now, I am only getting historical authority ids from Reason behind this is, |
| /// Gets current and historical authority ids using session_info module. | ||
| pub fn get_current_and_historical_authority_ids<T: initializer::Config>() -> Vec<AuthorityDiscoveryId> { | ||
| pub fn get_historical_authority_ids<T: initializer::Config>() -> Vec<AuthorityDiscoveryId> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| let mut next_authorities = <pallet_authority_discovery::Module<T>>::next_authorities(); |
There was a problem hiding this comment.
session_info module defines AuthorityDiscoveryConfig trait to help with mock testing,
we can add next_authorities method to it, this would avoid having the pallet_authority_discovery::Config bound here
and call it
<T as session_info::AuthorityDiscoveryConfig>::next_authorities()also this can go into initialization, i.e. instead of
let mut authority_ids = vec![];There was a problem hiding this comment.
@ordian IMHO I think it would be more elegant to have explicit trait bound rather than use the trait impl that is primarily used for testing. Also it will create shared dependency between mock testing and actual code.
There was a problem hiding this comment.
I don't agree it's primarily used for tests, rather it is to allow testing as implementing pallet_authority_discovery::Config manually is much harder. But it's also not that important.
Overrides RuntimeApi::Authorities() to include past sessions as well with the help of session_info module.
Fixes p.2 of #2460
cc: @ordian