Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Accept PVF code hashes in validation host #3655

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open

Accept PVF code hashes in validation host #3655

wants to merge 35 commits into from

Conversation

slumber
Copy link
Contributor

@slumber slumber commented Aug 17, 2021 •
edited
Loading

Resolves paritytech/polkadot-sdk#973

  • Update Implementer's guide

@slumber slumber requested a review from pepyakin August 17, 2021 20:51
@pepyakin pepyakin added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Aug 18, 2021
pepyakin
pepyakin suggested changes Aug 18, 2021
View reviewed changes
Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

My main concern is the candidate-validation changes. I think the code can be improved there by leveraging the fact that the candidate descriptor carries the code hash which we can query.

node/primitives/src/lib.rs Outdated Show resolved Hide resolved
node/core/pvf/src/host.rs Outdated Show resolved Hide resolved
node/core/candidate-validation/src/lib.rs Show resolved Hide resolved
node/core/candidate-validation/src/lib.rs Outdated Show resolved Hide resolved
@pepyakin pepyakin self-requested a review August 19, 2021 13:05
pepyakin
pepyakin suggested changes Aug 19, 2021
View reviewed changes
Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

candidate-validation looks way better now! However, there is another point which made me sad: "Code not found" thingy.

node/core/pvf/src/host.rs Show resolved Hide resolved
node/core/candidate-validation/src/lib.rs Outdated Show resolved Hide resolved
node/core/candidate-validation/src/lib.rs Show resolved Hide resolved
node/core/candidate-validation/src/lib.rs Show resolved Hide resolved
node/core/candidate-validation/src/lib.rs Outdated Show resolved Hide resolved
@slumber slumber force-pushed the slumber-use-cached-pvf branch from b099976 to 094d164 Compare August 19, 2021 14:46
pepyakin
pepyakin suggested changes Aug 20, 2021
View reviewed changes
node/core/candidate-validation/src/lib.rs Outdated Show resolved Hide resolved
node/core/pvf/src/host.rs Outdated Show resolved Hide resolved
node/core/candidate-validation/src/lib.rs Outdated Show resolved Hide resolved
node/core/candidate-validation/src/lib.rs Outdated Show resolved Hide resolved
node/core/candidate-validation/src/lib.rs Outdated Show resolved Hide resolved
@pepyakin pepyakin self-requested a review August 23, 2021 14:56
This was referenced Nov 15, 2023
Open
Closed
@pepyakin pepyakin added A1-onice and removed A0-please_review Pull request needs code review. labels Aug 27, 2021
@pepyakin
Copy link
Contributor

On ice until #3728 is landed.

pepyakin
pepyakin reviewed Aug 27, 2021
View reviewed changes
Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

drive-by comment

node/core/candidate-validation/src/lib.rs Outdated Show resolved Hide resolved
@slumber slumber mentioned this pull request Aug 27, 2021
Merged
slumber
slumber commented Dec 17, 2021
View reviewed changes
node/core/pvf/src/host.rs Outdated Show resolved Hide resolved
@slumber slumber force-pushed the slumber-use-cached-pvf branch from 65dfd4a to fb7e4e7 Compare January 19, 2022 13:57
@pepyakin pepyakin self-requested a review January 19, 2022 21:15
pepyakin
pepyakin reviewed Feb 9, 2022
View reviewed changes
node/primitives/src/lib.rs Outdated Show resolved Hide resolved
node/core/pvf/src/pvf.rs Outdated Show resolved Hide resolved
node/core/candidate-validation/src/lib.rs Outdated Show resolved Hide resolved
@slumber slumber requested a review from pepyakin February 14, 2022 17:34
@slumber slumber force-pushed the slumber-use-cached-pvf branch from afe374f to bccbbc3 Compare April 1, 2022 20:20
@mrcnski
Merge branch 'master' into slumber-use-cached-pvf
3668cfa
@mrcnski
Fix leftover errors from merge
32680ca 
Putting these changes in their own commit, to make it clear that there were
extra changes on top of the merge.
@slumber
Copy link
Contributor Author

slumber commented Oct 11, 2022

@pepyakin note that given async backing changes (in particular #5557) ValidateFromChainState is not used anymore, backing requires explicitly providing persisted validation data (which makes sense).

IMO this PR should be closed as it introduces rather unnecessary changes for now. The caching can be implemented later but would require some additional work.

@mrcnski
Copy link
Contributor

mrcnski commented Oct 11, 2022

I guess I should have asked around more before tackling this!

If this PR is closed then should this issue be closed as well? I believe it depends on this PR.

@slumber
Copy link
Contributor Author

slumber commented Oct 11, 2022

I guess I should have asked around more before tackling this!

If this PR is closed then should this issue be closed as well? I believe it depends on this PR.

No worries, there was no decision on closing it (yet), that's just my comment to give more context on whether this issue is still relevant. I started a discussion on how to proceed with it.
Another obvious option would be to rebase it straight to the async backing feature branch.

@pepyakin
Copy link
Contributor

@pepyakin note that given async backing changes (in particular #5557) ValidateFromChainState is not used anymore, backing requires explicitly providing persisted validation data (which makes sense).

Explicitly providing PVD? But this one is about passing PVF hashes instead of providing the preimages, right?

But yeah in general I am not surprised of this outcome, candidate-validation was asking for rework anyway.

@slumber
Copy link
Contributor Author

slumber commented Oct 12, 2022

Explicitly providing PVD? But this one is about passing PVF hashes instead of providing the preimages, right?

Yes, for this reason a PR is still relevant but requires additional work.

For example, ValidateFrom... message types could be unified into one and we would always attempt to look up pvf in the cache first.

@the-right-joyce the-right-joyce added S5-on_ice Work is currently on hold, see comments for further information. and removed A1-onice labels Dec 14, 2022
@slumber slumber mentioned this pull request Jan 6, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eskimor eskimor eskimor left review comments

@burdges burdges burdges left review comments

@drahnr drahnr drahnr approved these changes

@pepyakin pepyakin Awaiting requested review from pepyakin

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. S5-on_ice Work is currently on hold, see comments for further information.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PVF validation host: pull not push PVFs
7 participants
@slumber @pepyakin @mrcnski @drahnr @burdges @eskimor @the-right-joyce