Repository navigation
feat: prepare original-wallet migration proposals - #34
ben-kaufman wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: none.
QA for 4b1bccc:
Rust test suite and lint at the head ✅ cargo test passes 76 of 76; clippy adds no new warnings.
cargo test: 76 passed, 0 failed, 0 ignored, including the new preparation and canonical-vector tests. cargo clippy --all-targets: only the two module_inception warnings already present in the base's test modules.
Log: RUN/media/cargo-test-clippy.log
Not driven on a device: the PR adds in-crate Rust only, with no UniFFI export, release or app change, so no app behaviour changes.
Review covered the full range 435bb2a..4b1bccc. Checked in the code:
- Canonical digest order is UTF-8 byte order, for stores and component keys.
- Every
u32length prefix is bounded first: 2 stores, 32 objects, 600-byte keys, 120-byte identities, 10 MiB head. - The head is size-checked both before and after encryption.
- The archive keeps
build_key("WALLET")with its raw-envelope digest, and absence staysNone. - Archive/component and head/component conflicts are refused.
- Stores coalesce only when principal and store ID both match.
- A status read replaces the revision before its first await, so a pending, failed or dropped read still invalidates earlier proposals.
- No new type derives
Debug.
Not checked here: that the digest matches the candidate server's real implementation (only the fixed vectors in the tests were checked), and that the size limits match the deployed server.
Reviewed by claude-opus-5-5-medium
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Adds in-crate preparation of immutable original-wallet migration proposals: the retained WALLET archive at its original key and envelope digest, an initial app head encrypted once under the app key/AAD, app-only components, exact principal/store coalescing, and the canonical all-tuples digest, with per-domain status revisions that invalidate earlier proposals. Reviewed 4b1bccc, full tier, statically: stacked on #33, so the delta against feat/backup-migration-status is what I read.
What I checked, and 7 candidates I ruled out
Read in full: src/backup_session.rs, src/backup_session/preparation.rs, preparation/canonical.rs, plus the code they call in migration_status.rs, receipt.rs (get_with_receipt), implementation.rs (build_key, the StorableBuilder use in store)
Build: skipped, talos-pr-run could not confirm the author's permission on this repo and fails closed, so nothing from the branch ran here
CI: Tests, Clippy, iOS Bindgen, manifests green; Android Bindgen still pending at review time
Ruled out
- Archive key differs from what was read:
prepare_archiveusesself.app.build_key("WALLET"), the same mappingget_with_receiptapplied to the observed key, and the target is pinned toApp { key: "WALLET" }. - Archive store not checked against the status tuple: the receipt's
store_idis the session's appstore_id, anddecodeinmigration_status.rsrejects a status whosestore_iddiffers from it, so they cannot diverge. - Head key bound on plaintext only: the 600-byte check runs again on the obfuscated key via
canonical::valid_key. - Stale status admitted after a concurrent refresh: the revision is replaced before the read starts, and both
preparation_statusandprepared_migrationcompare byArc::ptr_eq; a racing older read just yieldsStalePolicy.prepare_migrationhas no await, so nothing interleaves inside it. Covered bypreparation_pending_then_abandoned_status_cannot_reuse_previous_policy. - Byte vs char identity limit differs from
valid_identity: deliberate, the byte limit matches the activation bound and has its own test. - Head envelope version 1 vs the
-1used bystore: documented as the authenticated version for an expected-version-zero insert; plausibly the contract, not a slip. - Archive also listed as a component: allowed, with the digest conflict check mirroring the server-side rule in
committed().
Merge confidence: 4/5, no findings but the branch was not built here and Android Bindgen had not finished.
dzdidi
left a comment
There was a problem hiding this comment.
Independent review of exact head 4b1bccc3563acd0857ba75924d3786ca52b916b9 against base 435bb2a48459a556f3504981c4dc471c21166d02 found no blocking correctness, security, persistence, API-contract, or documentation issues. All six applicable exact-head hosted checks completed successfully.
Residual boundaries: canonical digest parity relies on frozen vectors rather than an independently executed external candidate server; server contract remains unpublished/undeployed; multi-read coherence, lifecycle exclusion, durable UUID/replay handling, and dual-provider proof enforcement remain explicit future activation requirements.
Description
Prepare immutable migration content from the original wallet session's app and LDK observations. Retain the existing logical WALLET at its original encrypted key/AAD and exact raw-envelope digest, encrypt the initial app head once without uploading it, and calculate the candidate server's canonical all-tuples digest. Explicit absence stays distinct from unreadable data.
Preparation rejects mixed owners/domains, revoked sessions, superseded or committed policies, duplicate/conflicting component keys and oversized input. Starting either domain's next status read invalidates earlier proposal access even when that read fails or is abandoned. Historical observations remain readable. Exact physical principal/store matches coalesce while retaining both domain labels; equal store IDs alone never combine identities. Mutable LDK objects cannot become immutable components.
Stacked on #33. This is in-crate Rust source preparation for terminal complete-WALLET work in synonymdev/bitkit-ios#1121 and synonymdev/bitkit-android#1686. It adds no activation request, UniFFI export, release, server change or app activation. Published native API versions/checksums are unchanged.
Validation
0.5.27-dev.5. Both iOS17 consumer links and Kotlin consumer compile pass. Android lint XML: 0 errors, 3 verified unchanged dependency warnings. Generated Swift/header/modulemap/Kotlin interfaces are byte-identical to the validated parent. Complete logs, archive CRCs/hashes, 862 arm64 members per iOS slice, four Android ABIs/16 KiB alignment/debug IDs and all 93 existing FFI symbols inspected. Hosted validation on exact-tree synthetic7e7bc007also passes: 4 attempt-one workflows, 6 jobs, 76 tests, zero failures/ignored tests and no introduced diagnostics. The complete 8,264 log lines and 14 annotations were inspected. Both uploaded native artifacts passed independent SHA/CRC, interface, platform/ABI and 93-symbol checks; Android debug/release build IDs match. Each artifact downloaded once. Five conditional Rustup bootstrap skips and two AndroidNO-SOURCEtasks are not test passes.Controlled cases cover original head key/AAD authentication, retained exact archive bytes including protobuf unknown fields, explicit absent versus unreadable WALLET, wrong/replaced owners, revocation, pending/failed status refreshes, committed policy refusal, coalesced-domain LDK rejection, duplicate/conflicting components, encoded size limits, fixed Rust/Java canonical vectors and an independently encoded present-archive/component Unicode vector. Same-handle ciphertext stability is asserted directly. These are native/component results, not authenticated app migration acceptance.
Remaining integration boundaries
App callers must prove complete/coherent capture and choose only already-immutable app components; ordinary same-key retries that re-encrypt are unsuitable. Every independent preparation requires a fresh UUID; later dispatch must retain one exact attempt across uncertain outcomes. This primitive supplies no durable UUID registry or replay authority.
The unpublished candidate server contract is not evidence of deployed enforcement. Future mutation requires fresh original-provider proofs and atomic tuple/object checks, lifecycle drains, running-node/RN and old reader/writer exclusion, durable outcome reconciliation and causal app restoration. In particular, a coalesced tuple retains two provider labels, but the preserved server's one-proof-per-target contract cannot freshly verify both mappings. That enforcement gap remains open before activation. This PR does not close terminal complete-WALLET acceptance.