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

docs: Document how CALL and SYSCALL work, and fix block stack table #1536

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Oct 16, 2024

Closes #483

@plafer plafer requested a review from bobbinth October 16, 2024 20:17
@plafer plafer added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Oct 16, 2024
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you! I left one comment inline. If this turns out to be a problem, we can probably create an issue for this and solve it in a different PR.

docs/src/design/decoder/main.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Al-Kindi-0 Al-Kindi-0 left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!
Left a few comments and questions to clarify my understanding.

docs/src/design/decoder/constraints.md Outdated Show resolved Hide resolved
docs/src/design/decoder/constraints.md Outdated Show resolved Hide resolved
docs/src/design/decoder/constraints.md Outdated Show resolved Hide resolved
* The following 8 columns are only set to non-zero values for `CALL` and `SYSCALL` operations, and save all the necessary information to be able to restore the parent context properly upon the corresponding `END` operation
- the `prnt_b0` and `prnt_b1` columns refer to the stack helper columns B0 and B1 (current stack depth and last overflow address, respectively)

In the above diagram, the first 2 rows were set from 2 different `CALL` operations. The first `CALL` operation is called from the root context, and hence its parent fn hash is the zero hash. We can infer that the first `CALL` called a procedure with hash `[h0, h1, h2, h3]` from the fact that the second `CALL` operation hash `[h0, h1, h2, h3]` as its parent fn hash.
Copy link
Collaborator

Choose a reason for hiding this comment

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

the last sentence is probably missing a verb? otherwise it is difficult to parse, to me at least

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 rewrote the sentence, let me know if that one is better

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is great

Comment on lines 424 to 426
#### CALL operation

Recall that the purpose of a `CALL` operation is to execute a procedure in a new execution context. Specifically, this means that the entire memory is zero'd, and the stack is truncated to a depth of 16 (i.e. any element in the stack overflow table are not avaiable in the new context). On the corresponding `END` instruction, the prover will restore the previous execution context (verified by the block stack table).
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: is "..the entire memory is zero'd" precise?

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: would it be better to say "is not available" instead of "are not available"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

question: is "..the entire memory is zero'd" precise?

I think so - in the new execution context, the entire memory is all 0s. How would you phrase it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Exactly, in the new execution context, thank you! Should we add this to make it clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup - done

docs/src/design/decoder/main.md Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left 2 small comments inline.

@@ -348,14 +354,19 @@ When the VM executes a `DYN` operation, it does the following:
Before an `END` operation is executed by the VM, the prover populates $h_0, ..., h_3$ registers with the hash of the block which is about to end. The prover also sets values in $h_4$ and $h_5$ registers as follows:
* $h_4$ is set to $1$ if the block is a body of a *loop* block. We denote this value as `f0`.
* $h_5$ is set to $1$ if the block is a *loop* block. We denote this value as `f1`.
* $h_6$ is set to $1$ if the block is a *call* block. We denote this value as `f2`.
* $h_7$ is set to $1$ if the block is a *syscall* block. We denote this value as `f3`.

![decoder_end_operation](../../assets/design/decoder/decoder_end_operation.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this picture needs to be updated to include f2 and f3 flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem though is that we don't include the excalidraw files in the repo, so to make I change to the picture, I have to redo it all. Do you have that one still somewhere?

Also I will include the excalidraw files for the new diagrams I just created

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! Not sure what's the best solution here - but here is my old excalidraw file for the decoder section (had to zip it because github does not allow uploading this file format).

decoder.excalidraw.zip

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the excalidraw files, and updated the END operation image with f2 and f3

(v_{join} + v_{split} + v_{loop} + v_{span} + v_{respan} + v_{dyn} + 1 - \\
(f_{join} + f_{split} + f_{loop} + f_{span} + f_{respan} + f_{dyn}))
(v_{join} + v_{split} + v_{loop} + v_{span} + v_{respan} + v_{dyn} + v_{call\_or\_syscall} + 1 - \\
(f_{join} + f_{split} + f_{loop} + f_{span} + f_{respan} + f_{dyn} + v_{call\_or\_syscall}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the last term here be f_{call_or_syscall} rather than v_{call_or_syscall}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

@Al-Kindi-0 Al-Kindi-0 left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!
One last thing is that the syscall or call flag is still not rendering well, at least when I open it on GitHub.

@plafer plafer force-pushed the plafer-483-docs branch 2 times, most recently from caf7e3a to 379d022 Compare October 22, 2024 15:54
@plafer
Copy link
Contributor Author

plafer commented Oct 22, 2024

One last thing is that the syscall or call flag is still not rendering well, at least when I open it on GitHub.

Oops, had forgotten a few. Should be fixed now

@plafer plafer merged commit 8029102 into next Oct 23, 2024
9 checks passed
@plafer plafer deleted the plafer-483-docs branch October 23, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This PR does not require an entry in the `CHANGELOG.md` file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants