Improve validation during read_ledger chunk parsing
#7492
Merged
Conversation
This file contains hidden or 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
This reverts commit d27bbab.
…ger_offset_table_validation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves validation during ledger chunk parsing to address a regression from version 5.x to 6.x. Previously, a truncated ledger chunk with a completely missing offsets table would incorrectly appear to read successfully (returning an empty file) instead of raising an error.
Key Changes:
- Added validation to check that the offset table position claimed in the file header doesn't exceed the actual file size
- Added validation to verify the number of transactions found in the file matches the expected count from the filename
- Added comprehensive regression tests covering 9 different corruption scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
python/src/ccf/ledger.py |
Adds two new validation checks in LedgerChunk.__init__: verifies offset table position is within file bounds and transaction count matches filename expectations |
tests/e2e_operations.py |
Adds regression tests that corrupt ledger chunks in 9 different ways and verify appropriate error messages are raised |
eddyashton
commented
Nov 28, 2025
achamayou
approved these changes
Nov 28, 2025
Member
achamayou
left a comment
achamayou
left a comment
There was a problem hiding this comment.
LGTM but needs a Python format
achamayou
added
auto-backport
eddyashton
added a commit
to eddyashton/CCF
that referenced
this pull request
Dec 2, 2025
(cherry picked from commit 52751f1)
eddyashton
added a commit
that referenced
this pull request
Dec 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Exploring behaviour with various corrupt ledger chunks spotted a significant regression from
5.xto6.x- a truncation that caused a completely missing offsets table was previously (correctly) an error, would now seem to read correctly (albeit returning an empty file).This adds 2 extra checks on file open, and some regression tests by manually corrupting some of our golden files.