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

Update Runtime to Support V5 Actors #1173

Merged
merged 16 commits into from
Jul 5, 2021
Merged

Update Runtime to Support V5 Actors #1173

merged 16 commits into from
Jul 5, 2021

Conversation

ec2
Copy link
Member

@ec2 ec2 commented Jun 18, 2021

Summary of changes
Changes introduced in this pull request:

  • Introduces 2 new methods to the Runtime Interface: verify_aggregate_seals and base_fee.
  • Update the chain and beacon randomness functions in the Runtime to look forward after Hyperdrive and backwards before.
  • Bump to v8 of proofs api to support aggregate seal verification.

TODO:

  • Update MockRuntime
  • Update networks crate to set Hyperdrive height (currently using placeholder

Reference issue to close (if applicable)

Closes

Other information and links

@cryptoquick
Copy link
Contributor

cryptoquick commented Jun 18, 2021

This still has todos and failing tests and linter. Is this RFR?

Copy link
Contributor

@creativcoder creativcoder left a comment

Choose a reason for hiding this comment

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

LGTM, left a few comments

@detailyang
Copy link
Contributor

@ec2 It looks good in my localhost. I merge this branch to my dev branch and now It can sync the whole latest actorv5 interopnet network:)

let step = self
.verify_aggregate_seal_steps
.get(&proof_type)
.expect("There is an implementation error where proof type does not exist in table");
Copy link

Choose a reason for hiding this comment

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

Same suggestion here as before. A HashMap is lacking the knowledge that there is a price for each variant of proof type.

&seeds,
inp,
)? {
Err("Invalid Aggregate Seal proof".into())
Copy link

Choose a reason for hiding this comment

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

Upsetting that this doesn't use a custom error type. Would it be worth it to create a ticket to define the error types here? Do you anticipate any complications will come up?

Copy link

Choose a reason for hiding this comment

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

This trait is fairly small.

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont see why its useful or important here. These methods are generally used in actors. And the way the error is handled is irrespective of what error happens here anyways. I think a refactor of error handling in this might mess up with error propagation in actors.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. I left a few questions and suggestions that I'd like to talk through before approving.

@ec2 ec2 requested a review from a user July 5, 2021 13:12
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks Eric

@ec2 ec2 merged commit d1d6f64 into main Jul 5, 2021
@ec2 ec2 deleted the ec2/v5-runtime branch July 5, 2021 14:33
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.

4 participants