Circuit for opcode COINBASE by scroll-dev · Pull Request #259 · privacy-ethereum/zkevm-circuits

scroll-dev · 2022-01-03T23:26:24Z

The PR includes the following features:

Add block table & lookup
Coinbase opcode circuit
Add tests

The spec can be found at privacy-ethereum/zkevm-specs#83

DreamWuGit · 2022-01-04T08:08:48Z

@han0110 Could you please have a look ?

CPerezz

Aside from @han0110's comment on the Lookup of previous blocks by hash and the panic question, the rest looks good to me!

Nice job!!! 😄

CPerezz · 2022-01-05T11:16:17Z

+    ) {
+        self.add_lookup(Lookup::Block {
+            field_tag: tag,
+            number: number.unwrap_or_else(|| 0.expr()),


If the block comes without a number provided shouldn't we panic directly? As it means that some deep internal logic must be wrong.

if the block construct not initialize explicitly with a number, the default value will set to zero, so will not panic directly, does it make sense?

But 0 would probbaly not be the correct value no? So we will be passing a wrong block to the circuit condtructor IIUC.

the block constants include block number will be pub input of circuit eventually as planned, so it will be easy to figure out if it is incorrect .

Yes, what I meant is that if you check the code, if there's any unexpected error/behavior and we receive a None/Err, putting a 0 will not be correct.

Therefore the question of paniking.

Ah, that is it , at first I used number parameter type Expression directly without Option wrapper, but considering maybe zero in cases , change to Option as now. what is your ideas ?

To me it looks like number should not be an Option or Result and instead a pure value directly.
In that case, it's impossible to construct a non-valid number and you don't need to unwrap_or_zero it.

Maybe @han0110 has some insight on that. I'm just wondering that this cannot be the source of errors in the future. If it cannot, we can simply resolve the conversation.

IMO it's just semantic difference, since the block_table now is constructed as it is, the field number will always be 0 for all fields except history_hashes. So it would not be source of errors, but just might not look so straightforward.

Another approach is to split this into 2 function w/o number as input, to make it less ambiguous.

Then you can proceed as you wish @DreamWuGit !!

DreamWuGit · 2022-01-11T03:54:17Z

+        state_ref.push_stack_op(
+            RW::WRITE,
+            StackAddress::from(1024 - 1),
+            block.ctants.coinbase.to_word(),


I want to refactor to use BlockConstants's coinbase field here and remove ChainConstants coinbase field, not sure why exists in chain constant of bussmapping before, want to hear from you guys first @ChihChengLiang @han0110 @CPerezz

Maybe @ed255 can provide some insight on the bus-mapping related stuff.
From my perspective, the coinbase will always be the same. And therefore is only passed once via ChainConstants instead of passing it on each tx/block.

coinbase is only block's field not related to tx， each block can have different coinbase address, chainid is global which make sense exists in ChainConstants

I originally put the coinbase in ChainConstants. I had the wrong idea of what the coinbase was, so it was a mistake. coinbase should be removed from ChainConstants.

The coinbase that you need here I think should be block.eth_block.author. See documentation here https://docs.rs/ethers/latest/ethers/core/types/struct.Block.html#structfield.author
Block miner, author, coinbase are all the same thing.

The struct BlockConstants defined in external_tracer has a name which is a bit misleading. That struct is really a configuration parameter to setup an environment for the external tracer.

Ohh my bad...

I understood coinbase on a different way...

thanks @ed255 , I will try remove coinbase in ChainConstants at this PR!

fixed in 21ad52f

ed255 · 2022-01-11T16:57:48Z

+    // converting to block context
+    let context = BlockContext {
+        coinbase: b.constants.coinbase,
+        ..Default::default()


Here there should be a TODO: bus_mapping::circuit_input_builder::Block should contain all the fields required in the BlockContext:

pub coinbase: Address, // u160 pub gas_limit: u64, pub block_number: F, pub time: u64, pub difficulty: Word, pub base_fee: Word, pub previous_block_hashes: Vec<Word>,

I can take care of that if you want! Just let me know and I'll open the issue and address it myself.

changed to use BlockConstant which holds all fields now.

CPerezz

LGTM!!

ed255 · 2022-01-13T09:54:24Z

@DreamWuGit it seems the branch has conflicts with main.

DreamWuGit · 2022-01-14T01:57:33Z

it seems the branch has conflicts with main.
yes, will rebase to main

Co-authored-by: Haichen Shen <shenhaichen@gmail.com>

Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>

DreamWuGit · 2022-01-14T04:07:27Z

it seems the branch has conflicts with main.
yes, will rebase to main

rebased in 723912d

CPerezz · 2022-01-14T08:25:45Z

Tests are failing @DreamWuGit

Maybe was introduced some inconistency in tbhe rebase?

DreamWuGit · 2022-01-14T08:50:03Z

Maybe was introduced some inconistency in tbhe rebase?

I am looking into this :)

han0110 · 2022-01-14T09:04:17Z

Need 3 fixes of the doc in crate bus-mapping

https://github.com/appliedzkp/zkevm-circuits/blob/40cc46fd044de225a07c381d1df255bc5dad1190/bus-mapping/src/lib.rs#L55-L58

+ //! use bus_mapping::external_tracer::BlockConstants;

https://github.com/appliedzkp/zkevm-circuits/blob/40cc46fd044de225a07c381d1df255bc5dad1190/bus-mapping/src/lib.rs#L109-L112

- //!     coinbase: Address::zero(),

https://github.com/appliedzkp/zkevm-circuits/blob/40cc46fd044de225a07c381d1df255bc5dad1190/bus-mapping/src/lib.rs#L120-L121

- //! let mut builder =
- //!     CircuitInputBuilder::new(sdb, CodeDB::new(), &eth_block, ctants);
+ //! let mut builder = CircuitInputBuilder::new(
+ //!     sdb,
+ //!     CodeDB::new(),
+ //!     &eth_block,
+ //!     ctants.clone(),
+ //!     BlockConstants::from_eth_block(
+ //!         &eth_block,
+ //!         &Word::from(ctants.chain_id),
+ //!     ),
+ //! );

DreamWuGit · 2022-01-14T09:22:14Z

thanks @han0110 and @CPerezz ! should be fixed by 776bc42, just be curious that why need commented code snippet in bus-mapping/src/lib.rs. can we remove them ?

CPerezz · 2022-01-14T09:29:21Z

thanks @han0110 and @CPerezz ! should be fixed by 776bc42, just be curious that why need commented code snippet in bus-mapping/src/lib.rs. can we remove them ?

Is an example of usage. We have it there so that is easier to see which are the purposes and the recommended way to use the bus-mapping.

Since it's an example we can also compiled on each test run and notice regressions in API or any other similar error introduced.

I'm not against removing it. But IMO it is useful.

DreamWuGit · 2022-01-14T09:34:35Z

understood, THX！

github-actions Bot added crate-bus-mapping Issues related to the bus-mapping workspace member T-opcode Type: opcode-related and focused PR/Issue crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Jan 3, 2022

DreamWuGit mentioned this pull request Jan 4, 2022

bus-mapping: add block coinbase op code #260

Closed

ChihChengLiang reviewed Jan 4, 2022

View reviewed changes

Comment thread zkevm-circuits/src/evm_circuit/execution/coinbase.rs Outdated

han0110 reviewed Jan 5, 2022

View reviewed changes

Comment thread bus-mapping/src/eth_types.rs Outdated

Comment thread zkevm-circuits/src/evm_circuit.rs

Comment thread zkevm-circuits/src/evm_circuit/witness.rs Outdated

CPerezz suggested changes Jan 5, 2022

View reviewed changes

DreamWuGit force-pushed the block_coinbase branch from 7429811 to 2c5bffd Compare January 6, 2022 09:42

DreamWuGit mentioned this pull request Jan 6, 2022

add Busmapping coinbase code scroll-tech/zkevm-circuits#58

Merged

DreamWuGit reviewed Jan 11, 2022

View reviewed changes

ed255 reviewed Jan 11, 2022

View reviewed changes

CPerezz approved these changes Jan 11, 2022

View reviewed changes

ed255 mentioned this pull request Jan 12, 2022

bus-mapping: Implement Opcode trait for all non-storage-related OpcodeIds #83

Closed

78 tasks

DreamWuGit and others added 14 commits January 14, 2022 11:34

add block table &lookup

febbe07

integrate block table into configure evm circuit

d68d6f2

fix coinbase test

77159e4

fix formation

7d6d27f

fix benchmark test circuit with block table

b3b87ed

make clippy change

f995701

fix comments

9757cbc

remove comment

5724032

change block number for F

be68402

try change block lookup parameter

6976328

Apply suggestions from code review

9adad99

Co-authored-by: Haichen Shen <shenhaichen@gmail.com>

fix one clippy

0eab8b1

restore comments

32fbbff

Update comment to push

610c122

Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>

DreamWuGit and others added 12 commits January 14, 2022 11:37

enable address to word with little endian

5c65288

add missing file

78f38b1

correct block hash assignment

11ee0e3

revert additinal to word change

8eae067

add coinbsae bus mapping

3565fa1

fix clippy

d0dcd32

fix typos

ec9fbf2

remove comment

aed10ab

change coinbase test with block from trace of bussmapping

49bdfab

refactor removing coinbase from ChainConstants

b853fda

remove unsed comments

515b084

rebase latest main and fix

723912d

DreamWuGit force-pushed the block_coinbase branch from 8522e39 to 723912d Compare January 14, 2022 04:04

fix ci doc error

776bc42

CPerezz merged commit 1c1a89b into privacy-ethereum:main Jan 14, 2022

CPerezz mentioned this pull request Jan 18, 2022

Fix COINBASE opcode impl #289

Closed

Conversation

scroll-dev commented Jan 3, 2022

Uh oh!

DreamWuGit commented Jan 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

CPerezz Jan 11, 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!

CPerezz Jan 11, 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!

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!

ed255 commented Jan 13, 2022

Uh oh!

DreamWuGit commented Jan 14, 2022

Uh oh!

DreamWuGit commented Jan 14, 2022

Uh oh!

CPerezz commented Jan 14, 2022

Uh oh!

DreamWuGit commented Jan 14, 2022

Uh oh!

han0110 commented Jan 14, 2022

Uh oh!

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

Uh oh!

Uh oh!

CPerezz commented Jan 14, 2022

Uh oh!

DreamWuGit commented Jan 14, 2022

Uh oh!

Reviewers

Assignees

Labels

CPerezz Jan 11, 2022 •

edited

Loading

CPerezz Jan 11, 2022 •

edited

Loading

DreamWuGit commented Jan 14, 2022 •

edited

Loading