feat: add state dump and loading for the sequencer #159

bajpai244 · 2023-09-22T07:47:08Z

Resolves: #119

Pull Request type

Please check the type of change your PR introduces:

What is the new behavior?

This PR adds the ability to dump and load sequencer state, the serialisable types have been borrowed from dojo's katana.

Does this introduce a breaking change?

Yes
No

Eikix · 2023-09-25T02:51:50Z

crates/sequencer/src/serde/contract.rs

+
+use super::program::SerializableProgram;
+
+#[derive(Debug, Clone, Serialize, Deserialize)]


can you annotate which code is taken from katana? for credit but also to know which code is technically reviewed, tested and works in katana currently

Makes sense, completed via this commit.

Eikix · 2023-09-25T02:53:19Z

crates/sequencer/src/serde/contract.rs

+}
+
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct SerializableEntryPointOffset(pub usize);


why is this needed?

This has been taken from Katana directly.

but can it be changed?

This is required for the serialization.

Eikix · 2023-09-25T02:54:15Z

crates/sequencer/src/serde/contract.rs

+    fn from(value: SerializableEntryPointV1) -> Self {
+        blockifier::execution::contract_class::EntryPointV1 {
+            selector: value.selector,
+            offset: value.offset.into(),


it looks like the only problem is offset!? that sounds cursed

This has been taken from Katana directly.

Eikix · 2023-09-25T03:01:40Z

crates/sequencer/src/state.rs

@@ -143,6 +170,27 @@ impl BlockifierStateReader for &mut State {
    }
 }

+impl From<&SerializableState> for State {


Why are we implementing the trait on a reference?

So that we want to create a State from SerializableState, we can do so without moving the value and rather than borrowing it.

I don't really see the need for this, specially as the implementation itself just copies everything? So in the end we are copying values

But if you will do it directly on type SerializableState, then this copying will happen twice right?

Firstly you have to call this method on a clone, and then there will another copying inside the implementation. I think the reference approach is hence more optimizied.

Lemme know what you think.

Eikix · 2023-09-25T03:03:54Z

crates/sequencer/src/state.rs

+            nonces: serializable_state.nonces.clone(),
+        };
+
+        serializable_state.classes.iter().for_each(|(class_hash, contract_class)|{


why do we need to do this?

The contract_class is of type SerializableClassRecord, hence we need a conversion from this to ContractClass
{ which is not seriaizable, and hence we have a seriaizable type for it } which is what we store in a normal state of the seqeuncer.

Eikix · 2023-09-25T03:04:23Z

crates/sequencer/src/state.rs

+        serializable_state.classes.iter().for_each(|(class_hash, contract_class)|{
+           state.classes.insert(
+               *class_hash,
+               contract_class.class.clone().try_into().unwrap_or_else(|error| panic!("failed to convert SerializableClassRecord to ContractClass for class_hash {},\n error {}", class_hash, error))


must we panic here?
can't we implement TryFrom and return a result?

I think panic here is fine right? Because this situation is a bug, it means that there is an error in our serialisation for contract class { which is already tested by dojo! }, and from my understanding, panics are reserved for bugs, hence, from our assumption, this should never happen!

I think we should never have a panic! in our code, would make a lot more sense to rely the error upstream if we have one

Eikix · 2023-09-25T03:05:24Z

crates/sequencer/src/state.rs

+        let contract_class = get_contract_class(include_str!(
+            "./test_data/contracts/compiled/universal_deployer.json"
+        ));
+        let compiled_class_hash = CompiledClassHash(stark_felt!("0x1"));


can you use @greged93 's constants? I think he wrote a similar flow

in the constant.rs file

I think we can, I have almost all values in there, and we can add if missing.

makes sense, done via this commit.

Eikix · 2023-09-25T03:11:18Z

crates/sequencer/src/state.rs

+
+    /// This will read a dump from a file and initialize the state from it
+    pub fn load_state_from_file(path: &PathBuf) -> Result<Self, SerializationError> {
+        let serilizable_state = SerializableState::load_state(path)?;


Suggested change

let serilizable_state = SerializableState::load_state(path)?;

let serializable_state = SerializableState::load_state(path)?;

resolved via this commit.

Eikix · 2023-09-25T03:11:34Z

crates/sequencer/src/state.rs

+    pub fn load_state_from_file(path: &PathBuf) -> Result<Self, SerializationError> {
+        let serilizable_state = SerializableState::load_state(path)?;
+
+        Ok(Self::from(&serilizable_state))


Suggested change

Ok(Self::from(&serilizable_state))

Ok(Self::from(&serializable_state))

resolved via this commit.

Eikix · 2023-09-25T03:13:40Z

crates/sequencer/src/serde/utils.rs

+
+/// Converts `starknet-rs` RPC [FlattenedSierraClass] type to Cairo's
+/// [ContractClass](cairo_lang_starknet::contract_class::ContractClass) type.
+pub fn rpc_to_cairo_contract_class(


why do we need this?

required in the serialization of contract class, required from a dojo borrowed file for contract serialization.

Eikix · 2023-09-25T03:15:59Z

crates/sequencer/src/serde/utils.rs

+    })
+}
+
+pub fn get_contract_class(contract_class_str: &str) -> ContractClass {


this doesn't look like a getter but rather an into_contract_class

Also we should avoid get prefix

This has been borrowed directly from katana and it is also used from the dojo borrowed file.

I have made a commit resolving it.

greged93

Couple of comments, great work!

greged93 · 2023-09-25T03:20:34Z

crates/sequencer/Cargo.toml

 # Starknet
 # TODO: remove the blockifier patch on the workspace once we can remove Katana.
 blockifier = { package = "blockifier", git = "https://github.com/starkware-libs/blockifier.git", tag = "v0.3.0-rc0" }
+cairo-lang-starknet = { workspace = true }
+cairo-vm = { workspace = true }
+serde.workspace = true


can you use the same syntax here please? { workspace = true }

resolved here.

greged93 · 2023-09-25T03:21:05Z

crates/sequencer/Cargo.toml


 # Other
 rustc-hash = "1.1.0"

 [dev-dependencies]
 lazy_static = { workspace = true }
+starknet = { workspace = true }


no need to depend on it for dev, we have in the crate deps

resolved here.

greged93 · 2023-09-25T03:25:44Z

crates/sequencer/src/serde/contract.rs

+impl TryFrom<SerializableContractClass> for blockifier::execution::contract_class::ContractClass {
+    type Error = anyhow::Error;
+
+    fn try_from(value: SerializableContractClass) -> Result<Self, Self::Error> {


Can you rework this? You are using a TryFrom trait but I don't see any error being thrown and I see an unwrap in the implementation.

@greged93 this whole file is borrowed from Dojo, LamdbaClass said they are working on serialization for these types, if we still need this file in sometime even after merging this, then might be we can refactor this otherwise we will probably not need these serde types in sometime!

greged93 · 2023-09-25T03:45:00Z

crates/sequencer/src/serde/contract.rs

+    use starknet::core::types::contract::SierraClass;
+
+    use super::*;
+    use crate::serde::utils::{get_contract_class, rpc_to_inner_class};


I added some cairo 0 and cairo 1 contracts already, can you use these instead? In order to avoid adding more and more test files.

required by dojo tests, I don't think we should change this, in sometime we should not need it mostly as discussed once we have serialization enabled for ContractClass.

greged93 · 2023-09-26T10:01:45Z

crates/sequencer/src/serde/state.rs

+
+/// TODO: wrap the underlying errors in them
+#[derive(Error, Debug)]
+pub enum SerializationError {


I think we can rework this a little to use more the syntax from thiserror examples?

#[derive(Error, Debug)] pub enum MyError { Io { #[from] source: io::Error, backtrace: Backtrace, }, }

source

greged93 · 2023-09-26T10:06:39Z

crates/sequencer/src/serde/state.rs

+}
+
+impl SerializableState {
+    pub fn dump_state(&self) -> Result<Vec<u8>, SerializationError> {


I'm wondering: should we rename this? This converts the SerializableState into a Vec, so naming it dump_state sounds too specific.

how about serialize_state?

greged93 · 2023-09-26T10:07:38Z

crates/sequencer/src/serde/state.rs

+        Ok(serialized_state_buffer)
+    }
+
+    pub fn load_state(path: &PathBuf) -> Result<Self, SerializationError> {


could we change this to new? new could take an Option and load the file if Option is not None.

I think, from_file_path is a better approach, it should be a static method and do one single thing!

We can see this pattern in other rust codebases as well, where there is options like from_str, etc.

Lemme know what you think about it.

greged93 · 2023-09-26T10:09:36Z

crates/sequencer/src/state.rs

+        let contract_class = get_contract_class(include_str!(
+            "./test_data/contracts/compiled/universal_deployer.json"
+        ));
+        let compiled_class_hash = CompiledClassHash(stark_felt!("0x1"));


I think we can, I have almost all values in there, and we can add if missing.

greged93 · 2023-09-26T10:10:31Z

crates/sequencer/src/state.rs

+        assert_eq!(
+            state.classes.get(&class_hash),
+            loaded_state.classes.get(&class_hash)
+        );
+        assert_eq!(
+            state.compiled_class_hashes.get(&class_hash),
+            loaded_state.compiled_class_hashes.get(&class_hash)
+        );
+        assert_eq!(
+            state.contracts.get(&contract_address),
+            loaded_state.contracts.get(&contract_address)
+        );
+        assert_eq!(
+            state.storage.get(&contract_storage_key),
+            loaded_state.storage.get(&contract_storage_key)
+        );
+        assert_eq!(
+            state.nonces.get(&contract_address),
+            loaded_state.nonces.get(&contract_address)
+        );


Can't we derive PartialEq and just have one assert_eq!?

Great suggestion! Implemented via this commit.

greged93 · 2023-09-26T10:11:10Z

crates/sequencer/src/test_data/contracts/compiled/cairo1_contract.json

@@ -0,0 +1,2534 @@
+{


You can remove both these contracts and use the ones I have added.

greged93 · 2023-09-29T07:51:46Z

We will fork cairo-vm and blockifier in order to implement serialize on the necessary types.

bajpai244 force-pushed the feat/dump-load-seqeuencer branch 2 times, most recently from 469bc32 to 5ca62c5 Compare September 22, 2023 07:56

bajpai244 marked this pull request as ready for review September 22, 2023 10:07

Eikix reviewed Sep 25, 2023

View reviewed changes

greged93 suggested changes Sep 26, 2023

View reviewed changes

Harsh Bajpai added 2 commits September 26, 2023 17:23

feat: add state dump and loading for the sequencer

430cf26

feat: annotate files which have been taken from katana directly

1f83daa

bajpai244 force-pushed the feat/dump-load-seqeuencer branch from d7001aa to 1f83daa Compare September 26, 2023 10:25

Harsh Bajpai added 6 commits September 26, 2023 17:44

dev: use lazy_statics in tests

3c67a7a

dev: fix typo in identifier name

536b155

dev: rename get_contract_class to contract_class

194d691

dev: refactor cargo.toml

bd6c716

dev: remove SerializableClassRecord

aef6614

dev: implement PartialEq for state

87e3c99

greged93 closed this Sep 29, 2023

bajpai244 mentioned this pull request Oct 19, 2023

feat: add dumping and loading of state #555

Merged

9 tasks


		use super::program::SerializableProgram;

		#[derive(Debug, Clone, Serialize, Deserialize)]

	let serilizable_state = SerializableState::load_state(path)?;
	let serializable_state = SerializableState::load_state(path)?;

	Ok(Self::from(&serilizable_state))
	Ok(Self::from(&serializable_state))

feat: add state dump and loading for the sequencer #159

feat: add state dump and loading for the sequencer #159

Conversation

bajpai244 commented Sep 22, 2023

Pull Request type

What is the new behavior?

Does this introduce a breaking change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greged93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajpai244 Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajpai244 Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greged93 commented Sep 29, 2023

bajpai244 Sep 26, 2023 •

edited

Loading

bajpai244 Sep 27, 2023 •

edited

Loading