Skip to content

Conversation

s0me0ne-unkn0wn
Copy link
Contributor

This aims to fix #4292
We have three cases of candidate validation where a proper set of executor environment parameters should be used:

  1. Backing, and we're currently doing the right thing, requesting the executor params at the relay parent at which the candidate was produced:
    let (session_index, groups, claim_queue, disabled_validators) = futures::try_join!(
    request_session_index_for_child(parent, ctx.sender()).await,
    request_validator_groups(parent, ctx.sender()).await,
    request_claim_queue(parent, ctx.sender()).await,
    request_disabled_validators(parent, ctx.sender()).await,
    )
    .map_err(Error::JoinMultiple)?;
  2. Approval voting, where a wrong session was used, and this PR fixes that;
  3. Disputes, where the session index, again, is hopefully derived from the relay parent at which the candidate was produced:
    /// The session the candidate appears in.
    session: SessionIndex,

So, hopefully, this PR fixes the only wrong case and harmonizes the executor param fetching over all the existing use cases.

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/17831830650
Failed job name: fmt

let ExtendedSessionInfo { ref session_info, ref executor_params, .. } =
match get_extended_session_info(
let (no_show_slots, needed_approvals) =
match get_session_info_by_index(
Copy link
Contributor

Choose a reason for hiding this comment

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

If a candidate is built on the last block of the previous session, the index you use here is different than the index you get on the second call below.

That is because block_entry.session is actually the session of the relay parent and not it's child.

@sandreim
Copy link
Contributor

  1. Disputes, where the session index, again, is hopefully derived from the relay parent at which the candidate was produced:
    /// The session the candidate appears in.
    session: SessionIndex,

So, hopefully, this PR fixes the only wrong case and harmonizes the executor param fetching over all the existing use cases.

I think this needs to be the session index where it was backed and not the relay parent. @eskimor ?

@s0me0ne-unkn0wn
Copy link
Contributor Author

@sandreim I think indeed that it doesn't matter at all (correct me if I'm wrong). We should be using exactly the same set of parameters for a candidate in all the situations, but it doesn't really matter which set, as long as it's the same. That's what I'm trying to achieve here — always use the relay parent's session.

@sandreim
Copy link
Contributor

@sandreim I think indeed that it doesn't matter at all (correct me if I'm wrong). We should be using exactly the same set of parameters for a candidate in all the situations, but it doesn't really matter which set, as long as it's the same. That's what I'm trying to achieve here — always use the relay parent's session.

Yes, you achieve what you said. But the session you use is the session of the child of the relay parent, which is fine, because this actually guarantees that the backed canidate always lands in same session. So you also end up using the same session index in disputes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Candidate should always be executed with executor parameters from the session it was produced in
4 participants