Add invariants check#7902
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the test framework’s shutdown-time ledger/snapshot verification to enforce stronger, CCF-like invariants (rather than simple file equality), and threads a new check_file_invariants toggle through network lifecycle helpers.
Changes:
- Replace “ledger files identical” verification with explicit ledger-history and snapshot-boundary invariant checks.
- Introduce
ccf.ledger.get_range_from_file()to derive an end seqno for chunks whose filename range is open-ended. - Update multiple tests to enable/disable invariant checking depending on scenario (recovery, compatibility, intentional divergence).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/recovery.py | Updates recovery tests to use check_file_invariants and adjusts behavior in mid-recovery/retirement cases. |
| tests/partitions_test.py | Switches shutdown validation to invariants-based checking. |
| tests/lts_compatibility.py | Replaces accept_ledger_diff with check_file_invariants based on snapshot usage. |
| tests/infra/remote.py | Refactors ledger path helpers (current vs read-only) to support invariant scanning. |
| tests/infra/network.py | Implements snapshot + ledger invariant checks and wires them into stop_all_nodes() and the network() context manager. |
| tests/e2e_operations.py | Disables invariant checks for scenarios that intentionally manipulate ledger files. |
| python/src/ccf/ledger.py | Adds file-based range derivation for ledger chunks with open-ended filename ranges. |
| def ledger_paths(self): | ||
| return [ | ||
| path | ||
| for path in [self.current_ledger_path(), *self.read_only_ledger_paths()] | ||
| if os.path.exists(path) | ||
| ] |
|
|
||
| def get_range_from_file(ledger_file_path: str) -> tuple[int, int | None]: | ||
| start, end = range_from_filename(ledger_file_path) | ||
| if end is not None: |
There was a problem hiding this comment.
Would it be sensible to still proceed reading the offset and assert naming matches the contents? Or maybe we do it elsewhere already?
There was a problem hiding this comment.
There is an efficiency tradeoff here.
I want to avoid reading the contents of the file if possible, and have it such that for most files we just use the name.
The only case is where we have:
ledger_3_5
ledger_6 //ends at 10
ledger_11
In this case we'll examine the contents of only ledger_6 and ledger_11 which I think is a good performance improvements.
| skip_verification=False, | ||
| verbose_verification=False, | ||
| accept_ledger_diff=False, | ||
| check_file_invariants=True, |
There was a problem hiding this comment.
Nit. Shall we invert to skip_ledger_invariants=False? For consistency with other args, as in "skip_some_checks=False" by default
There was a problem hiding this comment.
I have a personal preference towards positive statements rather than negative ones. But that is only a personal preference rather than anything concrete.
|
|
||
| # List ledger chunks, plus checksum, in order of starting seqno | ||
| def node_ledger_files(node, allow_uncommitted): | ||
| def pred(f, allow_uncommitted, allow_recovery): |
There was a problem hiding this comment.
Do we need to put allow_recovery as arg here? It's a const arg of the top level func, I'd think it's accessible from anyway?
There was a problem hiding this comment.
Same for allow_uncommitted?.. Or I'm missing smth obvious here maybe
There was a problem hiding this comment.
Yep there are a couple layers of shadowing here.
For a node's ledger files the node cares about only .commtted files in the read-only directory, and everything in the current files.
So the invariant only pulls the .committed files from the read-only directories and everything (modulo the global options) in the current dir.
The shadowing helps to achieve this.
This updates the pre-existing invariants check to more closely match CCF's behavior.
For ledger files this is three properties:
For snapshots it is just the chunk boundary property.