Circuit for opcode `BALANCE` by silathdiir · Pull Request #683 · privacy-ethereum/zkevm-circuits

silathdiir · 2022-08-16T07:06:54Z

Summary

Update BALANCE bus-mapping and add circuit.
Fix to lookup Balance if account exists, otherwise lookup NonExisting.
Add a HashSet to save non-existent account addresses in both bus-mapping and circuit ExeSteps.

Not confirm if necessary to constrain "If the given account doesn't exist, then it will push 0 onto the stack instead." in the circuit.
And is there a method (could be used in circuit) to lookup if account is existing? Thanks.

~~Blocked by PR #907~~

CPerezz · 2022-08-19T09:50:38Z

Blocked by privacy-ethereum/zkevm-specs#248 which is at the same time blocked by privacy-ethereum/zkevm-specs#249

silathdiir · 2022-12-03T11:49:44Z

Will try to update this PR since #907 has been merged.

silathdiir · 2022-12-15T13:13:52Z

Hi @CPerezz , I updated this BALANCE circuit with non-existing proofs. Could you help review again when you have time? Thanks.

CPerezz

Just wondering the balance passed in the assignation of the Opcode circuit impl.

For the rest this looks good. If you cold provide feedback, I'll approve immediately!

CPerezz · 2022-12-16T17:05:52Z

+        let balance = if exists {
+            block.rws[step.rw_indices[5]]
+                .table_assignment_aux(block.randomness)
+                .value


I'm unsure why do we need block.randomness to set the balance. Is it passed directly as an RLC??

Hi @CPerezz , I checked function table_assignment_aux. But it seems that only value is needed here, so I replaced it with value_assignment(block.randomness).

And reference function value_assignment, it seems that the argument randomness is used in function random_linear_combine as below:

AccountFieldTag::CodeHash | AccountFieldTag::Balance => { RandomLinearCombination::random_linear_combine(value.to_le_bytes(), randomness) }

Hi @CPerezz , could you help review this again when you have time? Thanks.

Ye sorry @silathdiir I missed this!!!

CPerezz

LGTM! I'd want another review from someone at Scroll before merging this! @icemelon or @silathdiir could you ping someone? 😄

silathdiir · 2022-12-20T08:32:47Z

LGTM! I'd want another review from someone at Scroll before merging this! @icemelon or @silathdiir could you ping someone? 😄

Yes. I will ping others to help review. Thanks a lot.

DreamWuGit · 2022-12-20T08:47:29Z

+            + (1.expr() - is_warm.expr()) * GasCost::COLD_ACCOUNT_ACCESS.expr();
+
+        let step_state_transition = StepStateTransition {
+            rw_counter: Delta(cb.rw_counter_offset()),


rw_counter: Delta(cb.rw_counter_offset()), not good style since we don't know exactly rw counter change, suggest to use normal way rw_counter: 7.expr()

Replaced cb.rw_counter_offset() with 7.expr().

DreamWuGit · 2022-12-20T08:50:17Z

    /// Error generated by this step
    pub error: Option<ExecError>,
+    /// Non existent account addresses
+    pub non_existent_accounts: HashSet<Address>,


most probably do not need this, for circuit side, existing can be check by reading buss mapping record, for AccountFieldTag::NonExisting, no need additional step info.

Yes exactly 🙏, I fixed to get both balance and exists in circuit function assign_exec_step as below:

let (balance, exists) = match block.rws[step.rw_indices[5]] { Rw::Account { field_tag: AccountFieldTag::Balance, value, .. } => ( RandomLinearCombination::random_linear_combine( value.to_le_bytes(), block.randomness, ), true, ), Rw::Account { field_tag: AccountFieldTag::NonExisting, .. } => (F::zero(), false), _ => unreachable!(), };

DreamWuGit · 2022-12-20T08:50:53Z

    /// The opcode corresponds to the step
    pub opcode: Option<OpcodeId>,
+    /// Non existent account addresses
+    pub non_existent_accounts: HashSet<Address>,


same as above comment, suggest to remove

DreamWuGit · 2022-12-20T09:02:55Z

+
+        let address = block.rws[step.rw_indices[0]].stack_value().to_address();
+        let mut address_bytes = address.0;
+        address_bytes.reverse();


want to know what reason for reverse it

Try to investigate ethers_core::types::Address and code in eth-types, it seems that the Address is big endian.

https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/main/eth-types/src/lib.rs#L167

…or account existence.

lispc · 2022-12-21T05:04:04Z

+
+        let exists = cb.query_bool();
+        let balance = cb.condition(exists.expr(), |cb| {
+            let balance = cb.query_cell();


can the "query_cell" be moved out of the closure? It is some "structure/allocation" info, better not be guarded with "if" branch.

Moved out query_cell as:

let balance = cb.query_cell(); let exists = cb.query_bool(); cb.condition(exists.expr(), |cb| { cb.account_read( from_bytes::expr(&address.cells), AccountFieldTag::Balance, balance.expr(), ); });

lispc · 2022-12-21T05:04:46Z

+        cb.stack_push(select::expr(exists.expr(), balance.expr(), 0.expr()));
+
+        let gas_cost = is_warm.expr() * GasCost::WARM_ACCESS.expr()
+            + (1.expr() - is_warm.expr()) * GasCost::COLD_ACCOUNT_ACCESS.expr();


use select::expr?

Fixed to:

let gas_cost = select::expr( is_warm.expr(), GasCost::WARM_ACCESS.expr(), GasCost::COLD_ACCOUNT_ACCESS.expr(), );

lispc · 2022-12-21T05:07:03Z

need to modify https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/3da1888cfdea1f05062477279d9c5088e01fac19/zkevm-circuits/src/witness/step.rs#L184

silathdiir · 2022-12-21T06:07:46Z

need to modify

https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/3da1888cfdea1f05062477279d9c5088e01fac19/zkevm-circuits/src/witness/step.rs#L184

Sorry for the wrong revert. Fixed to OpcodeId::BALANCE => ExecutionState::BALANCE and moved to next one of ADDRESS.

* implement RLC chip * keccak table interfaces * minor * mock chunk tests * aggregation tests * fixes * supporing empty hash preimage * minor * remove tmp data * clean up and address comments * update docs * partial fix clippy * fix chunk construction bug * fix compiling bug * fix padded chunk's data hash * fix bug in number of valid chunks * handle the case of all valid ones * add fixed columns to rlc config * enforce the chunks are orderred * fix bugs in challenges * add missing selectors for aggregation circuit * fix the fixed cells (privacy-ethereum#677) * try to reenable skip first pass * tr to fix skip first pass again * try to fix again * clean up * fixed skip first pass * aggregation parameter with k = 19 * removing unused features; partial address comments * fix clippy * fix clippy * address comment on fixed cells * fix dynamic hash test * try to fix new fixed cells * reverting last 3 commits * Add conversion from witness `Block` to `ChunkHash`. (privacy-ethereum#683) --------- Co-authored-by: Steven <asongala@163.com>

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 Aug 16, 2022

This was referenced Aug 16, 2022

Spec for opcode BALANCE privacy-ethereum/zkevm-specs#248

Merged

Circuit for opcode BALANCE scroll-tech/zkevm-circuits#185

Closed

silathdiir marked this pull request as draft August 16, 2022 07:21

CPerezz self-requested a review August 16, 2022 11:40

ed255 mentioned this pull request Aug 18, 2022

Meta: Opcode spec + implementation status #477

Closed

pinkiebell mentioned this pull request Nov 5, 2022

Required opcodes for basic L2 Bridge #369

Closed

19 tasks

silathdiir mentioned this pull request Nov 11, 2022

Support non-existing proofs in RwTable for circuit #895

Closed

silathdiir mentioned this pull request Dec 2, 2022

Support non-existing proofs in RwTable #907

Merged

silathdiir force-pushed the feat/opcode-balance branch from 6c27908 to b50ea9f Compare December 3, 2022 11:48

silathdiir mentioned this pull request Dec 7, 2022

Bug: fix to not invoke transfer for call related opcodes (except CALL) privacy-ethereum/zkevm-specs#335

Merged

silathdiir added 2 commits December 13, 2022 05:35

Add bus-mapping and circuit of opcode BALANCE.

9513e0f

Update BALANCE circuit with non-existing proofs.

2fd71d5

silathdiir force-pushed the feat/opcode-balance branch from 1730635 to 2fd71d5 Compare December 12, 2022 21:35

silathdiir added 2 commits December 13, 2022 06:49

Update tests.

6c50cf9

Update test comments.

1ee1d4c

silathdiir marked this pull request as ready for review December 12, 2022 23:00

Merge remote-tracking branch 'upstream/main' into feat/opcode-balance

fa3f845

silathdiir mentioned this pull request Dec 16, 2022

Implement opcode EXTCODESIZE in Circuit #993

Closed

CPerezz reviewed Dec 16, 2022

View reviewed changes

Replace table_assignment_aux invocation with value_assignment.

129d531

silathdiir force-pushed the feat/opcode-balance branch from 5842d77 to 129d531 Compare December 17, 2022 02:02

Merge branch 'main' into feat/opcode-balance

8859fed

silathdiir requested a review from CPerezz December 17, 2022 02:59

Merge branch 'main' into feat/opcode-balance

e109a58

CPerezz approved these changes Dec 20, 2022

View reviewed changes

DreamWuGit reviewed Dec 20, 2022

View reviewed changes

silathdiir added 2 commits December 20, 2022 17:03

Replace cb.rw_counter_offset() with 7.expr().

9c960f8

Delete non_existent_accounts hash set, and fix to match Rw type f…

065b3f9

…or account existence.

silathdiir requested a review from DreamWuGit December 20, 2022 09:47

Revert useless format.

c01d305

lispc reviewed Dec 21, 2022

View reviewed changes

Fix wrong revert in step.rs and others according to code review.

2adef8a

silathdiir requested review from lispc and removed request for DreamWuGit December 21, 2022 06:07

lispc approved these changes Dec 21, 2022

View reviewed changes

lispc merged commit 0b8f7de into privacy-ethereum:main Dec 21, 2022

lispc deleted the feat/opcode-balance branch December 21, 2022 06:16

Conversation

silathdiir commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

CPerezz commented Aug 19, 2022

Uh oh!

silathdiir commented Dec 3, 2022

Uh oh!

silathdiir commented Dec 15, 2022

Uh oh!

CPerezz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silathdiir Dec 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CPerezz left a comment

Choose a reason for hiding this comment

Uh oh!

silathdiir commented Dec 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DreamWuGit Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lispc commented Dec 21, 2022

Uh oh!

silathdiir commented Dec 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

silathdiir commented Aug 16, 2022 •

edited

Loading

silathdiir Dec 17, 2022 •

edited

Loading

DreamWuGit Dec 20, 2022 •

edited

Loading