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.

Sign up for GitHub

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

1190: better interface for launch-approval #1355

Merged
merged 1 commit into from
Sep 1, 2023
Merged

1190: better interface for launch-approval #1355

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
66 changes: 36 additions & 30 deletions polkadot/node/core/approval-voting/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use polkadot_node_subsystem_util::{
self,
database::Database,
metrics::{self, prometheus},
runtime::{Config as RuntimeInfoConfig, RuntimeInfo},
runtime::{Config as RuntimeInfoConfig, ExtendedSessionInfo, RuntimeInfo},
TimeoutExt,
};
use polkadot_primitives::{
Expand Down Expand Up @@ -643,32 +643,46 @@ impl CurrentlyCheckingSet {
}
}

async fn get_session_info<'a, Sender>(
async fn get_extended_session_info<'a, Sender>(
runtime_info: &'a mut RuntimeInfo,
sender: &mut Sender,
relay_parent: Hash,
session_index: SessionIndex,
) -> Option<&'a SessionInfo>
) -> Option<&'a ExtendedSessionInfo>
where
Sender: SubsystemSender<RuntimeApiMessage>,
{
match runtime_info
.get_session_info_by_index(sender, relay_parent, session_index)
.await
{
Ok(extended_info) => Some(&extended_info.session_info),
Ok(extended_info) => Some(&extended_info),
Err(_) => {
gum::debug!(
target: LOG_TARGET,
session = session_index,
?relay_parent,
"Can't obtain SessionInfo"
Copy link
Member

Choose a reason for hiding this comment

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

This is really not how the usage was intended, but that was the case before already.

"Can't obtain SessionInfo or ExecutorParams"
);
None
},
}
}

async fn get_session_info<'a, Sender>(
runtime_info: &'a mut RuntimeInfo,
sender: &mut Sender,
relay_parent: Hash,
session_index: SessionIndex,
) -> Option<&'a SessionInfo>
where
Sender: SubsystemSender<RuntimeApiMessage>,
{
get_extended_session_info(runtime_info, sender, relay_parent, session_index)
.await
.map(|extended_info| &extended_info.session_info)
}

struct State {
keystore: Arc<LocalKeystore>,
slot_duration_millis: u64,
Expand Down Expand Up @@ -746,6 +760,7 @@ enum Action {
relay_block_hash: Hash,
candidate_index: CandidateIndex,
session: SessionIndex,
executor_params: ExecutorParams,
candidate: CandidateReceipt,
backing_group: GroupIndex,
},
Expand Down Expand Up @@ -968,6 +983,7 @@ async fn handle_actions<Context>(
relay_block_hash,
candidate_index,
session,
executor_params,
candidate,
backing_group,
} => {
Expand Down Expand Up @@ -1008,14 +1024,7 @@ async fn handle_actions<Context>(
},
None => {
let ctx = &mut *ctx;
let executor_params = match session_info_provider
.get_session_info_by_index(ctx.sender(), block_hash, session)
.await
{
Err(_) => None,
Ok(extended_session_info) =>
Some(extended_session_info.executor_params.clone()),
};

currently_checking_set
.insert_relay_block_hash(
candidate_hash,
Expand Down Expand Up @@ -2337,17 +2346,18 @@ async fn process_wakeup<Context>(
_ => return Ok(Vec::new()),
};

let session_info = match get_session_info(
session_info_provider,
ctx.sender(),
block_entry.parent_hash(),
block_entry.session(),
)
.await
{
Some(i) => i,
None => return Ok(Vec::new()),
};
let ExtendedSessionInfo { ref session_info, ref executor_params, .. } =
match get_extended_session_info(
session_info_provider,
ctx.sender(),
block_entry.parent_hash(),
Copy link
Member Author

Choose a reason for hiding this comment

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

the only difference is that we now fetch executor params using parent_hash of relay_block instead of relay_block, but they should be in the same session, right? Otherwise, why would be passing block_entry.session() and not session of parent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, why do we use parent_hash here? Shouldn't it be relay_block as in other places

polkadot-sdk/polkadot/node/core/approval-voting/src/import.rs

Line 214 in ccdf636

let session_info = get_session_info(env.runtime_info, ctx.sender(), block_hash, session_index)

Copy link
Member

Choose a reason for hiding this comment

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

They don't even have to be in the same session. The more current head is technically better, for reasons Rob pointed out but also because even if those two would at some point (with impl changes) end up being in different sessions, the newer one would still have access to the session of the other, while the opposite might not be true.

Copy link
Contributor

Choose a reason for hiding this comment

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

They don't even have to be in the same session. The more current head is technically better, for reasons Rob pointed out but also because even if those two would at some point (with impl changes) end up being in different sessions, the newer one would still have access to the session of the other, while the opposite might not be true.

yes, this sounds right to me.

block_entry.session(),
)
.await
{
Some(i) => i,
None => return Ok(Vec::new()),
};

let block_tick = slot_number_to_tick(state.slot_duration_millis, block_entry.slot());
let no_show_duration = slot_number_to_tick(
Expand Down Expand Up @@ -2434,6 +2444,7 @@ async fn process_wakeup<Context>(
relay_block_hash: relay_block,
candidate_index: i as _,
session: block_entry.session(),
executor_params: executor_params.clone(),
candidate: candidate_receipt,
backing_group,
});
Expand Down Expand Up @@ -2475,7 +2486,7 @@ async fn launch_approval<Context>(
validator_index: ValidatorIndex,
block_hash: Hash,
backing_group: GroupIndex,
executor_params: Option<ExecutorParams>,
executor_params: ExecutorParams,
span: &jaeger::Span,
) -> SubsystemResult<RemoteHandle<ApprovalState>> {
let (a_tx, a_rx) = oneshot::channel();
Expand Down Expand Up @@ -2546,11 +2557,6 @@ async fn launch_approval<Context>(
// Force the move of the timer into the background task.
let _timer = timer;

let executor_params = match executor_params {
Some(ep) => ep,
None => return ApprovalState::failed(validator_index, candidate_hash),
};

let available_data = match a_rx.await {
Err(_) => return ApprovalState::failed(validator_index, candidate_hash),
Ok(Ok(a)) => a,
Expand Down