Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

[proof chunk] introduce Padding virtual step #1718

Closed
wants to merge 1 commit into from
Closed

[proof chunk] introduce Padding virtual step #1718

wants to merge 1 commit into from

Conversation

hero78119
Copy link
Member

@hero78119 hero78119 commented Dec 19, 2023 •
edited
Loading

Description

Introduce this virtual step to carry transition context, which do not incur any extra rw_table lookup.
This also eliminate ambiguous end_block_non_last with unified padding virtual step.

Design Rationale

Why introduce Padding

  • Currently we use EndBlock as padding, which still append few unnecessary rw_table lookup, so it make slight hard to control evm_row and max_rws separate and independently.
  • EndBlock as padding only work in single chunk. When comes to multiple chunk, we will have the case that we need to padding execution steps. Padding as [...EndBlock, EndBlock, ... EndChunk] is semantically unpreferred. Another option is [... EndChunk,... EndChunk, EndChunk] however EndChunk introduce > 10 rw_table lookup, so padding like this will easily to hit max_rws bound.

Therefore, introduce virtual step Padding to solve it.

2 use cases for this

Padding then following by EndChunk

[
...
Padding,
Padding,
EndChunk
]

Padding then following by EndBlock

[
...
Padding,
Padding,
EndBlock
]

@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Dec 19, 2023
@hero78119 hero78119 changed the title introduce Padding virtual step [proof chunk] introduce Padding virtual step Dec 19, 2023
@hero78119 hero78119 mentioned this pull request Dec 19, 2023
Merged
4 tasks
hero78119
hero78119 commented Dec 19, 2023
View reviewed changes
bus-mapping/src/circuit_input_builder.rs
.last()
.map(|tx| tx.last_step())
.unwrap_or_else(|| &self.block.block_steps.padding);
let mut padding = last_step.clone(); // padding step need to keep transition context
Copy link
Member Author

Choose a reason for hiding this comment

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

Heads up: here use last_step because it's at the end of block.
In chunking, we should use next_tx.first_step instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure this_tx.last_step has the same transition witness as next_tx.first_step right?

Copy link
Member Author

@hero78119 hero78119 Jan 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No they are different. I thought we discussed this issue in Pr #1690 review section :)
Feel free to bring it up further if you thought it doenst make sense for why they different

@github-actions github-actions bot added crate-external-tracer Issues related to the external-tracer workspace member CI Issues related to the Continuous Integration mechanisms of the repository. crate-mock Issues related to the mock workspace member T-bench Type: benchmark improvements crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-integration-tests Issues related to the integration-tests workspace member crate-eth-types Issues related to the eth-types workspace member crate-geth-utils Issues related to the geth-utils workspace member labels Dec 19, 2023
@github-actions github-actions bot removed crate-external-tracer Issues related to the external-tracer workspace member CI Issues related to the Continuous Integration mechanisms of the repository. crate-mock Issues related to the mock workspace member T-bench Type: benchmark improvements crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-integration-tests Issues related to the integration-tests workspace member crate-eth-types Issues related to the eth-types workspace member crate-geth-utils Issues related to the geth-utils workspace member labels Dec 19, 2023
@hero78119 hero78119 added CI Issues related to the Continuous Integration mechanisms of the repository. crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member and removed crate-zkevm-circuits Issues related to the zkevm-circuits workspace member crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member labels Dec 19, 2023
@hero78119 hero78119 added crate-zkevm-circuits Issues related to the zkevm-circuits workspace member and removed CI Issues related to the Continuous Integration mechanisms of the repository. labels Dec 19, 2023
CeciliaZ030
CeciliaZ030 reviewed Dec 23, 2023
View reviewed changes
bus-mapping/src/circuit_input_builder.rs
@@ -421,7 +428,7 @@ impl CircuitInputBuilder<FixedCParams> {
if is_first_chunk {
push_op(
&mut state.block.container,
&mut end_block_last,
&mut end_block,
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we just move StartOp and PaddingOp here in to the padding ExecStep?

@hero78119
Copy link
Member Author

This PR was included in #1690

@hero78119
Copy link
Member Author

included in #1690

@hero78119 hero78119 closed this Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CeciliaZ030 CeciliaZ030 CeciliaZ030 left review comments

Assignees
No one assigned
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hero78119 @CeciliaZ030