build(blockifier_reexecution): add dummy consecutive state readers

[aner-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.84%. Comparing base (e3165c4) to head (171c75f).
Report is 162 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1703      +/-   ##
==========================================
+ Coverage   40.10%   41.84%   +1.74%     
==========================================
  Files          26      196     +170     
  Lines        1895    23139   +21244     
  Branches     1895    23139   +21244     
==========================================
+ Hits          760     9683    +8923     
- Misses       1100    13000   +11900     
- Partials       35      456     +421     

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

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 289 at r1 (raw file):

}

pub type StarknetContractClassMapping = HashMap<ClassHash, StarknetContractClass>;

docstring with why this type is needed?

Code quote:

pub type StarknetContractClassMapping = HashMap<ClassHash, StarknetContractClass>;

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 291 at r1 (raw file):

pub type StarknetContractClassMapping = HashMap<ClassHash, StarknetContractClass>;

pub struct DummyStateReader(StateMaps, StarknetContractClassMapping);

1. named values, not a tuple struct
2. please rename, don't use the word "dummy" - it's not a mock object, it is a real object. maybe Offline or Local instead of Dummy?
3. why is StarknetContractClassMapping needed as an intermediate type? just for ease of use?

Suggestion:

pub struct DummyStateReader {
    contract_classes: HashMap<ClassHash, StarknetContractClass>,
    state_maps: StateMaps
}

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 291 at r1 (raw file):

pub type StarknetContractClassMapping = HashMap<ClassHash, StarknetContractClass>;

pub struct DummyStateReader(StateMaps, StarknetContractClassMapping);

I prefer you separate between the struct containing all the data required,
and the state reader that uses the data, like we said.
that way you won't need a new function; later you can implement a from_json method when you want to read the data

// no "new" required, fields are public
pub struct ReexecutionData { pub s: StateMaps, pub c: StarknetContractClassMapping };
// no "new" required; "from_json" optional
pub struct OfflineStateReader(pub ReexecutionData);

WDYT?
non-blocking

Code quote:

pub struct DummyStateReader(StateMaps, StarknetContractClassMapping);

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 302 at r1 (raw file):

            Some(val) => Ok(*val),
            None => Err(StateError::StateReadError("Missing Value.".to_string())),
        }

should be equivalent
if you need to return T and this returns &T, you can try like this:

        Ok(*self.0.storage.get(&(contract_address, key)).ok_or(
            StateError::StateReadError("Missing Value.".to_string())
        )?)

same comment for get_nonce_at, get_class_hash_at and get_compiled_class_hash

Suggestion:

        self.0.storage.get(&(contract_address, key)).ok_or(
            StateError::StateReadError("Missing Value.".to_string())
        )

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 334 at r1 (raw file):

            None => Err(StateError::StateReadError("Missing Value.".to_string())),
        }
    }

each time you return Missing value please also stringify the key(s) that are missing

Code quote:

        match self.0.storage.get(&(contract_address, key)) {
            Some(val) => Ok(*val),
            None => Err(StateError::StateReadError("Missing Value.".to_string())),
        }
    }

    fn get_nonce_at(&self, contract_address: ContractAddress) -> StateResult<Nonce> {
        match self.0.nonces.get(&contract_address) {
            Some(val) => Ok(*val),
            None => Err(StateError::StateReadError("Missing Value.".to_string())),
        }
    }

    fn get_class_hash_at(&self, contract_address: ContractAddress) -> StateResult<ClassHash> {
        match self.0.class_hashes.get(&contract_address) {
            Some(val) => Ok(*val),
            None => Err(StateError::StateReadError("Missing Value.".to_string())),
        }
    }

    fn get_compiled_contract_class(
        &self,
        class_hash: ClassHash,
    ) -> StateResult<BlockifierContractClass> {
        match self.get_contract_class(&class_hash)? {
            Sierra(sierra) => sierra_to_contact_class_v1(sierra),
            Legacy(legacy) => legacy_to_contract_class_v0(legacy),
        }
    }

    fn get_compiled_class_hash(&self, class_hash: ClassHash) -> StateResult<CompiledClassHash> {
        match self.0.compiled_class_hashes.get(&class_hash) {
            Some(val) => Ok(*val),
            None => Err(StateError::StateReadError("Missing Value.".to_string())),
        }
    }

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 362 at r1 (raw file):

            None => Err(StateError::StateReadError("Missing Value.".to_string())),
        }
    }

aren't these methods required in the rpc state reader as well?
will they be in Aviv's trait?

Code quote:

    pub fn get_transaction_executor(
        self,
        block_context_next_block: BlockContext,
        transaction_executor_config: Option<TransactionExecutorConfig>,
    ) -> ReexecutionResult<TransactionExecutor<DummyStateReader>> {
        Ok(TransactionExecutor::<DummyStateReader>::new(
            CachedState::new(self),
            block_context_next_block,
            transaction_executor_config.unwrap_or_default(),
        ))
    }

    pub fn get_contract_class(&self, class_hash: &ClassHash) -> StateResult<StarknetContractClass> {
        match self.1.get(class_hash) {
            Some(contract_class) => Ok(contract_class.clone()),
            None => Err(StateError::StateReadError("Missing Value.".to_string())),
        }
    }

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 366 at r1 (raw file):

pub struct OfflineConsecutiveStateReaders {
    pub dummy_state_reader: DummyStateReader,

state reader for which block?
I like how you have next in the other fields, add next or prev to this var name

Code quote:

dummy_state_reader: DummyStateReader,

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 377 at r1 (raw file):

        // TODO(Aner): Create a struct containing the following parameters.
        state_maps: StateMaps,
        contract_class_mapping: StarknetContractClassMapping,

see above... can be a single arg already in this PR

Code quote:

        // TODO(Aner): Create a struct containing the following parameters.
        state_maps: StateMaps,
        contract_class_mapping: StarknetContractClassMapping,

[aner-starkware]

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 291 at r1 (raw file):

Previously, dorimedini-starkware wrote…

1. named values, not a tuple struct
2. please rename, don't use the word "dummy" - it's not a mock object, it is a real object. maybe Offline or Local instead of Dummy?
3. why is StarknetContractClassMapping needed as an intermediate type? just for ease of use?

Regarding 3, yes, for ease of use, it's used in several places, so it makes it more convenient.

[aner-starkware]

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 291 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I prefer you separate between the struct containing all the data required,
and the state reader that uses the data, like we said.
that way you won't need a new function; later you can implement a from_json method when you want to read the data

// no "new" required, fields are public
pub struct ReexecutionData { pub s: StateMaps, pub c: StarknetContractClassMapping };
// no "new" required; "from_json" optional
pub struct OfflineStateReader(pub ReexecutionData);

WDYT?
non-blocking

This is an "inner" struct, the struct that should read from JSONs is the OfflineConsecutiveStateReaders

[dorimedini-starkware]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 291 at r1 (raw file):

Previously, aner-starkware wrote…

This is an "inner" struct, the struct that should read from JSONs is the OfflineConsecutiveStateReaders

ah... good point.
ok, just use named fields

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 291 at r1 (raw file):

Previously, aner-starkware wrote…

Regarding 3, yes, for ease of use, it's used in several places, so it makes it more convenient.

ok, sgtm.
1+2 still relevant

[aner-starkware]

Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions (waiting on @AvivYossef-starkware and @dorimedini-starkware)

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 289 at r1 (raw file):

Previously, dorimedini-starkware wrote…

docstring with why this type is needed?

It's just for convenience... Is a docstring really necessary?

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 291 at r1 (raw file):

Previously, dorimedini-starkware wrote…

ok, sgtm.
1+2 still relevant

1. Done.
2. It is a dummy, it mostly replies as in its storage... (except the functions that compile, but those are code duplications that should be moved into the trait IMO)

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 302 at r1 (raw file):

Previously, dorimedini-starkware wrote…

should be equivalent
if you need to return T and this returns &T, you can try like this:

        Ok(*self.0.storage.get(&(contract_address, key)).ok_or(
            StateError::StateReadError("Missing Value.".to_string())
        )?)

same comment for get_nonce_at, get_class_hash_at and get_compiled_class_hash

Done.

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 334 at r1 (raw file):

Previously, dorimedini-starkware wrote…

each time you return Missing value please also stringify the key(s) that are missing

Done.

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 362 at r1 (raw file):

Previously, dorimedini-starkware wrote…

aren't these methods required in the rpc state reader as well?
will they be in Aviv's trait?

Yes, they should be (or at least the second one).

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 366 at r1 (raw file):

Previously, dorimedini-starkware wrote…

state reader for which block?
I like how you have next in the other fields, add next or prev to this var name

Done.

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 377 at r1 (raw file):

Previously, dorimedini-starkware wrote…

see above... can be a single arg already in this PR

Done.

[aner-starkware]

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 362 at r1 (raw file):

Previously, aner-starkware wrote…

Yes, they should be (or at least the second one).

The second one does exist in the trait, but it has the wrong signature. I guess that's why it's also not implemented in TestStateReader in the correct place.

[dorimedini-starkware]

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @AvivYossef-starkware)

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 362 at r1 (raw file):

Previously, aner-starkware wrote…

It's in this PR: https://reviewable.io/reviews/starkware-libs/sequencer/1736

rebase and implement the function as part of the trait implementation?

[aner-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @dorimedini-starkware)

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 362 at r1 (raw file):

Previously, dorimedini-starkware wrote…

rebase and implement the function as part of the trait implementation?

Done.

[dorimedini-starkware]

Reviewed 1 of 2 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware and @AvivYossef-starkware)

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 291 at r1 (raw file):

It is a dummy

I don't think dummy is a well-defined term. If you mean it like "trivial", please rename to Trivial, but I think Local or Offline is much better because it indicates why this object is useful (in the context of reexecution, we have "RPC" and "X" - makes sense that "X" is "Offline").

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 362 at r1 (raw file):

Previously, aner-starkware wrote…

The second one does exist in the trait, but it has the wrong signature. I guess that's why it's also not implemented in TestStateReader in the correct place.

is it already merged?
can you fix it in this PR? (fix the signature on the trait, and implement this method as part of the trait impl on the DummyStateReader)

[aner-starkware]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware and @dorimedini-starkware)

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 291 at r1 (raw file):

Previously, dorimedini-starkware wrote…

It is a dummy

I don't think dummy is a well-defined term. If you mean it like "trivial", please rename to Trivial, but I think Local or Offline is much better because it indicates why this object is useful (in the context of reexecution, we have "RPC" and "X" - makes sense that "X" is "Offline").

Done.

---

crates/blockifier_reexecution/src/state_reader/test_state_reader.rs line 362 at r1 (raw file):

Previously, dorimedini-starkware wrote…

is it already merged?
can you fix it in this PR? (fix the signature on the trait, and implement this method as part of the trait impl on the DummyStateReader)

It's in this PR: https://reviewable.io/reviews/starkware-libs/sequencer/1736

[dorimedini-starkware]

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)

[dorimedini-starkware]

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)