Skip to content

feat: retain owner-bound backup read sessions - #32

Open
ben-kaufman wants to merge 1 commit into
feat/backup-object-receiptsfrom
feat/backup-read-session
Open

ben-kaufman wants to merge 1 commit into
feat/backup-object-receiptsfrom
feat/backup-read-session

Conversation

@ben-kaufman

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

Copy link
Copy Markdown
Collaborator

Description

Retain app and LDK backup reads under one original wallet context. BackupReadSession constructs both clients from one seed and one endpoint/store/auth configuration, preserving the existing truncated app and full-seed LDK derivations. It never assembles a pair from independently replaceable global slots.

Each observation, including explicit absence, belongs to that session. Another session cannot adopt it, even with identical wallet arguments. Revocation closes local read admission, wakes pending credential/body reads and rejects later receipt access. Cancellation schedules no retry. The strict encrypted-envelope reader now also supports the dedicated LDK client's V1 namespace.

Stacked on #31. This is in-crate Rust source preparation for terminal complete-WALLET work in synonymdev/bitkit-ios#1121 and synonymdev/bitkit-android#1686. There is no new UniFFI export or downstream rlib; released interfaces, binary tags and checksums remain unchanged. No homeserver change, SDK release, app activation or migration is included.

Validation

  • Full Rust suite: 45 passed, 0 failed/ignored. Normal release build, docs, whole-repository formatting and diff checks pass. Final all-target Clippy has only two warnings in unchanged base test modules.
  • Two independent security/recovery and integration reviews completed with affected-delta rechecks. A missing production Tokio macro feature, fixture-only LNURL domain error, incomplete body-hold coverage and test lint finding were corrected before final validation.
  • Both iOS slices and all four Android ABIs built in an isolated 0.5.27-dev.3 validation copy. Generated Swift and Kotlin interfaces exactly match the existing exports. Both iOS17 existing-API links and Kotlin consumer compile pass. Android lint XML: 0 errors, 3 verified unchanged dependency-version warnings, 0 introduced findings.
  • Complete logs and structured diagnostics inspected. Archive CRCs/hashes, both 862-object arm64 platform inventories, Android 16 KiB load alignment, stripped release/matching debug IDs and unchanged FFI symbols verified. Existing Python/SDK XML and optional ktlint notices are retained. No CI waiver.

The initial disposable LNURL fixture used an IP-literal callback, which the pinned provider rejected; its 1-pass/7-failure result remains recorded. The corrected localhost fixture verifies actual challenge signatures and encrypted objects. A first Clippy pass found a redundant test pattern, fixed before the final full test/lint pass. These are controlled library fixtures, not production-service or app acceptance. All six final-head hosted jobs passed on attempt 1: Rust, manifest checks, iOS Bindgen and Android Bindgen. The synthetic checkout tree exactly matches signed head 17c8373419b892ec0eda9070551c8f22f25b5e54. All 8,229 log lines and 14 annotations were audited: 45 tests passed, with only the two verified base Clippy warnings. Both uploaded artifacts passed GitHub/log SHA-256 and nested CRC checks. Hosted iOS contains 865 correctly targeted arm64 objects per slice, with deployment declarations at or below iOS17; all four Android ABIs have correct ELF architecture, 16 KiB alignment, matching release/debug IDs and stripped release copies. Every slice/ABI preserves exactly the 93 existing declared FFI symbols; Swift/Kotlin interface bytes remain unchanged. Both artifact transfers completed once, with no CI retry. Conditional tool-bootstrap skips and Java/shader NO-SOURCE tasks are recorded separately, not test passes. This verifies native packaging, not authenticated app recovery.

QA Notes

  • 1. Both original derivation domains authenticate and decrypt; changing only the seed's second half changes LDK identity/storage while preserving app identity/storage.
  • 2. Same-wallet replacement sessions reject old observations; typed absence retains its target and owner; revocation rejects previously returned results.
  • 3. Revocation before dispatch and during credentials or a partially received body; caller drop abandons one read without replay or revoking the reusable session.
  • 4. Both global slots replaced during a held original challenge: original app/LDK values and auth endpoints remain selected; replacement auth/storage endpoints receive nothing.
  • 5. Exact encrypted hash/version and legacy -1 handling; cross-namespace AAD/wrong-key rejection, response cap and existing strict receipt parser controls.
  • 6. Required Rust/native/interface/consumer/lint/format checks and independent source reviews above.

Limits

Receipt access checks local ownership at that instant, not admission for a later mutation. Revocation cannot preempt every already-admitted transport poll or revoke previously borrowed/copied plaintext. The byte cap bounds storage responses, not the inherited credential provider's separate responses. LDK V1 absence says nothing about older V0 objects; the dedicated client does not control a running ldk-node.

Physical principal mapping, lifecycle drains before mutation, coherent capture, legacy reader/writer exclusion, immutable archives/conditional heads and actual restore acceptance remain unfinished. Terminal complete-WALLET availability remains open. Never log observations or plaintext.

@ben-kaufman
ben-kaufman requested review from dzdidi and ovitrif November 4, 2026 04:24

@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 17c8373:

Full Rust test suite at the head ✅ cargo test ran 45 tests: 45 passed, 0 failed, 0 ignored.

RUN/media/cargo-test.log

Session revocation, drop and replacement tests stay green when repeated ✅ The 8 backup_session tests passed in all 20 repeated runs.

RUN/media/session-repeat.log

Check: generated Swift and Kotlin bindings match the base ✅ uniffi-bindgen Swift and Kotlin output from the base and head release libraries is byte-identical.

RUN/media/bindings-diff.txt

Check: release build, rustfmt and Clippy add no new warnings ✅ The release build succeeds and cargo fmt --check is clean. All-target Clippy shows only the two module_inception warnings in the unchanged src/tests.rs and src/ffi_tests.rs.

RUN/media/clippy.log

Not driven: there was no device or app run. The PR adds no UniFFI export and activates nothing in the apps, and the bindings check above shows the Swift and Kotlin interfaces are unchanged. iOS and Android cross-builds were not repeated.

Review covered the full diff 03035b8..17c8373:

  • LDK receipts build the same V1 obfuscated key and use it as AAD, matching LdkVssClient::get and ldk-node's VssStore.
  • The ldk_client.rs change leaves every derivation as it was.
  • read() subscribes before its first revocation check and checks again after the biased select!, so it never misses a revoke.
  • receipt() checks ownership by Arc::ptr_eq on an Arc that every observation holds.

Tests reached every changed path except a real homeserver: the tests use the in-crate localhost LNURL/VSS fixture.


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, which builds the app and LDK clients from one seed and one config, binds every observation (absence included) to the session that made it, and adds a strict V1 receipt read to LdkVssClient. Reviewed 17c8373, full tier, statically: the build gate refused to run this PR's code here (it could not confirm the author's access with the token it has), so I did not build or run the tests myself and relied on CI.

What I checked, and 6 candidates I ruled out

Read in full: src/backup_session.rs, src/backup_session/tests.rs, src/receipt.rs, the ldk_client.rs and implementation.rs constructors, Cargo.toml, the README section
Compared against: ldk-node src/io/vss_store.rs (V1 key layout and AAD), vss-server postgres_store.rs (version handling for -1 writes)
CI: Tests, Clippy and the manifest checks green; Android and iOS Bindgen still pending at review time

Ruled out

  • Revocation lost between subscribe() and the select: subscribe() marks the current value as seen, so a revoke before it never fires changed(), but ensure_active() runs right after and again after the read, so both orderings return Revoked.
  • Session dropped while a read is pending: read borrows &self, so the session cannot drop under it; Drop revoking only affects observations held past the session, and receipt() cannot be called on a dropped session anyway.
  • Cross-session adoption: receipt() compares the Arc pointer of the session's own sender, so an identical-seed second session gets DifferentOwner, as the test asserts.
  • LDK receipt AAD or key layout drifting from ldk-node: build_key is obfuscate(primary#secondary)#obfuscate(key) and the AAD is that full storage key, the same as ldk-node's V1 build_obfuscated_key and its write path.
  • version <= 0 rejecting ldk-node objects: ldk-node writes with -1, which vss-server stores as a positive version, and the embedded -1 passes the existing legacy branch in the decoder.
  • LNURL doc comment change hiding a behaviour change: the code already derived the LDK auth key from the full 64-byte seed; only the comment was wrong before, and the test asserting the two auth keys differ pins that.

Merge confidence: 4/5, no findings, but the two Bindgen jobs were still pending and I could not run the suite locally.

@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.

Reviewed exact head 17c8373419b892ec0eda9070551c8f22f25b5e54 against base 03035b8865f6837d54e632af43c2cde6aedc5a38. No blocking findings. Exact-head Tests, Clippy, manifest, iOS Bindgen, and Android Bindgen checks passed; independent same-digest final pass agreed.

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