Repository navigation
Bound the spending history by count, so a moved clock cannot erase it - #1067
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughVelocity tracking now retains history based on transaction count rather than age. Stored velocity records are checked against byte and transaction limits when loaded. Tests cover time-based totals, retained history, and policy evaluation with an over-capacity record. ChangesVelocity history and persistence
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the main change: spending history is bounded by transaction count instead of being erased when the device clock moves forward.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the ledger’s bounds,
And keeps old tracks where time still counts.
The daily sum looks close at hand,
The weekly sum spans wider land.
Safe records hop back to their stand.
Comment @coderabbitai help to get the list of available commands.
1 participant
The spending history that backs
daily_limitandweekly_limitwas pruned against the device wall clock, and the prune ran while reading the record. A clock moved forward past the weekly window matched every entry, so loading the counters discarded all of them, and the next spend persisted the pruned history. Correcting the clock did not bring it back: both totals read zero from then on, and the limits they back counted nothing.Physical access to an unlocked device and the date setting is enough. No root, no prompt, and nothing in the record afterwards shows what was lost.
The prune was also unnecessary.
daily_totalandweekly_totaleach select their own window when they are asked, so the limits were never relying on the stored history having been trimmed. Pruning by age only bounded the size of the record, and the count cap already does that: the history is now bounded byMAX_TRACKED_TRANSACTIONSalone, which is a number the clock cannot move. This is the same principleload_descriptorsalready states in the same file, that a read path must not prune, because pruning is a write that makes an absence permanent.What that costs, stated plainly: the worst-case record size is unchanged, because the count cap was already the ceiling, but the typical steady state grows. Before, the record shrank to the last seven days on every load; now a device that has ever recorded a large number of spends keeps them, up to roughly 800 KB of JSON at the cap, re-encrypted and rewritten on each spend. Compacting the at-rest encoding, which stores each timestamp as an RFC3339 string with nanoseconds, would roughly halve it and is tracked separately rather than folded in here, since changing the stored format needs a migration path for records already written.
Since the cap is now the only bound, a record arriving above it would be loaded whole and would not come back under the cap for thousands of spends, because
recordevicts one entry per call.load_velocitytherefore refuses such a record instead of trimming it: trimming would put a destructive write back on the read path, which is the shape this change exists to remove, and refusing leaves the counters unknown, which already denies every rule that depends on them. The record is also bounded by length before it is parsed, as the policy record is.Not closed by this change: while the clock is wrong, the totals themselves still select the wrong window, so a device whose date is pushed forward reports zero for a window in which spends did happen and the limits do not bind during that period. Separating that out needs a monotonic reference rather than the wall clock, which is tracked separately. The difference this makes is between a bypass that lasts as long as the clock is wrong and one that is permanent and silent.
Tests cover that reading the record preserves what it holds whatever the clock says about the age of the entries, that recording a spend afterwards cannot launder the loss into storage, and that the totals select their own window. The forward clock cannot be produced by moving the real clock under test, so the entries carry the age a shifted clock would make them appear to have; reverting the fix drops exactly those entries and the test fails.
Summary by CodeRabbit