-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Calculate and compare CRC when writing and reading ledger snapshots #1319
Draft
geo2a
wants to merge
17
commits into
main
Choose a base branch
from
892-checksum-snaphot-file
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
geo2a
force-pushed
the
892-checksum-snaphot-file
branch
from
November 21, 2024 13:38
41891a0
to
e449818
Compare
jasagredo
reviewed
Nov 21, 2024
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
jorisdral
reviewed
Nov 25, 2024
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Util/CBOR.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
geo2a
force-pushed
the
892-checksum-snaphot-file
branch
2 times, most recently
from
November 28, 2024 08:14
fe08707
to
b001c90
Compare
Split snapshot type into DiskSnapshotName and DistSnapshot
geo2a
force-pushed
the
892-checksum-snaphot-file
branch
from
November 28, 2024 08:16
b001c90
to
a599027
Compare
jasagredo
reviewed
Nov 28, 2024
ouroboros-consensus/changelog.d/20241128_084625_georgy.lukyanov_892_checksum_snaphot_file.md
Outdated
Show resolved
Hide resolved
ouroboros-consensus/changelog.d/20241128_084625_georgy.lukyanov_892_checksum_snaphot_file.md
Outdated
Show resolved
Hide resolved
...s-consensus-cardano/changelog.d/20241127_163624_georgy.lukyanov_892_checksum_snaphot_file.md
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Show resolved
Hide resolved
jasagredo
reviewed
Nov 28, 2024
ouroboros-consensus/changelog.d/20241128_084625_georgy.lukyanov_892_checksum_snaphot_file.md
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs
Outdated
Show resolved
Hide resolved
geo2a
force-pushed
the
892-checksum-snaphot-file
branch
from
November 28, 2024 11:34
67dc690
to
9c9a3b7
Compare
geo2a
force-pushed
the
892-checksum-snaphot-file
branch
from
November 28, 2024 11:37
9c9a3b7
to
ba830ac
Compare
geo2a
force-pushed
the
892-checksum-snaphot-file
branch
2 times, most recently
from
November 29, 2024 11:16
28e8320
to
31b892a
Compare
geo2a
force-pushed
the
892-checksum-snaphot-file
branch
from
November 29, 2024 11:19
31b892a
to
5d39721
Compare
I've compared the performance of reading and then writing a snapshot using I would like to spend some more time looking at the microbenchmarks that we currently have and maybe adding one for reading-writing ledger state snapshots. Logs of `db-analyser` with timingsFeature 101.325841s
Main 99.685827s
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #892
In this PR, we change the reading and writing disk snapshots of ledger state. When a snapshot is taken and written to disk, an additional file with the
.checksum
extension is written alongside it. The checksum file contains a string that represent the CRC32 checksum of the snapshot.The checksum is calculated incrementally, alongside writing the snapshot to disk. When a snapshot is read from dist, the checksum is again calculated and compared to the tracked one. If the checksum is different,
readSnaphot
returns theReadSnapshotDataCorruption
error, indicating data corruption.The checksum is calculated incrementally, alongside reading a writing the data. On write, we use the
[hPutAllCRC](https://input-output-hk.github.io/fs-sim/fs-api/src/System.FS.CRC.html#hPutAllCRC)
function fromfs-sim
, and on read we modify the readIncremental function to compute the checksum as data is read.To enable seamless integration into
cardano-node
, we make the check optional. When initialising the ledger state from a snapshot ininitLedgerDB
, we issue a warning in case the checksum file is missing for a snapshot, but do not fail as in case of invalid snapshots.The
db-analyser
tool ignores the checksum files by default when reading the snapshots. We add--disk-snapshot-checksum
flag to enabled the check. When writing a snapshot to disk, e.g. as a result of the--store-ledger
analysis,db-analyser
will always write calculate the checksum and write it into the snapshot's.checksum
file.Effects on Performance:
Running
db-analyser
to read a ledger snapshot and store the snapshot of the state at the next slot shows a difference of 2 seconds on my machine. See a comment below for the logs.To precisely evaluate the effects, we need a micro-benchmark of the reading and writing of snapshots with and without the checksum calculation.