-
Notifications
You must be signed in to change notification settings - Fork 857
Implementation of tx circuit (shortcut 1) #484
Conversation
5042e3c
to
914c2bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking fine so far. Amazing work.
Haven't checked everything, as I want to do a second review once the first comments are discussed/addressed :)
ecdsa = { git = "https://github.com/appliedzkp/halo2wrong", rev = "92b96893b5699ff40723e201a2416313aeafd267", features = ["kzg"] } | ||
secp256k1 = { git = "https://github.com/appliedzkp/halo2wrong", rev = "92b96893b5699ff40723e201a2416313aeafd267", features = ["kzg"] } | ||
ecc = { git = "https://github.com/appliedzkp/halo2wrong", rev = "92b96893b5699ff40723e201a2416313aeafd267", features = ["kzg"] } | ||
maingate = { git = "https://github.com/appliedzkp/halo2wrong", rev = "92b96893b5699ff40723e201a2416313aeafd267", features = ["kzg"] } | ||
integer = { git = "https://github.com/appliedzkp/halo2wrong", rev = "92b96893b5699ff40723e201a2416313aeafd267", features = ["kzg"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should release a tag
as we do for halo2_proofs
. Otherways this might be hard to track in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree!
// by keccak table lookup, where pub_key_bytes is built from the pub_key | ||
// in the ecdsa_chip | ||
// keccak lookup | ||
meta.lookup_any("keccak", |meta| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking out loud to @CPerezz. Wondering what interface can we add to keccak config to simplify this gate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the same! ahhahah
That's why I wanted to meet with you. Since we can do a proxy-table to simplify the API (at a cost obviously). And I thought that once the state_tag stuff is handled (I'm working on it), We should meet with @miha-stopar and @ed255 to see what are they expecting exactly as API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall
Implement the Tx Circuit as specified in https://github.com/appliedzkp/zkevm-specs/blob/master/specs/transactions-proof.md#circuit-behaviour-shortcut-1 This implementation uses ~205 columns and ~170k rows per transaction. New dependencies to the zkevm-circuits subcrate: - `halo2wrong` subcrates. This is where the ECDSA verification chip is implemented. This dependency uses `halo2` and this means that we'll require the version of `halo2` used in `halo2wrong` and `zkevm-circuits` to match. - `group`: Required for field and curve traits - `libsecp256k1`: Requiered to perform the ECDSA public key recovery with access to the public key coordinates. - `rlp`: Required to calculate the RLP of the transaction to get the transaction hash (to sign) - `num-bigint`: Used to hold an integer bigger than the field, to latter apply mod Fq (this is required for the message hash in the ECDSA signature operation) - `subtle`: Used to map `CtOption` to `Result`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, amazing work @ed255 !
This is the implementation of the Transaction Circuit as specified in https://github.com/appliedzkp/zkevm-specs/blob/master/specs/transactions-proof.md#circuit-behaviour-shortcut-1
Depends on:
New dependencies to the zkevm-circuits subcrate:
halo2wrong
subcrates. This is where the ECDSA verification chip is implemented. This dependency useshalo2
and this means that we'll require the version ofhalo2
used inhalo2wrong
andzkevm-circuits
to match.group
: Required for field and curve traitslibsecp256k1
: Requiered to perform the ECDSA public key recovery with access to the public key coordinates.rlp
: Required to calculate the RLP of the transaction to get the transaction hash (to sign)num-bigint
: Used to hold an integer bigger than the field, to latter apply mod Fq (this is required for the message hash in the ECDSA signature operation)subtle
: Used to mapCtOption
toResult
The added tests for tx_circuit use
k=19
, which use a lot of memory even using the MockProver. Rust tests run in parallel by default, which may make the tests run out of memory when the tx_circuit tests are run concurrently with other tests. For this reason I've split the tests into "light" (which are all the test declared previously), and "heavy" which are marked with ignore and start with "serial_" and are run in a different step in serial both in the github actions and Makefile.Notes on current implementation: