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

fix empty block assigning error#713

Merged
lispc merged 2 commits into
privacy-ethereum:mainfrom
scroll-tech:fix_empty_block
Aug 30, 2022
Merged

fix empty block assigning error#713
lispc merged 2 commits into
privacy-ethereum:mainfrom
scroll-tech:fix_empty_block

Conversation

@DreamWuGit
Copy link
Copy Markdown
Collaborator

@DreamWuGit DreamWuGit commented Aug 26, 2022

refer to issue #710

@github-actions github-actions Bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Aug 26, 2022
@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Aug 26, 2022

I have a question about this fix. I noticed that the way we add the EndBlock step requires having > 0 txs. This is because this step is added here: https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/d16ac9b8bda39c45c82a41d2d59054c85abbc9a1/zkevm-circuits/src/evm_circuit/witness.rs#L1405-L1410
Which is only called if there's a last tx.

In this case there are no txs, so the witness generation is not adding an EndBlock step. But for an empty block we should still have an EndBlock right?

Maybe we can address this in a different PR.

ed255
ed255 approved these changes Aug 26, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

I've made a minor comment about a variable naming, but it's just a suggestion! Feel free to ignore it if you think different about it.

LGTM! Thanks for the fix :)

Comment thread zkevm-circuits/src/evm_circuit/execution.rs Outdated
@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Aug 29, 2022

Completelly agree with #713 (comment).
This unwrap will cause errors if there's a period of low chain-activity.

CPerezz
CPerezz approved these changes Aug 29, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM!

Agree with @ed255 on #713 (comment) and also the comment on the variable name.

@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Aug 29, 2022

You'll need to update this @DreamWuGit so that we can merge.

@DreamWuGit
Copy link
Copy Markdown
Collaborator Author

DreamWuGit commented Aug 30, 2022 •
edited
Loading

Which is only called if there's a last tx.
In this case there are no txs, so the witness generation is not adding an EndBlock step. But for an empty block we should still have an EndBlock right?

Maybe we can address this in a different PR

@ed255 good point , for two issues:

  1. if there's a last tx, maybe be have problem because first and last tx leading to different rw_counter, open an issue Check end_block's rw_counter when block contains only one tx  #721 to trace this separately.
  2. for an empty block, there is no txs, so no tx_convert ,hence no EndBlock added.

@DreamWuGit DreamWuGit mentioned this pull request Aug 30, 2022
Closed
@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Aug 30, 2022

@DreamWuGit Feel free to merge this PR whenever you like! We can fix the issue #721 in a future PR.

@lispc lispc merged commit 3725041 into privacy-ethereum:main Aug 30, 2022
@lispc lispc deleted the fix_empty_block branch August 30, 2022 22:26
@DreamWuGit DreamWuGit mentioned this pull request Aug 31, 2022
Closed
lispc added a commit that referenced this pull request Aug 28, 2023
@zhenfeizhang @lightsing
@noel2004 @lispc @kunxian-xia @silathdiir
#711 + #713 remove num chunks from public input; relax the constraint…
8f3e82a 
…s for the dummy chunk proofs to avoid dummy chunk proofgen time (#712)

* partiall address review comments of #670

* disable disable_proof_agg by default

* fix soundness issues with data hash

* add fixed cells for chunk_is_valid

* fix typo

* well... need morning coffee

* typo in readme

* fix soundness issue in is_smaller_than

* [feat] unify u8 u16 lookup table (#694)

* add range table

* replace Range256Table of rlp_circuit_fsm

* replace u16_table of tx_circuit

* use TableColumn

* add type alias

* use type alias

* annotate lookup column

* opt min_num_rows_block in poseidon_circuit (#700)

prune debug prints and lint

* speedup ci using verify_at_rows (#703)

* speedup ci using verify_at_rows

* use verify_at_rows to speedup super circuit ci tests

* update super circuit row estimation API (#695)

* remove num_of_chunks in aggregation circuit's instance column (#704)

Co-authored-by: kunxian xia <xiakunxian130@gmail.com>

* fix: lost tx_type when doing type conversion (#710)

* fix condition (#708)

* gates for zero checks

* statement 1 is correct

* reenable statements 3,6,7

* reenable statement 4

* everything seems to work again

* update aggregation test accordingly

* update spec

* minor clean up

* Fix fmt.

* Make `ChunkHash` fields public.

* fix decompose function

* update figures

* clean up

* address comments

* add is_final checks

* update readme

* constraint hash input length

* fix clippy

---------

Co-authored-by: Akase Cho <lightsing@users.noreply.github.com>
Co-authored-by: Ho <noel.wei@gmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
Co-authored-by: kunxian xia <xiakunxian130@gmail.com>
Co-authored-by: Steven Gu <asongala@163.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ed255 ed255 ed255 approved these changes

@CPerezz CPerezz CPerezz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

4 participants

@DreamWuGit @ed255 @CPerezz @lispc