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

FM-362: Save snapshot chunks #441

Merged
merged 4 commits into from
Nov 27, 2023
Merged

FM-362: Save snapshot chunks #441

merged 4 commits into from
Nov 27, 2023

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Nov 21, 2023

Closes consensus-shipyard/ipc#179

  • Split the snapshot crate into multiple submodules to manage code size
  • Change App::offer_snapshot to put into the state a new SnapshotDownload record which shows which manifest we are downloading to what temporary directory
  • SnapshotDownload cleans up the temporary directory when the last clone of it is dropped, e.g. if CometBFT restarts with a new snapshot
  • Add App::apply_snapshot_chunk to ingest new chunks of a snapshot, saving them to the temporary directory and validating the checksum when they are all present

Not done in this PR:

  • Tests are deferred to End-to-end test for snapshots ipc#178
  • Any locking - the docs say CometBFT calls apply_snapshot_chunk sequentially, but I'm not sure if it waits before passing the next chunk, or is it similar to end_block which was called while deliver_tx was still running. We'll see.
  • Here we just save all the snapshot parts and verify the checksum at the end; the import of the snapshot is deferred to Commit snapshots to the blockstore ipc#176
  • Not trying to load partial CAR files into memory to check that the CIDs have been referenced by something seen earlier; this is deferred to Validate snapshots during/after loading ipc#177

@aakoshh aakoshh force-pushed the fm-362-take-snapshot-chunks branch from a7770d7 to 2c06867 Compare November 21, 2023 15:42
match manifest::parts_checksum(cd.download_dir.as_ref()) {
Ok(checksum) => {
if checksum == cd.manifest.checksum {
// TODO: Import Snapshot.
Copy link
Contributor Author

@aakoshh aakoshh Nov 21, 2023

Choose a reason for hiding this comment

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

I'm not sure yet if we should import the snapshot here, or make another call from the App. In any case, the App will have to write to its app state history, so we'll have to return the full SnapshotManifest so it can construct the necessary record.

The benefit of returning from here would be just a bit of extra visibility in the logs, showing explicitly when we start the import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was done in 6e3c587

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a few questions on how you import and if you already persist this snapshot so it is available to offer it to other peers, but I see this is addressed in the follow-up PR. Will ask my questions there if they are still not solved. 🙏

match manifest::parts_checksum(cd.download_dir.as_ref()) {
Ok(checksum) => {
if checksum == cd.manifest.checksum {
// TODO: Import Snapshot.
Copy link
Contributor

Choose a reason for hiding this comment

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

I had a few questions on how you import and if you already persist this snapshot so it is available to offer it to other peers, but I see this is addressed in the follow-up PR. Will ask my questions there if they are still not solved. 🙏

@aakoshh aakoshh force-pushed the fm-367-handle-snapshot-offer branch from f7f1ff4 to c9be10d Compare November 27, 2023 09:54
Base automatically changed from fm-367-handle-snapshot-offer to main November 27, 2023 09:54
@aakoshh aakoshh force-pushed the fm-362-take-snapshot-chunks branch from 8693151 to 1e2e5a7 Compare November 27, 2023 09:57
@aakoshh aakoshh merged commit 89a3fb8 into main Nov 27, 2023
1 check passed
@aakoshh aakoshh deleted the fm-362-take-snapshot-chunks branch November 27, 2023 09:57
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.

Take snapshot chunks in ABCI
2 participants