rw table: add committed_value to Rw::Account

[lispc]

both bus-mapping and evm circuit according to spec https://github.com/appliedzkp/zkevm-specs/blob/master/specs/tables.md

[z2trillion]

I'm planning to add an initial_value advice column (which will be the commited value at the beginning of the block for Rw::Account rows and the commited value at the start of the transaction for Rw::AccountStorage rows ) to the state circuit in this PR: #591. As part of that, I looked into adding a committed_value field to Rw::Account but decided not to, because that value is not needed for evm rw lookups.

In #591, because there's no MPT lookups, for Rw::Account rows, I can just assign row.value to initial_value where row is the first access to (address, field_tag). In the followup PR, I can a correct initial value based on the MPT lookups that will be based in to the StateCircuit constructor.

@lispc and @ed255, what do you think about this plan?

[ed255]

I'm planning to add an initial_value advice column (which will be the commited value at the beginning of the block for Rw::Account rows and the commited value at the start of the transaction for Rw::AccountStorage rows ) to the state circuit in this PR: #591. As part of that, I looked into adding a committed_value field to Rw::Account but decided not to, because that value is not needed for evm rw lookups.

Note that currently in the spec this column is called aux0. I think it makes sense to rename it to initial_value or committed_value.

About the committed_value not used in Account lookup; I think I don't follow. The committed_value column will exist for all targets (as they all are defined by a single rw_table). In all of the targets but AccountStorage and Account, this value will be 0. In Account this value may be non-zero, and it should be the same in all rows that share the same keys, so this value should appear in the lookup, right? From the EVM side there will be no constraint on this value, but it must appear as a witness to build the lookup expression AFAIK.

On the other hand, the types RW::AccountStorage and Rw::Account maybe don't need this field, because it can be easily inferred (at assignment time) from the first operation as you describe below.

In #591, because there's no MPT lookups, for Rw::Account rows, I can just assign row.value to initial_value where row is the first access to (address, field_tag). In the followup PR, I can a correct initial value based on the MPT lookups that will be based in to the StateCircuit constructor.

Isn't the way you propose already the correct initial value? The State circuit already constraints that the committed_value on the first access for a given set of keys is equal to the row.value (the previous value). And that's the value that must match the MPT lookup.

[z2trillion]

Note that currently in the spec this column is called aux0. I think it makes sense to rename it to initial_value or committed_value.

I'm planning to go with initial_value in #591.

About the committed_value not used in Account lookup; I think I don't follow. The committed_value column will exist for all targets (as they all are defined by a single rw_table). (...) In Account this value may be non-zero, and it should be the same in all rows that share the same keys, so this value should appear in the lookup, right? From the EVM side there will be no constraint on this value, but it must appear as a witness to build the lookup expression AFAIK.

Right now, the only witness information that is passed into the state circuit is rows: Vec<Rw>. Rather than append the mpt update information onto the existing Rw type, I'm planning to pass in another struct mpt_updates: Map<(address, field_tag) | (tx_id, address, storage_key), (opd_value, new_value, old_root, new_root)>. This way, the Rw type will map 1-1 to lookups the evm circuit does, and we won't have to pass around the initial_value in the AccountOps structs the bus mapping uses.

In all of the targets but AccountStorage and Account, this value will be 0.

This currently isn't the case (but I would like to make it so). CallContext and TxReceipt rows have initial values are can be non-zero, which is why I need to match on them during the assignment here: https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/591/files#diff-f13d8103faf1d1581fc8ab5061a9b71a6163cd6189af976ad024ea10c8b1f432R245. For more context see this discussion: #602 (comment)

Isn't the way you propose already the correct initial value? The State circuit already constraints that the committed_value on the first access for a given set of keys is equal to the row.value (the previous value). And that's the value that must match the MPT lookup.

The current logic for determining the assignment for initial_value here https://github.com/privacy-scaling-explorations/zkevm-circuits/pull/591/files#diff-f13d8103faf1d1581fc8ab5061a9b71a6163cd6189af976ad024ea10c8b1f432R185 is correct, but once I pass in mpt_updates when constructing the state circuit, the end assignement will still be the same, but the logic will be greatly simplified.

[aguzmant103]

@lispc , @z2trillion just checking, is this task still open or was it completed before?

[ed255]

Currently the state circuit has an initial_value column that I think fulfills the issue:

zkevm-circuits/zkevm-circuits/src/state_circuit.rs

Lines 52 to 55 in 878e5e6

	// Assigned value at the start of the block. For Rw::Account and
	// Rw::AccountStorage rows this is the committed value in the MPT, for
	// others, it is 0.
	initial_value: Column<Advice>,

So I believe this issue is complete. @z2trillion could you confirm this?

[ashWhiteHat]

I found comment related to this issue and said not necessary.

As part of that, I looked into adding a committed_value field to Rw::Account but decided not to, because that value is not needed for evm rw lookups.

#557 (comment)

[lispc]

agree with @z2trillion . seems don't need to add committed_value to Rw::Account since evm circuit does not need that.

[ashWhiteHat]

@lispc (CC: @z2trillion )
I pinged him and he said that he was investigating whether we really don't need.
He will comment or close this issue after investigation.
I am going to work on investigation too after state circuit synchronization.

[ashWhiteHat]

Hi @z2trillion
I think we already have the same concept column with committed_value in state circuit.
I close this issue.

Please reopen if we have something to discuss 😀