Unify circuit types names and interfaces

[ed255]

Given a circuit Foo:

• Unify the struct that contains the columns to FooCircuitConfig
• Unify the struct that contains the witness to FooCircuit
• Unify the config constructor to FooCircuitConfig::new, following the new SubCircuitConfig trait
• Unify the creation of a circuit from a block to FooCircuit::new_from_block following the new SubCircuit trait
• Unify the loading of fixed internal tables to load_aux_tables
• Unify the synthesis step of subcircuits to synthesize_sub following the new SubCircuit trait
• Remove generic associated const from circuit+config types as much as possible - Only possible in TxCircuit. Not possible in PiCircuit due to halo2 Circuit trait
• Implement the Circuit trait to all subcircuits uniformly
• Move circuit tunning parameters to CircuitParams so that they can be taken from a single source
• Add an instance function to all circuits following the new SubCircuit trait

[ed255]

Currently some testool unit tests are failing with the error EthTypeError(Signature(InvalidSignature)). I've added the calculation of all the keccak inputs in the witness::block_convert (so that all circuits can consume the witness from a single source), which requires processing the transactions signatures. I believe the failing tests in the testool have invalid signatures (which were not processed before), and that's why they are failing.

The rest of the tests should be passing.

Please if you have some time, take a look at the changes made and comment if you agree with this proposal or open a discussion to change! Basically I've added this new traits and implemented them for all subcircuits:

/// SubCircuit is a circuit that performs the verification of a specific part of
/// the full Ethereum block verification.  The SubCircuit's interact with each
/// other via lookup tables and/or shared public inputs.  This type must contain
/// all the inputs required to synthesize this circuit (and the contained
/// table(s) if any).
pub trait SubCircuit<F: Field> {
    /// Configuration of the SubCircuit.
    type Config: SubCircuitConfig<F>;

    /// Create a new SubCircuit from a witness Block
    fn new_from_block(block: &witness::Block<F>) -> Self;

    /// Returns the instance columns required for this circuit.
    fn instance(&self) -> Vec<Vec<F>> {
        vec![]
    }
    /// Assign only the columns used by this sub-circuit.  This includes the
    /// columns that belong to the exposed lookup table contained within, if
    /// any; and excludes external tables that this sub-circuit does lookups
    /// to.
    fn synthesize_sub(
        &self,
        config: &Self::Config,
        challenges: &Challenges<Value<F>>,
        layouter: &mut impl Layouter<F>,
    ) -> Result<(), Error>;
}

/// SubCircuit configuration
pub trait SubCircuitConfig<F: Field> {
    /// Config constructor arguments
    type ConfigArgs;

    /// Type constructor
    fn new(meta: &mut ConstraintSystem<F>, args: Self::ConfigArgs) -> Self;
}

[pinkiebell]

Looks great so far 💪 . This will simplify a lot and I hope we can get rid of const generics in the future too 😁

[pinkiebell]

Currently some testool unit tests are failing with the error EthTypeError(Signature(InvalidSignature))

Yes, I think the ethereum tests suite include invalid signatures too. So we must handle those somehow

[adria0]

It seems a very good interface @ed255 !

Let's me try to check what's happening with the testool checks

[ed255]

Let's me try to check what's happening with the testool checks

I just found it! The signatures of the transactions were not being propagated to the transaction types passed to the rest of the code.

[pinkiebell]

Let's me try to check what's happening with the testool checks

I just found it! The signatures of the transactions were not being propagated to the transaction types passed to the rest of the code.

Great! Does that mean we don't test/support invalid signatures yet?

[ed255]

Great! Does that mean we don't test/support invalid signatures yet?

The circuit that checks the transaction signatures is the TxCircuit. Currently it doesn't support invalid signatures. For proof of validity, invalid signatures support should not be required; but for a rollup we may require it. As an extra note, the signature verification uses the EcdsaChip from halow2rong, which currently doesn't support invalid signatures.

So strictly speaking, only tests that contain the TxCircuit need signed signatures; for the rest of the tests (other circuits) the txs signatures are not needed. Nevertheless in this PR I added the calculation of all keccak inputs required to proof a full block as part of the circuit's input block preparation. And part of the keccak inputs involves recovering the sender public key from the signature transaction (this is because the Tx Circuit proves that the signature of the tx was signed with a public key, that when hashed gives the sender address). And this is the step that was giving errors.

For the testool this was easy to solve, because the transactions are signed, but in a conversion step the signature was not being propagated.

For the unit tests this is a bit more complicated, because we have a block builder used to generate test inputs where we only specify the tx sender as an address (which means that the block builder doesn't know the private key and txs are not signed). I think in the future we could update the block builder to work with wallets, so that all generated transactions are signed; but that's a significant refactor. So for now I decided to skip generating the keccak inputs for the tx circuit for any tx that is not signed (and do a warn about it).

EDIT: Actually the block builder can be used to generate txs that can later be signed, but it requires generating wallets for sender accounts like this

zkevm-circuits/zkevm-circuits/src/super_circuit.rs

Line 474 in 43da558

let wallet_a = LocalWallet::new(&mut rng).with_chain_id(chain_id);

and doing an extra step to sign (whereas in many unit tests we're creating the block with a single call to TestContext::<_, _>::new

[adria0]

Let's me try to check what's happening with the testool checks

I just found it! The signatures of the transactions were not being propagated to the transaction types passed to the rest of the code.

Oh, I just found it now also, hehe

[pinkiebell]

@ed255 Thanks that clears it up. Regarding the rollup, we can either recover the signatures or just do a cheaper pre-validation so that the ecdsa chip can recover a signature

[han0110]

LGTM! It's so clear now without those const generic in sub-circuits.

I think it'd be even better to move all impl Circuit for *Circuit into mod test, since they should never be used independently, then we never accidentally call *Circuit::synthesize (currently there are ExpCircuit, StateCircuit, TxCircuit, BytecodeCircuit and KeccakCircuit implemented not in mod test).

Edit: After chatted with Edu, since currently even zkevm-chain is using zkevm-circuits with feature test, it seems worth nothing to do this now, so this is moved to issue #953).

[ed255]

@ed255 Thanks that clears it up. Regarding the rollup, we can either recover the signatures or just do a cheaper pre-validation so that the ecdsa chip can recover a signature

Ah, sorry I didn't expand too much on the rollup story: In a rollup we may have a mechanism to include "forced" transactions to avoid censorship. That would be a transaction that is sent to L1 and the node is forced to include in a block. Since these transactions wouldn't be validated, they could contain an invalid signature that makes it impossible to recover the public key; but they must still be processed in the circuits. So the EcdsaChip would need to be extended to prove that a tx signature is invalid (currently the constraints won't pass with an invalid signature).

[kunxian-xia]

@ed255 Thanks that clears it up. Regarding the rollup, we can either recover the signatures or just do a cheaper pre-validation so that the ecdsa chip can recover a signature

Ah, sorry I didn't expand too much on the rollup story: In a rollup we may have a mechanism to include "forced" transactions to avoid censorship. That would be a transaction that is sent to L1 and the node is forced to include in a block. Since these transactions wouldn't be validated, they could contain an invalid signature that makes it impossible to recover the public key; but they must still be processed in the circuits. So the EcdsaChip would need to be extended to prove that a tx signature is invalid (currently the constraints won't pass with an invalid signature).

Why is that the L1 "forced" transaction will include an invalid signature? Do you mean that the L1 "forced" message might be triggered by a smart contract? @ed255

[ed255]

Why is that the L1 "forced" transaction will include an invalid signature? Do you mean that the L1 "forced" message might be triggered by a smart contract? @ed255

Let me update on this. My thought was that an "L1" forced transaction would be sent by any L1 address (that may not be the same as the forcet_tx.from), and the L1 contract would not do any kind of validation, just keep it to be included in a future L2 block. This forced_tx would have a unverified signature, so the zkEVM would need to support having txs with invalid signatures that must be skipped.

But after a quick discussion with Haichen (scroll) and Brecth (taiko) I learned that this won't be necessary; because in both zkRollup projects they plan for the L1 contract to perform the signature verification and reject the forced_tx if the signature is invalid. So we won't have the case of encountering a tx with invalid signature included in a block.