Skip to content

feat: reconcile original-wallet migration receipts - #35

Open
ben-kaufman wants to merge 2 commits into
feat/backup-migration-planfrom
feat/backup-migration-reconciliation
Open

ben-kaufman wants to merge 2 commits into
feat/backup-migration-planfrom
feat/backup-migration-reconciliation

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Reconcile a retained original-wallet migration proposal through fresh app and LDK credentials, including when the original physical tuple was shared. Compare both effective principal/store pairs and the exact UUID, all-tuples digest, archive presence/hash, frozen encrypted initial-head hash and complete component set. A later current head is not used.

Each read acquires credentials with its captured original signer. An unexpired ordinary JWT cannot hide an issuer-side remap or substitute when fresh authentication fails. Ordinary backup reads keep their existing credential behavior. Errors, mixed outcomes, superseding reads and cancellation remain distinct from observed committed/uncommitted results. Starting reconciliation invalidates preparation access; this comparison never restores it.

Stacked on #34. This is in-crate Rust source for terminal complete-WALLET work in synonymdev/bitkit-ios#1121 and synonymdev/bitkit-android#1686. It adds no migration dispatch, UniFFI export, released dependency, server change or app activation.

Validation

  • 94 Rust tests passed, zero failures/ignored; debug/release builds and docs passed. Complete structured Clippy results contain only two byte-verified unchanged parent test-module warnings. Final formatting and diff checks passed.
  • Two independent security/recovery and integration reviews completed with affected rechecks. Fixed the valid-cached-JWT gap and tested actual minted-token remapping for each original signer, split/coalesced domains and issuer refusal without fallback. The published cancellation-test finding is fixed in 4a1b1f8: a bounded post-drop request observation plus a scheduled-read positive control. Its affected independent recheck is clean. Earlier development formatting and fixture compilation failures and initial-head CI are retained.
  • Required iOS/two-slice and Android/four-ABI bindings built once in isolated validation version 0.5.27-dev.7, private Maven destination only. Both iOS17 consumer links and Kotlin consumer compilation passed. Android lint XML has 0 errors and 3 verified unchanged parent dependency warnings. Complete build logs, archives/CRCs/hashes, 862 arm64 members per iOS slice, all Android architectures/16 KiB alignment/matching debug IDs and all 93 unchanged FFI symbols were inspected. Generated interfaces match the validated parent exactly.
  • All 55 frozen inputs match. Full seven-commit live-master range verified locally and on GitHub; the child range contains two signed commits. All four final-head hosted workflows and six jobs passed on synthetic checkout 1ed547b7, whose tree exactly matches this head.
  • Final hosted audit: 94 tests, zero failures/ignored/filtered; all 6,676 log lines and 14 annotations inspected against the actual parent. No introduced diagnostics. Rust tests/Clippy, manifest validation, iOS bindgen and Android bindgen each completed on attempt 1. Both uploaded archives were downloaded once and verified against GitHub/upload hashes, nested CRCs, unchanged generated interfaces and all 93 FFI symbols. Hosted iOS contains 865 arm64 objects per slice; all four Android release/debug pairs have matching build IDs and 16 KiB alignment. Rustup bootstrap skips and Gradle NO-SOURCE tasks are not test passes.

Finite native controls cover exact committed and legacy pairs, all bound receipt fields, protected absence, complete/order-independent components, original-provider remaps, unavailable/malformed/unauthorized/redirected reads, wrong store, owner replacement/drop/revocation, held first/second reads, abandoned futures and overlapping status reads. These are controlled native cases, not authenticated app migration acceptance. NO-SOURCE build tasks are not tests.

Integration boundaries

The two reads are sequential observations, never an atomic snapshot or write/replay permit. Each ten-second deadline includes authentication; VSS POST redirects/retries are disabled. Inherited LNURL GET redirect/body behavior remains. The unpublished candidate status route may initialize empty legacy policy metadata; this client performs no backup upload/deletion or migration request.

Retained in-memory ownership is not durable UUID/restart adoption. Read-only checks through both providers do not repair the candidate server's one-proof-per-physical-target gap for coalesced domains. Actual RN/running-node capabilities, lifecycle drains, legacy reader/writer exclusion, complete coherent capture and causal restoration remain separate prerequisites. This PR does not close terminal complete-WALLET acceptance.

@ben-kaufman
ben-kaufman requested review from dzdidi and ovitrif November 4, 2026 07:26

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve

Findings:

  • LOW src/backup_session/tests/reconciliation.rs:398: abandoned_reconciliation_does_not_restore_or_schedule_continuation drops the future, calls fixture.release.notify_one() and asserts status_requests.len() == 3 straight away. Nothing yields to the runtime in between, so a regression that spawned a continuation would not have run yet and the test would still pass. A short yield or sleep after the release, before counting, makes the "does not schedule continuation" half real. The StalePolicy assertion above it is sound.

QA for 5aa1201:

Check: Crate test suite at the head ✅ cargo test ran 93 tests: 93 passed, 0 failed, 0 ignored, including all 17 new reconciliation tests.

Run locally on 5aa1201.

Check: Clippy and formatting at the head ✅ cargo clippy --all-targets shows only the two existing module_inception warnings in src/tests.rs and src/ffi_tests.rs, files this PR does not touch; cargo fmt --check is clean.

Run locally on 5aa1201.

No device run: this is in-crate Rust source with no UniFFI export, binding change or app activation, so nothing is user-visible. The PR lists no device tests.

Review covered the full range 4b1bccc..5aa1201 (this PR's commit on top of #34). Verified in the code: matches_receipt checks principal/store before the receipt, so a remapped provider is never counted as committed or uncommitted; archive absence, head hash, UUID, proposal digest and the component set (order-independent, both sides unique) are all bound; FreshMigrationReader builds a new LnurlAuthToJwtProvider per read, so a cached JWT is never reused and an auth failure has no fallback; the revision bump in migration_observation leaves the preparation stale after any reconcile, including failed or dropped ones; ensure_reconciliation_current catches overlapping status reads and revocation after both reads. The read_migration_state refactor keeps the ordinary read's ordering unchanged.


Reviewed by claude-opus-5-5-medium

@talosmachina talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No findings. Adds BackupReadSession::reconcile_migration, which reads both original domains through fresh LNURL credentials from the captured signers and compares each principal/store pair and receipt against the retained PreparedMigration. Reviewed 5aa1201, full tier (credential path), stacked on #34 and reviewed as the delta against feat/backup-migration-plan.

Build skipped: talos-pr-run could not confirm the author's permission and fails closed, so this review is reasoned from the code and from this head's CI, not from a local run.

What I checked, and 4 candidates I ruled out

Read in full: src/backup_session.rs, src/backup_session/preparation.rs, src/backup_session/preparation/reconciliation.rs, src/migration_status.rs, the src/implementation.rs and src/ldk_client.rs constructor hunks, the reconciliation test list.
Traced: migration_observation (shared by read_migration_state and reconciliation, so each reconciliation read bumps the domain revision and invalidates prepared_migration, as the docs state); FreshMigrationReader::read builds a new LnurlAuthToJwtProvider per call, so no cached JWT is reused, and ReceiptReader::post wraps header acquisition in the same 10 s timeout with redirects off.
CI on this head: Tests, Clippy, manifests green; Android and iOS Bindgen still pending when this was written.

Ruled out

  • Coalesced store matched twice: both domains resolve to the same PreparedMigrationStore via store_index, and a shared physical tuple returns the same receipt to both reads, so (true, true) is the right result.
  • Component set compared as a multiset: canonical::validate_objects rejects duplicate prepared keys and committed() rejects duplicate receipt keys, so equal length plus containment is set equality.
  • Head digest over the wrong bytes: heads_match hashes the encoded Storable, the same bytes receipt.rs hashes for envelope_sha256.
  • Superseded check after the receipt comparison: a superseded pair can return ParticipantChanged/ReceiptMismatch instead of SupersededObservation, but those are still true server observations, never a positive outcome, so the order cannot turn a stale read into ObservedCommitted.

Merge confidence: 4/5, no findings, but the Bindgen jobs were still pending and the change was not built here.

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve

Findings: none.

QA for 4a1b1f8:

Check: Crate test suite at the head ✅ cargo test ran 94 tests: 94 passed, 0 failed, 0 ignored, including the changed cancellation test and the new positive control.

Run locally on 4a1b1f8.

Check: Reconciliation tests stay stable across repeated runs ✅ The 18 reconciliation tests passed on each of 10 consecutive runs, so the 250 ms observation window does not flake.

Run locally on 4a1b1f8 (cargo test --lib reconciliation, 10×).

Check: Clippy and formatting at the head ✅ cargo clippy --all-targets shows only the two existing module_inception warnings; cargo fmt --check is clean.

Run locally on 4a1b1f8.

No device run: the delta is test-only Rust, with no UniFFI export, binding change or app activation, and the PR lists no device tests.

Review covered the delta 5aa1201..4a1b1f8 (test files only). Our earlier LOW on the abandoned-reconciliation test is fixed: the fixture calls status_arrived.notify_one() before it holds the response, Notify keeps at most one permit, so consuming it after the held request arrives clears every earlier read, and the 250 ms timeout after release yields to the runtime long enough for a spawned continuation to issue its request and be caught. continuation_observer_detects_an_explicitly_scheduled_status_read shows the same detector fires on a real scheduled read. Earlier review of the full change: 5aa1201.


Reviewed by claude-opus-5-5-medium

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck

@dzdidi dzdidi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent exact-head review of 4a1b1f8 found no correctness, security, persistence, API, or documentation blockers. Reviewed all 10 changed files and full context; verified fresh-auth reconciliation, exact immutable receipt matching, ownership/revocation, cancellation, supersession, and sequential-observation limits. Exact-head Tests, Clippy, manifest, iOS bindgen, Android bindgen, and SwiftPM checks are successful. Recommended verdict: approve.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants