Handle accounts with non-zero code hashes in BeginTx

[z2trillion]

This occurs when an address already exists because it has a non-zero balance.

[ed255]

@z2trillion this PR has been draft for a while. The code only introduces a new test, does it still make sense to move forward with this?

[z2trillion]

8 months ago the test failed because the code hash for accounts in the process of being created wasn't being set the the correct value of keccak(nil). I think this got fixed now that we're using transfer_with_fee with must_create = call.is_create() https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/main/bus-mapping/src/evm/opcodes/begin_end_tx.rs#L111.

I can add a comment to the test explaining what it catches, or I can just close this PR.

[lispc]

must_create = call.is_create()

Oh this is incorrect in fact. I remember some time ago we discussed this issue in pse repo issues. And i later fixed it in scroll repo. Sometimes the address can already has some ether. So need to first read the codehash.. ignore this..

[z2trillion]

must_create = call.is_create()

Oh this is incorrect in fact. I remember some time ago we discussed this issue in pse repo issues. And i later fixed it in scroll repo. Sometimes the address can already has some ether. So need to first read the codehash.. ignore this..

This is actually still a problem. 771f58f adds a test that fails when the account that's being created already has a non-zero balance (and hence a non-zero code hash).

[ChihChengLiang]

LGTM with some minor feedback and notes.

[ChihChengLiang]

This comment is not a request for change but a note for other reviewers:

I wrapped my head around this expression and finally figured out a more straightforward way to convince myself the proposed change is correct.

• If the expression evaluates true, we create an account with an empty hash. Specifically, update the codehash from 0 to the EMPTY_CODE_HASH.
• In the previous expression, the must_create dominates the outcome. If the call is CREATE, then even if the receiver_exists, the receiver account is still created with the empty hash.
• In the proposed expression, the !receiver_exists dominates the outcome. If the receiver already exists, there's no way to mess up the account code hash again. This is the behavior that makes sense.

The failing case is captured in the test case create_tx_for_existing_account.

I've included my previous elaborate boolean analysis in the "Details".

Details

Let A = (!receiver_exists), B = !value.is_zero(), and C = must_create,

• before: (A && B) || C
• after: A && (B || C)

The two expressions disagree on the following cases

• A = False, B = False, C = True
• A = False, B = True, C = True

In both cases, the receiver exists, and the call is CREATE.

If the value is zero, the expression before still creates the account with the empty codehash, but the proposed one doesn't.

If the value is NOT zero, the expression before still creates the account with the empty codehash, but the proposed one doesn't.

A	B	C	Output
0	0	0	F
0	0	1	T
0	1	0	F
0	1	1	T
1	0	0	F
1	0	1	T
1	1	0	T
1	1	1	T

A	B	C	Output
0	0	0	F
0	0	1	F
0	1	0	F
0	1	1	F
1	0	0	F
1	0	1	T
1	1	0	T
1	1	1	T

[ed255]

LGTM! I've left a few minor questions, and I think following CC's request to add some comments #1079 (comment) would be really nice.