fix: snapshot fetch without aria2 #2814
Conversation
hanabi1224
requested review from
elmattic,
lemmih,
LesnyRumcajs,
jdjaustin,
creativcoder and
sudo-shashank
as code owners
| echo "Cleaning up snapshots" | ||
| $FOREST_CLI_PATH --chain calibnet snapshot clean -s "$SNAPSHOT_DIRECTORY" --force | ||
| echo "Cleaning up snapshots again" | ||
| $FOREST_CLI_PATH --chain calibnet snapshot clean -s "$SNAPSHOT_DIRECTORY" --force |
There was a problem hiding this comment.
Why do we need to repeat snapshot clean again?
There was a problem hiding this comment.
It's a good question. It might be worth adding this issue in the code to make it clear from the start.
| @@ -375,13 +378,20 @@ where | |||
| let mut downloaded: u64 = 0; | |||
| let mut stream = snapshot_response.into_body(); | |||
|
|
|||
| let mut snapshot_hasher = Sha256::new(); | |||
| let mut snapshot_hasher = if use_compressed { | |||
| None | |||
There was a problem hiding this comment.
What is the reason behind not checking against checksum in case of compressed snapshot and without aria2? Don't we have a digest in both case? (with and without aria2)
There was a problem hiding this comment.
Checksum is not provided for a compressed snapshot. Also zstd has checksum mechanism internally
There was a problem hiding this comment.
Hmm if I look into the digest file I have:
cat 468720_2023_04_13T12_13_00Z.sha256sum
ae6d21b3bd88fa0d21528343b05e6f323a7131fad5d3e1148a5ed7c603d915a5 *468720_2023_04_13T12_13_00Z.car
180819491d0dfd37cc09ebc5ceded668c09fa6ec3f1fefe54ad20ca0fa71805a *468720_2023_04_13T12_13_00Z.car.zst
But if zstd does it internally, then we can skip it. 👍
There was a problem hiding this comment.
Did not realize checksum is there. I can investigate adding back checksum check then. Thx for the info
There was a problem hiding this comment.
Just checked the zstd spec again, its checksum mechanism is optional, we should indeed verify checksum if it's available
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #2786
Other information and links
Change checklist