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

feat: [UTP-242] [v2] Make subnet replica version of latest block available to execution environment #2201

Closed
wants to merge 4 commits into from

Conversation

michael-weigelt
Copy link
Contributor

@michael-weigelt michael-weigelt commented Oct 23, 2024

Alternative to #2082

This PR makes the replica_version part of RegistryExecutionSettings so that the execution environment has access to it when processing messages.

This PR will have a follow-up to make replica_version available via a management canister call as specified here.

@github-actions github-actions bot added the feat label Oct 23, 2024
Copy link

@crusso crusso left a comment

Choose a reason for hiding this comment

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

Approving superficial changes to drun (do you agree @luc-blaeser?)

Comment on lines +1188 to +1189
/// This field is not from the registry, but from the most recent block.
pub replica_version: ReplicaVersion,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing to me. Why are we putting it in the RegistryExecutionSettings if it doesn't come from the registry? Maybe instead it would make more sense as a new argument to execute_round? Or another option would be to combine the other arguments to execute_round that have info on the specific block into a single struct argument.

Then I think you also wouldn't need to pass the ReplicaVersion through all the message routing logic to read the registry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see now where this is coming from after looking at the old PR. I'd say do either:

  1. Add it as a new argument to execute_round and execute_subnet_message.
  2. Combine it with idkg_subnet_public_keys into a struct which is passed to both execute_round and execute_subnet_message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I share your concerns. The previous PR writes batch.replica_version it into the replicated state (metadata) and uses it from there, but it was questioned whether that is necessary. I believe RegistryExecutionSettings was suggested because it's already being passed along.

But I can instead change the signatures of all methods along the way and introduce a new, conceptually clean argument for this data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is another draft, PTAL:
#2248

@michael-weigelt michael-weigelt marked this pull request as draft October 24, 2024 12:53
@michael-weigelt michael-weigelt changed the title feat: [UTP-242] Make subnet replica version of latest block available to execution environment feat: [UTP-242] [v2] Make subnet replica version of latest block available to execution environment Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants