Update testool

[adria0]

Description

This PR updates the testool

Type of change

• New feature (non-breaking change which adds functionality)

Contents

• Upstream some of the changes did by scroll-tech team
• Removes the individual test selection that there are no longer useful in this stage (this feature was added initially for enabling/disabling some specific tests that that crashed)
• Updates tests to last version (includes codehash.txt update)

[adria0]

Depends on #1554

[ChihChengLiang]

Run cargo test -p testool statetest::yaml::test::bad_balance to reproduce the failure.

I tried rebasing on #1585, but it didn't fix it. Need more investigation

[lispc]

@ed255 i think the check itself may not be right? i checked the failed case, it indeed first do a Rw { is_write: true, filed: CodeHash, prev_value: 0x0, value: keccak(nil) }, then Rw { is_write: true, filed: Balance, prev_value: 0x0, value: 0x0 }. I think this pattern is right, should not be rejected.

Btw i made a change for this. Here i distinguish on "empty account" and "empty account but been created(writen codehash) " https://github.com/scroll-tech/zkevm-circuits/pull/819/files

[ed255]

@ed255 i think the check itself may not be right? i checked the failed case, it indeed first do a Rw { is_write: true, filed: CodeHash, prev_value: 0x0, value: keccak(nil) }, then Rw { is_write: true, filed: Balance, prev_value: 0x0, value: 0x0 }. I think this pattern is right, should not be rejected.

I see an issue in this pattern. Currently the EndTx is doing the following in this case

1. Create the coinbase account (by setting CodeHash to empty_hash)
2. Transfer 0 ETH to coinbase

After this we end up with an account in the state that is empty, so technically it shouldn't exist. This account should not exists as a leaf in the MPT, because it's empty. I believe this can be solved in two ways:
a. Let the MPT circuit handle this case: whenever an update uses data of empty account, skip the proof
b. On the EVM circuits, make sure to never create an account that is empty.

I'm more favorable towards option b, because it's easy to add this logic to the EVM gadgets. For example take a look at how a transfer is handled in callop:

zkevm-circuits/zkevm-circuits/src/evm_circuit/util/common_gadget.rs

Line 583 in 77c16e4

TransferToGadget::creat_account(

If the account doesn't exist but the transfer value is 0, the account is not created. I think we should treat the coinbase transfer in EndTx the same way. Something like this:

if !coinbase_balance.is_zero() {
  if coinbase_account.is_empty() {
    account_write(coinbase, CodeHash, empty_hash, zero)
  }
  account_write(coinbase, Balance, coinbase_balance, coinbase_balance_prev)
}

What do you think about this @lispc ?

[lispc]

@ed255 i think the check itself may not be right? i checked the failed case, it indeed first do a Rw { is_write: true, filed: CodeHash, prev_value: 0x0, value: keccak(nil) }, then Rw { is_write: true, filed: Balance, prev_value: 0x0, value: 0x0 }. I think this pattern is right, should not be rejected.

I see an issue in this pattern. Currently the EndTx is doing the following in this case

1. Create the coinbase account (by setting CodeHash to empty_hash)
2. Transfer 0 ETH to coinbase

After this we end up with an account in the state that is empty, so technically it shouldn't exist. This account should not exists as a leaf in the MPT, because it's empty. I believe this can be solved in two ways: a. Let the MPT circuit handle this case: whenever an update uses data of empty account, skip the proof b. On the EVM circuits, make sure to never create an account that is empty.

I'm more favorable towards option b, because it's easy to add this logic to the EVM gadgets. For example take a look at how a transfer is handled in callop:

zkevm-circuits/zkevm-circuits/src/evm_circuit/util/common_gadget.rs

Line 583 in 77c16e4

TransferToGadget::creat_account(

If the account doesn't exist but the transfer value is 0, the account is not created. I think we should treat the coinbase transfer in EndTx the same way. Something like this:

if !coinbase_balance.is_zero() {
  if coinbase_account.is_empty() {
    account_write(coinbase, CodeHash, empty_hash, zero)
  }
  account_write(coinbase, Balance, coinbase_balance, coinbase_balance_prev)
}

What do you think about this @lispc ?

Totally agree. We will change this

[z2trillion]

There is already a value_is_zero gadget in the TransferToGadget: https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/main/zkevm-circuits/src/evm_circuit/util/common_gadget.rs#L345. We can use that to only update the code hash if the value > 0?

[lispc]

The "update branch" button disappeared？

[ChihChengLiang]

First round

[ChihChengLiang]

@lispc, I saw the update branch button again.
I think the update branch button disappeared because it was even with the main branch.
I also noticed that #1609 was merged against the branch of this PR but not the main branch.

[ChihChengLiang]

LGTM. (Shamelessly pat myself on the back)

I helped Adria to address the feedback because he couldn't reach his device recently due to personal reasons.

[ed255]

LGTM!