Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

Export snapshot #167

Merged
merged 19 commits into from
Aug 16, 2023
Merged

Export snapshot #167

merged 19 commits into from
Aug 16, 2023

Conversation

cryptoAtwill
Copy link
Contributor

@cryptoAtwill cryptoAtwill commented Aug 1, 2023

Initial attempt to add snapshot functionalities to fendermint. The ideas are as follows:

  1. We are only taking the latest StateParams, BlockHeight and StateTree. (Maybe we can consider snapshot some past StateParams?)
  2. Snapshot are directly written to CAR file by a generic IPLD walker that loops over all the cids and their values in the underlyiing blockstore.
  3. Taking snapshot of the latest states can be triggered from cron job at the start/end of some blocks as a async process running in the background.
  4. To interface with tendermint list_snapshots call, we can return the path of the snapshot files in metadata field and chunk the car file accordingly.

This PR is starting to address point 1 and 2.

@cryptoAtwill cryptoAtwill marked this pull request as draft August 1, 2023 06:56
Copy link
Contributor

@adlrocha adlrocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, but I think we are in a good path, let me know if you need me to look deeper into anything. Thanks!

fendermint/vm/interpreter/src/fvm/state/snapshot.rs Outdated Show resolved Hide resolved
StateTree::new_from_root(ReadOnlyBlockstore::new(store), &state_params.state_root)?;
let state_tree_root = state_params.state_root;

let block_state_params_bytes = fvm_ipld_encoding::to_vec(&(state_params, block_height))?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you are serializing a tuple of state_params, height, right? Wouldn't it be better to create a clear data structure that include both so its de/serialization is clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just two fields so I feel making a type alias is easier and clear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way it is now, with the ability to inspect version before trying to deserialize the actual payload, should make it possible to migrate later 👍

fendermint/vm/interpreter/src/fvm/state/snapshot.rs Outdated Show resolved Hide resolved
fendermint/vm/interpreter/src/fvm/state/snapshot.rs Outdated Show resolved Hide resolved
todo!()
}

async fn write_car(self, car: CarHeader, write: tokio::fs::File) -> anyhow::Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why create the CarHeader here, and then the car_header_roots function is not needed, you can delay serializing the state_params to a Block to get the CID up to this point, and not need to store bytes and CID in the struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated slightly to use stream instead. But originally I was thinking the caller, i.e. Snapshot should control the root cids.

}

#[pin_project::pin_project]
struct StateTreeStreamer<BlockStore> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ususally we use BS or DB as a generic parameter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more intuitive when you look at bs: BlockStore that it's not a regular type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You gave a thumbs up but didn't change the parameter name, just resolved the comment. I'm not sure whether you intended to change it or not 🙂

Copy link
Contributor Author

@cryptoAtwill cryptoAtwill Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, I changed them all. Oh, I know, the commit was lost when I was trying to rebase the branch. Will add them back. Updated them now

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

There are some crates for me to catch up on, I have no idea about pin_project :)

There shouldn't be any changes to the ipc_actors, unless the dev branched changed there. But definitely gateway.rs should not exist any more if you rebase.

Base automatically changed from fix_estimate_gas to master August 3, 2023 17:25
@cryptoAtwill cryptoAtwill marked this pull request as ready for review August 4, 2023 03:28
@cryptoAtwill
Copy link
Contributor Author

@aakoshh @adlrocha I have addressed the comments above and updated the code to have better handling of versioning. Please help take a look.

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I have only very minor remarks left 👏

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up on all the comments, great job!

Copy link
Contributor

@adlrocha adlrocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing job! A minor comment but I think this is good to go.

@cryptoAtwill cryptoAtwill merged commit 116ba04 into main Aug 16, 2023
5 checks passed
@cryptoAtwill cryptoAtwill deleted the snapshot branch August 16, 2023 05:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants