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

State circuit #23

Merged
merged 47 commits into from
Sep 22, 2021
Merged

State circuit #23

merged 47 commits into from
Sep 22, 2021

Conversation

miha-stopar
Copy link
Collaborator

File memory.rs renamed into state.rs and made it flexible enough to cover the stack circuit too. Also, range value tables (global_counter and address) now load the maximum value too - consequently, for example, now you specify 1023 for max address in the stack circuit and not 1024.

src/gadget/is_zero.rs Outdated Show resolved Hide resolved
src/gadget/is_zero.rs Outdated Show resolved Hide resolved
src/state_circuit/state.rs Outdated Show resolved Hide resolved
@therealyingtong
Copy link
Collaborator

Force-pushed to rebase on main.


/// In the state proof, memory operations are ordered first by address, and then by global_counter.
/// Memory is initialised at 0 for each new address.
/// Memory is a word-addressed byte array, i.e. a mapping from a 253-bit word -> u8.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if we're still doing 253-bit addresses, or using the full 256-bit instead. Let's add a TODO here.

Suggested change
/// Memory is a word-addressed byte array, i.e. a mapping from a 253-bit word -> u8.
/// Memory is a word-addressed byte array, i.e. a mapping from a 253-bit word -> u8.
/// TODO: Confirm if we are using 253- or 256-bit memory addresses.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that when you use the compressed form the address will be a253 number its just the random linear combination of the 8 bit words making up the 256 bit address.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we can assume a circuit memory size hard bound (say 2**24), it would be easier to use actual value instead of random linear combination for memory index.

For example, when one is going to do like mload(250), we will need to do memory bus mapping lookup in range (250, 250 + 1, 250 + 2, ..., 281) from byte to byte. If we use random linear combination, then some of indexes would have the first byte which exceeds 256 and produces carry to the second byte, which makes the random linear combination harder to calculate (hard do that without requiring intermediate witness).

If we assume a hard bound, we just take first 3 bytes and recompose it to 2**24 value to lookup memory, which seems to be easier, but is incompatible with current evm which doesn't has any hard bound.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this makes sense. Hard bound means we can't overflow 2**253 memory elements.

I was thinking if we used compressed form we would be able to avoid a decompression of the index when it comes from the stack. But you have to decompress to get the ordering right anyway so probably good to use the decompressed form.

Copy link
Collaborator

Choose a reason for hiding this comment

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

253 bit width range come from Pasta Curves ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It comes from the bn254 curve we are using. We replace Pasta curve with bn254 and halo2 polynomial commitment with kate polynomial commitment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you mean code "pasta_curves::arithmetic::FieldExt;" has been with bn254 or happens on other branch ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it happened on another branch but that is our plan.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The BN254 / KZG fork of halo2 is on @kilic's branch: https://github.com/kilic/halo2/tree/kzg

Copy link
Collaborator

Choose a reason for hiding this comment

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

@therealyingtong I see & Thx

src/state_circuit/state.rs Outdated Show resolved Hide resolved
src/state_circuit/state.rs Outdated Show resolved Hide resolved
src/state_circuit/state.rs Outdated Show resolved Hide resolved
src/state_circuit/state.rs Outdated Show resolved Hide resolved
src/state_circuit/state.rs Outdated Show resolved Hide resolved
src/state_circuit/state.rs Outdated Show resolved Hide resolved
src/state_circuit.rs Outdated Show resolved Hide resolved
src/state_circuit/state.rs Outdated Show resolved Hide resolved
Comment on lines 383 to 385
// We pad all remaining memory rows to avoid the check at the first unused row.
// Without padding, (address_cur - address_prev) would not be zero at the first unused row
// and some checks would be triggered.
Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: We eventually want to exclude the padding rows from the bus mapping lookup.

@miha-stopar miha-stopar changed the title Stack circuit State circuit Jul 29, 2021
// If address_cur != address_prev, this is an `init`. We must constrain:
// - values[0] == [0]
// - flags[0] == 1
// - global_counters[0] == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does global_counter need to be 0 for a new address? Isn't global_counter supposed to keep increasing across all addresses?

Copy link
Contributor

Choose a reason for hiding this comment

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

For a new call context, the global_counter should be init with 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

@spartucus This is not a new call context. It just moves to check the next memory/stack address.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The init at each new address is effectively a "dummy write" of value = 0. This handles the case where the first operation at an address is a read. Since this address has never been written to, the first read should return 0. (This "dummy write" isn't a real operation and doesn't affect the global_counter.)

In terms of circuit constraints: note that the q_read case involves querying the value on the previous row value_prev. Without a dummy init row, the first read would end up querying the last value from the previous address.

(Thanks @han0110 for explaining this to me.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @therealyingtong!

I asked @han0110 a couple of related questions - global_counter can be set to 0 in the init operation as this is not put in the bus mapping. We don't check global_counter uniqueness in the state circuit - this will be done in the evm circuit. In the evm circuit, we will (1) lookup global_counter one by one and (2) check the degree of bus mapping is equal to the number of operations. If a malicious prover skips a write, (1) will fail. If a malicious prover inserts a non-existing write, (2) fails.

@gaswhat: I think global_counter doesn't need to be increasing across all addresses, it only needs to be ordered increasingly for each address.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation @therealyingtong @miha-stopar. I missed the init row at each new address previously.

I have a follow up question. Could we skip this init row for each new address to save some rows? It seems to me a bit redundant. I think we just need to prove the first row of a new address must be a write operation. Should be enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gaswhat I removed stack init rows in #1bdbe5d. This requires some more code and some more gates, but indeed we save quite some rows. I left the memory init rows in there as they seem to be required. I will try to summarize @han0110 explanation below:

Memory init rows are needed to mimic the EVM interpreter's behaviour. For example, when memory is empty and mload(32) is executed (which returns memory[32..64]), EVM expands memory size to 64 and returns 0. Thus, in state circuit we allocate to 0 all addresses that are used to be compatible with EVM.

Indeed, we don't have to do such things for stack operations. Thanks for your comments!

@barryWhiteHat
Copy link
Contributor

@miha-stopar to make memory byte addressable is all that we need to do is apply a range check on every element we write to memory being 1 byte ?

If we do this we know that each address maps to a single byte. We would also have to change mstore to access 32 bus mapping element in evm proof.

@miha-stopar
Copy link
Collaborator Author

@miha-stopar to make memory byte addressable is all that we need to do is apply a range check on every element we write to memory being 1 byte ?

If we do this we know that each address maps to a single byte. We would also have to change mstore to access 32 bus mapping element in evm proof.

@barryWhiteHat yes, I plan to add this constraint as it's in the specs. Was having some problems with switching between memory and stack operations - I still have a couple of things to add, but it works now.

Copy link
Collaborator

@therealyingtong therealyingtong left a comment

Choose a reason for hiding this comment

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

LGTM! some non-blocking nits and questions.

Comment on lines 136 to 139
let one = Expression::Constant(F::one());
let two = Expression::Constant(F::from_u64(2));
let three = Expression::Constant(F::from_u64(3));
let four = Expression::Constant(F::from_u64(4));
Copy link
Collaborator

Choose a reason for hiding this comment

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

These constant expressions can be brought outside the scope of this closure, so that we don't have to replicate them in later closures.

let e = q_memory_first(meta);
// q_memory_first is 12 when q_target_cur is 1 and q_target_next is
// 2, we use 1/12 to normalize the value
let inv = F::from_u64(12_u64).invert().unwrap_or(F::zero());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let inv = F::from_u64(12_u64).invert().unwrap_or(F::zero());
let inv = F::from_u64(12_u64).invert().unwrap();

let e = q_memory_not_first(meta);
// q_memory_not_first is 4 when target is 2, we use 1/4 to normalize
// the value
let inv = F::from_u64(4_u64).invert().unwrap_or(F::zero());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let inv = F::from_u64(4_u64).invert().unwrap_or(F::zero());
let inv = F::from_u64(4_u64).invert().unwrap();

let q_stack_first_norm = |meta: &mut VirtualCells<F>| {
let e = q_stack_first(meta);
// q_stack_first is 12, we use 1/12 to normalize the value
let inv = F::from_u64(12_u64).invert().unwrap_or(F::zero());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let inv = F::from_u64(12_u64).invert().unwrap_or(F::zero());
let inv = F::from_u64(12_u64).invert().unwrap();

let e = q_stack_not_first(meta);
// q_stack_not_first is 6 when target is 3, we use 1/6 to normalize
// the value
let inv = F::from_u64(6_u64).invert().unwrap_or(F::zero());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let inv = F::from_u64(6_u64).invert().unwrap_or(F::zero());
let inv = F::from_u64(6_u64).invert().unwrap();

e * i
};

let q_stack_first = |meta: &mut VirtualCells<F>| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can consider inlining the non-normalised closures and never using them outside of the *_norm versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure how you mean as both (non-normalised and normalised) versions are used on multiple occasions?

let is_not_padding = one - padding;

let q_target = meta.query_fixed(q_target, Rotation::cur());
let one = Expression::Constant(F::one());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let one = Expression::Constant(F::one());

Comment on lines 352 to 353
q_stack_first * (one - flag), /* first stack op has to be
* write (flag = 1) */
Copy link
Collaborator

Choose a reason for hiding this comment

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

An unlikely situation, but: what if the first stack op is a padding = 1 row? (Similarly for memory)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are two tests that cover these two cases: memory_values doesn't have stack ops and stack_first_write doesn't have memory ops.

@miha-stopar
Copy link
Collaborator Author

LGTM! some non-blocking nits and questions.

Thanks @therealyingtong! I addressed most of the comments and left one question about inlining the closures.

* storage operations added to the state circuit

* memory and stack closures for getting specific rows

* storage closures; redundancy stuff removed

* binary value benchmark; padding constraint changed

* StorageOp impl. and integration in state circuit
@miha-stopar miha-stopar merged commit ab0b56c into main Sep 22, 2021
@miha-stopar miha-stopar deleted the stack_circuit branch September 22, 2021 15:18
@ChihChengLiang ChihChengLiang added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Oct 6, 2021
CPerezz added a commit that referenced this pull request Nov 15, 2021
Introduce a generic struct which holds a `Provider` (can be Http, Ws,
Ipc) which will be used to make any calls/requests to Geth.

The struct also implements the endpoints that the bus-mapping needs to
query at some point.
In this case, `eth_getBlockByHash` is the one included for now.

Resolves: #23
CPerezz added a commit that referenced this pull request Nov 15, 2021
Introduce a generic struct which holds a `Provider` (can be Http, Ws,
Ipc) which will be used to make any calls/requests to Geth.

The struct also implements the endpoints that the bus-mapping needs to
query at some point.
In this case, `eth_getBlockByHash` is the one included for now.

Resolves: #23

Co-authored-by: Eduard S. <eduardsanou@posteo.net>
CPerezz added a commit that referenced this pull request Nov 16, 2021
Following @ed255 suggestions the following extra JSON-RPC methods have
been provided support:
- debug_traceBlockByHash
- debug_traceBlockByNumber

Resolves: #23
CPerezz added a commit that referenced this pull request Nov 16, 2021
Introduce a generic struct which holds a `Provider` (can be Http, Ws,
Ipc) which will be used to make any calls/requests to Geth.

The struct also implements the endpoints that the bus-mapping needs to
query at some point.
In this case, `eth_getBlockByHash` is the one included for now.

Resolves: #23

Co-authored-by: Eduard S. <eduardsanou@posteo.net>
CPerezz added a commit that referenced this pull request Nov 16, 2021
Following @ed255 suggestions the following extra JSON-RPC methods have
been provided support:
- debug_traceBlockByHash
- debug_traceBlockByNumber

Resolves: #23
CPerezz added a commit that referenced this pull request Nov 17, 2021
Introduce a generic struct which holds a `Provider` (can be Http, Ws,
Ipc) which will be used to make any calls/requests to Geth.

The struct also implements the endpoints that the bus-mapping needs to
query at some point.
In this case, `eth_getBlockByHash` is the one included for now.

Resolves: #23

Co-authored-by: Eduard S. <eduardsanou@posteo.net>
CPerezz added a commit that referenced this pull request Nov 17, 2021
Following @ed255 suggestions the following extra JSON-RPC methods have
been provided support:
- debug_traceBlockByHash
- debug_traceBlockByNumber

Resolves: #23
CPerezz added a commit that referenced this pull request Nov 17, 2021
… bus-mapping. (#171)

* Add ProviderError

* Add rpc module

* Add ethrs-providers to deps and url & tokio to dev-deps

* Add `GethQueries` and getBlockByHash fn

* Change `GethQueries` by `GethClient`

* Add `eth_getBlockByNumber` support

* Update `serde` version to `1.0.130`.

* Impl `Serialize` for `GethExecStep` & `GethExecStepInternal`

Since the `json_rpc` lib that we use requires the response types to be:
`Serialize + Deserialize`, it has been needed to impl `Serialize` for
the types that were lacking it.

- GethExecStepInternal now derives `Serialize`.
- Implemented `From<GethExecStep> for GethExecStepInternal` which is
  used in the next point.
- Implement `Serialize` for `GethExecStep` serializing it as a
  `GethExecStepInternal` using the previous `From` impl mentioned.

* Add `ResultGethExecTrace` & `ResultGethExecStep`

Geth return value for `debug_traceBlockByHash` and
`debug_traceBlockByNumber` isn't the expected.

While the expected format was to get from these calls
`Vec<GethExecTrace>`, you are given a similar vector but that contains a
`result` Json field that makes no sense previous to each trace.

Therefore it has been needed to simulate this as a new type and derive
`Serialize` and `Deserialize` for it so that the response from this call
can be parsed from the JSON and turned into usable data structures.

* Add support for debug_traceBlockByHash/Number

Following @ed255 suggestions the following extra JSON-RPC methods have
been provided support:
- debug_traceBlockByHash
- debug_traceBlockByNumber

Resolves: #23

* Fix clippy complaints

* Fix review suggestions from @ed255

- Renames ResultGethExecTraces and ResultGethExecTrace.
- Fix `From<GethExecStep` impl for `GethExecStepInternal`.

Co-authored-by: Eduard S. <eduardsanou@posteo.net>
miha-stopar added a commit that referenced this pull request Dec 21, 2021
* Implement PC opcode (#109)

* add pc gadget

* add pc constraint

* fix

* tab -> space

* use new way to build op circuit

* remove redundent assign

* fix

Co-authored-by: mask-pp <huan_zi266@sina.com>
Co-authored-by: gaswhat <hs@scroll.tech>

* Show better error message when Go isn't installed (#130)

* Introduce a global Context for bus-mapping (#134)

* Introduce a global Context for bus-mapping

- Add Context type that can be accessed an mutated by
  `gen_associated_ops` of each execution step which serves two purposes
  - Maintain an execution state so that we can fill information about
    the current Context in which the step was executed (for example, we
    will add a Call context to handle reverts)
  - Keep track of all the data required for the circuits (like the list
    of Operations related to the Bus Mapping)
- Remove the global counter from each {StorageOp, MemoryOp, StackOp},
  and create a generic wrapper type `Operation` with the global counter
  and an op.
- In OperationContainer, separate each Operation Type into a different
  vector, but keeping a single method for inserting Operations.
- Add a helper method in Context to push an Operation while assigning
  and incrementing the GlobalCounter automatically.  This simplifies
  implementation of new bus-mapping opcodes.
- Introduce ProgramCounter and GlobalCounter `inc_pre` methods to
  replace the `advance_pc` and `advance_gc` macros.
- Fix increment of the GlobalCounter per ExecutionStep (an ExecutionStep
  will not increment the GlobalCounter, only [data] Operations will).

Resolve #128
Resolve #126

* Change `Context` by `TraceContext`

* Remove `NoOp` arm from `Target` enum

* Fix `OperationContainer` broken intra-doc links

Co-authored-by: CPerezz <c.perezbaro@gmail.com>

* Fix the test error for geth-utils on MacOS (#131)

* Fix the test error for geth-utils on MacOS

* fix the flag

Co-authored-by: gaswhat <hs@scroll.tech>

* fix: remove unnecessary constraint which requires zero on unused cells (#135)

* Change CI to not trigger for Draft PRs (#146)

Since we share a lot of code in Draft PRs with no purpose at all of
running tests for them, it's a waste of time and resources to run the
entire CI each time we push something to a Draft PR or we simply open
one.

Therefore, thanks to @han0110 who found that was possible to avoid this
in the workflow config, it has been added.

Resolves: #145

* Split gas info (#142)

* split gas info into gas and gas cost

* remove gas info reference

* Add EQ support in LtGadget (#120)

* Add EQ to comparator gadget

* [Opcode] Jumpdest  (#143)

* implement jumpdest code

* fix fmt

* change format

* mark _step

Co-authored-by: Dream Wu <wwuwwei@126.com>

* udpate toolchain to enable the array_map feature (#157)

* udpate toolchain

* fix ci lint stable: remove override and use toolchain

* Add operation (#158)

* fix operation container and write test

* implement ADD opcode in bus-mapping

* extend stack methods

* Refactor all opcodes to use the constraint builder + other utils (#147)

* Refactor all opcodes to use constraint builder + other utils

* Small misc improvements

* minor import and doc improvements

* Merge fixes

* Implement MLOAD/MSTORE opcodes (#87)

* Implement MLOAD/MSTORE opcodes

* Feedback

* Refactor bus-mapping (#162)

- Introduce the CircuitInputBuilder, which allows building the circuit
  inputs by taking types returned by geth and parsing them to generate
  the appropiate outputs.  The CircuitInputBuilder works by firs taking
  a block and then transactions and their associated execution traces.
  - The CircuitInputBuilder works by steps:
    1. Take a block
    2. Take each tx in the block
    3. Take each exec trace in the tx
  - The CircuitInpubBuilder in general is designed with 3 concepts that
    apply to 3 levels (block, tx, exec step):
    a. Input data: types obtained from geth (or in the future other
    modules like the partial merkle tree).
    b. context: data that must be available from one step to the other.
    c. Output data: data generated from Input data + context, that forms
    the circuit inputs.
- Previously there was a single type for ExecutionStep used both for
  deserializing and for circuit input.  Now the types are split into
  eth_types (which contains all types that geth / web3 can return), and
  internal structs in the CircuitInputBuilder which only contain the
  inputs required by the circuit.
- Introduce the eth_types modules (mentioned before).  Some of the types
  are reexported from rust-web3, others not available in external crates
  are defined.
- Introduce helper macros useful for tests to declare Address, Word and
  Word->Word maps from hex strings.
- Introduce mock data generators for tests.
- Rename `EvmWord` to `Word`.
- Rename `ExecutionTrace` to `ExecTrace`.
- Rename `ExecutionStep` to `ExecStep`.

* Add rho related checks (#156)

* add rho related checks

* feedback: add reasons to panic

* doc coef converter

* revamp biguint_to_f

* doc rho checks

* randomly modify the test and catch a bug

* add more docs to block counts

* complete the block count doc

* Implement MSTORE8 opcode (#160)

* build for apple silicon (#169)

* Implement some opcodes in bus-mapping (#167)

- Implement all the opcodes already implemented in the circuit, and some
  more.
- Add convencience `map` methods to StackAddress and MemoryAddress.
- New implemented opcodes in bus-mapping:
    - All unary opcodes: ISZERO, NOT.
    - All binary opcodes: ADD, SUB, MUL, DIV, SDIV, MOD, SMOD,
      SIGNEXTEND, LT, GT, SLT, SGT, EQ, AND, OR, XOR, BYTE, SHL, SHR,
      SAR.
    - All ternary opcodes: ADDMOD, MULMOD.
    - MSTORE
    - PC
    - JUMPDEST
    - PUSH2..32
    - DUP1..16
    - SWAP1..16

* Add GethClient and support for minimum required JSON-RPC endpoints by bus-mapping. (#171)

* Add ProviderError

* Add rpc module

* Add ethrs-providers to deps and url & tokio to dev-deps

* Add `GethQueries` and getBlockByHash fn

* Change `GethQueries` by `GethClient`

* Add `eth_getBlockByNumber` support

* Update `serde` version to `1.0.130`.

* Impl `Serialize` for `GethExecStep` & `GethExecStepInternal`

Since the `json_rpc` lib that we use requires the response types to be:
`Serialize + Deserialize`, it has been needed to impl `Serialize` for
the types that were lacking it.

- GethExecStepInternal now derives `Serialize`.
- Implemented `From<GethExecStep> for GethExecStepInternal` which is
  used in the next point.
- Implement `Serialize` for `GethExecStep` serializing it as a
  `GethExecStepInternal` using the previous `From` impl mentioned.

* Add `ResultGethExecTrace` & `ResultGethExecStep`

Geth return value for `debug_traceBlockByHash` and
`debug_traceBlockByNumber` isn't the expected.

While the expected format was to get from these calls
`Vec<GethExecTrace>`, you are given a similar vector but that contains a
`result` Json field that makes no sense previous to each trace.

Therefore it has been needed to simulate this as a new type and derive
`Serialize` and `Deserialize` for it so that the response from this call
can be parsed from the JSON and turned into usable data structures.

* Add support for debug_traceBlockByHash/Number

Following @ed255 suggestions the following extra JSON-RPC methods have
been provided support:
- debug_traceBlockByHash
- debug_traceBlockByNumber

Resolves: #23

* Fix clippy complaints

* Fix review suggestions from @ed255

- Renames ResultGethExecTraces and ResultGethExecTrace.
- Fix `From<GethExecStep` impl for `GethExecStepInternal`.

Co-authored-by: Eduard S. <eduardsanou@posteo.net>

* Upgrade the toolchain to nightly (#181)

* use nightly

* cargo fmt

* clippy happy

* rename to remove stable

* lint more

* handle unused fields in keccak

* fix doc

* specify exact nightly version

* bus-mapping: Introduce EVM error detection (#183)

Implement error detection for EVM execution steps.  This PR adds all the
possible errors that can happen during EVM execution, with some of the
error detection incomplete due to missing access to a state trie.  List
of errors as declared by geth:
- Errors not reported
    - [x] ErrInvalidJump
    - [x] ErrReturnDataOutOfBounds
    - [x] ErrExecutionReverted
- Errors ignored
    - [x] ErrCodeStoreOutOfGas
    - [x] ErrMaxCodeSizeExceeded
    - [x] ErrInvalidCode
    - [ ] ErrContractAddressCollision
    - [ ] ErrInsufficientBalance
    - [x] ErrDepth
- Errors reported
    - [x] ErrWriteProtection
    - [x] ErrOutOfGas
    - [x] ErrGasUintOverflow
    - [x] ErrStackUnderflow
    - [x] ErrStackOverflow
    - [x] ErrInvalidOpCode

Each error has a test that uses the external_tracer and produces the
error.

The two incomplete error detections are for
`ErrContractAddressCollision` and `ErrInsufficientBalance`.  Detecting
these errors requires keeping track of the state, which we don't do yet.

Also, bump go-ethereum version in geth-utils.

* bus-mapping: Extend CallContext (#191)

Add the required fields in the CallContext, and use it to detect
CREATE/CREATE2 RETURN errors, and to fill the address in SLOAD
StorageOp.

Resolve #137
Resolve #185

* Implement XOR opcode (#75)

* bitwise circuit

* fix rebase error

* rebase and fix lint and tests

* fix the xor test

Co-authored-by: gaswhat <hs@scroll.tech>
Co-authored-by: Haichen Shen <shenhaichen@gmail.com>

* feat: mitigate the long testing time by allowing partial fixed table (#198)

* bus-mapping: Add StateDB key-value database (#193)

Resolve #184

* JUMP and JUMPI opcode circuit (#176)

* first draft jump circuit

* fix operations lookup

* load bytecode from executionstep

* change comments

* remove tag&push_rindex

* remove assgin_byte_table method as unused now

* add jumpi circuit & lookup selector

* remove unused comment

* make impl_op_gadget return vec of contraint and revert lookup selector feature

* remove bytecode in bussmapping

* use vec! to simplify Vec::

* remove reset_expression

* update with latest main branch

* fix fmt check

* fix clippy

* fmt resolve

* fix ci & rebase long test time mitigation

* use select expression

* update to use dest cells intead of linear combination

Co-authored-by: Dream Wu <wwuwwei@126.com>

* bus-mapping: Extend circuit_input_builder (#201)

Extend the types returned by the CircuitInputBuilder to match the
requirements from the circuits.

Rename CallContext to Call because the struct will be required for the
circuits.  Instead of keeping call metadata in a call stack, keep all
call metadata in a vector, and use a call stack with indices to this
vector.

Extend Call, Transaction, ExecStep and Block with fields required by the
circuits.

Calculate addresses in bus-mapping.

Integrate StateDB into CircuitInputBuilder and detect
InsufficientBalance and ContractAddressCollision execution errors.

Remove web3 dependency in favour of ethers-core for eth_types.

Resolve #190
Resolve #187
Resolve #180
Resolve #172

* bus-mapping: Bump ethers dependency (#208)

Remove unecessary Serialize implementations for types used in rpc
results

Resolve #179

* enable rho check selectors (#182)

* enable rho check selectors

* fix chunk rotate, 13->9 lookup, and block count check

* docs and nitpicks

* Replace with kate (#199)

* replace with kate

* fix clippy

* update dev graph

* igonore evm_word test

* fix evm word

* fix unexpected changes

* update reference

* test compilation success

* fix from_u64 and fmt

* fix test

* feat: Add get_proof RPC (#205)

* Generate AccessTrace and AccessSet (#209)

Generate a trace of state accesses from a transaction execution trace.

From a state access trace, generate a state access set.

The AccessSet will be used to query necessary State data from geth to
process transactions and generate the associated circuit inputs.

* Add integration testing with geth (#221)

Introduce a new crate `integration-tests` to do integration tests of the
rest of the crates with geth.  This crate contains a binary used to
generate data for the geth dev blockchain, and groups of integration
tests.  For now we only have the `rpc` test group.  See
`integration-tests/README.md` for more details.

Add a github action workflow that runs the integration test for the
bus-mapping rpc methods.  The action runs the `run.sh` integration test
script by steps to make it easier to see how long steps take, and to
quickly find which step fails in case of an error.

bus-mapping: Add `get_code_by_address` rpc method.

bus-mapping: Set the fields in the types returned by `get_proof` public,
and move them to `eth_types`.

Co-authored by: NoCtrlZ <phantomofrotten@gmail.com>

* Build keccak all toghether (#144)

* Add KeccakFConfig & allocation structure def

The KeccakFConfig contains all of the gadget configurations of the
gadgets plus the logic for the allocations of each of the keccak steps
on each of the regions.

This is the first design guideline that seems can fit in with the infra
we have.
Works with #105

* Remove biguint_to_pallas duplicity

* Add aux functions to switch state repr

We need to move from `FieldExt` to `BigUint` Repr in order to execute
KeccaK intermediate steps so that we can allocate all the intermediate
states of the keccak algorithm inside of the circuit.

Therefore we need functions that allow us to swap between both
representations.

* Add `assign_state` placeholders for Pi and Rho Configs

* Add 24-loop state allocation phase in KeccakConfig

* Add state_assign minus mixing stage

* Add configure initial impl for `KeccakConfig`

* Add basic b9 & b13 ROUND_CTANTS allocation

* Change gadgets state allocation to add out_state

We now also allocate the out_state of the gadget when we allocate the
entire witness for the gadget in keccak.

* Merge `next_input` and state assigment to single fn

We can simply do the assigment of the `out_state`, `state` and
`next_input` in a single function reducing the overhead and the
verbosity.

* Change `q_enable` activations to happen in `assign_state`

* Add missing offset increments in KeccakConfig allocation

* Set IotaB9Config Selector as generic Expression

* Set IotaB13 Selector as Expression

* Change AbsorbConfig design and allocation

We now allocate the Absorb as:
- State Row
- Next Mixing Row
- Out State Row

* Move state transformation fns to arith_helpers mod

* Add MixingConfig preliminary design

* Externalize state conversion functions

* Add out_state computation during `assign_state` runtime for B13 & B9

* Add `State` creation function in arith_helpers

* Change AbsorbConfig assigment to compute out_state internally

* Add assign_state_and_mixing_flag_and_rc for IotaB9Config

* Finalize first MixingConfig configure fn

* Change AbsorbConfig to copy_cell strategy

* Add IotaB13Config Cell copy constrains strategy & modify tests

* Update IotaB9Config assigment functions

* Change KeccakF circuit calls to IotaB9 and Mixing configs

* Fix `state_bigint_to_pallas` slice copy lengths

* Add mixing step to KeccakFArith

* test_absorb_gate: Witness input state to get (Cell, Value) tuples.

* Fix range of `state_to_state_bigint`

* IotaB9:_Fix test_flag wrong assignation_err

* iota_b9: Introduce q_last, q_not_last selectors.

These are used to differentiate between gates for the steady state,
and gates for the final round (where an is_mixing flag is witnessed
by the prover).

In the final round, q_last * flag is used as a composite selector.

* Add IotaB9 missing test cases

* IotaB13: Add internal selector + flag setup

With the previous setup, the gate was producing `ConstraintPoisoned` due
to the usage of `round_ctant_b13` at rotation:next to store the
`is_mixing` flag inside.

It also was activated/deactivated following the same bool logic as
IotaB9, and has been changed.

- IotaB13 now activates when `is_mixing = false` so no matter the inputs
  the verification will pass as the gate is not active.
- IotaB13 contains now an internal selector `q_mixing` which is always
  active and prevents the gate equations to fail due to queriyng
  `round_ctant_b13` cells that they shouldn't.

This completes all the development needed for IotaB9 and IotaB13 in
order to add them inside the `MixingConfig` and so work towards closing
issue #105

* Absorb: Add internal selector + flag setup

With the previous setup, the gate was producing `ConstraintPoisoned` due
to the usage of `absorb_next_inputs` at rotation:next to store the
`is_mixing` flag inside.

It also was activated/deactivated following the same bool logic as
IotaB9, and has been changed.

- Absorb now activates when `is_mixing = false` so no matter the inputs
  the verification will pass as the gate is not active.
- Absorb contains now an internal selector `q_mixing` which is always
  active and prevents the gate equations to fail due to queriyng
  `absorb_next_inputs` cells that they shouldn't.

ASSIGNATION MAP:
- STATE (25 columns) (offset -1)
- NEXT_INPUTS (17 columns) + is_mixing flag (1 column) (offset +0) (current rotation)
- OUT_STATE (25 columns) (offset +1)

This completes all the development needed for `AbsorbConfig` in
order to add them inside the `MixingConfig` and so work towards closing
issue #105

* Add state computation fn's for configs

It's much easier, clean and less verbose to compute
`in_state`, `out_state` and `next_inputs` with an associated function
for the MixingConfig sub-configs. And also makes the tests much less
verbose.

* Update StateBigint in compute_states signatures

* Mixing: Add `MixingConfig` impl + tests lacking base conversion

* mixing: Witness flag in state assignation

* Rho: Derive `Debug` for all configs

* xi: Apply copy_constraints for xi inputs

It is critical for the correctness of the keccak circuit to apply copy
constraints between the gates while executing the rounds.

Works towards solving: #219

* Add OFFSET associated consts

* Ignore failing Mixing tests

* Clippy fixes

* Replace pallas by field

* Add zeroed_bytes assertion

Co-authored-by: ying tong <yingtong@z.cash>

* use (Cell, F) (#224)

* Make gadgets to handle an execution state instead of an opcode per step (#196)

* feat(bus-mapping): implment ToLittleEndian for U256

* refactor: refactor EvmCircuit and all *Gadgets with ConstraintBuilder (without MemoryGadget)

* feat: add execution gadget name and constraint names for easier debugging

* feat: refactor MemoryGadget with ConstraintBuilder

* feat: refactor all with SameContextGadget for opcode and gas_left check

* feat: add ErrorOOGPureMemoryGadget as original MemoryGadget's OOG handler

* feat: make struct in bus_mapping_tmp be like the being implemented one in bus_mapping

* feat: make EvmCircuit pub for further benchs and examples usage

* feat: add bitwise gadgets back as a single AndGadget

* feat: assign transaction context

* feat: test AND, OR, XOR in the same trace to further shorten testing time

* feat: fix comment of ADD, add comment for PUSH

* chore: remove Range17 table

* chore: add rw_lookup and rw_lookup_at for all read/write lookup

* feat: refactor util and extend AddWordsGadget to add N words

* feat: adopt JumpGadget and JumpiGadget from upstream with new interface

* fix: add missing execution result ErrorOutOfGasCodeStore

* chore: cap jump and jumpi's testing time as k upto 11

* fix: pass test after rebasing

* chore: remove moved files

* feat: add number of cells used for StepState into param as constant

* chore: rename ExecutionResult to ExecutionState

* chore: rename ExecutionResult to ExecutionState

* chore: rename StateTransition to StepStateTransition

* chore: rename q_execution_state to execution_state_selector

* chore: renaming ExecutionState BITWISE, CMP, SCMP, MEMORY

* fix: fix name and_gadget to bitwise_gadget

* chore: calculate rotation_offset from state instead of hardcoding

* chore: add comment on opcode_source and update namings

* fix: use OpcodeId for checking whether an u8 is PUSH*

* fix: remove incorrect comments

* fix: add more docs

* fix: improve comment

* chore: init array with same value F::zero() and length

* fix: use *_lookup_with_counter instead of *_lookup_inner

* feat: add docs on Lookup and its variants

* fix: make MemoryExpansionGadget's expression method more straightforward

* Circuit benchmarks addition to workspace (#226)

* Add bench profile to workspace Cargo.toml

* Add benchmark running rules to Makefile

* Change CI to test the benchmarks without run

* Add circuit-benchmarks crate

This crate is responsible for allowing to run the circuit
benchmarks on a more easy and clean way.

* Update visibility of Circuit structs to allow benchmarks

* Compile crate before checking rustfmt

* Fix rebase unused_imports

* Add state_circuit module during build.rs run

* Change to more descriptive env var error messages

Co-authored-by: Eduard S. <eduardsanou@posteo.net>

* Fix Makefile and CI

* Fix ethers dep to pull grom git

While a new version is not published in crates.io, we need to pull from
git.

* Change state_circuit generation from build.rs

* Update evm_circuit to latest main version

Co-authored-by: Han <tinghan0110@gmail.com>

* Fix contract deployment breaking change issues

`ethers-solc` had a breaking change within patch versions.
This solves it while we still depend on the git dep.

* Update solc version used in integration CI job to 0.8.10

Co-authored-by: Eduard S. <eduardsanou@posteo.net>

Co-authored-by: Eduard S. <eduardsanou@posteo.net>
Co-authored-by: Han <tinghan0110@gmail.com>

* chore: add Cargo.lock (#232)

* chore: add Cargo.lock

* sync with main

* docs: fix typos (#229)

* gitignore circuit_benchmarks bench_params (#238)

* radical Pi (#234)

* Update Theta to copy state Cells for in/out (#241)

* Update Theta to copy state Cells for in/out

As stated in #219, the gates need to apply copy constraints between the
input cells and the witnessed values to ensure correctness between
keccak steps.

Also, now the state_assignation fn returns `out_state` as `[(Cell,F);25]` instead of simply the `F` values.

* Fix nit for arr + vec creation order

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

* Use Layouter instead of `Region` + `offset`

* bus-mapping: Extend CircuitInputBuilder (#235)

- CircuitInputBuilder:
    - Fix `gen_state_access_trace` by splitting the code and address in the
      context of a call.
    - Add `codes` field in the CircuitInputBuilder to keep a mapping of
      code hash -> code.
    - When doing the `gen_associated_ops` step for opcodes not yet
      implemented, instead of panic show a log warning, so that we can
      generate circuit inputs for blocks eventhough the implementation
      is not complete.  This will allow performing integration tests.
    - Resolve #207
- rpc:
    - Add `eth_coinbase` and `eth_chainId` methods.
    - Remove `BlockNumber` in favour of the implementation in `ethers`.
- General:
    - Replace usage of `BlockConstants` by the new struct
      `ChainConstants`.  This way we split constant parameters of the
      chain into `ChainConstants` and block parameters into `Block`.
      `BlockConstants` is now only used by the external_tracer.
    - Move `BlockConstants` from `bus-mapping/src/exec_trace.rs` to
      `bus-mapping/src/external_tracer.rs`.
    - Implement the Debug trait for some types manually to make debug
      output nicer.
    - Introduce a `circuit_input_builder` integration test with geth.
      The test builds the circuit inputs for the block where the
      Greeter.sol contract is deployed, up to step 5 from
      #222 (whith
      some missing parts)
    - Fix `mload` by allowing memory reads at addresses higher than the
      memory size found in the trace (in such case, a 0 value is read)

* Construct EVM circuit test inputs from Geth trace (#239)

* construct evm circuit test inputs from geth trace

* add `mstore8_opcode_impl` test (#22)

* WHOLEWORD -> IS_MSTORE8

* clean up

clean up

* more docs

* Update zkevm-circuits/src/evm_circuit/witness.rs

Co-authored-by: Han <tinghan0110@gmail.com>

* Update zkevm-circuits/src/evm_circuit/witness.rs

Co-authored-by: Han <tinghan0110@gmail.com>

* fix conflicts caused by merge

fix conflicts caused by merge

fix conflicts caused by merge

* fix needless_borrow

* combine imports

* set expected value explicitly for mstore8_opcode_impl test

* use larger value for mstore8_opcode_impl test

* more clean up

* remove TODOs

fix typos

Co-authored-by: HAOYUatHZ <haoyu@protonmail.com>
Co-authored-by: HAOYUatHZ <37070449+HAOYUatHZ@users.noreply.github.com>
Co-authored-by: Han <tinghan0110@gmail.com>

* rebasing

* branch json added; assignment of s_advices and c_advices

* some initial branch constraints

* keccak table; fixes

* keccak lookup simulation

* non assignment errors fixed

* some docs

* row type info integrated

* simulation of keccak lookups now works

* renaming

* test path changed

* printlns removed

* two layer test added

* branch children constraints

* branch children constraints

* some keccak related constraints

* fixing keccak constraints

* fixed keccak constraint problem

* keccak constraints

* into keccak words constraints

* started on branch hashing

* initial branch acc constraints

* minor changes

* assigning branch random linear accumulator

* Branch accumulator chip

* branch RLP row assigned after all branch nodes

* is_branch_rlp removed; will be replace by lookup into rlp circuit

* keccak leaf row removed; leaf key rows added

* constraints for key hex -> nibbles

* leaf key compression constraints

* key compression constraints moved into a chip

* leaf key c removed as both branches have the same key

* Branches of different (RLP) length supported

* key length considered when checking key compression

* key rlc acc

* leaf key checked to have zeros after nibbles end

* fixing key RLC

* all key rlc constraints

* branch RLC keccak lookups

* leaf hash constraints reanbled with RLC

* leaf hash chip

* account leaf selectors added

* acount leaf assignments

* accout leaf key constraints

* account_leaf_nonce_balance partially implemented

* account leaf nonce balance multiplier constraints now work

* account leaf nonce balance constraints for zeros after nonce and balance end

* r_table generated outside chips

* account leaf storage codehash chip

* missing rlp meta data  bytes added in nonce balance

* account leaf hash checked to be in the parent branch

* witness updated

* key (address) rlc distinguished for account and storage proof

* storage root in account leaf constraint

* address compression

* Address compression for even addresses

* Key compression constraints for long RLPs

* key compression long RLP even length

* separate row for leaf value

* hash accumulator for leaf

* storage leaf generalized for short and long RLPs

* branch S and C can have different number of RLP meta data bytes

* r_table removed 1 from first position

* r_table instead of curr_r in account_leaf_nonce_balance

* fixing r_table

* branch_acc now uses r_table

* nibbles removed

* key rlc changed from nibbles to bytes

* leaf key RLC enabled on bytes instead of nibbles

* leaf storage key rlc reimplemented with bytes (instead of nibbles)

* account address now uses bytes for rlc (instead of nibbles); additionally, nonce balance c row has been removed

* nonce balance C removed; fix in leaf_key

* key_compr and address_compr removed

* switched to appliedzkp/halo2

Co-authored-by: Scroll Dev <dev@scroll.tech>
Co-authored-by: mask-pp <huan_zi266@sina.com>
Co-authored-by: gaswhat <hs@scroll.tech>
Co-authored-by: Brecht Devos <Brechtp.Devos@gmail.com>
Co-authored-by: Eduard S <edu@aragonlabs.com>
Co-authored-by: CPerezz <c.perezbaro@gmail.com>
Co-authored-by: Han <tinghan0110@gmail.com>
Co-authored-by: Carlos Pérez <37264926+CPerezz@users.noreply.github.com>
Co-authored-by: ash <phantomofrotten@gmail.com>
Co-authored-by: Dream Wu <wwuwwei@126.com>
Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>
Co-authored-by: Eduard S. <eduardsanou@posteo.net>
Co-authored-by: Haichen Shen <shenhaichen@gmail.com>
Co-authored-by: Tengfei Niu <spartucus@users.noreply.github.com>
Co-authored-by: ying tong <yingtong@z.cash>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
Co-authored-by: HAOYUatHZ <37070449+HAOYUatHZ@users.noreply.github.com>
Co-authored-by: HAOYUatHZ <haoyu@protonmail.com>
Co-authored-by: Miha Stopar <miha@Mihas-MacBook-Pro.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

9 participants