repro: wallet readers panic on corrupt bytes and accept unknown versions - #2726
Draft
dorianvp wants to merge 17 commits into
Draft
repro: wallet readers panic on corrupt bytes and accept unknown versions#2726dorianvp wants to merge 17 commits into
dorianvp wants to merge 17 commits into
Conversation
…cept unknown versions Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 22, 2026
…ders Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t bytes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ix/wallet-readers-corrupt-bytes
Every wallet reader opened with a bare `read_u8` and then, on the next line, asked `reject_unknown_version` whether the byte it had just taken was a layout this build understands. The two statements were separable, so nothing stopped a fifteenth reader from taking the byte and skipping the question. This commit folds them into `read_version`, which reads one byte, rejects any version newer than the latest the build can read, and returns the byte to the caller. The version byte does two jobs, and only the first of them delegates. As a compatibility gate it is uniform across every type, which is what `read_version` now owns. As a layout discriminator it is local: eleven of the fourteen readers branch on the returned value to decide whether a later field is present, so the function hands the byte back rather than consuming it. The three readers that never branch call it as a statement. The pure decision survives underneath. `reject_unknown_version` keeps its signature and its independence from any reader, and it loses only its visibility, because `read_version` is now its single caller. The type name stays a literal argument at each site. A trait constant would erase it, but `WalletNote` and `OutgoingNote` are each one generic impl serving both Sapling and Orchard, so a single associated name would report `WalletNote` where the reader deliberately says `SaplingNote` or `OrchardNote`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
refactor(`pepper-sync`): read a version byte only through its gate
…ion-versions fix(pepper-sync): reject unknown serialization versions in wallet readers
…ix/wallet-readers-corrupt-bytes
…idData errors Every fixed-width read in the wallet serialization module now goes through a const-generic `read_array` helper, and every canonicality check goes through `parse_field`, which converts an empty parse result into an `io::Error(InvalidData)` naming the corrupt field. This removes the thirteen `.expect()` calls that aborted a wallet load when a stored orchard address, rho, rseed, or note, a jubjub rseed, or a transparent account id, value, or child index failed to parse, completing the repair that issue #2732 began for nullifiers and string lengths. The buffer widths are now named constants: FIELD_ELEMENT_SIZE, BLOCK_HASH_SIZE, MEMO_SIZE, and RAW_ADDRESS_SIZE derived from DIVERSIFIER_SIZE plus FIELD_ELEMENT_SIZE. No bare width literals remain at the read sites. The `read_string` truncation guard now returns `InvalidData` with a message naming both the claimed and the delivered length, as issue #2732 specifies, instead of a bare `UnexpectedEof`. The new repro test `read_string_truncation_error_names_both_lengths` fails against the old guard on the error kind, and fails against any guard whose message omits either length. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aces MEMO_SIZE now derives from zcash_note_encryption's plaintext widths, FIELD_ELEMENT_SIZE from `PrimeField::Repr` with a compile-time assertion that the jubjub and pallas widths agree, BLOCK_HASH_SIZE and DIVERSIFIER_SIZE from their pub-field newtypes, and RAW_ADDRESS_SIZE stays the sum of DIVERSIFIER_SIZE and FIELD_ELEMENT_SIZE. No width in the module is a bare literal any longer. Grounding the field widths adds direct manifest edges to ff and pasta_curves. Both crates were already compiled with the same versions and features, so the Cargo.lock package set is unchanged; ADR 0051 rules such lock-neutral edges acceptable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The decision rules that a new manifest dependency edge is acceptable when it replaces a locally minted literal with a definition an already-compiled crate offers, and when the Cargo.lock package set is unchanged. The first application grounds the serialization width constants through ff and pasta_curves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The constant derives from the sapling diversifier newtype but names a protocol-wide width that also sizes the orchard address reads. A compile-time assertion against orchard::keys::Diversifier now proves the two families agree, mirroring the FIELD_ELEMENT_SIZE pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fix(pepper-sync): return errors instead of panicking on corrupt wallet bytes
Every fixed-width read already flows into a function whose signature fixes the width: `PaymentAddress::from_bytes(&[u8; 43])`, `Address::from_raw_address_bytes(&[u8; 43])`, `Rho::from_bytes(&[u8; 32])`, `RandomSeed::from_bytes([u8; 32], _)`, `Nullifier::from_bytes(&[u8; 32])`, `jubjub::Fr::from_bytes(&[u8; 32])`, or a pub `[u8; 32]` newtype field. `read_array::<N>` is const-generic, so rustc infers `N` at each site from that signature. A mismatch is a type error at the read site itself. This removes FIELD_ELEMENT_SIZE, BLOCK_HASH_SIZE, DIVERSIFIER_SIZE and RAW_ADDRESS_SIZE, and the two compile-time assertions that tried to justify sharing one width across sapling and orchard. Those reads are not one kind of thing (BLAKE2s output, compressed jubjub point, raw seed, pallas base element, jubjub scalar); they share the number 32 by protocol design, not by abstraction. Only MEMO_SIZE remains, because `Memo::from_bytes` takes `&[u8]`. The sapling nullifier newtype has a pub field, so its read is now infallible and the hand-written map_err is gone. The `ff` and `pasta_curves` manifest edges existed only to name the abstraction; they are removed, and Cargo.lock returns to the base state. ADR 0051 was written to permit those edges and is withdrawn with them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ix/wallet-readers-corrupt-bytes
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
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.
Summary
Reproduction only, no fix. Adds unit tests to
pepper-sync/src/wallet/serialization.rsthat assert wallet readers return an error on corrupt or unknown input. Five tests fail ondev.What it affects
Tests
Failing on
dev:nullifier_map_read_rejects_non_canonical_orchard_nullifierread_string_rejects_oversized_length_without_allocatingsync_state_read_rejects_unknown_future_versionsync_config_read_rejects_unknown_future_versionscan_target_read_rejects_unknown_future_versionPassing guard:
sync_state_roundtrip_preserves_every_scan_prioritypins the currentScanPriorityorder so a reorder becomes visible.🤖 Generated with Claude Code