Repository navigation
feat: reconcile original-wallet migration receipts - #35
ben-kaufman wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings:
- LOW
src/backup_session/tests/reconciliation.rs:398:abandoned_reconciliation_does_not_restore_or_schedule_continuationdrops the future, callsfixture.release.notify_one()and assertsstatus_requests.len() == 3straight 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. TheStalePolicyassertion 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
left a comment
There was a problem hiding this comment.
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
PreparedMigrationStoreviastore_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_objectsrejects duplicate prepared keys andcommitted()rejects duplicate receipt keys, so equal length plus containment is set equality. - Head digest over the wrong bytes:
heads_matchhashes the encodedStorable, the same bytesreceipt.rshashes forenvelope_sha256. - Superseded check after the receipt comparison: a superseded pair can return
ParticipantChanged/ReceiptMismatchinstead ofSupersededObservation, but those are still true server observations, never a positive outcome, so the order cannot turn a stale read intoObservedCommitted.
Merge confidence: 4/5, no findings, but the Bindgen jobs were still pending and the change was not built here.
There was a problem hiding this comment.
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
dzdidi
left a comment
There was a problem hiding this comment.
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.
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
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.1ed547b7, whose tree exactly matches this head.NO-SOURCEtasks 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-SOURCEbuild 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.