Add PVF module documentation

[mrcnski]

PULL REQUEST

Overview

So I found out that there was already module-level documentation for pvf. Somehow I totally missed it this whole time -- I need to get in the habit of running cargo doc more often.

I decided to expand on the existing docs instead of adding a new section to the implementer's guide, as was my original idea in the linked issue. I figured the IG might not be a great fit, as pvf is not its own subsystem.

Changes

• I updated the existing docs a bit.
• I added a couple new sections for recently-added features.

Followups (once the PRs land)

• Document executor parametrization.
• Document CPU time measurement of timeouts.

Issues closed

Closes #6263.

[acatangiu]

lgtm

[slumber]

IMO these changes belong to implementers guide, a crate documentation is

1. intended for developers
2. should only describe what this crate does

Otherwise, maintaining could become time-consuming

[pepyakin]

Somehow I totally missed it this whole time -- I need to get in the habit of running cargo doc more often.

Perhaps that's a signal that corroborates @slumber 's concern and maybe that piece of info will be better off in the book?

[mrcnski]

@slumber @pepyakin I'm not sure where in the IG this doc should go -- under "Utility Subsystems", even though it's not a subsystem? Should the whole doc go there, leaving just a few explanatory sentences (as in e.g. pvf-checker/src/lib.rs)?

[slumber]

@slumber @pepyakin I'm not sure where in the IG this doc should go -- under "Utility Subsystems", even though it's not a subsystem? Should the whole doc go there, leaving just a few explanatory sentences (as in e.g. pvf-checker/src/lib.rs)?

Subtask-section in the candidate validation could be a good fit

[slumber]

My point was that changes belong to the book, not the whole crate documentation.

It would've been enough to mention that it's not a subsystem as it's not communicated with via overseer, but directly through a channel. And a high level description of messages one can send to it. Dispute mitigation is closely tied to how candidate validation interacts with the host.

The documentation you added is decent on its own, but it needs a good structure.
@pepyakin can you please take a look a suggest one

[pepyakin]

@slumber @pepyakin I'm not sure where in the IG this doc should go -- under "Utility Subsystems", even though it's not a subsystem? Should the whole doc go there, leaving just a few explanatory sentences (as in e.g. pvf-checker/src/lib.rs)?

As @slumber hints, this should probably be in the candidate-validation subsystem. Once upon a time I wanted to actually make it a part of candidate-validation (although not sure about merging the crates) and still think it might make sense.

My #6293 (review) was that changes belong to the book, not the whole crate documentation.

I see now what you meant. Now the whole thing actually looks like a copy of the module doc into the book. Although, I also don't think this is the end of the world. In principle, I am ok either way. We can make it less specific down the line. I also agree with @slumber 's suggestions.

In either case, though, you should probably cross-link: add links from the code to the book and vice versa.

The documentation you added is decent on its own, but it needs a good structure.
@pepyakin can you please take a look a suggest one

What do you have in mind? I think this + the possible outline #6263 looks fine to me.

[slumber]

I also don't think this is the end of the world. In principle, I am ok either way. We can make it less specific down the line.

I don't have strong objections either, thus asked :)

My suggestions are:

1. Keep the crate doc as is, apart from maybe a small note that previously-failed execution requests are acceptable after a small timeout if the error was transient
2. Add a "PVF host" subsection to the candidate-validation part of the book with a brief description of what it is and how we mitigate disputes

[mrcnski]

I'm confused as to what should be in crate docs vs. the implementer's guide. Are
there any guidelines?

IMO these changes belong to implementers guide, a crate documentation is

1. intended for developers

2. should only describe what this crate does

Otherwise, maintaining could become time-consuming

But to me it seems like all of this applies to the IG, too. Or am I
misunderstanding the purpose of the IG?

I can make the changes described by @slumber above, I just want to understand
what I'm doing first.

In either case, though, you should probably cross-link: add links from the
code to the book and vice versa.

Makes sense, will do that when I make the changes.

Add a "PVF host" subsection to the candidate-validation part of the book

@slumber What do you mean by subsection? Above I understood it should be
a separate page under candidate-validation, but here it sounds like you want it
as a section on the candidate-validation page itself?

[eskimor]

Did we really go with 1 second? Andronik suggested 3 seconds back then.

[mrcnski]

Oh oops, I saw that this was resolved so I didn't make the change. I should have read more closely, because it sounds like we do want to go with 3 seconds. I can raise a separate PR, LMK.

[eskimor]

Indeed also to implementer's guide targets developers. To me the implementer's guide should cover the reasoning behind the code. High level design considerations, even why certain other "obvious" solutions have not been picked. What requirements have been considered, .... Basically anything that does not quite fit into code documentation.

The line is blurry, one thing I am particularly not a fan of is duplication. So once we are at the level of actual structs, traits, functions and types, I would cover the documentation in the source code and not in the guide. Reasons are simple:

1. If you are at that code level ... the documentation just should be there, right at the code.
2. DRY. I really don't see the point of copying stuff around, it is a maintenance burden without any clear benefit.

The implementer's guide should not bother at what types you are using exactly, it should much rather explain that the operating system can kill a PVF execution process¹ for various reasons ... list the reasons and explain what counter measures were taken, how they work and what guarantees they provide. Those are general considerations that would apply to multiple different implementations of the same protocol, regardless of concrete implementation.

Ideally by reading the guide you will know what high level properties the code should provide and why, e.g. being resistant to this and that attack and then can chose a different implementation, making sure you are still providing those properties.

Footnotes

1. Why are those standalone processes? ... That's a question that should be answered in the guide. ↩

[slumber]

But to me it seems like all of this applies to the IG, too. Or am I
misunderstanding the purpose of the IG?

I was referring mostly to describing candidate validation in pvf host. This however is fine
to do in IG if you put their docs next to each other.

IG is also intended for developers, but as mentioned by others, it should hint on why, not on low-level how.

What do you mean by subsection? Above I understood it should be
a separate page under candidate-validation, but here it sounds like you want it
as a section on the candidate-validation page itself?

You can do both, I like the latter more since these are closely tied.

[mrcnski]

DRY. I really don't see the point of copying stuff around, it is a maintenance burden without any clear benefit.

I agree, though the IG already has a whole chapter about Data Structures and Types. I do think that this amount of detail is too low-level, hard to maintain, and of questionable usefulness in that context.

[mrcnski]

Pushed the changes described here, except for this:

Keep the crate doc as is, apart from maybe a small note that previously-failed execution requests are acceptable after a small timeout if the error was transient

I thought this was better-suited in the IG with the dispute mitigations.

[slumber]

You can go ahead and drop it then