Skip to content

fix(wallet-storage): validate transaction bodies during bulk history load - #5781

Draft
Claudius-Maginificent wants to merge 4 commits into
fix/pr-5126from
fix/sqlite-bulk-history-identity
Draft

Claudius-Maginificent wants to merge 4 commits into
fix/pr-5126from
fix/sqlite-bulk-history-identity

Conversation

@Claudius-Maginificent

Copy link
Copy Markdown
Collaborator

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 f8d15b06e44c76820fcca91310bed45bd3e00161 head.

What was done?

Route the single core_state::load_state decoder call through existing decode_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:

  • I have performed a self-review of my own code
  • My Rust changes follow the repository coding conventions
  • I have added relevant tests
  • Independent review of the feature and updated parent integration

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

…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>
@coderabbitai

coderabbitai Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • 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.

Share

@thepastaclaw

thepastaclaw commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

🕓 Review not started yet because this PR is a draft.

  • Request normal review — click when the PR is ready for review.
  • Request priority review — click to move this review to the front of the queue.

Commit c95c0c0. Normal review starts when eligible; priority review starts as soon as a slot is available.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: low Low priority — nice to have

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants