Repository navigation
fix(wallet-storage): validate transaction bodies during bulk history load - #5781
Draft
Claudius-Maginificent wants to merge 4 commits into
Draft
Claudius-Maginificent wants to merge 4 commits into
Claudius-Maginificent wants to merge 4 commits into
Conversation
…load Use the existing record identity decoder before applying typed-column drift policy. Reject body/identifier corruption under Strict and Recovery while preserving valid-body column-drift behavior. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Inherit the published accounting fixes and historical identity decoder before validating the separate bulk-history read path. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Exercise the inherited old-writer AssetUnlock fixture through bulk load under Strict and Recovery without rekeying or rewriting record bytes. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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. |
Collaborator
|
🕓 Review not started yet because this PR is a draft.
Commit c95c0c0. Normal review starts when eligible; priority review starts as soon as a slot is available. |
8 of 14 tasks
7 of 8 tasks
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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
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.
Basic explanation
TL;DR: Reject corrupted stored transaction bodies when loading wallet history.
Value: Wallet history uses consistent transaction contents and identifiers.
Risks: Corrupt records previously accepted by bulk loading are rejected. Existing Recovery handling may omit the affected wallet. No consensus change.
User story
As a wallet user, I want corrupted transaction records to be rejected so my history is not reconstructed from inconsistent data.
Scenario
A saved transaction's contents or recorded identifier changes after the database has already been upgraded. Currently, loading the wallet's complete history can accept that inconsistent record. The loader should reject it in both strict and recovery modes.
Detailed discussion
Issue being fixed or feature implemented
The bulk history loader bypasses the body-to-identifier check used by point reads and accounting migration. This is a focused follow-up stacked on #5150, based on its published
f8d15b06e44c76820fcca91310bed45bd3e00161head.What was done?
Route the single
core_state::load_statedecoder call through existingdecode_transaction_record. Valid-body typed-column drift retains its existing Strict/Recovery policy. The existing outer Recovery boundary can still omit a failed wallet and report degradation; it cannot reconstruct that wallet from a body-invalid record. No schema, migration, write-path, dependency or ABI change.The shared decoder inherits #5150's narrow historical AssetUnlock read adapter. Bulk reads preserve those old identifiers and exact bytes; incoming writes still require current canonical identifiers. This PR changes only the bulk caller and its regression tests relative to #5150.
How Has This Been Tested?
Two regressions fail on the original caller and pass after the change. Each uses a migrated in-memory database, a valid-read control, body and embedded-identifier tampering, and unchanged-blob assertions after rejected reads. Five existing point/bulk column-drift tests also pass. A bulk-load test reads both immutable old-writer fixture records under Strict and Recovery and checks their exact bytes, bodies and identifiers. Parent migration, historical-tamper and canonical-write controls also pass (11/11 selected, offline).
Scoped formatting and whitespace checks pass. Scoped all-target/all-feature CI Clippy passes with
--no-deps -D warnings(locked/offline).Breaking Changes
No public API or serialized-format changes. Inconsistent record bodies now fail bulk decoding under both load policies.
Checklist:
No GroveDB structure, system contract, versioned consensus generation or durable documentation changes apply. Milestone assignment remains with the coordinator.
🤖 Co-authored by Claudius the Magnificent AI Agent