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

Precompile ECRECOVER #529

Merged
merged 15 commits into from
Jul 4, 2023
Merged

Precompile ECRECOVER #529

merged 15 commits into from
Jul 4, 2023

Conversation

roynalnaruto
Copy link

@roynalnaruto roynalnaruto commented Jun 8, 2023

Description

Add support for verification of the precompile ecRecover.

Issue Link

#527

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Contents

The PR aims at supporting verification of ecRecover precompile call.

The PR can be summarised by the following parts:

Copy Circuit Updates There are 3 sets of memory operations associated with a precompile call, namely, memory reads from the `caller` to the precompile `call`, memory writes from the precompile call result to the precompile `call` context and lastly, memory writes from the precompile call result to the original `caller` context.

We introduce a new CopyDataType for the target precompile, which is the following variant: CopyDataType::Precompile(PrecompileCalls) where PrecompileCalls is another enum that covers various precompile call types.

The bytes copied to/from precompile target must support the accumulator check.

Since the CopyDataType now has 15 variants, we need an additional bit for the BinaryNumberChip inside copy circuit. Consequently, this increases the degree of the copy circuit and in order to bring the degree back to 9, the following changes were had to be made:

  • Introduce the following columns
    /// Indicates whether or not the copy event copies bytes to a precompiled call or copies bytes
    /// from a precompiled call back to caller.
    pub is_precompiled: Column<Advice>,
    /// Booleans to indicate what copy data type exists at the current row.
    pub is_tx_calldata: Column<Advice>,
    /// Booleans to indicate what copy data type exists at the current row.
    pub is_bytecode: Column<Advice>,
    /// Booleans to indicate what copy data type exists at the current row.
    pub is_memory: Column<Advice>,
  • Constrain the boolean value assigned to the above columns
    vec![
    enabled.expr() * (is_precompile - precompiles),
    enabled.expr()
    * (is_tx_calldata
    - tag.value_equals(CopyDataType::TxCalldata, Rotation::cur())(meta)),
    enabled.expr()
    * (is_bytecode
    - tag.value_equals(CopyDataType::Bytecode, Rotation::cur())(meta)),
    enabled.expr()
    * (is_memory - tag.value_equals(CopyDataType::Memory, Rotation::cur())(meta)),
    ]
Bus Mapping Updates The memory RWs mentioned above are handled by the [`CallOp`](https://github.com/scroll-tech/zkevm-circuits/blob/752f4f4af416c5323e7294f83b360067012a5cdb/bus-mapping/src/evm/opcodes/callop.rs#L254) opcode.

The ecRecover arguments ([v, r, s, msg_hash, recovered_addr]) must be pushed to the CircuitInputBuilder's block, so it can later be used as witness data to populate the SignVerifyTable. The SignVerifyTable is a work-in-progress PR and this PR depends on it.

ecRecover Gadget The difference between signature verification (for tx signature) and elliptic-curve signer recovery is that the signature is not verified in the case of ecRecover. For more details refer "4.1.4 (sig verify)" and "4.1.6 (recovery)" in https://www.secg.org/sec1-v2.pdf.

So in the lookup to the SigVerifyTable, ecRecover does not care about the is_valid column. An example is this playground setup in https://evm.codes. Setting the calldata_length to 0x65 (an arbitrary value between 0x60 and 0x80) does recover a signer address without verifying the signature (the signature in this case would have been invalid).

The ecRecover gadget is responsible for verifying the internal execution state PrecompileEcrecover. The gadget also has sig_v, sig_r, sig_s, msg_hash, recovered_addr and recovered cells to verify the input and output from the precompile call (this verification is done in the util::PrecompileGadget).

Precompile Gadget Updates Add a conditional constraint for `input_rlc` and `output_rlc` (input and output of the precompile call). The `output_rlc` is `0` for the case where no address could be recovered, so the failure case is covered as well. If the address was recovered, then the output RLC is constrained to be equal to the recovered address.

How Has This Been Tested?

@roynalnaruto roynalnaruto changed the title Feat/ecrecover Precompile ECRECOVER Jun 8, 2023
@lispc
Copy link

lispc commented Jun 15, 2023

we can merge #538 into this branch so SigTable::dev_load can be tested.

After #538 merge, we can merge this.

@lispc
Copy link

lispc commented Jun 22, 2023

it is time to add the sig lookup? @roynalnaruto

@roynalnaruto
Copy link
Author

it is time to add the sig lookup? @roynalnaruto

Yes, I will add it now.

roynalnaruto and others added 3 commits June 28, 2023 18:11
* potential approach (pow of rand lookup)

* add lookup for pow of rand

* fix: right pad only if needed

* fix: missing constraint on padded_rlc

* constrain pow of rand table
@roynalnaruto roynalnaruto marked this pull request as ready for review July 4, 2023 10:00
@lispc
Copy link

lispc commented Jul 4, 2023

\b run testool

@zkevm-circuits-bots
Copy link

@lispc
Copy link

lispc commented Jul 4, 2023

should have a look at the new failures in testool report:

Panic(attempt to subtract with overflow)
Panic(assertion failed: n_padded_zeroes < 128)

@roynalnaruto
Copy link
Author

\b run testool

@zkevm-circuits-bots
Copy link

@lispc
Copy link

lispc commented Jul 4, 2023

i noticed the ci takes more time than before (even the "default" mode ci fail to run..).

retrying

Copy link

@lispc lispc left a comment

Choose a reason for hiding this comment

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

you can merge it when the above 3 comments addressed

@roynalnaruto roynalnaruto merged commit 4cd5956 into develop Jul 4, 2023
16 checks passed
@roynalnaruto roynalnaruto deleted the feat/ecrecover branch July 4, 2023 14:45
github-merge-queue bot pushed a commit to privacy-scaling-explorations/zkevm-circuits that referenced this pull request Feb 19, 2024
closed #1665 

This PR was ported from 
- scroll-tech#529
- scroll-tech#930

which manly includes,
1. the main logic in ecrecover gadget (`ecrecover.rs`)
2. signature verifcation circuiit (`sig_circuit.rs`). What I did is to
change rlc to word lo/hi.
3. a new table, `sig_table.rs`
4. ecc circuit (`ecc_circuit.rs`). It's not be used in `ecRecover`, but
it was implemented in Scroll's PR. If I removed ecc_circuit, it would be
inconvienent for people porting `ecAdd`, `ecMul` or `ecPairing`. That's
why I keep it here.
5. dependencies update, using `halo2lib` (includes `halo2-base` and
`halo2-ecc`)

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Co-authored-by: Zhang Zhuo <mycinbrin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants