Add nonce count and consider it in the modified contracts.

[noaov1]

This change is 

[giladchase]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @elintul and @noaov1)

---

crates/blockifier/src/transaction/transactions_test.rs line 335 at r1 (raw file):

        actual_fee: expected_actual_fee,
        actual_resources: ResourcesMapping(HashMap::from([
            (abi_constants::GAS_USAGE.to_string(), 2448),

Any good way of verifying that this new gas number makes sense?

[noaov1]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @elintul and @giladchase)

---

crates/blockifier/src/transaction/transactions_test.rs line 335 at r1 (raw file):

Previously, giladchase wrote…

Any good way of verifying that this new gas number makes sense?

Great question. Adding the nonce_counter to the modified contracts added one more contract that was modified (the account contract).
This results in:

Code snippet:

n_modified_contracts +=1
-> onchain_data_segment_length +=2 (for each modified contract we write: contract address, number of modified storage cells)
-> sharp_gas_usage +=2 *612 (SHARP_GAS_PER_MEMORY_WORD)
-> l1_gas_usage += 1224 (as desired)

[elintul]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @giladchase and @noaov1)

---

crates/blockifier/src/state/cached_state.rs line 52 at r1 (raw file):

    /// Returns the number of storage changes done through this state.
    /// Any change to the contract's state (storage, nonce, class hash) is considered.
    pub fn count_actual_state_changes(&self) -> (usize, usize, usize, usize) {

Perhaps it's time for a struct?
Only if it's more readable.

Code quote:

(usize, usize, usize, usize)

---

crates/blockifier/src/state/cached_state.rs line 55 at r1 (raw file):

        // Storage Update.
        let storage_updates = &self.cache.get_storage_updates();
        let mut modified_contracts: HashSet<ContractAddress> =

Add a short comment (maybe in the docstring) that for each contract instance (address) we have three attributes: (class hash, nonce, storage root); the state updates correspond to them.

Code quote:

modified_contracts

---

crates/blockifier/src/transaction/transaction_utils.rs line 46 at r1 (raw file):

    l1_handler_payload_size: Option<usize>,
) -> TransactionExecutionResult<ResourcesMapping> {
    let (n_storage_changes, n_modified_contracts, n_class_updates, _n_nonce_updates) =

Will be used next PR?

Code quote:

_n_nonce_updates

[noaov1]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul and @giladchase)

---

crates/blockifier/src/state/cached_state.rs line 52 at r1 (raw file):

Previously, elintul (Elin) wrote…

Perhaps it's time for a struct?
Only if it's more readable.

Done.

---

crates/blockifier/src/transaction/transaction_utils.rs line 46 at r1 (raw file):

Previously, elintul (Elin) wrote…

Will be used next PR?

Actually no. Currently, we don't plan to use it as it is meaningless in DA. Yoni said to include it in the return value for symmetry.

[elintul]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @giladchase and @noaov1)

---

crates/blockifier/src/state/cached_state.rs line 46 at r2 (raw file):

    /// Returns the number of storage changes done through this state.
    /// For each contract instance (address) we have three attributes: `(class hash, nonce, storage
    /// root)`; the state updates correspond to them.

Suggestion:

    /// For each contract instance (address) we have three attributes: (class hash, nonce, storage
    /// root); the state updates correspond to them.

---

crates/blockifier/src/state/cached_state.rs line 53 at r2 (raw file):

        let storage_updates = &self.cache.get_storage_updates();

        let mut modified_contracts: HashSet<ContractAddress> =

Remove blank line above.

Code quote:

let mut modified_contracts: HashSet<ContractAddress> =

---

crates/blockifier/src/state/cached_state.rs line 77 at r2 (raw file):

    /// Same for class hash and nonce writes.
    // TODO(Noa, 30/07/23): Consider adding DB getters in bulk (via a DB read transaction).
    fn update_initial_values_of_write_only_access(&mut self) -> StateResult<()> {

Can you please move this to another PR?

Code quote:

update_initial_values_of_write_only_access

---

crates/blockifier/src/transaction/transaction_utils.rs line 46 at r1 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Actually no. Currently, we don't plan to use it as it is meaningless in DA. Yoni said to include it in the return value for symmetry.

Return value of what?

[noaov1]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @elintul and @giladchase)

---

crates/blockifier/src/state/cached_state.rs line 77 at r2 (raw file):

Previously, elintul (Elin) wrote…

Can you please move this to another PR?

It was already merged :) (maybe you see it because I had conflicts)

---

crates/blockifier/src/transaction/transaction_utils.rs line 46 at r1 (raw file):

Previously, elintul (Elin) wrote…

Return value of what?

Of count_actual_state_changes.

[elintul]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @giladchase and @noaov1)

---

crates/blockifier/src/state/cached_state.rs line 77 at r2 (raw file):

Previously, noaov1 (Noa Oved) wrote…

It was already merged :) (maybe you see it because I had conflicts)

Okay, got it. Can you briefly explain this change then? Is it the 0 -> 5 -> 0 fix?

[noaov1]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul and @giladchase)

---

crates/blockifier/src/state/cached_state.rs line 77 at r2 (raw file):

Previously, elintul (Elin) wrote…

Okay, got it. Can you briefly explain this change then? Is it the 0 -> 5 -> 0 fix?

First, we deal with cases where we set a value before reading it (hence, we do not update the initial_value mapping). Second, the relevant cases are setting a value that has already been written before. See #648 .

[elintul]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase)