feat(state): bouncer DA count: use StateWriteKeys to count state diff…

[Yoni-Starkware]

… size

---

This change is 

[codecov-commenter]

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (8fe05a4) 69.04% compared to head (f210f56) 68.82%.
Report is 1 commits behind head on main-v0.13.1.

Files	Patch %	Lines
...ates/native_blockifier/src/transaction_executor.rs	12.50%	21 Missing ⚠️
crates/blockifier/src/state/cached_state.rs	92.59%	2 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           main-v0.13.1    #1462      +/-   ##
================================================
- Coverage         69.04%   68.82%   -0.22%     
================================================
  Files                58       58              
  Lines              7949     7952       +3     
  Branches           7949     7952       +3     
================================================
- Hits               5488     5473      -15     
- Misses             2005     2023      +18     
  Partials            456      456              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ArniStarkware]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elintul, @noaov1, and @Yoni-Starkware)

---

crates/native_blockifier/src/transaction_executor.rs line 121 at r1 (raw file):

                // Count residual state diff size (w.r.t. the OS output encoding).
                let tx_state_write_keys = transactional_state.cache.get_write_keys();
                let tx_unique_write_keys = tx_state_write_keys.difference(&self.state_write_keys);

Isn't it still wrong here as it was in #1438? If it is relevant - can you explain the issue? write a TODO?

Code quote:

let tx_unique_write_keys = tx_state_write_keys.difference(&self.state_write_keys);

[elintul]

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)

---

crates/native_blockifier/src/transaction_executor.rs line 91 at r1 (raw file):

            Ok(tx_execution_info) => {
                // Prepare bouncer info; the countings here should be linear in the transactional
                // state and execution info rather than the cumulative state attributes.

If my suggestion isn't true, what does "linear in transactional state" mean?

Suggestion:

                // Prepare bouncer info; the countings here should be linear in the transactional
                // state changes and execution info rather than the cumulative state attributes.

[Yoni-Starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @noaov1)

---

crates/native_blockifier/src/transaction_executor.rs line 121 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Isn't it still wrong here as it was in #1438? If it is relevant - can you explain the issue? write a TODO?

No, I'm subtracting keys.

Consider this:

tx0_changes: {0: 100}
tx1_changes: {0: 200}

// Not good. The overall state diff size does not increase.
len(tx1 - tx0) == 1
//Good.
len(tx1.keys() - tx0.keys()) == 0 

[elintul]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @noaov1)

[ArniStarkware]

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @noaov1)

[elintul]

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

---

crates/native_blockifier/src/transaction_executor.rs line 37 at r3 (raw file):

    // This member should (and must) be consistent with the state's write keys.
    // Kept in separate since the cache's members are private.
    pub state_write_keys: StateWriteKeys,

To keep the invariant you're describing.

Suggestion:

    // This member should (and must) be consistent with the state's write keys.
    // Kept in separate since the cache's members are private.
    state_write_keys: StateWriteKeys,

---

crates/native_blockifier/src/transaction_executor.rs line 120 at r3 (raw file):

                // Count residual state diff size (w.r.t. the OS output encoding).
                let tx_state_write_keys = transactional_state.cache.get_write_keys();

You can implement get_write_keys in an impl block of TransactioalState and avoid changing cache's visibility.

Code quote:

transactional_state.cache.get_write_keys()

The base branch was changed.

[ArniStarkware]

Reviewed 1 of 6 files at r5, 19 of 19 files at r6.
Reviewable status: 20 of 22 files reviewed, 2 unresolved discussions (waiting on @elintul, @noaov1, and @Yoni-Starkware)

[elintul]

Reviewed 3 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)

[elintul]

Reviewed 19 of 19 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)

[Yoni-Starkware]

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

---

crates/native_blockifier/src/transaction_executor.rs line 120 at r3 (raw file):

Previously, elintul (Elin) wrote…

You can implement get_write_keys in an impl block of TransactioalState and avoid changing cache's visibility.

Done.

[elintul]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @Yoni-Starkware)

---

crates/blockifier/src/state/cached_state.rs line 345 at r7 (raw file):

}

impl<S: StateReader> CachedState<S> {

I think it's better to impl this method for TransactionalState, since this is a transactional semantic.

Code quote:

impl<S: StateReader> CachedState<S>

[elintul]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[elintul]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @Yoni-Starkware)

[elintul]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @Yoni-Starkware)

---

crates/native_blockifier/src/transaction_executor.rs line 57 at r9 (raw file):

            // Note: the state might not be empty even at this point; it is the creator's
            // responsibility to tune the bouncer according to pre and post block process.
            state_write_keys: StateWriteKeys::default(),

I wanted to suggest that, too. :)
Then you don't take into account the fact that state might not be empty?

Code quote:

StateWriteKeys::default()

[Yoni-Starkware]

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

---

crates/native_blockifier/src/transaction_executor.rs line 57 at r9 (raw file):

Previously, elintul (Elin) wrote…

I wanted to suggest that, too. :)
Then you don't take into account the fact that state might not be empty?

Yes but it's not too bad since I can only count (constant amount of) extra changes, which is fine

[elintul]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[ArniStarkware]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[elintul]

Reviewed 1 of 3 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)

---

crates/blockifier/src/state/cached_state.rs line 782 at r11 (raw file):

    pub fn into_keys(self) -> StateChangesKeys {
        let modified_contracts = self.get_modified_contracts();

Pass directly.

Code quote:

self.get_modified_contracts()

---

crates/native_blockifier/src/transaction_executor.rs line 123 at r11 (raw file):

                    transactional_state.get_actual_state_changes()?.into_keys();
                let tx_state_changes_keys =
                    tx_state_changes_keys.difference(&self.state_changes_keys);

Consider moving into a small method.

Code quote:

                let tx_state_changes_keys =
                    transactional_state.get_actual_state_changes()?.into_keys();
                let tx_state_changes_keys =
                    tx_state_changes_keys.difference(&self.state_changes_keys);

[elintul]

Reviewed 2 of 3 files at r12, 2 of 2 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[elintul]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @Yoni-Starkware)

a discussion (no related file):
Python PR, please.

---

[elintul]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)