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

Change padding rule for CPU #1234

Merged
merged 12 commits into from
Sep 15, 2023
Merged

Conversation

Nashtare
Copy link
Collaborator

I assumed it would be simpler to just open a PR for it, so that you can share your comments directly on the code @nbgl. Feel free to override the PR if you have a better/simpler/more efficient approach!

@Nashtare Nashtare added this to the Optimization - Phase 1 milestone Sep 13, 2023
@Nashtare Nashtare requested a review from nbgl September 13, 2023 20:25
@Nashtare Nashtare self-assigned this Sep 13, 2023
Copy link
Contributor

@nbgl nbgl left a comment

Choose a reason for hiding this comment

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

This approach looks good—it's basically the same as mine. A few details can be simplified; I've noted those in comments.

Two missing constraints:

  1. Make sure that lv.null is mutually exclusive with the op flags.
  2. We must ensure that the prover can only start emitting null rows after we've jumped to halt. The prover should not be able to stop execution midway through.

evm/src/cpu/columns/mod.rs Outdated Show resolved Hide resolved
evm/src/cpu/stack.rs Outdated Show resolved Hide resolved
evm/src/cpu/control_flow.rs Outdated Show resolved Hide resolved
evm/src/cpu/control_flow.rs Outdated Show resolved Hide resolved
evm/src/cpu/control_flow.rs Outdated Show resolved Hide resolved
evm/src/cpu/control_flow.rs Outdated Show resolved Hide resolved
evm/src/cpu/kernel/asm/halt.asm Show resolved Hide resolved
@Nashtare
Copy link
Collaborator Author

Nashtare commented Sep 14, 2023

Thanks for the review @nbgl! I've applied your comments. I just have a few questions regarding your main comment:

Make sure that lv.null is mutually exclusive with the op flags.

That was indeed an overlook on my side. I've added this constraint implicitely through constraining the first row to NOT be a padding row. On bootstrap_kernel side, we are enforcing then that the op flags must be zero on the first row.
We then have
yield_constr.constraint_transition(is_cpu_cycle * (is_cpu_cycle_next + nv.halt_state - P::ONES));
which guarantees us that subsequent rows never have both op flags and the halt flag activated.

Regarding your comment on adding PANIC under the halt label, this forces to handle the padding slightly differently (i.e. we don't execute the last instruction (as it would hence panic)) and instead directly process dummy rows. Is that correct?

@nbgl
Copy link
Contributor

nbgl commented Sep 14, 2023

Regarding your comment on adding PANIC under the halt label, this forces to handle the padding slightly differently (i.e. we don't execute the last instruction (as it would hence panic)) and instead directly process dummy rows. Is that correct?

Exactly. There's no point continuing to process instructions once we've jumped to halt. It's simpler not to give the prover the choice.

@Nashtare
Copy link
Collaborator Author

great! Let me know if there is anything else bugging you.
Regarding my previous message, I'll add a comment in the code explaining how the constraints relate between modules for clarity.

@Nashtare
Copy link
Collaborator Author

Nashtare commented Sep 15, 2023

The CI error is unrelated and happened on other branches as well, opening #1236 to fix clippy.

@nbgl nbgl self-requested a review September 15, 2023 19:03
Copy link
Contributor

@nbgl nbgl 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!!

A few comments:

  • One small, subtle bug,
  • Two redundant constraints,
  • Two organization notes (not very important, consider them optional/it's your call to make).

evm/src/cpu/bootstrap_kernel.rs Outdated Show resolved Hide resolved
evm/src/cpu/control_flow.rs Outdated Show resolved Hide resolved
evm/src/cpu/control_flow.rs Outdated Show resolved Hide resolved
evm/src/cpu/control_flow.rs Outdated Show resolved Hide resolved
evm/src/generation/mod.rs Outdated Show resolved Hide resolved
@Nashtare
Copy link
Collaborator Author

Apologies for the back and forth. Now I think all your comments have been addressed.

Copy link
Contributor

@nbgl nbgl left a comment

Choose a reason for hiding this comment

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

Thank you!! Looks great :)

@Nashtare Nashtare merged commit 8903aec into 0xPolygonZero:main Sep 15, 2023
2 checks passed
@Nashtare Nashtare deleted the cpu-padding branch September 15, 2023 21:47
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.

2 participants