Repository navigation
feat: retain owner-bound backup read sessions - #32
ben-kaufman wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
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::getand ldk-node'sVssStore. - The
ldk_client.rschange leaves every derivation as it was. read()subscribes before its first revocation check and checks again after the biasedselect!, so it never misses a revoke.receipt()checks ownership byArc::ptr_eqon anArcthat 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
left a comment
There was a problem hiding this comment.
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 fireschanged(), butensure_active()runs right after and again after the read, so both orderings returnRevoked. - Session dropped while a read is pending:
readborrows&self, so the session cannot drop under it;Droprevoking only affects observations held past the session, andreceipt()cannot be called on a dropped session anyway. - Cross-session adoption:
receipt()compares theArcpointer of the session's own sender, so an identical-seed second session getsDifferentOwner, as the test asserts. - LDK receipt AAD or key layout drifting from ldk-node:
build_keyisobfuscate(primary#secondary)#obfuscate(key)and the AAD is that full storage key, the same as ldk-node's V1build_obfuscated_keyand its write path. version <= 0rejecting ldk-node objects: ldk-node writes with-1, which vss-server stores as a positive version, and the embedded-1passes 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.
dzdidi
left a comment
There was a problem hiding this comment.
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.
Description
Retain app and LDK backup reads under one original wallet context.
BackupReadSessionconstructs 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
0.5.27-dev.3validation 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.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/shaderNO-SOURCEtasks are recorded separately, not test passes. This verifies native packaging, not authenticated app recovery.QA Notes
-1handling; cross-namespace AAD/wrong-key rejection, response cap and existing strict receipt parser controls.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.