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.

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

remove transitional code wrt executor parameters #2112

Merged

Conversation

jpserrat
Copy link
Contributor

@jpserrat jpserrat commented Nov 1, 2023

I've replaced the code with the comment suggestion, @s0me0ne-unkn0wn.

Closes #714

@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 1, 2023 11:29
@mrcnski mrcnski requested review from mrcnski and s0me0ne-unkn0wn and removed request for a team November 1, 2023 11:58
@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 1, 2023 11:59
@s0me0ne-unkn0wn
Copy link
Contributor

Well done, it's definitely a legit change, as the executor params have been on Polkadot for a long time already... I'm just asking myself if, for the sake of paranoia, it needs a burn-in prior to merging. I'll ask fellows if we have the capacity for that.

@mrcnski mrcnski added the T0-node This PR/Issue is related to the topic “node”. label Nov 1, 2023
@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 1, 2023 13:05
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

I don't see any reason for a burn in. In general I have to say that the runtime api returns an Option was not a very good idea here. As we actually expect that this always returns something...

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Actually this implementation is broken for chains that start from genesis. We should keep the current code. We should just remove the comment. As I said above, this runtime api function should never have returned an option..

@s0me0ne-unkn0wn
Copy link
Contributor

Actually this implementation is broken for chains that start from genesis.

Why? TBH, I didn't test that, but currently, we should bootstrap session zero executor parameters from HostConfiguration::executor_params. I believe the host configuration should be there either way.

@s0me0ne-unkn0wn
Copy link
Contributor

As I said above, this runtime api function should never have returned an option.

The executor parameters are side-data for SessionInfo, and session_info() returns an Option. What would you want it returned if requested for a non-existing session index?

@bkchr
Copy link
Member

bkchr commented Nov 1, 2023

Why? TBH, I didn't test that, but currently, we should bootstrap session zero executor parameters from HostConfiguration::executor_params. I believe the host configuration should be there either way.

There is no code that does this. At least I couldn't find it?

@s0me0ne-unkn0wn
Copy link
Contributor

There is no code that does this. At least I couldn't find it?

This one:

SessionExecutorParams::<T>::insert(&new_session_index, config.executor_params);

initializer_on_new_session() is run for the session zero, right?

@bkchr
Copy link
Member

bkchr commented Nov 1, 2023

initializer_on_new_session() is run for the session zero, right?

Ahh yeah, that is actually executed on genesis. Then sorry!

@bkchr
Copy link
Member

bkchr commented Nov 1, 2023

The executor parameters are side-data for SessionInfo, and session_info() returns an Option. What would you want it returned if requested for a non-existing session index?

Maybe a Result, but that wouldn't change that much probably. I mean we could then have at least covered the case where the session was already pruned. (but you could only have guessed what the parameters were back then)

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Approving based on the discussion above.

@s0me0ne-unkn0wn s0me0ne-unkn0wn enabled auto-merge (squash) November 1, 2023 15:30
@s0me0ne-unkn0wn s0me0ne-unkn0wn merged commit 9987bbb into paritytech:master Nov 1, 2023
87 of 106 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

Remove transitional code wrt executor parameters
5 participants