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

PVF: Re-check file integrity before voting against; document #2399

Open
mrcnski opened this issue Nov 19, 2023 · 6 comments
Open

PVF: Re-check file integrity before voting against; document #2399

mrcnski opened this issue Nov 19, 2023 · 6 comments
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. T0-node This PR/Issue is related to the topic “node”.

Comments

@mrcnski
Copy link
Contributor

mrcnski commented Nov 19, 2023

Before voting against a candidate, re-check the file integrity.

Document it: add an # Artifact integrity section to artifacts.rs. It could explain that artifacts must remain valid to satisfy the SAFETY constraints of execute_artifact. Therefore we do two things: check file integrity and version compatibility on host startup, and also before voting against a candidate we re-check the file integrity.

Originally posted by @mrcnski in #1918 (comment)

@mrcnski mrcnski added T0-node This PR/Issue is related to the topic “node”. C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. labels Nov 19, 2023
@Jpserrat
Copy link
Contributor

@mrcnski Is this issue about re-checking the file integrity and adding the # Artifact integrity?
Why the re-checking is needed? Do we not vote against it in case of failure?

@mrcnski
Copy link
Contributor Author

mrcnski commented Nov 26, 2023

@mrcnski Is this issue about re-checking the file integrity and adding the # Artifact integrity?

Yes, exactly! We still need to do both, re-checking was not implemented yet.

Why the re-checking is needed? Do we not vote against it in case of failure?

We had the idea to re-check the file hash of the artifact before execution, to make sure it wasn't corrupted. However, this would add a few ms to each execution. As an optimization, we can only re-check after execution, if the execution failed. Reason being, that corruption should be very very rare, and if it happens then the execution will fail, so we only need to check it there. Hope that makes sense. Thanks for asking, for sure it's not obvious so should be documented. 👍

@mrcnski
Copy link
Contributor Author

mrcnski commented Nov 26, 2023

If we re-check and it's corrupted, we should return a specific error that indicates that the host should purge this artifact from the artifacts cache.

@eagr
Copy link
Contributor

eagr commented Nov 28, 2023

I'm about to take a shot at the integrity re-check. Wondering if you have started on this? @Jpserrat

@Jpserrat
Copy link
Contributor

@eagr I haven't, you can take it

@eskimor
Copy link
Member

eskimor commented Nov 29, 2023

Do we not vote against it in case of failure?

We do and this is precisely the problem. If the file got corrupted on disk, we would currently vote invalid on the candidate and raise a dispute, despite the issue being our hard disk and not the candidate. We would get slashed and disabled for raising a dispute on a valid candidate. Obviously there should be a big fat warning in the logs if a corruption is detected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. T0-node This PR/Issue is related to the topic “node”.
Projects
Status: Backlog
Development

No branches or pull requests

4 participants